Deprecate MouseEvent.mozPressure
Categories
(Core :: DOM: Events, task, P3)
Tracking
()
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)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
2.59 KB,
text/plain
|
liuche
:
data-review+
|
Details |
Updated•9 years ago
|
Comment 1•9 years ago
|
||
Updated•9 years ago
|
Reporter | ||
Comment 2•9 years ago
|
||
Comment 3•9 years ago
|
||
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
Comment 7•8 years ago
|
||
Comment 8•8 years ago
|
||
Comment 9•8 years ago
|
||
Comment 10•8 years ago
|
||
Comment 12•8 years ago
|
||
Comment 13•8 years ago
|
||
Comment 14•8 years ago
|
||
Comment 15•8 years ago
|
||
Comment 16•8 years ago
|
||
Comment 17•8 years ago
|
||
Comment 18•8 years ago
|
||
Comment 19•8 years ago
|
||
Comment 21•7 years ago
|
||
Updated•7 years ago
|
Updated•6 years ago
|
Comment 22•6 years ago
|
||
Updated•6 years ago
|
Comment 23•6 years ago
|
||
Mirko will be helping this out. This bug is still valid.
Assignee | ||
Comment 24•6 years ago
|
||
Assignee | ||
Comment 25•6 years ago
|
||
Depends on D22302
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 26•6 years ago
|
||
checkin-needed |
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.
Updated•6 years ago
|
Comment 27•6 years ago
|
||
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!
Updated•6 years ago
|
Reporter | ||
Comment 28•6 years ago
|
||
Btw, you might want to land this patch in another bug, and keep this one open until the attribute is actually removed.
Assignee | ||
Comment 29•6 years ago
•
|
||
@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
Assignee | ||
Comment 30•6 years ago
|
||
Added the completed data collection request form for review.
Assignee | ||
Updated•6 years ago
|
Comment 31•6 years ago
|
||
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
Comment 32•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 33•6 years ago
•
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Comment 34•6 years ago
|
||
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.
Assignee | ||
Comment 35•6 years ago
|
||
(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.
Comment 36•6 years ago
|
||
First off, thanks, :liuche for the detailed data-review here while :chutten is out.
Indeed, our Use Counters are a bit weird.
- 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).
- 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.
- The description is auto-generated and there's no way for users to define something else for use counters.
- 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.
Assignee | ||
Comment 37•6 years ago
|
||
(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.
- 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).
- 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.
- The description is auto-generated and there's no way for users to define something else for use counters.
- 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.
Assignee | ||
Comment 38•6 years ago
|
||
Added an updated version of the data collection review. :liuche, please let me know if you need more information.
Comment 39•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 40•6 years ago
|
||
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
Comment 41•6 years ago
|
||
bugherder |
Assignee | ||
Comment 42•6 years ago
•
|
||
[Tracking Requested - why for this release]: because we want to remove MouseEvent.mozPressure
as soon as possible.
Updated•6 years ago
|
Updated•5 years ago
|
Comment 43•5 years ago
|
||
Posted site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2019/mouseevent-mozpressure-has-been-deprecated/
Comment 44•5 years ago
|
||
Documentation updated:
- https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent
- Mentioned on Firefox 68 for developers
Updated•1 year ago
|
Description
•