1255338 - Change the way pointerlock works on e10s

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+10]

How pointerlock work in general:
For each mouse move, if the pointer stops at a location other than the center of the window, we ask the system to move it back to the center.
If we detect that an event is triggered by a mouse move we synthesized, we suppress it.

In the current settings for e10s, all the logic above works inside the content process, which means, for each mouse move event, we would have at least two more IPC messages (one for synthesizing the mousemove, one for the additional to-be-suppressed mousemove event). This is very inefficient.

We should move the main logic of pointerlock to the parent process, so that no additional IPC happens for mousemove events. In addition, issues like bug 1244546 is hard to fix for e10s due to coordinate alignment requirement of some platforms.

Comment 1

[Jim Mathies [:jimm]]

would love to see this api cleaned up, and the tests for it enabled. this is an enhancement though so doesn't block e10s.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Without this change, pointer lock would not work properly on HiDPI Linux with e10s. The pointer position will constantly move upward even the user doesn't touch the mouse at all. If we can live with this issue, sure, it is not a blocker.

Note that it is not a regression. Before bug 1244546 gets fixed, pointer lock is not usable at all on Linux with HiDPI.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Well, we should disable pointerlock API on e10s-linux, if this wasn't fixed. It is just so broken.
We could have done it for bug 1244546 too for non-e10s case.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

It isn't broken for e10s-linux. It is only broken for e10s Linux with HiDPI. But yeah, if this isn't very high priority, we should probably disable pointer lock for HiDPI Linux with e10s enabled. But I still think we should fix pointer lock for e10s given it is currently so inefficient...

Comment 5

[Andrew Overholt [:overholt]]

Xidorn, do you think this is something you'll get to in the next few months?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Yes, it is currently in my Q2 plan.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

It seems to me the HiDPI issue appears on OS X as well, when in fullscreen mode.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

I used to use pointer-lock-demo for testing pointer lock, but I just realized that the demo itself is poorly coded. So I guess we don't actually have any serious performance issue on PointerLock. (If there is really any performance difference for pointer-lock-demo between e10s and non-e10s, that should be because of handling tons of rAFs, not PointerLock).

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

But we may still want to fix this, both for an even better performance, and for correceness on HiDPI Linux and Mac.

Comment 11

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

quote from bug 853160 comment# 13:

The issue here is that we are just repeatedly trying to move the pointer back to the center of the window, but we never really restrict the pointer within the window area.

IIRC all desktop platforms provide some kind of mouse movement restriction API, for example on macOS we may use CGAssociateMouseAndMouseCursorPosition to forbid the mouse cursor from being moved at all.

Comment 12

[Adam Vandolder [:avandolder]]

Attachment: WIP: Bug 1255338 - Part 1: Use native pointer grabbing API under X11. r?edgar,#dom-core

Using this API prevents the mouse pointer from escaping from
PointerLocked windows.

Comment 13

[Adam Vandolder [:avandolder]]

Attachment: WIP: Bug 1255338 - Part 2: Use native pointer grapping API under Windows. r?edgar,#dom-core

Comment 14

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Bug 1255338 - Part 1: Remove unused argument aElement from EventStateManager::SetPointerLock;

Comment 15

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: WIP: Bug 1255338 - Part 1: Remove unused member from UIEvent;

Depends on D195726

Comment 16

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: WIP: Bug 1255338 - Part 2: Make PointerLockManager::SetPointerLock won't dispatch pointerlockchange event;

by factoring out PointerLockManager::DispatchPointerLockChange() from
PointerLockManager::ChangePointerLockedElement().

This is needed for subsequent patches which tries to setup pointer lock in
chrome document as well when content request pointer lock and we don't want to
trigger pointerlock change event in chrome document in this case.

And this should not change currrent behavior.

Depends on D195728

Comment 17

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=6e4c55d29c14165261b8925601476e329ebf0158

Comment 18

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=3addcd2c8f7b11814fc5bb5165a248fa98fb83ea

Comment 19

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: WIP: Bug 1255338 - Part 3: Use Maybe for sPreLockScreenPoint;

Comment 20

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: WIP: Bug 1255338 - Part 4: Lock in parent;

Comment 21

[Phabricator Automation]

Comment on attachment 9367377 [details]
WIP: Bug 1255338 - Part 1: Remove unused member from UIEvent;

Revision D195728 was moved to bug 1880187. Setting attachment 9367377 [details] to obsolete.