Drag-and-drop: Target drag messages (the sequel).

Drag-and-drop: Target drag messages (the sequel).

This patch follows up on https://codereview.chromium.org/2505113002/,
completing drag-and-drop targeting in a few remaining places, such as in
Mac-specific code as well as in WebContentsImpl::SystemDragEnded(). This
is part of an ongoing effort to allow D&D to work with out-of-process
iframes.

BUG=647249, 655063
R=avi@chromium.org, dcheng@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/0c58c371878e696f904efd8e13886b23abd41eea

[paulmeyer, 2016-11-17 00:46:17]

Description was changed from

==========
Drag-and-drop: Target drag messages (the sequel).

This patch follows up on https://codereview.chromium.org/2505113002/,
completing drag-and-drop targeting in a few remaining places, such as in
Mac-specific code as well as in WebContentsImpl::SystemDragEnded(). This
is part of an ongoing effort to allow D&D to work with out-of-process
iframes.

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

to

==========
Drag-and-drop: Target drag messages (the sequel).

This patch follows up on https://codereview.chromium.org/2505113002/,
completing drag-and-drop targeting in a few remaining places, such as in
Mac-specific code as well as in WebContentsImpl::SystemDragEnded(). This
is part of an ongoing effort to allow D&D to work with out-of-process
iframes.

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

[paulmeyer, 2016-11-17 00:47:04]

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

[paulmeyer, 2016-11-17 00:49:48]

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

[commit-bot: I haz the power, 2016-11-17 00:51:09]

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

[Charlie Reis, 2016-11-17 00:53:11]

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

[Charlie Reis, 2016-11-17 00:53:11]

Adding Avi as a possible reviewer in case he gets a chance to look before Nick,
especially since for the Mac code.

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

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

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

Dry run: Try jobs failed on following builders:
  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-17 02:03:13]

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

[commit-bot: I haz the power, 2016-11-17 02:04:34]

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

[Avi (use Gerrit), 2016-11-17 02:50:28]

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_mac.mm:560:
dragOperationMask:operationMask]);
Why the indent?

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:252: gfx::Point(viewPoint.x,
viewPoint.y), &transformedPt);
You do this three times; can you abstract it out?

[commit-bot: I haz the power, 2016-11-17 04:07:34]

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

[commit-bot: I haz the power, 2016-11-17 04:07:35]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)

[paulmeyer, 2016-11-17 16:01:52]

PTAL

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_mac.mm:560:
dragOperationMask:operationMask]);
On 2016/11/17 02:50:28, Avi wrote:
> Why the indent?

Not intentional. Removed.

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:252: gfx::Point(viewPoint.x,
viewPoint.y), &transformedPt);
On 2016/11/17 02:50:28, Avi wrote:
> You do this three times; can you abstract it out?

Done.

[paulmeyer, 2016-11-17 16:27:34]

Patchset #5 (id:80001) has been deleted

[paulmeyer, 2016-11-17 16:48:24]

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_mac.mm:560:
dragOperationMask:operationMask]);
On 2016/11/17 16:01:52, paulmeyer wrote:
> On 2016/11/17 02:50:28, Avi wrote:
> > Why the indent?
> 
> Not intentional. Removed.

Nvm, git cl format insists on it. I think it actually is right because it wants
"dragOperationMask" to be indented 4 spaces in, which is correct.

[paulmeyer, 2016-11-17 17:31:53]

Patchset #4 (id:60001) has been deleted

[paulmeyer, 2016-11-17 17:32:05]

Patchset #4 (id:100001) has been deleted

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

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:252: gfx::Point(viewPoint.x,
viewPoint.y), &transformedPt);
On 2016/11/17 16:01:52, paulmeyer wrote:
> On 2016/11/17 02:50:28, Avi wrote:
> > You do this three times; can you abstract it out?
> 
> Done.

Actually, I'd like to do this abstraction in a followup CL because I need to
land this soon.

[dcheng, 2016-11-17 17:44:19]

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

[dcheng, 2016-11-17 17:44:20]

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3029: rwh->Send(new
DragMsg_SourceSystemDragEnded(rwh->GetRoutingID()));
We should definitely only be sending this to the drag source. The renderer
happens to guard against dragends that should be no-ops, but we shouldn't be
relying on that.

(Looking at this more, it seems like our DnD code is generally confused about
what it should be doing here: I think we're actually sending both
DragMsg_SourceEnded (which dispatches the dragend event) and
DragMsg_SourceSystemDragEnded in the normal DnD completion scenario... but it's
OK to fix that later)

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:148:
currentRWHForDrag_->FilterDropData(dropData.get());
This filters URLs from the drag, and the filtering might be different depending
on the RPH for the widget. This should be a TODO to fix.

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:213: gfx::Point(viewPoint.x,
viewPoint.y), &transformedPt);
This block feels like it should be a helper.

If this changes widgets, doesn't it also need to dispatch a dragenter to the new
RWH?

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:252: gfx::Point(viewPoint.x,
viewPoint.y), &transformedPt);
On 2016/11/17 17:32:11, paulmeyer wrote:
> On 2016/11/17 16:01:52, paulmeyer wrote:
> > On 2016/11/17 02:50:28, Avi wrote:
> > > You do this three times; can you abstract it out?
> > 
> > Done.
> 
> Actually, I'd like to do this abstraction in a followup CL because I need to
> land this soon.

We should abstract it out: as-is, this logic doesn't dispatch dragenter to the
newly entered render widget (which means the drag and drop code might just
explode)

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_drag_source_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_source_mac.mm:302:
dragStartRWH_->DragSourceEndedAt(
I think we should be plumbing the start RWH through to WebContents, and have
WebCOntents be the one responsible for sending IPCs.

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

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

[commit-bot: I haz the power, 2016-11-17 19:27:36]

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 19:31:53]

PTAL

[Avi (use Gerrit), 2016-11-17 19:39:23]

LGTM for when Daniel is happy.

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_view_mac.mm:560:
dragOperationMask:operationMask]);
On 2016/11/17 16:48:24, paulmeyer wrote:
> On 2016/11/17 16:01:52, paulmeyer wrote:
> > On 2016/11/17 02:50:28, Avi wrote:
> > > Why the indent?
> > 
> > Not intentional. Removed.
> 
> Nvm, git cl format insists on it. I think it actually is right because it
wants
> "dragOperationMask" to be indented 4 spaces in, which is correct.

Acknowledged.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_drag_dest_mac.h (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_dest_mac.h:16: class RenderViewHost;
v < w

[dcheng, 2016-11-17 19:59:02]

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2972: if (GetRenderViewHost())
{
Should this check source_rwh instead?

(My recollection is that this null check is required since a window can close
itself during drag)

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3028: if (GetRenderViewHost())
Ditto.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:565: if (drag_start_rwh_)
{
Let's centralize the null checks in WebContents, per the previous suggestions.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_dest_mac.mm:135: NSPoint screenPoint =
[self flipWindowPointToScreen:windowPoint view:view];
Can these three lines be moved into the helper as well? We seem to be doing the
same transforms.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_dest_mac.mm:146:
currentRWHForDrag_->FilterDropData(dropData.get());
I think this still needs a TODO: it means we pull the data from the pasteboard
multiple times per drag. Ideally, we'd only do it once, and filter it as
appropriate.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_dest_mac.mm:216: [self
draggingEntered:info view:view];
The various delegates for dragging can now be invoked multiple times per drag,
without the drag ever leaving the window. I'm concerned that this will confuse
things: for example, we potentially call delegate_->DragInitialize(),
delegate_->OnDragEnter(), and WebContentsDelegate::CanDragEnter() multiple times
without ever leaving the content area.

It might be OK to land this as-is, but I think it needs a TODO to fix.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_drag_source_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_source_mac.mm:276: if (dragStartRWH_) {
Let's centralize the null checking in WebCOntents.

[paulmeyer, 2016-11-17 20:39:21]

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:39:45]

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:46:43]

dcheng@: PTAL

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2972: if (GetRenderViewHost())
{
On 2016/11/17 19:59:01, dcheng wrote:
> Should this check source_rwh instead?
> 
> (My recollection is that this null check is required since a window can close
> itself during drag)

I thought about that. It was always GetRenderViewHost() before, but it does make
more sense to check |source_rwh| now. Done.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3028: if (GetRenderViewHost())
On 2016/11/17 19:59:01, dcheng wrote:
> Ditto.

Ditto done.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:565: if (drag_start_rwh_)
{
On 2016/11/17 19:59:01, dcheng wrote:
> Let's centralize the null checks in WebContents, per the previous suggestions.

Done.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_drag_dest_mac.h (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_dest_mac.h:16: class RenderViewHost;
On 2016/11/17 19:39:23, Avi wrote:
> v < w

Done.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_dest_mac.mm:135: NSPoint screenPoint =
[self flipWindowPointToScreen:windowPoint view:view];
On 2016/11/17 19:59:02, dcheng wrote:
> Can these three lines be moved into the helper as well? We seem to be doing
the
> same transforms.

Maybe... though I wasn't sure exactly how it should be done, so I'd rather
continue any of that cleanup in a separate CL because I want to get this landed
soon.

All of these lines were already in all of these functions before my change as
well.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_dest_mac.mm:146:
currentRWHForDrag_->FilterDropData(dropData.get());
On 2016/11/17 19:59:02, dcheng wrote:
> I think this still needs a TODO: it means we pull the data from the pasteboard
> multiple times per drag. Ideally, we'd only do it once, and filter it as
> appropriate.

Added TODO.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_dest_mac.mm:216: [self
draggingEntered:info view:view];
On 2016/11/17 19:59:02, dcheng wrote:
> The various delegates for dragging can now be invoked multiple times per drag,
> without the drag ever leaving the window. I'm concerned that this will confuse
> things: for example, we potentially call delegate_->DragInitialize(),
> delegate_->OnDragEnter(), and WebContentsDelegate::CanDragEnter() multiple
times
> without ever leaving the content area.
> 
> It might be OK to land this as-is, but I think it needs a TODO to fix.

Added TODO.

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_drag_source_mac.mm (right):

https://codereview.chromium.org/2509933002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_drag_source_mac.mm:276: if (dragStartRWH_) {
On 2016/11/17 19:59:02, dcheng wrote:
> Let's centralize the null checking in WebCOntents.

Done.

[ncarter (slow), 2016-11-17 21:11:24]

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

[dcheng, 2016-11-17 21:51:40]

LGTM, but let's make it a priority to clean up the logic for drag enter: there's
some redundancy there (we double-hit test for dragging over and dropping if we
changed RWHs) along with the duplicated delegate calls makes me pretty nervous.

[paulmeyer, 2016-11-17 22:07:12]

The CQ bit was unchecked by paulmeyer@chromium.org

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

The CQ bit was checked by paulmeyer@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2509933002/#ps140001
(title: "Addressed additional comments by avi@, dcheng@.")

[commit-bot: I haz the power, 2016-11-17 22:08:15]

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-17 23:06:46]

Description was changed from

==========
Drag-and-drop: Target drag messages (the sequel).

This patch follows up on https://codereview.chromium.org/2505113002/,
completing drag-and-drop targeting in a few remaining places, such as in
Mac-specific code as well as in WebContentsImpl::SystemDragEnded(). This
is part of an ongoing effort to allow D&D to work with out-of-process
iframes.

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

to

==========
Drag-and-drop: Target drag messages (the sequel).

This patch follows up on https://codereview.chromium.org/2505113002/,
completing drag-and-drop targeting in a few remaining places, such as in
Mac-specific code as well as in WebContentsImpl::SystemDragEnded(). This
is part of an ongoing effort to allow D&D to work with out-of-process
iframes.

BUG=647249, 655063
R=avi@chromium.org, dcheng@chromium.org

Committed: https://crrev.com/0c58c371878e696f904efd8e13886b23abd41eea
Cr-Commit-Position: refs/heads/master@{#433003}
==========

[commit-bot: I haz the power, 2016-11-17 23:06:47]

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

[paulmeyer, 2016-11-17 23:10:17]

Description was changed from

==========
Drag-and-drop: Target drag messages (the sequel).

This patch follows up on https://codereview.chromium.org/2505113002/,
completing drag-and-drop targeting in a few remaining places, such as in
Mac-specific code as well as in WebContentsImpl::SystemDragEnded(). This
is part of an ongoing effort to allow D&D to work with out-of-process
iframes.

BUG=647249, 655063
R=avi@chromium.org, dcheng@chromium.org

Committed: https://crrev.com/0c58c371878e696f904efd8e13886b23abd41eea
Cr-Commit-Position: refs/heads/master@{#433003}
==========

to

==========
Drag-and-drop: Target drag messages (the sequel).

This patch follows up on https://codereview.chromium.org/2505113002/,
completing drag-and-drop targeting in a few remaining places, such as in
Mac-specific code as well as in WebContentsImpl::SystemDragEnded(). This
is part of an ongoing effort to allow D&D to work with out-of-process
iframes.

BUG=647249, 655063
R=avi@chromium.org, dcheng@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/0c58c371878e696f904efd8e1388...
==========

[paulmeyer, 2016-11-17 23:10:19]

Committed patchset #5 (id:140001) manually as
0c58c371878e696f904efd8e13886b23abd41eea (presubmit successful).