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

Drag-and-drop: Move startDrag 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
TBR=tommycli@chromium.org

Committed: https://crrev.com/6ef5a7989fc1b10c8b642863b74714585ebe845d
Cr-Commit-Position: refs/heads/master@{#430701}

[paulmeyer, 2016-11-02 16:06:06]

Description was changed from

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

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

to

==========
Drag-and-drop: Move startDrag 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-02 16:06:33]

Description was changed from

==========
Drag-and-drop: Move startDrag 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 startDrag 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-02 17:18:31]

Patchset #1 (id:1) has been deleted

[paulmeyer, 2016-11-02 17:20:47]

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

[commit-bot: I haz the power, 2016-11-02 17:21:27]

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-02 17:21:43]

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

[paulmeyer, 2016-11-02 17:21:44]

+dcheng@

[commit-bot: I haz the power, 2016-11-02 19:33:02]

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

[commit-bot: I haz the power, 2016-11-02 19:33:03]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-11-03 21:22:52]

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

https://codereview.chromium.org/2475443003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.cc:76: return nullptr;
Can you help me understand what this is for?

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

https://codereview.chromium.org/2475443003/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:1284:
GetWidget()->set_possible_drag_event_info(
Does this still need to go through the RenderWidgetOwnerDelegate? Maybe this can
just all be moved into RenderWidget now?

https://codereview.chromium.org/2475443003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2475443003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.h:509: bool m_doingDragAndDrop;
Let's add a setter for this instead of directly manipulating it from another
class.

https://codereview.chromium.org/2475443003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/2475443003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidgetClient.h:189: virtual void
startDragging(WebLocalFrame*,
Can we move this to WebFrameWidget?

[Charlie Reis, 2016-11-03 21:25:17]

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

[Charlie Reis, 2016-11-03 21:25:17]

[Drive by: Please CC site-isolation-reviews@ for OOPIF related CLs.  Thanks!]

[Charlie Reis, 2016-11-03 21:25:25]

creis@chromium.org changed reviewers:
- creis@chromium.org

[paulmeyer, 2016-11-04 19:01:18]

PTAL.

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

https://codereview.chromium.org/2475443003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.cc:76: return nullptr;
On 2016/11/03 21:22:52, dcheng wrote:
> Can you help me understand what this is for?

This allows RenderWidgetHostImpl to access it's RenderViewHostImpl via
WebContentsImpl::GetRenderViewHost().

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

https://codereview.chromium.org/2475443003/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:1284:
GetWidget()->set_possible_drag_event_info(
On 2016/11/03 21:22:52, dcheng wrote:
> Does this still need to go through the RenderWidgetOwnerDelegate? Maybe this
can
> just all be moved into RenderWidget now?

Done.

https://codereview.chromium.org/2475443003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2475443003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.h:509: bool m_doingDragAndDrop;
On 2016/11/03 21:22:52, dcheng wrote:
> Let's add a setter for this instead of directly manipulating it from another
> class.

Done.

https://codereview.chromium.org/2475443003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/2475443003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidgetClient.h:189: virtual void
startDragging(WebLocalFrame*,
On 2016/11/03 21:22:52, dcheng wrote:
> Can we move this to WebFrameWidget?

As we discussed offline, |frame| no longer plumbed through here.

[dcheng, 2016-11-07 21:11:00]

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

https://codereview.chromium.org/2475443003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.cc:76: return nullptr;
On 2016/11/04 19:01:17, paulmeyer wrote:
> On 2016/11/03 21:22:52, dcheng wrote:
> > Can you help me understand what this is for?
> 
> This allows RenderWidgetHostImpl to access it's RenderViewHostImpl via
> WebContentsImpl::GetRenderViewHost().

Is this something we'll be able to clean up in followup patches?

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

https://codereview.chromium.org/2475443003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1394: for
(std::vector<ui::FileInfo>::const_iterator it =
Nit: mind changing this to a range-based for loop while you're here? =)

https://codereview.chromium.org/2475443003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2475443003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:265: if (!frameWidget ||
!frameWidget->client())
Nit: do we need to null-check client or widget here? I'd like to understand how
we can get to this code with either of these being null.

https://codereview.chromium.org/2475443003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/2475443003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidgetClient.h:189: // Called when a
drag-n-drop operation should begin.
Nit: drag-and-drop

[paulmeyer, 2016-11-07 22:18:13]

PTAL.

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

https://codereview.chromium.org/2475443003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.cc:76: return nullptr;
On 2016/11/07 21:11:00, dcheng wrote:
> On 2016/11/04 19:01:17, paulmeyer wrote:
> > On 2016/11/03 21:22:52, dcheng wrote:
> > > Can you help me understand what this is for?
> > 
> > This allows RenderWidgetHostImpl to access it's RenderViewHostImpl via
> > WebContentsImpl::GetRenderViewHost().
> 
> Is this something we'll be able to clean up in followup patches?

It could be cleaned up if drag-and-drop code is moved out of
WebContentsViewAura, though I think that would only be done as a clean-up
refactor after getting drag-and-drop working in the first place.

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

https://codereview.chromium.org/2475443003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1394: for
(std::vector<ui::FileInfo>::const_iterator it =
On 2016/11/07 21:11:00, dcheng wrote:
> Nit: mind changing this to a range-based for loop while you're here? =)

Done.

https://codereview.chromium.org/2475443003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2475443003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:265: if (!frameWidget ||
!frameWidget->client())
On 2016/11/07 21:11:00, dcheng wrote:
> Nit: do we need to null-check client or widget here? I'd like to understand
how
> we can get to this code with either of these being null.

Probably not. Removed.

https://codereview.chromium.org/2475443003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/2475443003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidgetClient.h:189: // Called when a
drag-n-drop operation should begin.
On 2016/11/07 21:11:00, dcheng wrote:
> Nit: drag-and-drop

Done.

[dcheng, 2016-11-07 22:21:54]

LGTM

[paulmeyer, 2016-11-07 22:22:51]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2016-11-07 22:23:40]

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-07 22:33:22]

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

[commit-bot: I haz the power, 2016-11-07 22:33:23]

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...)

[paulmeyer, 2016-11-07 23:30:04]

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

[paulmeyer, 2016-11-07 23:30:04]

+jochen@ for OWNER review of content/ and components/

[paulmeyer, 2016-11-07 23:35:21]

paulmeyer@chromium.org changed reviewers:
- jochen@chromium.org

[paulmeyer, 2016-11-07 23:39:26]

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

[paulmeyer, 2016-11-07 23:39:27]

+rbyers@ for OWNER review of components/test_runner

[paulmeyer, 2016-11-07 23:41:00]

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

[paulmeyer, 2016-11-07 23:41:01]

+tommycli@ for OWNER review of webview_plugin

[nasko, 2016-11-07 23:44:26]

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

[nasko, 2016-11-07 23:44:27]

content/ LGTM.

https://codereview.chromium.org/2475443003/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/2475443003/diff/60001/content/renderer/render...
content/renderer/render_view_impl.h:83: class WebPeerConnection00HandlerClient;
Are those part of the removed header?

[paulmeyer, 2016-11-08 17:07:29]

https://codereview.chromium.org/2475443003/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/2475443003/diff/60001/content/renderer/render...
content/renderer/render_view_impl.h:83: class WebPeerConnection00HandlerClient;
On 2016/11/07 23:44:27, nasko wrote:
> Are those part of the removed header?

Actually, I don't know where those came from. Removed.

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

Thanks for fixing the delegate interface! Even more LGTM!

[Rick Byers, 2016-11-08 17:14:22]

On 2016/11/07 23:39:27, paulmeyer wrote:
> +rbyers@ for OWNER review of components/test_runner

test_runner LGTM

[paulmeyer, 2016-11-08 18:51:35]

paulmeyer@chromium.org changed reviewers:
- tommycli@chromium.org

[paulmeyer, 2016-11-08 18:51:36]

-tommycli (looks like OOO)

[paulmeyer, 2016-11-08 18:55:13]

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

[commit-bot: I haz the power, 2016-11-08 18:55:41]

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-08 18:59:24]

Description was changed from

==========
Drag-and-drop: Move startDrag 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 startDrag 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

TBR=tommycli@chromium.org
==========

[commit-bot: I haz the power, 2016-11-08 20:20:06]

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

[commit-bot: I haz the power, 2016-11-08 20:20:07]

Dry run: This issue passed the CQ dry run.

[paulmeyer, 2016-11-08 20:25:55]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2016-11-08 20:25:56]

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/2475443003/#ps100001
(title: "Removed unneeded declarations.")

[commit-bot: I haz the power, 2016-11-08 20:26:24]

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-08 20:34:27]

Description was changed from

==========
Drag-and-drop: Move startDrag 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

TBR=tommycli@chromium.org
==========

to

==========
Drag-and-drop: Move startDrag 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

TBR=tommycli@chromium.org
==========

[commit-bot: I haz the power, 2016-11-08 20:34:30]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-11-08 20:41:38]

Description was changed from

==========
Drag-and-drop: Move startDrag 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

TBR=tommycli@chromium.org
==========

to

==========
Drag-and-drop: Move startDrag 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

TBR=tommycli@chromium.org

Committed: https://crrev.com/6ef5a7989fc1b10c8b642863b74714585ebe845d
Cr-Commit-Position: refs/heads/master@{#430701}
==========

[commit-bot: I haz the power, 2016-11-08 20:41:39]

Patchset 5 (id:??) landed as
https://crrev.com/6ef5a7989fc1b10c8b642863b74714585ebe845d
Cr-Commit-Position: refs/heads/master@{#430701}

[tommycli, 2016-11-14 18:41:52]

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

[tommycli, 2016-11-14 18:41:53]

webview_plugin lgtm thanks