Closed Bug 1165211 Opened 9 years ago Closed 5 years ago

Deprecate MouseEvent.mozPressure

Categories

(Core :: DOM: Events, task, P3)

Product:
Core
Component:
DOM: Events
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68

People

(Reporter: Ms2ger, Assigned: mbrodesser-Igalia, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files, 3 obsolete files)

Bug 1165211: Add UseCounter for 'MouseEvent.mozPressure' and deprecate it.
47 bytes, text/x-phabricator-request
Details | Review
Data collection review request version 2
2.59 KB, text/plain
liuche
: data-review+
Details 
      No description provided.
Why would we unprefix mozPressure? We should perhaps try to get rid of it.
(Pointer events have .pressure)
Summary: Unprefix or remove MouseEvent.mozPressure. → Remove MouseEvent.mozPressure.
I saw someone mention it on twitter earlier.
Is it a good time now to try removing mozPressure, or is further time/research needed?
Flags: needinfo?(Ms2ger)
I can see some comments about mozPressure when searching for it in the web, so perhaps we could have a quick check with telemetry whether anyone is using it and then re-evaluate?
None that I personally know of.
Flags: needinfo?(Ms2ger)
I take it you mean to just add a Telemetry::Accumulate for it for a couple of releases?
Flags: needinfo?(bugs)
Right. Some telemetry probe telling whether the property is used at all. One release might be enough for this.
Flags: needinfo?(bugs)
Alright. I'm new to how telemetry works in the codebase, so I'm not sure if this is 100% right, but here's a patch that seems like it should do the trick.

I take it I should add/use someone else's email address?
Assignee: nobody → wisniewskit
Status: NEW → ASSIGNED
Attachment #8783618 - Flags: review?(bugs)
Comment on attachment 8783618 [details] [diff] [review]
1165211-add-telemetry-counting-uses-of-mozPressure.diff

This should be fine.
https://telemetry.mozilla.org/ can be then used to track the uses.
Attachment #8783618 - Flags: review?(bugs) → review+
Alright thanks. Requesting check-in.
Keywords: checkin-needed, leave-open
This actually needs to go through the Data Review process first AFAICT.
https://wiki.mozilla.org/Firefox/Data_Collection
Keywords: checkin-needed
Comment on attachment 8783618 [details] [diff] [review]
1165211-add-telemetry-counting-uses-of-mozPressure.diff

>This actually needs to go through the Data Review process first AFAICT.

Thanks Ryan. Benjamin, would you mind doing the honors, or should I ask someone else?
Attachment #8783618 - Flags: feedback?(benjamin)
Comment on attachment 8783618 [details] [diff] [review]
1165211-add-telemetry-counting-uses-of-mozPressure.diff

I'd like the description to be better. Describe *what* and *when* a value is recorded. So for instance "A counter incremented every time event.mozPressure is used by a page." or "A counter incremented for each document that uses event.mozPressure." or something (it's important to say whether this is per-use or per-document.

data-review if you're confident making that change yourself, or come back to me if you're not sure.

FWIW, I don't consider this a blocker, but I'm skeptical that this telemetry will actually answer your question. If people *are* using mozPressure, then what? Especially because this is prerelease-only, which is not a great way to collect webcompat data. Do you have a specific threshold or evaluation plan once you have collected the data?
Attachment #8783618 - Flags: feedback?(benjamin) → feedback+
>I'm skeptical that this telemetry will actually answer your question. If people *are* using mozPressure, then what? Especially because this is prerelease-only, which is not a great way to collect webcompat data. Do you have a specific threshold or evaluation plan once you have collected the data?

Good points. I'm not 100% sure if we have a plan beyond "if it's being used, we won't remove it yet". I'm new to this, so I'm not at all sure what threshold would be sane. smaug, what do you think?
Flags: needinfo?(bugs)
Well, if we get data that no one using the property, then removing should be quite safe.
If we see some use, we could add warning about use of deprecated property and hopefully get the usage down and remove.
If we see lots of use, like via some script library, then we may need to keep the property or investigate more why and how it is being used.

