[PointerLock] Move "silent mouse lock" logic from renderer to RWHI

[PointerLock] Move "silent mouse lock" logic from renderer to RWHI

RWHI should control whether we want to show the "Press Esc to show your cursor"
warning instead of renderer.

This CL moved existing "silent mouse lock" logic to RWHI without changing the
behavior. Future work could be done to optimize the logic, for example, based on
bubble timeout.

-- Logic for |BrowserPluginGuest|
1. Chrome Apps <webview> send |LockMouse| request to |BrowserPluginGuest|
2. |BrowserPluginGuest| send permission request to host page (via JS)
3. If granted, |BrowserPluginGuest| forward |LockMouse| request to host renderer
4. Host renderer send request to host RWHI

Usually we only have step 4. for a normal page.

BUG=725365
Review-Url: https://codereview.chromium.org/2915613004
Cr-Commit-Position: refs/heads/master@{#477639}
Committed: https://chromium.googlesource.com/chromium/src/+/ba34097511607ca63c7377a5db4cbab3e79decf0

[chongz, 2017-05-30 20:59:59]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 21:00:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 23:06:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 23:06:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chongz, 2017-05-31 17:34:31]

Description was changed from

==========
Move last_unlocked_by_target flag to RWHI

BUG=
==========

to

==========
[PointerLock] Move |last_unlocked_by_target| flag to RWHI

RWHI should control whether we want to show the "Press Esc to show your cursor"
warning.

Moving the flag could also make it easier to fix the "lock, unlock, relock"
issue.

-- Logic for |BrowserPluginGuest|
1. Chrome Apps <webview> send |LockMouse| request
2. |BrowserPluginGuest| request permission from host page
3. If granted, |BrowserPluginGuest| forward |LockMouse| request to host renderer
4. Host renderer send request to host RWHI as usual

BUG=725365
==========

[chongz, 2017-05-31 17:54:16]

Description was changed from

==========
[PointerLock] Move |last_unlocked_by_target| flag to RWHI

RWHI should control whether we want to show the "Press Esc to show your cursor"
warning.

Moving the flag could also make it easier to fix the "lock, unlock, relock"
issue.

-- Logic for |BrowserPluginGuest|
1. Chrome Apps <webview> send |LockMouse| request
2. |BrowserPluginGuest| request permission from host page
3. If granted, |BrowserPluginGuest| forward |LockMouse| request to host renderer
4. Host renderer send request to host RWHI as usual

BUG=725365
==========

to

==========
[PointerLock] Move |last_unlocked_by_target| flag to RWHI

RWHI should control whether we want to show the "Press Esc to show your cursor"
warning.

Moving the flag could also make it easier to fix the "lock, unlock, relock"
issue.

-- Logic for |BrowserPluginGuest|
1. Chrome Apps <webview> send |LockMouse| request to |BrowserPluginGuest|
2. |BrowserPluginGuest| send permission request to host page (via JS)
3. If granted, |BrowserPluginGuest| forward |LockMouse| request to host renderer
4. Host renderer send request to host RWHI

Usually we only have step 4. for a normal page.

BUG=725365
==========

[chongz, 2017-05-31 17:58:51]

chongz@chromium.org changed reviewers:
+ dtapuska@chromium.org

[chongz, 2017-05-31 17:58:51]

dtapuska@ PTAL, thanks!

[dtapuska, 2017-05-31 20:04:03]

https://codereview.chromium.org/2915613004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2915613004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.h:522: void
RejectMouseLockOrUnlockIfNecessary(bool is_unlocked_by_target = false);
You can't use default initialized parameters in chromium. Also this bool should
be an enum.

[chongz, 2017-05-31 21:48:28]

Description was changed from

==========
[PointerLock] Move |last_unlocked_by_target| flag to RWHI

RWHI should control whether we want to show the "Press Esc to show your cursor"
warning.

Moving the flag could also make it easier to fix the "lock, unlock, relock"
issue.

-- Logic for |BrowserPluginGuest|
1. Chrome Apps <webview> send |LockMouse| request to |BrowserPluginGuest|
2. |BrowserPluginGuest| send permission request to host page (via JS)
3. If granted, |BrowserPluginGuest| forward |LockMouse| request to host renderer
4. Host renderer send request to host RWHI

Usually we only have step 4. for a normal page.

BUG=725365
==========

to

==========
[PointerLock] Move "silent mouse lock" logic from renderer to RWHI

RWHI should control whether we want to show the "Press Esc to show your cursor"
warning instead of renderer.

This CL moved existing "silent mouse lock" logic to RWHI without changing the
behavior. Future work could be done to optimize the logic, for example, based on
bubble timeout.

-- Logic for |BrowserPluginGuest|
1. Chrome Apps <webview> send |LockMouse| request to |BrowserPluginGuest|
2. |BrowserPluginGuest| send permission request to host page (via JS)
3. If granted, |BrowserPluginGuest| forward |LockMouse| request to host renderer
4. Host renderer send request to host RWHI

