1356183 - MousePosTracker should use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout

Status: RESOLVED WONTFIX

(Firefox :: General, defect, P1)

Description

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from bug 1307134 comment 14)
>     var fullZoom = this._windowUtils.fullZoom;
>     this._x = event.screenX / fullZoom - window.mozInnerScreenX;
>     this._y = event.screenY / fullZoom - window.mozInnerScreenY;
> 
> Not sure why mozInnerScreenX ("the X coordinate of the top-left corner of
> the window's viewport, in screen coordinates") flushes layout in the first
> place, but maybe we can just use event.clientX instead? Not sure if this
> gives the same results, this whole stuff is a bit confusing.

(In reply to Dão Gottwald [::dao] from bug 1307134 comment 26)
> It would also be good if somebody could clarify if using event.clientX
> instead of event.screenX - window.mozInnerScreenX would be a good idea. It
> seems to work in my testing, but maybe it implies a layout flush as well?

(In reply to Timothy Nikkel (:tnikkel) from bug 1307134 comment 27)
> It doesn't look like clientX flushes:
> 
https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/dom/events/Event.cpp#983

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1356183 - Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout.

Review commit: https://reviewboard.mozilla.org/r/129902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129902/

Comment 2

[Dão Gottwald [:dao]]

Would be nice if QA could verify that there's no behavior change in how the status panel at the bottom of the window (that displays the loading status and link URLs) interacts with the mouse pointer, using different DPI settings and window modes (maximized, restored, fullscreen). Bonus points for verifying that the layout flush is gone, but I'm not sure if QA is equipped for that.

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8857875 [details]
Bug 1356183 - Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout.

https://reviewboard.mozilla.org/r/129902/#review132518

Let's try this, thanks :-). On my fast macbook I can't get the code without the patch to be visible in the profiles, so I can't verify if applying the patch helps. Once this is on Nightly I'll profile again on my very old linux netbook to confirm that I no longer see it in profiles.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3edd47730527
Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout. r=florian

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3edd47730527

Comment 6

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> Once this is on Nightly I'll profile again on my very old linux
> netbook to confirm that I no longer see it in profiles.

Here is a new profile from a 2017-04-14 nightly on that same netbook: https://perfht.ml/2ofbaB5

ClientX accounts for 2ms out of 50ms spent in that event listener.
The forEach overhead is 26ms (more than half the total time), that's bug 1355874.
nsDOMWindowUtils::GetFullZoom(float*) is 4ms, should we somehow cache this value?

This confirms that the layout flush is gone.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> (In reply to Florian Quèze [:florian] [:flo] from comment #3)
> > Once this is on Nightly I'll profile again on my very old linux
> > netbook to confirm that I no longer see it in profiles.
> 
> Here is a new profile from a 2017-04-14 nightly on that same netbook:
> https://perfht.ml/2ofbaB5
> 
> ClientX accounts for 2ms out of 50ms spent in that event listener.
> The forEach overhead is 26ms (more than half the total time), that's bug
> 1355874.

I think forEach can handle a listener removing itself in the loop? I can't recall how for..of behaves here.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8857875 [details]
Bug 1356183 - Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout.

Approval Request Comment
[Feature/Bug causing the regression]: /
[User impact if declined]: possibly poor performance when moving the mouse over the browser window
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: it's been confirmed that the performance problem is solved
[Needs manual test from QE? If yes, steps to reproduce]: see comment 2
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no (once verified that using clientX doesn't change behavior)
[Why is the change risky/not risky?]: very small, isolated fix
[String changes made/needed]:

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8857875 [details]
Bug 1356183 - Let MousePosTracker use event.clientX instead of event.screenX - window.mozInnerScreenX to avoid flushing layout.

Fix a perf issue when moving the mouse over the browser window. Aurora54+.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4786c5b193e1

Comment 11

[Dão Gottwald [:dao]]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/f18e288534fbc91b47a4d11a25409fdae274a1d2

clientX/Y don't work the way we need it with non-e10s content (bug 1357841), and this patch isn't necessary anymore since bug 1307134 is fixed. However I'm going to leave this patch in 54 unless bug 1307134 gets uplifted.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f18e288534fb