Perform direct routing of mouse events when the pointer is locked.

Perform direct routing of mouse events when the pointer is locked.

BUG=618460
Review-Url: https://codereview.chromium.org/2592243002
Cr-Commit-Position: refs/heads/master@{#451031}
Committed: https://chromium.googlesource.com/chromium/src/+/84763c90521d17f4b30d3abaa8af472b579a63ff

[lfg, 2016-12-21 22:44:10]

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

[commit-bot: I haz the power, 2016-12-21 22:44:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-12-21 23:04:22]

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

[lfg, 2016-12-21 23:04:23]

Ken, please take a look.

[commit-bot: I haz the power, 2016-12-21 23:58:11]

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

[commit-bot: I haz the power, 2016-12-21 23:58:12]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-12-22 15:14:16]

Thanks for putting this together. This needs a test, and other than that just a
couple of comments.

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:176: if
(root_view->IsMouseLocked()) {
You need to transform the event's coordinates into the widget's coordinate
space. Also, do you need to null check the return of GetView(), to guard against
a race with a process crash? I gave it a quick look, and don't see a way that
the lock would get immediately released after RWHVCF::RenderProcessGone.

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:259: if
(root_view->IsMouseLocked()) {
Likewise for the transform.

[kenrb, 2016-12-22 15:15:52]

Description was changed from

==========
Perform direct routing of mouse events when the pointer is locked.

BUG=618460
==========

to

==========
Perform direct routing of mouse events when the pointer is locked.

BUG=618460
==========

[lfg, 2017-01-18 16:32:25]

Patchset #2 (id:20001) has been deleted

[lfg, 2017-02-13 18:18:52]

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

[commit-bot: I haz the power, 2017-02-13 18:19:37]

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-02-13 20:07:40]

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

[commit-bot: I haz the power, 2017-02-13 20:07:41]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-02-13 20:16:07]