Usually we only have step 4. for a normal page.

BUG=725365
==========

[chongz, 2017-06-01 14:49:20]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 14:49:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-06-01 14:58:21]

dtapuska@ I've updated CL description & comments as we've discussed offline.
PTAL again, thanks!

https://codereview.chromium.org/2915613004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2915613004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.h:522: void
RejectMouseLockOrUnlockIfNecessary(bool is_unlocked_by_target = false);
On 2017/05/31 20:04:03, dtapuska wrote:
> You can't use default initialized parameters in chromium. Also this bool
should
> be an enum.

Removed the parameter. Please see the other comment.

https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2104: }
Actually I don't think we should use |RejectMouseLockOrUnlockIfNecessary()|
here. |RejectMouseLockOrUnlockIfNecessary()| might 1. Reject lock request, or 2.
Automatically unlock mouse, which should only be used on destruction.

Moved unlock logic here instead of passing the boolean bit.

[commit-bot: I haz the power, 2017-06-01 16:28:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 16:28:25]

Dry run: This issue passed the CQ dry run.

[chongz, 2017-06-05 14:17:00]

On 2017/06/01 14:58:21, chongz wrote:
> dtapuska@ I've updated CL description & comments as we've discussed offline.
> PTAL again, thanks!
> 
>
https://codereview.chromium.org/2915613004/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_impl.h (right):
> 
>
https://codereview.chromium.org/2915613004/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_impl.h:522: void
> RejectMouseLockOrUnlockIfNecessary(bool is_unlocked_by_target = false);
> On 2017/05/31 20:04:03, dtapuska wrote:
> > You can't use default initialized parameters in chromium. Also this bool
> should
> > be an enum.
> 
> Removed the parameter. Please see the other comment.
> 
>
https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_impl.cc:2104: }
> Actually I don't think we should use |RejectMouseLockOrUnlockIfNecessary()|
> here. |RejectMouseLockOrUnlockIfNecessary()| might 1. Reject lock request, or
2.
> Automatically unlock mouse, which should only be used on destruction.
> 
> Moved unlock logic here instead of passing the boolean bit.

dtapuska@ Can you take a look again? Thanks!

[dtapuska, 2017-06-05 14:49:50]

https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2104: }
On 2017/06/01 14:58:20, chongz wrote:
> Actually I don't think we should use |RejectMouseLockOrUnlockIfNecessary()|
> here. |RejectMouseLockOrUnlockIfNecessary()| might 1. Reject lock request, or
2.
> Automatically unlock mouse, which should only be used on destruction.
> 
> Moved unlock logic here instead of passing the boolean bit.

This pending_mouse_lock_request boolean is a little odd since it appears to all
be synchronous. Is this possibly some dead code that we might be able to get rid
of? See
https://chromium.googlesource.com/chromium/src/+/b6adb5143ab0cc79a64c64a42f96...

Maybe we can just change the RequestToLockMouse to actually be a boolean return
value?

[chongz, 2017-06-05 15:17:25]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 15:17:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-06-05 15:33:57]

dtapuska@ PTAL again, thanks!

https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:2104: }
On 2017/06/05 14:49:49, dtapuska wrote:
> On 2017/06/01 14:58:20, chongz wrote:
> > Actually I don't think we should use |RejectMouseLockOrUnlockIfNecessary()|
> > here. |RejectMouseLockOrUnlockIfNecessary()| might 1. Reject lock request,
or
> 2.
> > Automatically unlock mouse, which should only be used on destruction.
> > 
> > Moved unlock logic here instead of passing the boolean bit.
> 
> This pending_mouse_lock_request boolean is a little odd since it appears to
all
> be synchronous. Is this possibly some dead code that we might be able to get
rid
> of? See
>
https://chromium.googlesource.com/chromium/src/+/b6adb5143ab0cc79a64c64a42f96...
> 
> Maybe we can just change the RequestToLockMouse to actually be a boolean
return
> value?
> 

Added back |RejectMouseLockOrUnlockIfNecessary()| as per our offline discussion.
We could set |is_last_unlocked_by_target_ = true;| before calling
|RejectMouseLockOrUnlockIfNecessary()|, but I'm feeling doing it after would
make the code easier to reason about.

Filed crbug.com/729584 to track the cleanup.

[commit-bot: I haz the power, 2017-06-05 16:42:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 16:42:23]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-06-05 17:05:16]

