1285069 - Simplify PointerLock DOM code after removing permission requirement of that

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

Most of work of getting rid of permission requirement of PointerLock in the browser side would be done in bug 1273351, but there are still some DOM code can be simplified after that.

Doing so would make fixing other bugs easier, so make this bug block them.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1285069 part 1 - Remove code around pending fullscreen request.

This code is no longer needed as we are always granting pointerlock request.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1285069 part 2 - Remove nsPointerLockPermissionRequest, only keep the Allow method under a different name.

We still want to keep ApplyPointerLock asynchronous so that the timing
when pointer lock takes effect is not changed by this patch.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1285069 part 3 - Move code around to make pointer lock code together.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8769521 [details]
Bug 1285069 part 2 - Remove nsPointerLockPermissionRequest, only keep the Allow method under a different name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63386/diff/1-2/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8769522 [details]
Bug 1285069 part 3 - Move code around to make pointer lock code together.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63388/diff/1-2/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e76d37bbf13

Comment 7

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

Comment on attachment 8769520 [details]
Bug 1285069 part 1 - Remove code around pending fullscreen request.

https://reviewboard.mozilla.org/r/63384/#review60350

Comment 8

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

Comment on attachment 8769521 [details]
Bug 1285069 part 2 - Remove nsPointerLockPermissionRequest, only keep the Allow method under a different name.

https://reviewboard.mozilla.org/r/63386/#review60352

r-, because of runnablefunction issue..

One option is to file a separate bug to move NewRunnableFunction from chromium code to some better place.

But memory management needs still some comment or fix. Why do we not leak if dispatching runnable fails?

::: dom/base/nsDocument.cpp:12326
(Diff revision 2)
>    if (!d->ShouldLockPointer(e, pointerLockedElement, true)) {
>      DispatchPointerLockError(d);
> -    return NS_OK;
> +    return;
> +  }
> +
> +  // If it is neither user input initiated, nor requested in fullscree,

fullscreen

::: dom/base/nsDocument.cpp:12447
(Diff revision 2)
>  
>    bool userInputOrChromeCaller = EventStateManager::IsHandlingUserInput() ||
>                                   nsContentUtils::IsCallerChrome();
>  
> -  gPendingPointerLockRequest =
> -    new nsPointerLockPermissionRequest(aElement, userInputOrChromeCaller);
> +  NS_DispatchToMainThread(
> +    NewRunnableFunction(ApplyPointerLock, userInputOrChromeCaller,

Please don't use NewRunnableFunction, which is some IPC thingie living in ipc/chromium.
Perhaps you want NS_NewRunnableFunction?

And even then, please explain the memory management here regarding the return values of do_GetWeakReference calls.


I think I actually do prefer the semantics of NewRunnableFunction over NS_NewRunnableFunction, but making dom/base to depend on such ipc/chromium thingie would be annoying.

Comment 9

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

Comment on attachment 8769522 [details]
Bug 1285069 part 3 - Move code around to make pointer lock code together.

https://reviewboard.mozilla.org/r/63388/#review60358

MozReview showed the move quite annoyingly ;)
(and since I don't trust mr, code moves are a bit annoying to review in it.)

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8769521 [details]
Bug 1285069 part 2 - Remove nsPointerLockPermissionRequest, only keep the Allow method under a different name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63386/diff/2-3/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8769522 [details]
Bug 1285069 part 3 - Move code around to make pointer lock code together.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63388/diff/2-3/

Comment 12

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

Comment on attachment 8769521 [details]
Bug 1285069 part 2 - Remove nsPointerLockPermissionRequest, only keep the Allow method under a different name.

https://reviewboard.mozilla.org/r/63386/#review60622

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ac08f46f71
part 1 - Remove code around pending fullscreen request. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a43543bd1f59
part 2 - Remove nsPointerLockPermissionRequest, only keep the Allow method under a different name. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/15eec1fadc74
part 3 - Move code around to make pointer lock code together. r=smaug

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27ac08f46f71
https://hg.mozilla.org/mozilla-central/rev/a43543bd1f59
https://hg.mozilla.org/mozilla-central/rev/15eec1fadc74

Comment 15

[Alexandru Simonca [:asimonca], Release Desktop QA]

Hi Xidorn,
Is there any way for us to manually test this? Thank you.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

This is more a cleanup change. As far as there is no regression, I don't think there's any way to manually test this change.