Cancel drag operation when the tab RenderWidgetHostView does not exist (Mac)

Cancel drag operation when the tab RenderWidgetHostView does not exist (Mac)

When the tab's RenderWidgetHostView is nullptr, we cannot and should not select any
RenderWidgetHosts for a drag destination. Therefore, in response to draggingEntered
we should return no operation.

This CL will fix that issue as well as making RenderWidgetHostInputEventRouter::GetRenderWidgetHostAtPoint()
handle nullptr |root_view| by returning nullptr.

BUG=670645
Committed: https://crrev.com/5190330169f1e6c1d8418e6bcc0499fdc99ec287
Cr-Commit-Position: refs/heads/master@{#436146}

[EhsanK, 2016-12-02 23:06:57]

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

[EhsanK, 2016-12-02 23:06:57]

PTAL. Thanks!

[Avi (use Gerrit), 2016-12-02 23:07:24]

lgtm with fix

https://codereview.chromium.org/2547213002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/2547213002/diff/1/content/browser/web_content...
content/browser/web_contents/web_drag_dest_mac.mm:139: // (see
https://cebug.com/670645).
typo in the link

[EhsanK, 2016-12-02 23:09:51]

Thanks for the prompt review Avi! I will CQ soon.

https://codereview.chromium.org/2547213002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/2547213002/diff/1/content/browser/web_content...
content/browser/web_contents/web_drag_dest_mac.mm:139: // (see
https://cebug.com/670645).
On 2016/12/02 23:07:24, Avi wrote:
> typo in the link

Done.

[EhsanK, 2016-12-02 23:09:55]

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

[commit-bot: I haz the power, 2016-12-02 23:10:44]

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-12-02 23:12:34]

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

https://codereview.chromium.org/2547213002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:214: // Create the appropriate
mouse locations for WebCore. The draggingLocation
add a blank line above this; space things out a bit.

[EhsanK, 2016-12-02 23:15:37]

Thanks! PTAL.

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

https://codereview.chromium.org/2547213002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:214: // Create the appropriate
mouse locations for WebCore. The draggingLocation
On 2016/12/02 23:12:33, Avi wrote:
> add a blank line above this; space things out a bit.

Done.

[Charlie Reis, 2016-12-02 23:19:12]

Description was changed from

==========
Cancel drag operation when the tab RenderWidgetHostView does not exist (Mac)

When the tab's RenderWidgetHostView is nullptr, we cannot and should not select
any
RenderWidgetHosts for a drag destination. Therefore, in response to
draggingEntered
we should return no operation.

This CL will fix that issue as well as making
RenderWidgetHostInputEventRouter::GetRenderWidgetHostAtPoint()
handle nullptr |root_view| by returning nullptr.

BUG=670645
==========

to

==========
Cancel drag operation when the tab RenderWidgetHostView does not exist (Mac)

When the tab's RenderWidgetHostView is nullptr, we cannot and should not select
any
RenderWidgetHosts for a drag destination. Therefore, in response to
draggingEntered
we should return no operation.

This CL will fix that issue as well as making
RenderWidgetHostInputEventRouter::GetRenderWidgetHostAtPoint()
handle nullptr |root_view| by returning nullptr.

BUG=670645
==========

[EhsanK, 2016-12-02 23:49:10]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-12-02 23:49:10]

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/2547213002/#ps40001
(title: "Added a blink line")

[commit-bot: I haz the power, 2016-12-02 23:50:19]

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

[dcheng, 2016-12-03 00:33:06]

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

[dcheng, 2016-12-03 00:33:07]

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

https://codereview.chromium.org/2547213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:213: return
NSDragOperationNone;
Seems unusual that this condition is checked twice in this function.

[EhsanK, 2016-12-03 00:36:44]

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

https://codereview.chromium.org/2547213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:213: return
NSDragOperationNone;
On 2016/12/03 00:33:07, dcheng wrote:
> Seems unusual that this condition is checked twice in this function.

I think draggingEntered could also mutate |canceled_|.

[dcheng, 2016-12-03 00:49:14]

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

https://codereview.chromium.org/2547213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:213: return
NSDragOperationNone;
On 2016/12/03 00:36:44, EhsanK wrote:
> On 2016/12/03 00:33:07, dcheng wrote:
> > Seems unusual that this condition is checked twice in this function.
> 
> I think draggingEntered could also mutate |canceled_|.

Right, I understand, that was there even before we rewrote this to support
OOPIF.

It just feels weird: canceled_ originally represented whether or not the
delegate cancelled the drag. Now it means that, and "there is no RWHV", and it
feels like the two checks are entangled in a slightly unusual way.

[EhsanK, 2016-12-03 00:58:55]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-12-03 00:59:37]

Stopped CQ to address Daniel's comment.

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

https://codereview.chromium.org/2547213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:213: return
NSDragOperationNone;
On 2016/12/03 00:49:14, dcheng wrote:
> On 2016/12/03 00:36:44, EhsanK wrote:
> > On 2016/12/03 00:33:07, dcheng wrote:
> > > Seems unusual that this condition is checked twice in this function.
> > 
> > I think draggingEntered could also mutate |canceled_|.
> 
> Right, I understand, that was there even before we rewrote this to support
> OOPIF.
> 
> It just feels weird: canceled_ originally represented whether or not the
> delegate cancelled the drag. Now it means that, and "there is no RWHV", and it
> feels like the two checks are entangled in a slightly unusual way.

I see your point. I could instead not change canceled_ but at the top verify if
|RWHV| exists.

[dcheng, 2016-12-03 01:02:32]

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