On 2017/06/05 15:33:57, chongz wrote:
> dtapuska@ PTAL again, thanks!
> 
>
https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2915613004/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_impl.cc:2104: }
> On 2017/06/05 14:49:49, dtapuska wrote:
> > On 2017/06/01 14:58:20, chongz wrote:
> > > Actually I don't think we should use
|RejectMouseLockOrUnlockIfNecessary()|
> > > here. |RejectMouseLockOrUnlockIfNecessary()| might 1. Reject lock request,
> or
> > 2.
> > > Automatically unlock mouse, which should only be used on destruction.
> > > 
> > > Moved unlock logic here instead of passing the boolean bit.
> > 
> > This pending_mouse_lock_request boolean is a little odd since it appears to
> all
> > be synchronous. Is this possibly some dead code that we might be able to get
> rid
> > of? See
> >
>
https://chromium.googlesource.com/chromium/src/+/b6adb5143ab0cc79a64c64a42f96...
> > 
> > Maybe we can just change the RequestToLockMouse to actually be a boolean
> return
> > value?
> > 
> 
> Added back |RejectMouseLockOrUnlockIfNecessary()| as per our offline
discussion.
> We could set |is_last_unlocked_by_target_ = true;| before calling
> |RejectMouseLockOrUnlockIfNecessary()|, but I'm feeling doing it after would
> make the code easier to reason about.
> 
> Filed crbug.com/729584 to track the cleanup.

lgtm

[chongz, 2017-06-05 17:28:39]

chongz@chromium.org changed reviewers:
+ jochen@chromium.org

[chongz, 2017-06-05 17:28:40]

jochen@ PTAL, thanks!

[jochen (gone - plz use gerrit), 2017-06-06 14:59:47]

lgtm with nit

https://codereview.chromium.org/2915613004/diff/40001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2915613004/diff/40001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:949: const bool
is_last_unlocked_by_target =
why const? that looks unusual..

[chongz, 2017-06-06 17:08:24]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 17:08:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-06-06 17:11:37]

chongz@chromium.org changed reviewers:
+ kenrb@chromium.org

[chongz, 2017-06-06 17:11:38]

kenrb@ PTAL at 'content/common/view_messages.h', thanks!

https://codereview.chromium.org/2915613004/diff/40001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2915613004/diff/40001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:949: const bool
is_last_unlocked_by_target =
On 2017/06/06 14:59:47, jochen wrote:
> why const? that looks unusual..

Done. Hmmm I though we prefer using 'const', no...?

[commit-bot: I haz the power, 2017-06-06 18:08:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 18:08:45]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-06-06 18:58:45]

ipc lgtm

[chongz, 2017-06-07 14:34:20]

The CQ bit was checked by chongz@chromium.org

[chongz, 2017-06-07 14:34:21]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2915613004/#ps60001
(title: "jochen's comment: Remove 'const' before local bool")

[commit-bot: I haz the power, 2017-06-07 14:34:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 14:40:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496846060445360,
"parent_rev": "7f27e84b4a2982b5e83b5b6ff4e9e03b087f57f3", "commit_rev":
"6e51082b1b52f3dd3ff6a1adc7553e94a399104e"}

[commit-bot: I haz the power, 2017-06-07 14:40:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496846060445360,
"parent_rev": "322f674790e53f25053adf10e2afd0109806dd06", "commit_rev":
"ba34097511607ca63c7377a5db4cbab3e79decf0"}

[commit-bot: I haz the power, 2017-06-07 14:40:28]

Description was changed from

==========
[PointerLock] Move "silent mouse lock" logic from renderer to RWHI

RWHI should control whether we want to show the "Press Esc to show your cursor"
warning instead of renderer.

This CL moved existing "silent mouse lock" logic to RWHI without changing the
behavior. Future work could be done to optimize the logic, for example, based on
bubble timeout.

-- Logic for |BrowserPluginGuest|
1. Chrome Apps <webview> send |LockMouse| request to |BrowserPluginGuest|
2. |BrowserPluginGuest| send permission request to host page (via JS)
3. If granted, |BrowserPluginGuest| forward |LockMouse| request to host renderer
4. Host renderer send request to host RWHI

Usually we only have step 4. for a normal page.

BUG=725365
==========

to

==========
[PointerLock] Move "silent mouse lock" logic from renderer to RWHI

RWHI should control whether we want to show the "Press Esc to show your cursor"
warning instead of renderer.

This CL moved existing "silent mouse lock" logic to RWHI without changing the
behavior. Future work could be done to optimize the logic, for example, based on
bubble timeout.

-- Logic for |BrowserPluginGuest|
1. Chrome Apps <webview> send |LockMouse| request to |BrowserPluginGuest|
2. |BrowserPluginGuest| send permission request to host page (via JS)
3. If granted, |BrowserPluginGuest| forward |LockMouse| request to host renderer
4. Host renderer send request to host RWHI

Usually we only have step 4. for a normal page.

BUG=725365

Review-Url: https://codereview.chromium.org/2915613004
Cr-Commit-Position: refs/heads/master@{#477639}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba34097511607ca63c7377a5db4c...
==========

[commit-bot: I haz the power, 2017-06-07 14:40:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ba34097511607ca63c7377a5db4c...