Closed Bug 1285069 Opened 4 years ago Closed 4 years ago

Simplify PointerLock DOM code after removing permission requirement of that

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
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/
Attachment #8769520 - Flags: review?(bugs)
Attachment #8769521 - Flags: review?(bugs)
Attachment #8769522 - Flags: review?(bugs)
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 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 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/
Assignee: nobody → xidorn+moz
Attachment #8769520 - Flags: review?(bugs) → review+
Comment on attachment 8769520 [details]
Bug 1285069 part 1 - Remove code around pending fullscreen request.

https://reviewboard.mozilla.org/r/63384/#review60350
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.
Attachment #8769521 - Flags: review?(bugs) → review-
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.)
Attachment #8769522 - Flags: review?(bugs) → review+
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/
Attachment #8769521 - Flags: review- → review?(bugs)
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 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
Attachment #8769521 - Flags: review?(bugs) → review+
Pushed by xquan@mozilla.com:
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
Hi Xidorn,
Is there any way for us to manually test this? Thank you.
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.