Unify the code paths for handling mouse events when pointer is locked on

Unify the code paths for handling mouse events when pointer is locked on
WebViewImpl and WebFrameWidgetImpl.

BUG=618460
Review-Url: https://codereview.chromium.org/2696523003
Cr-Commit-Position: refs/heads/master@{#450791}
Committed: https://chromium.googlesource.com/chromium/src/+/9b5618b8aa52feeccbb3203046b56b8e75d9423a

[lfg, 2017-02-13 19:04:24]

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

[commit-bot: I haz the power, 2017-02-13 19:04:57]

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-13 20:18:43]

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

[lfg, 2017-02-13 20:18:43]

Daniel, please take a look.

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

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

[commit-bot: I haz the power, 2017-02-13 20:43:42]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-13 23:21:28]

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:340: if (m_client &&
m_client->isPointerLocked() &&
Why would the client be null here?

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2534:
mainFrameImpl()->frameWidget()->didLosePointerLock();
Can we remove these from being exposed via WebView altogether?

[lfg, 2017-02-14 15:39:45]

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

[commit-bot: I haz the power, 2017-02-14 15:40:18]

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-14 15:42:49]

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:340: if (m_client &&
m_client->isPointerLocked() &&
On 2017/02/13 23:21:28, dcheng wrote:
> Why would the client be null here?

It shouldn't be. I just copied the code from WebViewImpl, but the way
WebFrameWidgetImpl is setup we shouldn't have a null client.

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2534:
mainFrameImpl()->frameWidget()->didLosePointerLock();
On 2017/02/13 23:21:28, dcheng wrote:
> Can we remove these from being exposed via WebView altogether?

Not until more work is done; in particular, the RenderWidget calls these
functions, and the WebWidget that's associated with the RenderWidget is still a
WebViewImpl, instead of a WebFrameWidget.

[commit-bot: I haz the power, 2017-02-14 16:59:38]

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

[commit-bot: I haz the power, 2017-02-14 16:59:39]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-14 19:20:28]

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2534:
mainFrameImpl()->frameWidget()->didLosePointerLock();
On 2017/02/14 15:42:49, lfg wrote:
> On 2017/02/13 23:21:28, dcheng wrote:
> > Can we remove these from being exposed via WebView altogether?
> 
> Not until more work is done; in particular, the RenderWidget calls these
> functions, and the WebWidget that's associated with the RenderWidget is still
a
> WebViewImpl, instead of a WebFrameWidget.

Is this something the content side patches that you mentioned earlier today
would do?

[lfg, 2017-02-14 19:29:03]

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2534:
mainFrameImpl()->frameWidget()->didLosePointerLock();
On 2017/02/14 19:20:28, dcheng wrote:
> On 2017/02/14 15:42:49, lfg wrote:
> > On 2017/02/13 23:21:28, dcheng wrote:
> > > Can we remove these from being exposed via WebView altogether?
> > 
> > Not until more work is done; in particular, the RenderWidget calls these
> > functions, and the WebWidget that's associated with the RenderWidget is
still
> a
> > WebViewImpl, instead of a WebFrameWidget.
> 
> Is this something the content side patches that you mentioned earlier today
> would do?

No, unfortunately not. This is due to the design of
content::MouseLockDispatcher, which gets created in RenderWidget::Init, before
the WebViewFrameWidget is created, so it can only be associated with the
WebViewImpl at that point. To remove those from being exposed in WebView, we
would either have to redesign the lifetime of the mouse lock objects, or
associate the RenderWidget with the WebViewFrameWidget (I tried to do that in
the past, and there was a lot of other lifetime issues related closing the
WebViewFrameWidget and the WebViewImpl).

See https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?l=539

[dcheng, 2017-02-14 23:46:10]

LGTM

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2696523003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2534:
mainFrameImpl()->frameWidget()->didLosePointerLock();
On 2017/02/14 19:29:03, lfg wrote:
> On 2017/02/14 19:20:28, dcheng wrote:
> > On 2017/02/14 15:42:49, lfg wrote:
> > > On 2017/02/13 23:21:28, dcheng wrote:
> > > > Can we remove these from being exposed via WebView altogether?
> > > 
> > > Not until more work is done; in particular, the RenderWidget calls these
> > > functions, and the WebWidget that's associated with the RenderWidget is
> still
> > a
> > > WebViewImpl, instead of a WebFrameWidget.
> > 
> > Is this something the content side patches that you mentioned earlier today
> > would do?
> 
> No, unfortunately not. This is due to the design of
> content::MouseLockDispatcher, which gets created in RenderWidget::Init, before
> the WebViewFrameWidget is created, so it can only be associated with the
> WebViewImpl at that point. To remove those from being exposed in WebView, we
> would either have to redesign the lifetime of the mouse lock objects, or
> associate the RenderWidget with the WebViewFrameWidget (I tried to do that in
> the past, and there was a lot of other lifetime issues related closing the
> WebViewFrameWidget and the WebViewImpl).
> 
> See
https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?l=539

I assume we have a long-term bug for cleaning this up. If not, do you mind
filing one?

[lfg, 2017-02-15 18:38:09]

On 2017/02/14 23:46:10, dcheng wrote:
> I assume we have a long-term bug for cleaning this up. If not, do you mind
> filing one?

Filed https://crbug.com/692641

[lfg, 2017-02-15 18:38:17]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2017-02-15 18:39:36]

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-15 20:43:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487183897363020,
"parent_rev": "aae4c7cdac8ce3bae51257e05de6bba8e7492c98", "commit_rev":
"9b5618b8aa52feeccbb3203046b56b8e75d9423a"}

[commit-bot: I haz the power, 2017-02-15 20:44:26]

Description was changed from

==========
Unify the code paths for handling mouse events when pointer is locked on
WebViewImpl and WebFrameWidgetImpl.

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

to

==========
Unify the code paths for handling mouse events when pointer is locked on
WebViewImpl and WebFrameWidgetImpl.

BUG=618460

Review-Url: https://codereview.chromium.org/2696523003
Cr-Commit-Position: refs/heads/master@{#450791}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b5618b8aa52feeccbb3203046b5...
==========

[commit-bot: I haz the power, 2017-02-15 20:44:27]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9b5618b8aa52feeccbb3203046b5...