Closed Bug 1357880 Opened 7 years ago Closed 7 years ago

Telemetry for keyboard APZ

Categories

(Core :: Panning and Zooming, enhancement, P2)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: milan, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners
59 bytes, text/x-review-board-request
smaug
: review+
benjamin
: review+
Details
Bug 1357880 - Add a telemetry probe for mousemove event listeners
59 bytes, text/x-review-board-request
smaug
: review+
benjamin
: review+
Details 
We want to add telemetry probes to measure how often pages have non-passive keyboard event listeners on the root content document/root scrollframe. We might also want to know how often pages have mousemove events on the root, as it will affect whether we want to do the bonus step above.

See bug 1351783 comment 1 for background.
Ryan, can you help with this?  Kats can provide details.  We'd want it sooner rather than later.
Assignee: nobody → rhunt
Blocks: apz-keyboard
Flags: needinfo?(rhunt)
Yes, I'll take a look at this.
Flags: needinfo?(rhunt)
Priority: -- → P2
Whiteboard: [gfx-noted]
Discussing this on #apz, I think we have a potential implementation plan for checking keyboard event listeners.

1. Add a 'everHadNonPassiveKeyListener' flag to EventListenerManager and populate it
2. Add a method like HasDocumentLevelListenersForApzAwareEvents()[1] that checks for this flag
3. Use this method in ~nsDocument (or wherever fits for the end of a document and it's content) to check if the page has ever had a non passive key listener that would impact keyboard-apz

Olli, is this reasonable, or is there a better way to do this?

[1] https://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#8621
Flags: needinfo?(bugs)
HasDocumentLevelListenersForApzAwareEvents sounds wrong, since it is more like window, document, document.documentElement or document.body.

~nsDocument might not be the best place since then one would need to consider also data documents etc.
Perhaps inner window would be better place to report.
We have already Telemetry::INNERWINDOWS_WITH_MUTATION_LISTENERS there.
Flags: needinfo?(bugs)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95d95660074fee17fc95608c095ddc928aeb47ec

Mostly looks fine.

There is one failure I'm not sure about [1], a content process hang that is spewing out:

'###!!! [Child][MessageChannel] Error: (msgtype=0x4800FA,name=PContent::Msg_AccumulateChildKeyedHistograms) Closed channel: cannot send/recv'

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=95d95660074fee17fc95608c095ddc928aeb47ec&selectedJob=94745074
Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

https://reviewboard.mozilla.org/r/134248/#review137252

::: dom/base/nsGlobalWindow.cpp:10788
(Diff revision 1)
>  void
>  nsGlobalWindow::PageHidden()
>  {
>    FORWARD_TO_INNER_VOID(PageHidden, ());
>  
> +  nsIDocument* doc = GetDocument();

This is wrong place to report this. We end up reporting also pages entering bfcache.
And if the page comes out from bfcache and re-enters there, we end up reporting data about the same page again.

Could you possible move this to FreeInnerObjects();?
Attachment #8862314 - Flags: review?(bugs) → review-
Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

https://reviewboard.mozilla.org/r/134250/#review137256

::: commit-message-b5e0b:1
(Diff revision 1)
> +Bug 1357880 - Add a telemetry probe for mousemove event listeners r?smaug

Why we need this whole thing?

::: toolkit/components/telemetry/Histograms.json:10540
(Diff revision 1)
>      "bug_numbers": [1352654],
>      "expires_in_version": "58",
>      "kind": "boolean",
>      "description": "The percentage of pages with a non passive key event on the path to the root scrolling element."
>    },
> +  "APZ_AWARE_KEYBOARD_MOUSE_MOVE_LISTENERS": {

Weird name for the probe. Why APZ_AWARE in the name, or KEYBOARD
Attachment #8862315 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8862314 [details]
> Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners
> 
> https://reviewboard.mozilla.org/r/134248/#review137252
> 
> ::: dom/base/nsGlobalWindow.cpp:10788
> (Diff revision 1)
> >  void
> >  nsGlobalWindow::PageHidden()
> >  {
> >    FORWARD_TO_INNER_VOID(PageHidden, ());
> >  
> > +  nsIDocument* doc = GetDocument();
> 
> This is wrong place to report this. We end up reporting also pages entering
> bfcache.
> And if the page comes out from bfcache and re-enters there, we end up
> reporting data about the same page again.
> 
> Could you possible move this to FreeInnerObjects();?

I've moved it to FreeInnerObjects() and that works. I was aware we'd report multiple times, but it would be only when a user goes to a page and comes back, which is a measure of use. But this will be a better measure of distinct pages.
(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8862315 [details]
> Bug 1357880 - Add a telemetry probe for mousemove event listeners
> 
> https://reviewboard.mozilla.org/r/134250/#review137256
> 
> ::: commit-message-b5e0b:1
> (Diff revision 1)
> > +Bug 1357880 - Add a telemetry probe for mousemove event listeners r?smaug
> 
> Why we need this whole thing?
> 
> ::: toolkit/components/telemetry/Histograms.json:10540
> (Diff revision 1)
> >      "bug_numbers": [1352654],
> >      "expires_in_version": "58",
> >      "kind": "boolean",
> >      "description": "The percentage of pages with a non passive key event on the path to the root scrolling element."
> >    },
> > +  "APZ_AWARE_KEYBOARD_MOUSE_MOVE_LISTENERS": {
> 
> Weird name for the probe. Why APZ_AWARE in the name, or KEYBOARD

This probe is for determining how often APZ key scrolling could handle interleaved mousemove events vs. not because there is a listener on the page. From my understanding of the bonus step in bug 1351783 comment 1.

The name of the probe was gross though so I've simplified it a bit. I'm still open to suggestions. The prefix APZ_AWARE is just because this probe is going to be used for determining how many event listeners are 'apz aware'.

The mousemove one doesn't necessarily need the prefix because nothing is APZ specific about the probe, only the use of it.
It is still a bit mystery to me why mousemove is so special. Focus can change at any point.
But ok, perhaps we can add the telemetry.
Flags: needinfo?(mstange)
Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

https://reviewboard.mozilla.org/r/134248/#review137608
Attachment #8862314 - Flags: review?(bugs) → review+
Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

https://reviewboard.mozilla.org/r/134250/#review137610

r+, but before landing this, ask mstange or kats whether we actually need this.
(these probes do after all slow down other execution a bit and make code a tad more complicated)
Attachment #8862315 - Flags: review?(bugs) → review+
Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

https://reviewboard.mozilla.org/r/134250/#review137614

::: dom/base/nsPIDOMWindow.h:822
(Diff revision 2)
>  
>    /**
>     * Call this to check whether some node (this window, its document,
> +   * or content in that document) has or had a mousemove event listener.
> +   */
> +  bool HasMouseMoveEventListeners()

Call this something a tad more unreliable, since we don't catch cases when nodes with event listeners are moved from one document to another.
Perhaps negate it and call it

ProbablyDoesNotHaveMouseMoveListeners().


Or do what we do with mMayHaveMouseEnterLeaveEventListener and deal with this stuff properly, by updating the flag when adopting nodes.
(In reply to Olli Pettay [:smaug] from comment #16)
> r+, but before landing this, ask mstange or kats whether we actually need
> this.

Yeah I think we do. IIRC the plan was to allow APZ to do keyboard scrolling as long as we knew where the focus was, but if any non-keyboard event came through we would stop doing APZ keyboard scrolling until we can reconfirm the focus. This is because any of those events could change the focus via listeners. We further assumed that the most frequent events would be mousemove events, and so if we could maintain APZ keyboard scrolling even with interleaved mousemove events that would be an improvement (the "bonus" step in bug 1351783), but we can only do that if we know there are no mousemove listeners that could change the focus. Therefore this telemetry probe will tell us how many pages we can do this on.
Flags: needinfo?(mstange)
focus can change at any point, not only by event listeners. And mousemoves rarely change focus, so I'm still having hard time to understand the reasoning (as I did in the meeting :) ).
Yes, but if it changes by non-event listeners (i.e. via setTimeout) then there isn't an expectation of deterministic behaviour by the user. If they generate a sequence of input events then there is an expectation that the behaviour will be the same every time. If the focus is influenced by some non-user-input factor, then they can't really rely on it always happening at a particular point in their sequence of keyboard input. So even if we scroll the wrong thing for the next few events (until the repaint happens and the focus state is pushed over to APZ) it should be acceptable. I agree it's not ideal but basically we want to try and see if we can get away with it.
Also with respect to the performance implications of the telemetry probes - we don't need to let these probes go to release. Data from the beta channel should be sufficient. Anyway we would like to implement the keyboard scrolling pretty soon so we can't really afford to wait to get release-channel data.
(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8862315 [details]
> Bug 1357880 - Add a telemetry probe for mousemove event listeners
> 
> https://reviewboard.mozilla.org/r/134250/#review137614
> 
> ::: dom/base/nsPIDOMWindow.h:822
> (Diff revision 2)
> >  
> >    /**
> >     * Call this to check whether some node (this window, its document,
> > +   * or content in that document) has or had a mousemove event listener.
> > +   */
> > +  bool HasMouseMoveEventListeners()
> 
> Call this something a tad more unreliable, since we don't catch cases when
> nodes with event listeners are moved from one document to another.
> Perhaps negate it and call it
> 
> ProbablyDoesNotHaveMouseMoveListeners().
> 
> 
> Or do what we do with mMayHaveMouseEnterLeaveEventListener and deal with
> this stuff properly, by updating the flag when adopting nodes.

I'll update the patch to deal with this properly. I missed the code in nsNodeUtils.
I've update the patches with two changes,

1. I renamed ELM::EverHadAPZKeyAwareListeners to ELM::MayHaveAPZAwareKeyEventListener, so that it matches with all the other properties of the same type. This is patch 1, no functional changes.

2. I added the code to handle moving event listeners from one document to another in nsNodeUtils. This is patch 2.
One more update to update a histogram description. No functional changes.
Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

Flag for data-review.

We don't plan on allowing this probe to go to release. We should get enough data before then.
Attachment #8862314 - Flags: feedback?(benjamin)
Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

Flag for data-review
Attachment #8862315 - Flags: feedback?(benjamin)
Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

https://reviewboard.mozilla.org/r/134248/#review138074

data-r=me with that clarification in the histogram description.

::: toolkit/components/telemetry/Histograms.json:10436
(Diff revision 3)
> +  "APZ_AWARE_KEY_LISTENERS": {
> +    "alert_emails": ["rhunt@mozilla.com"],
> +    "bug_numbers": [1352654],
> +    "expires_in_version": "58",
> +    "kind": "boolean",
> +    "description": "The percentage of pages with a non-passive key event on the path to the root scrolling element that would disable APZ key scrolling."

Is this limited to certain kinds of pages? e.g.

* chrome windows?
* only toplevel windows or all subframes?

Please at least document these questions in the histogram description for clarity and testing purposes.
Attachment #8862314 - Flags: review+
Attachment #8862314 - Flags: feedback?(benjamin)
Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

https://reviewboard.mozilla.org/r/134250/#review138078

data-r=me with clarifications

::: toolkit/components/telemetry/Histograms.json:10443
(Diff revision 4)
> +  "APZ_AWARE_MOUSEMOVE_LISTENERS": {
> +    "alert_emails": ["rhunt@mozilla.com"],
> +    "bug_numbers": [1352654],
> +    "expires_in_version": "58",
> +    "kind": "boolean",
> +    "description": "The percentage of pages with a mousemove listener anywhere in the document that would disable APZ key scrolling."

ditto comments here about chrome windows/toplevel/subframes.
Attachment #8862315 - Flags: review+
Attachment #8862315 - Flags: feedback?(benjamin)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7129e53f3a9c
Add a telemetry probe for non-passive keyboard event listeners r=smaug, data-review=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/915489b79bbf
Add a telemetry probe for mousemove event listeners r=smaug, data-review=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/7129e53f3a9c
https://hg.mozilla.org/mozilla-central/rev/915489b79bbf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Data is starting to come in. On nightly the APZ_AWARE_KEY_LISTENERS flag is true ~13.89% of the time. APZ_AWARE_MOUSEMOVE_LISTENERS is true ~12.03% of the time.
Nice.  Can we tell how many have both, and consequently, how many just have one?
Not with the probes as they are. I posted some analysis over at bug 1351783 comment 4.
See Also: → 1373285
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: