Drag-and-drop across OOPIFs.

Drag-and-drop across OOPIFs.

This patch enables drag-and-drop to work across out-of-process iframes.

BUG=647249, 655063
Committed: https://crrev.com/90572ae460ace736685280a608590812abf6bc6a
Cr-Commit-Position: refs/heads/master@{#433068}

[paulmeyer, 2016-11-17 00:31:26]

Description was changed from

==========
Drag-and-drop across OOPIFs.

This Patch enables drag-and-drop to work across out-of-process iframes.

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

to

==========
Drag-and-drop across OOPIFs.

This patch enables drag-and-drop to work across out-of-process iframes.

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

[paulmeyer, 2016-11-17 00:52:50]

Patchset #1 (id:1) has been deleted

[paulmeyer, 2016-11-17 00:53:29]

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

[paulmeyer, 2016-11-17 00:53:30]

+dcheng@

[dcheng, 2016-11-17 18:02:58]

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:211: void
DragController::dragExited(DragData* dragData, LocalFrame* localRoot) {
LocalFrame& here and elsewhere, since it's never null

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:667: return false;
Nit: newline after this.

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.h (right):

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.h:90: DragOperation
operationForLoad(DragData*, LocalFrame* localRoot);
Let's be consistent about naming (or not naming this parameter). Probably we
should name it, since it's implied that it's the local root?

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

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:76:
page()->dragController().dragEnded();
Why does this need to call dragEnded()?

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:112:
page()->dragController().dragEnded();
Why does this need to call dragEnded() now?

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:186: return
static_cast<WebLocalFrameImpl*>(localRoot())->frame();
toCoreFrame(localRoot())

[paulmeyer, 2016-11-17 20:07:37]

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

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

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-17 20:22:25]

PTAL

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:211: void
DragController::dragExited(DragData* dragData, LocalFrame* localRoot) {
On 2016/11/17 18:02:58, dcheng wrote:
> LocalFrame& here and elsewhere, since it's never null

Done.

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:667: return false;
On 2016/11/17 18:02:58, dcheng wrote:
> Nit: newline after this.

Done.

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.h (right):

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.h:90: DragOperation
operationForLoad(DragData*, LocalFrame* localRoot);
On 2016/11/17 18:02:58, dcheng wrote:
> Let's be consistent about naming (or not naming this parameter). Probably we
> should name it, since it's implied that it's the local root?

Done.

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

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:76:
page()->dragController().dragEnded();
On 2016/11/17 18:02:58, dcheng wrote:
> Why does this need to call dragEnded()?

Because the state needs to be cleared in DragController. Specifically, there is
a test that fails if the DragCaretController is not cleared (done from
DragController).

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:112:
page()->dragController().dragEnded();
On 2016/11/17 18:02:58, dcheng wrote:
> Why does this need to call dragEnded() now?

See above comment.

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:186: return
static_cast<WebLocalFrameImpl*>(localRoot())->frame();
On 2016/11/17 18:02:58, dcheng wrote:
> toCoreFrame(localRoot())

Done.

[dcheng, 2016-11-17 20:30:29]

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

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:112:
page()->dragController().dragEnded();
On 2016/11/17 20:22:25, paulmeyer wrote:
> On 2016/11/17 18:02:58, dcheng wrote:
> > Why does this need to call dragEnded() now?
> 
> See above comment.

How come this Just Worked before?

https://codereview.chromium.org/2508013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/2508013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:536: // TODO: Isn't
|m_page->dragController()| the same as |this|?
Nit: TODO(username) or TODO(crbug.com/XXX)

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

https://codereview.chromium.org/2508013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:45: // corrected,
"DCHECK(!m_currentDragData);" should be added here.
What's causing this to be called twice?

https://codereview.chromium.org/2508013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:188: return
toWebLocalFrameImpl(localRoot())->frame();
Nit: the naming here is a bit confusing, we hvae both localRoot() and
localFrameRoot() (and they both return frames)

I would probably just add a static helper at the top for going from
WebLocalFrame to core frame: you can add a TODO(dcheng) to move this to
WebLocalFrame.h.

[paulmeyer, 2016-11-17 21:12:29]

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

[paulmeyer, 2016-11-17 21:13:17]

Patchset #4 (id:80001) has been deleted

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

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-17 21:14:45]

PTAL

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

https://codereview.chromium.org/2508013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:112:
page()->dragController().dragEnded();
On 2016/11/17 20:30:29, dcheng wrote:
> On 2016/11/17 20:22:25, paulmeyer wrote:
> > On 2016/11/17 18:02:58, dcheng wrote:
> > > Why does this need to call dragEnded() now?
> > 
> > See above comment.
> 
> How come this Just Worked before?

To be honest, I'm not entirely sure. That is, I don't know why the test didn't
fail before (and did now, without dragEnded() here). Lucas and I both looked
into this for multiple hours, and we couldn't figure it out.

https://codereview.chromium.org/2508013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/2508013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:536: // TODO: Isn't
|m_page->dragController()| the same as |this|?
On 2016/11/17 20:30:29, dcheng wrote:
> Nit: TODO(username) or TODO(crbug.com/XXX)

Done.

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

https://codereview.chromium.org/2508013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:45: // corrected,
"DCHECK(!m_currentDragData);" should be added here.
On 2016/11/17 20:30:29, dcheng wrote:
> What's causing this to be called twice?

Sorry, that's not true anymore. This TODO was in here before, and I must have
accidentally pulled this back in.

https://codereview.chromium.org/2508013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:188: return
toWebLocalFrameImpl(localRoot())->frame();
On 2016/11/17 20:30:29, dcheng wrote:
> Nit: the naming here is a bit confusing, we hvae both localRoot() and
> localFrameRoot() (and they both return frames)
> 
> I would probably just add a static helper at the top for going from
> WebLocalFrame to core frame: you can add a TODO(dcheng) to move this to
> WebLocalFrame.h.

I had named it in line with the function with the same name in LocalFrame, but I
do agree that it is confusing when combined with the exiting localFrame()
function.

I will use a static helper.

[paulmeyer, 2016-11-17 21:16:35]

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

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

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

[dcheng, 2016-11-17 21:58:13]

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

https://codereview.chromium.org/2508013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:122:
page()->dragController().dragEnded();
I'm having trouble understanding why the dragEnded() calls are needed: it makes
sense that we need to clear m_doingDragAndDrop, but I'm having trouble following
why we need this last bit too. This should really only be called on the source
RWH.

[commit-bot: I haz the power, 2016-11-17 23:05:38]

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

[commit-bot: I haz the power, 2016-11-17 23:05:39]

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

[Charlie Reis, 2016-11-17 23:38:53]

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

[commit-bot: I haz the power, 2016-11-17 23:39: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-18 01:20:48]

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

[commit-bot: I haz the power, 2016-11-18 01:22:52]

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-18 01:25:54]

PTAL

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

https://codereview.chromium.org/2508013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetBase.cpp:122:
page()->dragController().dragEnded();
On 2016/11/17 21:58:13, dcheng wrote:
> I'm having trouble understanding why the dragEnded() calls are needed: it
makes
> sense that we need to clear m_doingDragAndDrop, but I'm having trouble
following
> why we need this last bit too. This should really only be called on the source
> RWH.

Okay, I was able to remove this.

[dcheng, 2016-11-18 01:38:25]

lgtm

[paulmeyer, 2016-11-18 01:39:24]

The CQ bit was unchecked by paulmeyer@chromium.org

[paulmeyer, 2016-11-18 01:39:25]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2016-11-18 01:39:57]

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-18 03:42:50]

Description was changed from

==========
Drag-and-drop across OOPIFs.

This patch enables drag-and-drop to work across out-of-process iframes.

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

to

==========
Drag-and-drop across OOPIFs.

This patch enables drag-and-drop to work across out-of-process iframes.

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

[commit-bot: I haz the power, 2016-11-18 03:42:51]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-11-18 03:45:44]

Description was changed from

==========
Drag-and-drop across OOPIFs.

This patch enables drag-and-drop to work across out-of-process iframes.

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

to

==========
Drag-and-drop across OOPIFs.

This patch enables drag-and-drop to work across out-of-process iframes.

BUG=647249, 655063

Committed: https://crrev.com/90572ae460ace736685280a608590812abf6bc6a
Cr-Commit-Position: refs/heads/master@{#433068}
==========

[commit-bot: I haz the power, 2016-11-18 03:45:46]

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