https://codereview.chromium.org/2547213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:213: return
NSDragOperationNone;
On 2016/12/03 00:59:37, EhsanK wrote:
> On 2016/12/03 00:49:14, dcheng wrote:
> > On 2016/12/03 00:36:44, EhsanK wrote:
> > > On 2016/12/03 00:33:07, dcheng wrote:
> > > > Seems unusual that this condition is checked twice in this function.
> > > 
> > > I think draggingEntered could also mutate |canceled_|.
> > 
> > Right, I understand, that was there even before we rewrote this to support
> > OOPIF.
> > 
> > It just feels weird: canceled_ originally represented whether or not the
> > delegate cancelled the drag. Now it means that, and "there is no RWHV", and
it
> > feels like the two checks are entangled in a slightly unusual way.
> 
> I see your point. I could instead not change canceled_ but at the top verify
if
> |RWHV| exists.

Meh, I can't really think of anything better atm. We need to clean this code up
anyway (we double hit-test in the draggingUpdated but not draggingEntered yet
case), we can figure it out then =)

[EhsanK, 2016-12-03 01:13:50]

Daniel, PTAL. Thanks!

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

https://codereview.chromium.org/2547213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:213: return
NSDragOperationNone;
On 2016/12/03 01:02:32, dcheng wrote:
> On 2016/12/03 00:59:37, EhsanK wrote:
> > On 2016/12/03 00:49:14, dcheng wrote:
> > > On 2016/12/03 00:36:44, EhsanK wrote:
> > > > On 2016/12/03 00:33:07, dcheng wrote:
> > > > > Seems unusual that this condition is checked twice in this function.
> > > > 
> > > > I think draggingEntered could also mutate |canceled_|.
> > > 
> > > Right, I understand, that was there even before we rewrote this to support
> > > OOPIF.
> > > 
> > > It just feels weird: canceled_ originally represented whether or not the
> > > delegate cancelled the drag. Now it means that, and "there is no RWHV",
and
> it
> > > feels like the two checks are entangled in a slightly unusual way.
> > 
> > I see your point. I could instead not change canceled_ but at the top verify
> if
> > |RWHV| exists.
> 
> Meh, I can't really think of anything better atm. We need to clean this code
up
> anyway (we double hit-test in the draggingUpdated but not draggingEntered yet
> case), we can figure it out then =)

I added TODOs (on behalf of paulmeyer@ as well hope that is alright).  I could
also remove this cancel and repeat verifying GetRenderWidgetHostView() ==
nullptr like above. WDYT?

[dcheng, 2016-12-03 01:17:03]

PS3 or PS4 LGTM.

I don't think it's worth adding the manual checks (because then I think we'd
have to duplicate the check into draggingExited as well...)

[EhsanK, 2016-12-03 01:18:09]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-12-03 01:18:10]

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/2547213002/#ps60001
(title: "Added TODOs")

[commit-bot: I haz the power, 2016-12-03 01:18:36]

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

[EhsanK, 2016-12-03 01:18:41]

Thanks. Agreed. I will go with PS4 then.

[commit-bot: I haz the power, 2016-12-03 02:30:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480727889835120,
"parent_rev": "38ddc4910ee85f17bb83fb58841933b53453effe", "commit_rev":
"f64b62eb89534dc76cf9e684daf4c35cc16b9fe6"}

[commit-bot: I haz the power, 2016-12-03 02:31:18]

Description was changed from

==========
Cancel drag operation when the tab RenderWidgetHostView does not exist (Mac)

When the tab's RenderWidgetHostView is nullptr, we cannot and should not select
any
RenderWidgetHosts for a drag destination. Therefore, in response to
draggingEntered
we should return no operation.

This CL will fix that issue as well as making
RenderWidgetHostInputEventRouter::GetRenderWidgetHostAtPoint()
handle nullptr |root_view| by returning nullptr.

BUG=670645
==========

to

==========
Cancel drag operation when the tab RenderWidgetHostView does not exist (Mac)

When the tab's RenderWidgetHostView is nullptr, we cannot and should not select
any
RenderWidgetHosts for a drag destination. Therefore, in response to
draggingEntered
we should return no operation.

This CL will fix that issue as well as making
RenderWidgetHostInputEventRouter::GetRenderWidgetHostAtPoint()
handle nullptr |root_view| by returning nullptr.

BUG=670645
==========

[commit-bot: I haz the power, 2016-12-03 02:31:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-03 02:35:20]

Description was changed from

==========
Cancel drag operation when the tab RenderWidgetHostView does not exist (Mac)

When the tab's RenderWidgetHostView is nullptr, we cannot and should not select
any
RenderWidgetHosts for a drag destination. Therefore, in response to
draggingEntered
we should return no operation.

This CL will fix that issue as well as making
RenderWidgetHostInputEventRouter::GetRenderWidgetHostAtPoint()
handle nullptr |root_view| by returning nullptr.

BUG=670645
==========

to

==========
Cancel drag operation when the tab RenderWidgetHostView does not exist (Mac)

When the tab's RenderWidgetHostView is nullptr, we cannot and should not select
any
RenderWidgetHosts for a drag destination. Therefore, in response to
draggingEntered
we should return no operation.

This CL will fix that issue as well as making
RenderWidgetHostInputEventRouter::GetRenderWidgetHostAtPoint()
handle nullptr |root_view| by returning nullptr.

BUG=670645

Committed: https://crrev.com/5190330169f1e6c1d8418e6bcc0499fdc99ec287
Cr-Commit-Position: refs/heads/master@{#436146}
==========

[commit-bot: I haz the power, 2016-12-03 02:35:21]

Patchset 4 (id:??) landed as
https://crrev.com/5190330169f1e6c1d8418e6bcc0499fdc99ec287
Cr-Commit-Position: refs/heads/master@{#436146}