Sorry for the delay, adding the test was a bit tricky. Please take another look.

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:176: if
(root_view->IsMouseLocked()) {
On 2016/12/22 15:14:16, kenrb wrote:
> You need to transform the event's coordinates into the widget's coordinate
> space. Also, do you need to null check the return of GetView(), to guard
against
> a race with a process crash? I gave it a quick look, and don't see a way that
> the lock would get immediately released after RWHVCF::RenderProcessGone.

Done. I don't think we need to null check GetView(), since
WebContentsImpl::RenderWidgetDeleted releases the mouse lock when a widget dies.

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:259: if
(root_view->IsMouseLocked()) {
On 2016/12/22 15:14:16, kenrb wrote:
> Likewise for the transform.

Done.

[kenrb, 2017-02-14 19:58:49]

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:176: if
(root_view->IsMouseLocked()) {
On 2017/02/13 20:16:07, lfg wrote:
> On 2016/12/22 15:14:16, kenrb wrote:
> > You need to transform the event's coordinates into the widget's coordinate
> > space. Also, do you need to null check the return of GetView(), to guard
> against
> > a race with a process crash? I gave it a quick look, and don't see a way
that
> > the lock would get immediately released after RWHVCF::RenderProcessGone.
> 
> Done. I don't think we need to null check GetView(), since
> WebContentsImpl::RenderWidgetDeleted releases the mouse lock when a widget
dies.

I don't think child frame RWHImpls necessarily get destroyed when their process
crashes. Only their views do.

[lfg, 2017-02-15 20:12:03]

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2592243002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_input_event_router.cc:176: if
(root_view->IsMouseLocked()) {
On 2017/02/14 19:58:49, kenrb wrote:
> On 2017/02/13 20:16:07, lfg wrote:
> > On 2016/12/22 15:14:16, kenrb wrote:
> > > You need to transform the event's coordinates into the widget's coordinate
> > > space. Also, do you need to null check the return of GetView(), to guard
> > against
> > > a race with a process crash? I gave it a quick look, and don't see a way
> that
> > > the lock would get immediately released after RWHVCF::RenderProcessGone.
> > 
> > Done. I don't think we need to null check GetView(), since
> > WebContentsImpl::RenderWidgetDeleted releases the mouse lock when a widget
> dies.
> 
> I don't think child frame RWHImpls necessarily get destroyed when their
process
> crashes. Only their views do.

After further investigation, this isn't necessary. The mouse is unlocked when
the process is destroyed, it happens in the destructors of
RenderWidgetHostViewAura, RenderWidgetHostViewMac and in
RenderWidgetHostViewChildFrame::SetCrossProcessFrameConnector, called from
RenderWidgetHostViewChildFrame::Destroy.

We should never get to this point without an existing view.

[kenrb, 2017-02-15 20:46:14]

Thanks for looking into that. lgtm

[lfg, 2017-02-15 20:56:54]

lfg@chromium.org changed reviewers:
+ creis@chromium.org

[lfg, 2017-02-15 20:56:54]

Charlie, can you take a look?

[Charlie Reis, 2017-02-15 21:38:49]

Thanks-- LGTM with nits.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
File content/browser/pointer_lock_browsertest.cc (right):

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
content/browser/pointer_lock_browsertest.cc:21: #if defined(OS_LINUX) ||
defined(OS_CHROMEOS) || defined(OS_WIN)
Any reason not to use USE_AURA?  Might simplify the ones below as well.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
content/browser/pointer_lock_browsertest.cc:27: defined(OS_MACOSX)
Is there an easier way to do this in the build file?  It seems odd to me to
ifdef out the whole file on other platforms (e.g., Android).  Seems like we
could only build it on the platforms where it's needed?

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
content/browser/pointer_lock_browsertest.cc:46: 
nit: No need for blank line here.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
content/browser/pointer_lock_browsertest.cc:208: mouse_event.movementY = 10;
nit: Maybe set these to different values (e.g., 10, 11, 12, 13) to make sure
that they don't get interchanged via a bug?

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:189: virtual
RenderWidgetHostImpl* MouseLockWidget();
nit: GetMouseLockWidget

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:176:
RenderWidgetHostViewBase* target;
Given all the conditional assignments below, let's assign this to nullptr to
avoid any risk of uninitialized reads (e.g., after future refactors).

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:179: if
(root_view->IsMouseLocked()) {
It's probably worth putting a comment on this block, given that it handles mouse
lock specially and then early returns.

[lfg, 2017-02-15 21:59:48]

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

[commit-bot: I haz the power, 2017-02-15 22:01:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2017-02-15 22:02:48]

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
File content/browser/pointer_lock_browsertest.cc (right):

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
content/browser/pointer_lock_browsertest.cc:21: #if defined(OS_LINUX) ||
defined(OS_CHROMEOS) || defined(OS_WIN)
On 2017/02/15 21:38:49, Charlie Reis wrote:
> Any reason not to use USE_AURA?  Might simplify the ones below as well.

Done.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
content/browser/pointer_lock_browsertest.cc:27: defined(OS_MACOSX)
On 2017/02/15 21:38:49, Charlie Reis wrote:
> Is there an easier way to do this in the build file?  It seems odd to me to
> ifdef out the whole file on other platforms (e.g., Android).  Seems like we
> could only build it on the platforms where it's needed?

Done.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
content/browser/pointer_lock_browsertest.cc:46: 
On 2017/02/15 21:38:49, Charlie Reis wrote:
> nit: No need for blank line here.

Done.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/pointer...
content/browser/pointer_lock_browsertest.cc:208: mouse_event.movementY = 10;
On 2017/02/15 21:38:49, Charlie Reis wrote:
> nit: Maybe set these to different values (e.g., 10, 11, 12, 13) to make sure
> that they don't get interchanged via a bug?

Done.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:189: virtual
RenderWidgetHostImpl* MouseLockWidget();
On 2017/02/15 21:38:49, Charlie Reis wrote:
> nit: GetMouseLockWidget

Done.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:176:
RenderWidgetHostViewBase* target;
On 2017/02/15 21:38:49, Charlie Reis wrote:
> Given all the conditional assignments below, let's assign this to nullptr to
> avoid any risk of uninitialized reads (e.g., after future refactors).

Done.

https://codereview.chromium.org/2592243002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:179: if
(root_view->IsMouseLocked()) {
On 2017/02/15 21:38:49, Charlie Reis wrote:
> It's probably worth putting a comment on this block, given that it handles
mouse
> lock specially and then early returns.

Done.

[lfg, 2017-02-15 22:03:14]

lfg@chromium.org changed reviewers:
+ avi@chromium.org

[lfg, 2017-02-15 22:03:15]

Avi, can you do a pass on *mac* files?

[Avi (use Gerrit), 2017-02-15 22:18:09]

mac stuff lgtm

[commit-bot: I haz the power, 2017-02-15 22:36:53]

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

[commit-bot: I haz the power, 2017-02-15 22:36:54]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[lfg, 2017-02-16 17:35:38]

The CQ bit was checked by lfg@chromium.org

[lfg, 2017-02-16 17:35:39]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2592243002/#ps60001
(title: "addressing comments")

[commit-bot: I haz the power, 2017-02-16 17:36:14]

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-02-16 18:55:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487266538340950,
"parent_rev": "306a6a5a991467ce0cc34b56e05af420ecd90919", "commit_rev":
"84763c90521d17f4b30d3abaa8af472b579a63ff"}

[commit-bot: I haz the power, 2017-02-16 18:55:57]

Description was changed from

==========
Perform direct routing of mouse events when the pointer is locked.

BUG=618460
==========

to

==========
Perform direct routing of mouse events when the pointer is locked.

BUG=618460

Review-Url: https://codereview.chromium.org/2592243002
Cr-Commit-Position: refs/heads/master@{#451031}
Committed:
https://chromium.googlesource.com/chromium/src/+/84763c90521d17f4b30d3abaa8af...
==========

[commit-bot: I haz the power, 2017-02-16 18:55:59]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/84763c90521d17f4b30d3abaa8af...