Drag-and-drop: DragEnter, DragOver, DragLeave, DragDrop

Drag-and-drop: Move DragEnter, DragOver, DragLeave, and DragDrop 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
Committed: https://crrev.com/90f6c31d8095307a275eec336423e749ae6af956
Cr-Commit-Position: refs/heads/master@{#431692}

[paulmeyer, 2016-11-07 22:36:14]

Description was changed from

==========
Drag-and-drop: Move DragEnter, DragOver, DragLeave, and DragDrop 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 DragEnter, DragOver, DragLeave, and DragDrop 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
==========

[paulmeyer, 2016-11-07 22:43:54]

Patchset #1 (id:1) has been deleted

[paulmeyer, 2016-11-08 20:51:51]

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

[paulmeyer, 2016-11-08 20:51:52]

+dcheng@ for general review.

[dcheng, 2016-11-09 20:23:17]

https://codereview.chromium.org/2485693003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2485693003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1807: WebFrameWidget* widget
= static_cast<WebFrameWidget*>(webView->widget());
Is it possible to just go through the main frame's WebFrameWidget?

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

https://codereview.chromium.org/2485693003/diff/80001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:827:
host->GetWidget()->DragTargetDragEnter(filtered_data, location, location,
Is it worth adding a TODO here? Presumably we'll have to make this hit test at
some point.

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

https://codereview.chromium.org/2485693003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:84: #include
"ui/base/clipboard/clipboard.h"
I think the corresponding includes here can be cleaned up in
render_view_host_impl.cc

https://codereview.chromium.org/2485693003/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2485693003/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1153:
web_contents_->GetRenderViewHost()->GetWidget()->DragTargetDragOver(
Ditto: worth annotating this with a TODO?

https://codereview.chromium.org/2485693003/diff/80001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/2485693003/diff/80001/content/public/browser/...
content/public/browser/render_view_host.h:119: virtual void
FilterDropData(DropData* drop_data) = 0;
Does it make sense to move this somewhere else?

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
content/renderer/render_view_impl.cc:1967: return
RenderWidget::ConvertWindowPointToViewport(point);
Do we still need to forward this, or can the callers just use RW directly?

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
content/renderer/render_widget.cc:65: #include
"third_party/WebKit/public/platform/WebDragOperation.h"
Can we clean up the corresponding forward declares / using / etc in
render_view_impl.cc?

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
content/renderer/render_widget.cc:1672: if (!GetWebWidget())
How come this needs to do null checks, but RenderView didn't need to?

https://codereview.chromium.org/2485693003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetBase.cpp (right):

https://codereview.chromium.org/2485693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:79: WebDragOperationNone) {
 // IPC RACE CONDITION: do not allow this drop.
Nit: let's put this comment on the previous line, to avoid the funny wrapping.

[paulmeyer, 2016-11-09 23:19:11]

PTAL

https://codereview.chromium.org/2485693003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2485693003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1807: WebFrameWidget* widget
= static_cast<WebFrameWidget*>(webView->widget());
On 2016/11/09 20:23:16, dcheng wrote:
> Is it possible to just go through the main frame's WebFrameWidget?

Yes, done.

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

https://codereview.chromium.org/2485693003/diff/80001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:827:
host->GetWidget()->DragTargetDragEnter(filtered_data, location, location,
On 2016/11/09 20:23:16, dcheng wrote:
> Is it worth adding a TODO here? Presumably we'll have to make this hit test at
> some point.

Done.

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

https://codereview.chromium.org/2485693003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:84: #include
"ui/base/clipboard/clipboard.h"
On 2016/11/09 20:23:17, dcheng wrote:
> I think the corresponding includes here can be cleaned up in
> render_view_host_impl.cc

Done.

https://codereview.chromium.org/2485693003/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2485693003/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1153:
web_contents_->GetRenderViewHost()->GetWidget()->DragTargetDragOver(
On 2016/11/09 20:23:17, dcheng wrote:
> Ditto: worth annotating this with a TODO?

Done.

https://codereview.chromium.org/2485693003/diff/80001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/2485693003/diff/80001/content/public/browser/...
content/public/browser/render_view_host.h:119: virtual void
FilterDropData(DropData* drop_data) = 0;
On 2016/11/09 20:23:17, dcheng wrote:
> Does it make sense to move this somewhere else?

Yes, I'll move it into RenderWidgetHost now.

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
content/renderer/render_view_impl.cc:1967: return
RenderWidget::ConvertWindowPointToViewport(point);
On 2016/11/09 20:23:17, dcheng wrote:
> Do we still need to forward this, or can the callers just use RW directly?

Nope, fixed.

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
content/renderer/render_widget.cc:65: #include
"third_party/WebKit/public/platform/WebDragOperation.h"
On 2016/11/09 20:23:17, dcheng wrote:
> Can we clean up the corresponding forward declares / using / etc in
> render_view_impl.cc?

Done.

https://codereview.chromium.org/2485693003/diff/80001/content/renderer/render...
content/renderer/render_widget.cc:1672: if (!GetWebWidget())
On 2016/11/09 20:23:17, dcheng wrote:
> How come this needs to do null checks, but RenderView didn't need to?

It seems to be possible for this message to come during shutdown, and should be
ignored in that case.

https://codereview.chromium.org/2485693003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetBase.cpp (right):

https://codereview.chromium.org/2485693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:79: WebDragOperationNone) {
 // IPC RACE CONDITION: do not allow this drop.
On 2016/11/09 20:23:17, dcheng wrote:
> Nit: let's put this comment on the previous line, to avoid the funny wrapping.

Done.

[dcheng, 2016-11-10 07:16:21]

lgtm

https://codereview.chromium.org/2485693003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetBase.cpp (right):

https://codereview.chromium.org/2485693003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:69:
WebViewImpl::UserGestureNotifier notifier(view());
It's not clear to me that this is right, but I guess it's something we'll have
to resolve with the other UGI work.

[paulmeyer, 2016-11-10 14:29:48]

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

[paulmeyer, 2016-11-10 14:29:48]

+nasko for OWNER review of content/

[paulmeyer, 2016-11-10 14:30:13]

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

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

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-10 14:43:40]

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

[commit-bot: I haz the power, 2016-11-10 14:43:41]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[nasko, 2016-11-10 17:33:16]

content/ looks good, but there are compile failures in unit tests. Some small
comments.

https://codereview.chromium.org/2485693003/diff/100001/content/browser/browse...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2485693003/diff/100001/content/browser/browse...
content/browser/browser_plugin/browser_plugin_guest.cc:825: // RenderWidgetHost
to work with OOPIFs.
nit: Put a "See https://crbug.com/xxxxxx." so we can easily track all of the
TODOs for drag-n-drop.

https://codereview.chromium.org/2485693003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/2485693003/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:154:
std::vector<DropData::Metadata> DropDataToMetaData(const DropData& drop_data) {
This method can be removed, right? I don't see any code left calling it and you
copied it over to the RenderWidgetHostImpl.

https://codereview.chromium.org/2485693003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/2485693003/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.h:241: protected:
Incorrect indent. Editor being too aggressive?

https://codereview.chromium.org/2485693003/diff/100001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2485693003/diff/100001/content/public/browser...
content/public/browser/render_widget_host.h:281: // Filters drop data before it
is passed to RenderWidgetHostImpl.
nit: Since this is content/public, drop the Impl part, fine to keep it as
RenderWidgetHost.

[paulmeyer, 2016-11-10 21:32:22]

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

[commit-bot: I haz the power, 2016-11-10 21:32:54]

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-10 21:37:00]

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

[commit-bot: I haz the power, 2016-11-10 21:37:01]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[paulmeyer, 2016-11-11 01:18:03]

Patchset #6 (id:120001) has been deleted

[paulmeyer, 2016-11-11 14:36:33]

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

[commit-bot: I haz the power, 2016-11-11 14:36:40]

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-11 14:49:24]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[paulmeyer, 2016-11-11 18:17:59]

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

[paulmeyer, 2016-11-11 18:18:17]

Patchset #6 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-11-11 18:18: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-11 18:33:29]

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

[commit-bot: I haz the power, 2016-11-11 18:33:30]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[paulmeyer, 2016-11-11 21:36:24]

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

[commit-bot: I haz the power, 2016-11-11 21:36:57]

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-11 21:56:29]

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

[commit-bot: I haz the power, 2016-11-11 21:56:30]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[paulmeyer, 2016-11-11 22:05:29]

Patchset #7 (id:180001) has been deleted

[paulmeyer, 2016-11-11 22:06:01]

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

[commit-bot: I haz the power, 2016-11-11 22:06:54]

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

[paulmeyer, 2016-11-11 23:26:49]

https://codereview.chromium.org/2485693003/diff/100001/content/browser/browse...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/2485693003/diff/100001/content/browser/browse...
content/browser/browser_plugin/browser_plugin_guest.cc:825: // RenderWidgetHost
to work with OOPIFs.
On 2016/11/10 17:33:15, nasko wrote:
> nit: Put a "See https://crbug.com/xxxxxx.%22 so we can easily track all of the
> TODOs for drag-n-drop.

Done.

https://codereview.chromium.org/2485693003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/2485693003/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:154:
std::vector<DropData::Metadata> DropDataToMetaData(const DropData& drop_data) {
On 2016/11/10 17:33:15, nasko wrote:
> This method can be removed, right? I don't see any code left calling it and
you
> copied it over to the RenderWidgetHostImpl.

Done.

https://codereview.chromium.org/2485693003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/2485693003/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.h:241: protected:
On 2016/11/10 17:33:15, nasko wrote:
> Incorrect indent. Editor being too aggressive?

Wow, it was. I'll have to find a way to correct that part of the auto-indent.

https://codereview.chromium.org/2485693003/diff/100001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2485693003/diff/100001/content/public/browser...
content/public/browser/render_widget_host.h:281: // Filters drop data before it
is passed to RenderWidgetHostImpl.
On 2016/11/10 17:33:16, nasko wrote:
> nit: Since this is content/public, drop the Impl part, fine to keep it as
> RenderWidgetHost.

Done.

https://codereview.chromium.org/2485693003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetBase.cpp (right):

https://codereview.chromium.org/2485693003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:69:
WebViewImpl::UserGestureNotifier notifier(view());
On 2016/11/10 07:16:21, dcheng wrote:
> It's not clear to me that this is right, but I guess it's something we'll have
> to resolve with the other UGI work.

Acknowledged.

[dpapad, 2016-11-11 23:39:30]

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

[dpapad, 2016-11-11 23:39:31]

LGTM for browser/ui/webui/webui_webview_browsertest.cc

[commit-bot: I haz the power, 2016-11-11 23:43:18]

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

[commit-bot: I haz the power, 2016-11-11 23:43:20]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-11-11 23:46:01]

content/ LGTM!

[dmazzoni, 2016-11-11 23:59:37]

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

[dmazzoni, 2016-11-11 23:59:38]

components/test_runner lgtm

[paulmeyer, 2016-11-12 00:04:10]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2016-11-12 00:04:11]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2485693003/#ps200001
(title: "Rebased. More fixes.")

[commit-bot: I haz the power, 2016-11-12 00:04:48]

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-12 00:18:37]

Description was changed from

==========
Drag-and-drop: Move DragEnter, DragOver, DragLeave, and DragDrop 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 DragEnter, DragOver, DragLeave, and DragDrop 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
==========

[commit-bot: I haz the power, 2016-11-12 00:18:40]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2016-11-12 00:23:38]

Description was changed from

==========
Drag-and-drop: Move DragEnter, DragOver, DragLeave, and DragDrop 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 DragEnter, DragOver, DragLeave, and DragDrop 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

Committed: https://crrev.com/90f6c31d8095307a275eec336423e749ae6af956
Cr-Commit-Position: refs/heads/master@{#431692}
==========

[commit-bot: I haz the power, 2016-11-12 00:23:39]

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