Page should be able to request pointer lock without user gesture if it relinquished it without a user gesture

[Jeremy Jones]

Page should be able to request pointer lock without user gesture if it relinquished it without a user gesture

[Jeremy Jones]

rdar://problem/29539389

[Jeremy Jones]

*** Bug 167013 has been marked as a duplicate of this bug. ***

[Jeremy Jones]

Created attachment 299046 [details]
Patch

[Jon Lee]

Comment on attachment 299046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299046&action=review

> Source/WebCore/ChangeLog:11
> +        This change allows a page to lock the pointer pointer again without a user gesture if it was unlocked by

"pointer pointer"

> Source/WebCore/ChangeLog:14
> +        * page/PointerLockController.cpp:

Please provide some more detail here.

> Source/WebCore/page/PointerLockController.cpp:116
> +        m_lockPending = false;

Replacing clearElement() with two variable assignments seems fragile. Can we factor this out to a separate version of clearElement that indicates leaving m_unlockPending alone?

> Source/WebCore/page/PointerLockController.h:69
> +    RefPtr<Document> m_documentCanSkipUserGesture;

The variable name took a little time for me to understand its purpose since it sounded like a bool type rather than a RefPtr. Maybe m_documentAllowedToRelockWithoutUserGesture?

[Jeremy Jones]

(In reply to comment #4)
> Comment on attachment 299046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299046&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        This change allows a page to lock the pointer pointer again without a user gesture if it was unlocked by
> 
> "pointer pointer"

Done.

> 
> > Source/WebCore/ChangeLog:14
> > +        * page/PointerLockController.cpp:
> 
> Please provide some more detail here.

Added detail.

> 
> > Source/WebCore/page/PointerLockController.cpp:116
> > +        m_lockPending = false;
> 
> Replacing clearElement() with two variable assignments seems fragile. Can we
> factor this out to a separate version of clearElement that indicates leaving
> m_unlockPending alone?

Actually, the default should be to leave m_unlockPending alone. So, I've changed clearElement to do that. The two places that need to modify m_unlockPending, should directly modify that flag since it is explicitly part of their function. didNotAcquirePointerLock means we are no longer waiting on unlockPending, it should set the flag explicitly. This also means that more locations can use clearElement.

> 
> > Source/WebCore/page/PointerLockController.h:69
> > +    RefPtr<Document> m_documentCanSkipUserGesture;
> 
> The variable name took a little time for me to understand its purpose since
> it sounded like a bool type rather than a RefPtr. Maybe
> m_documentAllowedToRelockWithoutUserGesture?

Done.

[Jeremy Jones]

Created attachment 299830 [details]
Patch

[Jon Lee]

Rebase, please

[Jon Lee]

Comment on attachment 299830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299830&action=review

> Source/WebCore/page/PointerLockController.cpp:114
> +        clearElement();

Is this expectation that m_unlockPending is always false in clearElement()? I'd suggest adding an ASSERT there, then. On untrained eyes, one might expect that m_unlockPending should also be set to false in that function.

[Jeremy Jones]

(In reply to comment #8)
> Comment on attachment 299830 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299830&action=review
> 
> > Source/WebCore/page/PointerLockController.cpp:114
> > +        clearElement();
> 
> Is this expectation that m_unlockPending is always false in clearElement()?
> I'd suggest adding an ASSERT there, then. On untrained eyes, one might
> expect that m_unlockPending should also be set to false in that function.

That is not the expected case. clearElement() is used when document or element are removed, so the element is cleared, but the unlock is still pending. The unlock must be round tripped to the chrome client after the element is cleared. This feature requires knowing that there was a pending request when the unlock completes, so it can't be cleared by clearElement.

[Jeremy Jones]

Created attachment 299878 [details]
Patch

[Jeremy Jones]

Created attachment 299883 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 299883 [details]
Patch

Clearing flags on attachment: 299883

Committed r211249: <http://trac.webkit.org/changeset/211249>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.