So, I think the interesting data here is whether usage is 0 or very very close to 0, or something higher.
Flags: needinfo?(bugs)
That's the tricky part of this. The usage being 0 on the nightly channel tells you very little: that highly technical users running an en-US build on modern computers don't need this. You start getting much higher degrees of confidence from beta users, although they are still skewed in some ways:

* odd locale skews
* very few enterprise users

Collecting release data is a much better signal. It would only be skewed against a very small set of enterprise-related sites.

Which is why I want to see at least some sort of decision-tree:
* if nightly shows lots of usage we: leave it forever? add a deprecation warning?
* if nightly shows a little usage but not a lot, then what? And what is your definition of "a little"?
* if nightly shows no usage, then what?
smaug, what do you think we should do here in light of comment 17? If bsmedberg's right, then nightly channel telemetry might not really help us make a decision. Should we try gathering release telemetry instead (assuming the pool of users who enable release telemetry will provide enough information to base a decision on)?
Flags: needinfo?(bugs)
Yes, I'd expect us to need to wait until telemetry probe is in release builds before removing anything.
Flags: needinfo?(bugs)
Has anything landed here?
Flags: needinfo?(wisniewskit)
No, not to my knowledge.
Flags: needinfo?(wisniewskit)
Priority: -- → P3
The leave-open keyword is there and there is no activity for 6 months.
:wisniewskit, maybe it's time to close this bug?
Flags: needinfo?(wisniewskit)
Assignee: wisniewskit → nobody
Mentor: twisniewski, alchen
Status: ASSIGNED → NEW

Mirko will be helping this out. This bug is still valid.

Assignee: nobody → mbrodesser
Flags: needinfo?(wisniewskit)
Attachment #9048847 - Attachment description: Bug 1165211: Add UseCounter for 'MouseEvent.mozPressure' and deprecate it. r=alchen → Bug 1165211: Add UseCounter for 'MouseEvent.mozPressure' and deprecate it.
Attachment #9049137 - Attachment is obsolete: true

Since a similar issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1048291) didn't require a data review, I guess this change also doesn't. Requesting check-in.

Hey, per discussion on IRC, it's needed to ask data review. The policy changed 16-ish months ago. So we need to do something different from bug 1048291 ;)

Mirko, could you please refer to https://wiki.mozilla.org/Firefox/Data_Collection, fill out a quick form, attach to a bug, flag a Data Steward for data-review??

Once the data-review is done, and your try run looks green, you can add "checkin-needed" on the "keywords" blank. And remember to paste the good try run link here when you request checkin. Thank you!

Keywords: checkin-needed
Flags: needinfo?(mbrodesser)

Btw, you might want to land this patch in another bug, and keep this one open until the attribute is actually removed.

@Hsin-Yi: OK, will fill out the form, etc.

@Ms2ger: I already had in mind to create a follow-up bug to actually delete the event. Thanks for the reminder. :) Follow-up bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1534199

Flags: needinfo?(mbrodesser)
Attached file Data collection review request (obsolete) —

Added the completed data collection request form for review.

Attachment #9050369 - Flags: data-review+
Attachment #9050369 - Flags: data-review+ → data-review?(liuche)

Hmmm, who's the mentor for this bug? Are you planning to land BOTH the Phabricator patch (recently reviewed) and the Splinter patch (from 3 years ago)?
The Splinter patch is bit-rotted and based on the deprecation plan in place here, probably doesn't need to land anymore if we're planning on removing mozPressure anyway, right? It doesn't look like whatever data you collect would change this plan. If you don't need to land the Splinter telemetry patch, please obsolete that patch so it's not confusing - then you also don't need data review because you aren't changing any telemetry.

If you're planning on landing the Splinter patch though, here are few problems I see from a quick glance. Data review != code review, so you need to get this re-reviewed, and then additionally flag me for data review again:

  • Comment #14 requests a better description: "So for instance "A counter incremented every time event.mozPressure is used by a page." or "A counter incremented for each document that uses event.mozPressure.""
  • The data review says this will be collected for "all release populations" - is this true? Correct me if I'm wrong, but this looks like it's NOT for release populations, but actually pre-release
  • The expiry for this probe has already expired so this probe would do nothing
Flags: needinfo?(mbrodesser)
Comment on attachment 9050369 [details]
Data collection review request

