Drag-and-drop: updateDragCursor

Drag-and-drop: Move updateDragCursor to RenderWidgetHost.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7f6f1d4f12b96484ad238b5b0be7fd31d89e6906
Cr-Commit-Position: refs/heads/master@{#431997}

[paulmeyer, 2016-11-14 16:33:52]

Description was changed from

==========
Drag-and-drop: Move updateDragCursor out of WebView/RenderView.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
==========

to

==========
Drag-and-drop: Move updateDragCursor out of WebView/RenderView.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[paulmeyer, 2016-11-14 16:40:08]

Description was changed from

==========
Drag-and-drop: Move updateDragCursor out of WebView/RenderView.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Drag-and-drop: Move updateDragCursor out of WebView/RenderView.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[paulmeyer, 2016-11-14 16:40:37]

Description was changed from

==========
Drag-and-drop: Move updateDragCursor out of WebView/RenderView.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Drag-and-drop: Move updateDragCursor to RenderWidgetHost.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[paulmeyer, 2016-11-14 16:57:02]

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

[commit-bot: I haz the power, 2016-11-14 16:57:35]

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, 2016-11-14 18:16:48]

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

[commit-bot: I haz the power, 2016-11-14 18:16:49]

Dry run: This issue passed the CQ dry run.

[paulmeyer, 2016-11-14 18:35:06]

paulmeyer@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2016-11-14 20:23:45]

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

https://codereview.chromium.org/2503563002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:222: const
IPC::Message& message);
I think it's probably better to add 1 call to RenderWidgetHostDelegate for each
message we need to migrate. Basically, handle the message in the RWH message
dispatcher, and forward it to the webcontents via a virtual method in
RenderWidgetHostDelegate.

There's two reasons why I think this. First, the OnMessageReceived pattern is
not mojo compatible (in mojo we won't be able to pass around a polymorphic
IPC::Message-like object). Since we'll have to eventually move to
one-delegate-method-per-IPC anyway, we might as well do it now.

Second, I can't easily assess the risk of forwarding all widget messages from
RenderWidget to WebContents. For popup and fullscreen widgets, it's possible
that this will have unintended consequences; if there's a message that
WebContents handles on behalf of its RenderViewHosts, that Widget happens also
to send/handle, and now WebContents will start handling them.

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

https://codereview.chromium.org/2503563002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:552:
IPC_MESSAGE_HANDLER(DragHostMsg_UpdateDragCursor, OnUpdateDragCursor)
Seems like this message is actually handled by a WebContentsObserver in some
cases, but it's expecting it to arrive via the RFH:

https://cs.chromium.org/chromium/src/content/browser/browser_plugin/browser_p...

https://codereview.chromium.org/2503563002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2503563002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:688: if
(observer.OnMessageReceived(message))
Do we really want/need to do this for all widget messages?

https://codereview.chromium.org/2503563002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:699:
render_view_message_source_ = render_view_host;
Would we need render_widget_message_source_ too (This pattern is awful)?

[paulmeyer, 2016-11-14 22:03:53]

PTAL.

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

https://codereview.chromium.org/2503563002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:222: const
IPC::Message& message);
On 2016/11/14 20:23:45, ncarter wrote:
> I think it's probably better to add 1 call to RenderWidgetHostDelegate for
each
> message we need to migrate. Basically, handle the message in the RWH message
> dispatcher, and forward it to the webcontents via a virtual method in
> RenderWidgetHostDelegate.
> 
> There's two reasons why I think this. First, the OnMessageReceived pattern is
> not mojo compatible (in mojo we won't be able to pass around a polymorphic
> IPC::Message-like object). Since we'll have to eventually move to
> one-delegate-method-per-IPC anyway, we might as well do it now.
> 
> Second, I can't easily assess the risk of forwarding all widget messages from
> RenderWidget to WebContents. For popup and fullscreen widgets, it's possible
> that this will have unintended consequences; if there's a message that
> WebContents handles on behalf of its RenderViewHosts, that Widget happens also
> to send/handle, and now WebContents will start handling them.

Okay, that sounds very reasonable. Done.

https://codereview.chromium.org/2503563002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2503563002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:688: if
(observer.OnMessageReceived(message))
On 2016/11/14 20:23:45, ncarter wrote:
> Do we really want/need to do this for all widget messages?

No, I've removed the OnMessageReceived() for widgets now.

https://codereview.chromium.org/2503563002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:699:
render_view_message_source_ = render_view_host;
On 2016/11/14 20:23:45, ncarter wrote:
> Would we need render_widget_message_source_ too (This pattern is awful)?

Not anymore, I've removed the OnMessageReceived() for widgets now. (yeah...)

[paulmeyer, 2016-11-14 22:05:08]

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

[commit-bot: I haz the power, 2016-11-14 22:08:04]

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

[ncarter (slow), 2016-11-14 22:09:31]

lgtm

thanks for the quick turnaround

[paulmeyer, 2016-11-14 22:12:32]

On 2016/11/14 22:09:31, ncarter wrote:
> lgtm
> 
> thanks for the quick turnaround

Thank YOU for the quick turnaround. :)

[paulmeyer, 2016-11-14 22:12:41]

The CQ bit was unchecked by paulmeyer@chromium.org

[paulmeyer, 2016-11-14 22:12:46]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2016-11-14 22:14:27]

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, 2016-11-15 00:01:12]

Description was changed from

==========
Drag-and-drop: Move updateDragCursor to RenderWidgetHost.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Drag-and-drop: Move updateDragCursor to RenderWidgetHost.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-15 00:01:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-15 00:07:22]

Description was changed from

==========
Drag-and-drop: Move updateDragCursor to RenderWidgetHost.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Drag-and-drop: Move updateDragCursor to RenderWidgetHost.

This patch is part of the effort to transition D&D functions to all be
widget-based instead of view-based. This is necessary in order to allow
D&D to work with out-of-process iframes.

BUG=647249, 655063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7f6f1d4f12b96484ad238b5b0be7fd31d89e6906
Cr-Commit-Position: refs/heads/master@{#431997}
==========

[commit-bot: I haz the power, 2016-11-15 00:07:23]

Patchset 2 (id:??) landed as
https://crrev.com/7f6f1d4f12b96484ad238b5b0be7fd31d89e6906
Cr-Commit-Position: refs/heads/master@{#431997}

[kenrb, 2016-11-16 16:22:15]

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

[kenrb, 2016-11-16 16:22:16]

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

https://codereview.chromium.org/2503563002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1525:
view->UpdateDragCursor(current_op);
I have a question about this. Could this be racy if you are getting
DragHostMsg_UpdateDragCursor across different frames, and the renderers take
different amounts of time to get around to sending it?