Bug 165377 – [pointer-lock] Cursor should become visible when exiting pointer-lock via ESC key.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[pointer-lock] Cursor should become visible when exiting pointer-lock via ESC key.

[Jer Noble]

Created attachment 296146 [details]
Patch

[Darin Adler]

Comment on attachment 296146 [details]
Patch

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

> Source/WebCore/page/PointerLockController.h:50
> +    enum ForceCursorVisibleFlag {
> +        DoNotForceCursorVisible,
> +        ForceCursorVisible,
> +    };

I suggest using an enum class:

    enum class ShouldForceCursorVisible { Yes, No };

I also suggest defining this on a single line instead of four lines.

> Source/WebCore/page/PointerLockController.h:52
>      void elementRemoved(Element*);

These functions that take pointers are so old fashioned. They should take references unless there is some reason they need to work with null.

[Darin Adler]

Comment on attachment 296146 [details]
Patch

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

> Source/WebCore/page/PointerLockController.h:51
> +    void requestPointerUnlock(ForceCursorVisibleFlag);

I suggest two different functions instead of one function with a flag. We can use the flag internally, but it is less clear for callers than two separate functions with good names would be.

[Jer Noble]

Created attachment 296161 [details]
Patch for landing

[Jon Lee]

rdar://problem/29081718

[Jer Noble]

Created attachment 296201 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 296201 [details]
Patch for landing

Rejecting attachment 296201 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
x86_64/RenderButton.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/rendering/RenderButton.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/RenderButton.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/PointerLockController.o page/PointerLockController.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/2629953

[Jer Noble]

Committed r209394: <http://trac.webkit.org/changeset/209394>

[Darin Adler]

Comment on attachment 296201 [details]
Patch for landing

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

> Source/WebCore/page/PointerLockController.cpp:157
> +    if (m_forceCursorVisibleUponUnlock)
> +        m_page.chrome().client().setCursorHiddenUntilMouseMoves(false);

Shouldn't we set m_forceCursorVisibleUponUnlock back to false here?

[Jer Noble]

Comment on attachment 296201 [details]
Patch for landing

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

>> Source/WebCore/page/PointerLockController.cpp:157
>> +        m_page.chrome().client().setCursorHiddenUntilMouseMoves(false);
> 
> Shouldn't we set m_forceCursorVisibleUponUnlock back to false here?

Gah, you're right. I'll fix that s in a follow-up.

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 296392 [details]
Follow-Up Patch

[WebKit Commit Bot]

Comment on attachment 296392 [details]
Follow-Up Patch

Clearing flags on attachment: 296392

Committed r209464: <http://trac.webkit.org/changeset/209464>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.