Clearing data-review flag bc this patch is bit-rotted - please flag me again for data review once this is patch is updated
Attachment #9050369 - Flags: data-review?(liuche)
Attachment #8783618 - Attachment is obsolete: true
Flags: needinfo?(mbrodesser)

(In reply to Chenxia Liu [:liuche] from comment #31)

Hmmm, who's the mentor for this bug? Are you planning to land BOTH the Phabricator patch (recently reviewed) and the Splinter patch (from 3 years ago)?

My mentor is Alphan Chen.

The Splinter patch is bit-rotted and based on the deprecation plan in place here, probably doesn't need to land anymore if we're planning on removing mozPressure anyway, right? It doesn't look like whatever data you collect would change this plan. If you don't need to land the Splinter telemetry patch, please obsolete that patch so it's not confusing - then you also don't need data review because you aren't changing any telemetry.

No, the splinter patch will not be landed, a moment ago I've marked it as obsolete. The other patch still adds use-counters for _DOCUMENT and _PAGE (https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/use-counters.html#deprecated-dom-operations), hence the data review is still needed.

If you're planning on landing the Splinter patch though, here are few problems I see from a quick glance. Data review != code review, so you need to get this re-reviewed, and then additionally flag me for data review again:

  • Comment #14 requests a better description: "So for instance "A counter incremented every time event.mozPressure is used by a page." or "A counter incremented for each document that uses event.mozPressure.""
  • The data review says this will be collected for "all release populations" - is this true? Correct me if I'm wrong, but this looks like it's NOT for release populations, but actually pre-release

Yes, it should be collected in all release populations (as stated in the data review: mozilla-release, all countries, all locales, no filters). It is for release populations, not for pre-release.

  • The expiry for this probe has already expired so this probe would do nothing

"6)" in the data review contains "for one whole release (Firefox 66), until 2019-05-14.", this is the release date of Firefox 67 (https://wiki.mozilla.org/Release_Management/Calendar) which is still in the future, hence not expired.

Please let me know if I misunderstood something or if the data review can be accepted and the patch can be landed.

Flags: needinfo?(liuche)

Thanks for the link, this is the first time that I've seen UseCounters.

A few questions:

  • From the docs, it looks like:
    "The histograms that are generated out of use counters are set to never expire and are opt-in."

Did that change in bug 1477433, to include opt-out? (Or is opt-out okay for this probe?) I'm just confused because looking at the Telemetry Probe Dictionary in that bug, I only see pre-release probes, did that change to include release: https://telemetry.mozilla.org/probe-dictionary/?search=use_counter2&channel=nightly&version=68

My question in the earlier comment about release population collection is more of a "is this actually being collected in release BY the code?", and is not a challenge - I assume that's what you want to do, I'm just not clear on whether that is actually something that UseCounters does, so I want to double check so the filed document reflects what is happening, since I'm not familiar with webidl.

  • In the Dictionary list, I see a "description" column. Do you know where that comes from? For data review I'd like that to be added to this patch. That way there will be a human-readable description so when someone goes through the probes on the dashboard, it will be clear what that probe is measuring without needing to go through all these bugs.

It looks like these probes are "never" expire anyway, and it seems like this is a deprecated event on its way out, so the expiry is less of a problem - I'll ask the other stewards about that policy but it won't block this review.

Flags: needinfo?(liuche)

(In reply to Chenxia Liu [:liuche] from comment #34)

Thanks for the link, this is the first time that I've seen UseCounters.

A few questions:

  • From the docs, it looks like:
    "The histograms that are generated out of use counters are set to never expire and are opt-in."

Did that change in bug 1477433, to include opt-out? (Or is opt-out okay for this probe?) I'm just confused because looking at the Telemetry Probe Dictionary in that bug, I only see pre-release probes, did that change to include release: https://telemetry.mozilla.org/probe-dictionary/?search=use_counter2&channel=nightly&version=68

I also don't see release probes and assumed bug 1477433 changed them to be opt-out. @Jan-Erik can you shed some light on this or recommend someone who can?

My question in the earlier comment about release population collection is more of a "is this actually being collected in release BY the code?", and is not a challenge - I assume that's what you want to do, I'm just not clear on whether that is actually something that UseCounters does, so I want to double check so the filed document reflects what is happening, since I'm not familiar with webidl.

Yes, that's what we want here.

  • In the Dictionary list, I see a "description" column. Do you know where that comes from? For data review I'd like that to be added to this patch. That way there will be a human-readable description so when someone goes through the probes on the dashboard, it will be clear what that probe is measuring without needing to go through all these bugs.

No, I don't know here it comes from, probably it's automatically generated. Maybe Jan-Erik can tell.

It looks like these probes are "never" expire anyway, and it seems like this is a deprecated event on its way out, so the expiry is less of a problem - I'll ask the other stewards about that policy but it won't block this review.

Yes, indeed. Thanks for pointing this out.

Flags: needinfo?(jrediger)

First off, thanks, :liuche for the detailed data-review here while :chutten is out.

Indeed, our Use Counters are a bit weird.

  1. Since bug 1477433 are enabled on release ("opt-out") by default. I updated the docs in bug 1510566, but somehow missed the note further below (will fix that now).
  2. It looks like the probe dictionary for some reason didn't get the change, we failed to update how it generates the data for it. Will file a bug and get this fixed.
  3. The description is auto-generated and there's no way for users to define something else for use counters.
  4. Re for one whole release (Firefox 66) in the data-review request: Firefox 66 is currently beta and will be released on Tuesday. As this will need to land on Nightly first, it's impossible to uplift it anymore before the final merge on Monday. You will thus need to land it in the next cycle and then see if you can uplift to the following release (that is: land in Nightly 68, uplift to Beta 67). If you want to be really safe say Release 68.
Flags: needinfo?(jrediger)

(In reply to Jan-Erik Rediger [:janerik] from comment #36)

First off, thanks, :liuche for the detailed data-review here while :chutten is out.

Indeed, our Use Counters are a bit weird.

  1. Since bug 1477433 are enabled on release ("opt-out") by default. I updated the docs in bug 1510566, but somehow missed the note further below (will fix that now).
  2. It looks like the probe dictionary for some reason didn't get the change, we failed to update how it generates the data for it. Will file a bug and get this fixed.
  3. The description is auto-generated and there's no way for users to define something else for use counters.
  4. Re for one whole release (Firefox 66) in the data-review request: Firefox 66 is currently beta and will be released on Tuesday. As this will need to land on Nightly first, it's impossible to uplift it anymore before the final merge on Monday. You will thus need to land it in the next cycle and then see if you can uplift to the following release (that is: land in Nightly 68, uplift to Beta 67). If you want to be really safe say Release 68.

:janerik: thanks for the answers. I'll update the data review to Firefox 68.

Added an updated version of the data collection review. :liuche, please let me know if you need more information.

Attachment #9050369 - Attachment is obsolete: true
Attachment #9050369 - Flags: data-review?(liuche)
Attachment #9051264 - Flags: data-review?(liuche)
Comment on attachment 9051264 [details]
Data collection review request version 2

Thanks for the follow-up, and answering all my questions about UseCounters! data-review+

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?
Yes, UseCounters are turned into Histograms so it will be documented there. I'm unclear on how the description is added, but according to https://bugzilla.mozilla.org/show_bug.cgi?id=1165211#c36 it's not possible to add that manually, so this will be the same as the other UseCounters.

2) Is there a control mechanism that allows the user to turn the data collection on and off? 
Yes, users can turn off telemetry in the data collection section of Firefox

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**
No, expiry in release 68

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **
Type 2, interaction data on using mozPressure, which measures finger or touch pressure

5) Is the data collection request for default-on or default-off?
Default on, release

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?
No, counter for usage of this feature

7) Is the data collection covered by the existing Firefox privacy notice?
Yes

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**
No, trying to remove this code and deprecating it - will review in 68 release

9) Does the data collection use a third-party collection tool?
No
Attachment #9051264 - Flags: data-review?(liuche) → data-review+

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/1c31262a809e
Add UseCounter for 'MouseEvent.mozPressure' and deprecate it. r=alchen,smaug

Keywords: checkin-needed

[Tracking Requested - why for this release]: because we want to remove MouseEvent.mozPressure as soon as possible.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Type: defect → task
Summary: Remove MouseEvent.mozPressure. → Deprecate MouseEvent.mozPressure

Documentation updated:

Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: