Correctly set dragLeave and dragEnd coords for OOPIF drag and drop

Correctly set dragLeave and dragEnd coords for OOPIF drag and drop

Currently dragLeave events triggered from the browser process always
have (0, 0) set as coordinates, which is incorrect when the mouse
cursor has not left the page. This patch sets the correct coordinates
when the cursor moves between frames of different processes.

Also, dragEnd event coordinates were not being correctly transformed
before being passed to the target RenderWidgetHost. This patch
applies the transform to move the coordinates into the correct
coordinate space for the frame that receives the dragEnd.

BUG=669695
Review-Url: https://codereview.chromium.org/2655463015
Cr-Commit-Position: refs/heads/master@{#448797}
Committed: https://chromium.googlesource.com/chromium/src/+/07c272807932376eafddce27c267028b42ba6c31

[kenrb, 2017-01-27 17:47:20]

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

[commit-bot: I haz the power, 2017-01-27 17:47:44]

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, 2017-01-27 18:27:32]

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

[commit-bot: I haz the power, 2017-01-27 18:27:33]

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

[kenrb, 2017-01-27 20:53:53]

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

[commit-bot: I haz the power, 2017-01-27 20:54:13]

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, 2017-01-27 21:20:11]

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

[commit-bot: I haz the power, 2017-01-27 21:20:12]

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

[kenrb, 2017-01-27 21:47:06]

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

[commit-bot: I haz the power, 2017-01-27 21:47:36]

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, 2017-01-27 22:18:04]

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

[commit-bot: I haz the power, 2017-01-27 22:18:05]

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

[kenrb, 2017-01-31 23:32:06]

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

[commit-bot: I haz the power, 2017-01-31 23:33:54]

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

[kenrb, 2017-01-31 23:51:45]

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

[commit-bot: I haz the power, 2017-01-31 23:52:39]

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, 2017-02-01 01:34:14]

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

[commit-bot: I haz the power, 2017-02-01 01:34:15]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-02-01 03:00:41]

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

[commit-bot: I haz the power, 2017-02-01 03:01:03]

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, 2017-02-01 03:11:10]

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

[commit-bot: I haz the power, 2017-02-01 03:11:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kenrb, 2017-02-01 05:40:39]

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

[commit-bot: I haz the power, 2017-02-01 05:40: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, 2017-02-01 07:16:27]

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

[commit-bot: I haz the power, 2017-02-01 07:16:28]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-02-01 14:50:08]

kenrb@chromium.org changed reviewers:
+ dcheng@chromium.org, lukasza@chromium.org

[kenrb, 2017-02-01 14:50:09]

PTAL?
- lukasza for the test
- dcheng for Blink

[Łukasz Anforowicz, 2017-02-01 18:12:05]

On 2017/02/01 14:50:09, kenrb wrote:
> PTAL?
> - lukasza for the test
> - dcheng for Blink

New test expectations in
chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc LGTM.  I've also
tried an experiment (https://crbug.com/669695#c9) to see what other browsers do,
and I think the behavior reinforced by this CL is okay.  The good and important
thing is that after the CL get consistent behavior regardless whether OOPIFs are
enabled.

OTOH, I see that in some places we report (0,0) coordinates and in other places
we have to go through extra trouble/code to fix up the coordinates.  I've
pointed out one such place in a CR comment below, but there are more.  This
makes me wonder - if (0,0) is okay in some cases, when not in all cases.  Using
(0,0) everywhere would simplify the code (although might be seen as web
breaking, so probably would have to go through intent process and so forth).

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

https://codereview.chromium.org/2655463015/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:1272:
current_rwh_for_drag_->DragTargetDragLeave(gfx::Point(), gfx::Point());
Why do we need to grab and forward
display::Screen::GetScreen()->GetCursorScreenPoint() when calling
DragTargetDragLeave below, but not here?

[Łukasz Anforowicz, 2017-02-01 18:12:26]

+pwnall@ to CC

[kenrb, 2017-02-01 19:53:48]

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

https://codereview.chromium.org/2655463015/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:1272:
current_rwh_for_drag_->DragTargetDragLeave(gfx::Point(), gfx::Point());
On 2017/02/01 18:12:04, Łukasz Anforowicz wrote:
> Why do we need to grab and forward
> display::Screen::GetScreen()->GetCursorScreenPoint() when calling
> DragTargetDragLeave below, but not here?

Just because it is behavior-preserving. ScreenPoint is currently available for
drops on the same page but not when the cursor leaves the window.

[dcheng, 2017-02-01 20:45:31]

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

https://codereview.chromium.org/2655463015/diff/100001/content/browser/web_co...
content/browser/web_contents/web_drag_source_mac.mm:312:
&transformedScreenPoint);
Is it possible to have more of these utility functions shared in general? For
example, lines 279 - 287 seem pretty similar to flipWindowPointToView /
flipWindowPointToScreen in web_drag_dest_mac.mm, and then we have similar
transformations in the rest of the drag/drop code as well.

In addition, I find this block of code a bit difficult to read... there's a lot
of downcasting involved.

[kenrb, 2017-02-02 20:36:06]

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

[commit-bot: I haz the power, 2017-02-02 20:36:46]

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

[kenrb, 2017-02-02 21:21:03]

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

https://codereview.chromium.org/2655463015/diff/100001/content/browser/web_co...
content/browser/web_contents/web_drag_source_mac.mm:312:
&transformedScreenPoint);
On 2017/02/01 20:45:31, dcheng wrote:
> Is it possible to have more of these utility functions shared in general? For
> example, lines 279 - 287 seem pretty similar to flipWindowPointToView /
> flipWindowPointToScreen in web_drag_dest_mac.mm, and then we have similar
> transformations in the rest of the drag/drop code as well.
> 
> In addition, I find this block of code a bit difficult to read... there's a
lot
> of downcasting involved.

I have made changes to make the point transformation calls more readable.

As for the the coordinate flipping, is there something specific you had in mind?
There isn't a convenient Mac utility file under content/browser/web_contents
that I can see, and it is a stretch to create one for such a small change. I am
reluctant to undertake a larger refactor in part because I am a novice with
Objective-C++, and in part because I don't want to block this bug fix on
something like that.

[commit-bot: I haz the power, 2017-02-02 22:30:24]

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

[commit-bot: I haz the power, 2017-02-02 22:30:25]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-05 05:44:50]

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

[dcheng, 2017-02-05 05:44:51]

LGTM as-is.

+avi, who might have some thoughts on the Mac utility stuff (if we do anything
here, it's fine to do it in a followup)

[kenrb, 2017-02-06 20:48:52]

avi: Would you mind giving this a content owner review?

[Avi (use Gerrit), 2017-02-06 20:51:06]

lgtm

stamp

Refactoring utility functions is always good, but I don't have thoughts right
now about where that would go.

[kenrb, 2017-02-06 21:28:18]

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

[kenrb, 2017-02-06 21:28:19]

Oops, missed one...

rbyers@: Can you PTAL for a trivial change in components/test_runner?

[Rick Byers, 2017-02-07 18:10:50]

On 2017/02/06 21:28:19, kenrb wrote:
> Oops, missed one...
> 
> rbyers@: Can you PTAL for a trivial change in components/test_runner?

test_runner LGTM

[kenrb, 2017-02-07 19:33:49]

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

[commit-bot: I haz the power, 2017-02-07 19:34:31]

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, 2017-02-07 20:58:58]

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

[commit-bot: I haz the power, 2017-02-07 20:59:02]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-02-07 23:07:53]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-02-07 23:07:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, avi@chromium.org, dcheng@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2655463015/#ps140001
(title: "Added checks for null RWH on drag end")

[commit-bot: I haz the power, 2017-02-07 23:09:22]

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, 2017-02-07 23:48:21]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486508873871670,
"parent_rev": "b0785808c1a3d64ec590bf17d41db45c7b0d8b16", "commit_rev":
"07c272807932376eafddce27c267028b42ba6c31"}

[commit-bot: I haz the power, 2017-02-07 23:48:54]

Description was changed from

==========
Correctly set dragLeave and dragEnd coords for OOPIF drag and drop

Currently dragLeave events triggered from the browser process always
have (0, 0) set as coordinates, which is incorrect when the mouse
cursor has not left the page. This patch sets the correct coordinates
when the cursor moves between frames of different processes.

Also, dragEnd event coordinates were not being correctly transformed
before being passed to the target RenderWidgetHost. This patch
applies the transform to move the coordinates into the correct
coordinate space for the frame that receives the dragEnd.

BUG=669695
==========

to

==========
Correctly set dragLeave and dragEnd coords for OOPIF drag and drop

Currently dragLeave events triggered from the browser process always
have (0, 0) set as coordinates, which is incorrect when the mouse
cursor has not left the page. This patch sets the correct coordinates
when the cursor moves between frames of different processes.

Also, dragEnd event coordinates were not being correctly transformed
before being passed to the target RenderWidgetHost. This patch
applies the transform to move the coordinates into the correct
coordinate space for the frame that receives the dragEnd.

BUG=669695

Review-Url: https://codereview.chromium.org/2655463015
Cr-Commit-Position: refs/heads/master@{#448797}
Committed:
https://chromium.googlesource.com/chromium/src/+/07c272807932376eafddce27c267...
==========

[commit-bot: I haz the power, 2017-02-07 23:48:55]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/07c272807932376eafddce27c267...