Reland 4e4eae4cbe6136b538a: Make download item drags look like bookmark

Reland 4e4eae4cbe6136b538a: Make download item drags look like bookmark
drags.

This also looks the same as what you get when you drag the location icon.

BUG=700161
Review-Url: https://codereview.chromium.org/2750253002
Cr-Commit-Position: refs/heads/master@{#458246}
Committed: https://chromium.googlesource.com/chromium/src/+/9c4b2005a536042afa175da68877b8f3d241380c

[Evan Stade, 2017-03-16 00:03:30]

estade@chromium.org changed reviewers:
+ msw@chromium.org, sky@chromium.org

[Evan Stade, 2017-03-16 00:03:30]

https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
File chrome/browser/download/drag_download_item_views.cc (right):

https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
chrome/browser/download/drag_download_item_views.cc:47:
*views::Widget::GetTopLevelWidgetForNativeView(view), &data);
key to fixing the crash (which occurred for chrome://downloads drags) is
GetTopLevelWidgetForNativeView

https://codereview.chromium.org/2750253002/diff/20001/ui/views/button_drag_ut...
File ui/views/button_drag_utils.h (right):

https://codereview.chromium.org/2750253002/diff/20001/ui/views/button_drag_ut...
ui/views/button_drag_utils.h:35: const views::Widget& widget,
widget isn't allowed to be null and out params are supposed to come at the end.

[msw, 2017-03-16 00:28:29]

lgtm

[Evan Stade, 2017-03-16 01:12:50]

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

[commit-bot: I haz the power, 2017-03-16 01:13:12]

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

[Evan Stade, 2017-03-16 01:13:38]

Description was changed from

==========
Reland 4e4eae4cbe6136b538a: Make download item drags look like bookmark
drags.

BUG=700161
==========

to

==========
Reland 4e4eae4cbe6136b538a: Make download item drags look like bookmark
drags.

This also looks the same as what you get when you drag the location icon.

BUG=700161
==========

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

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

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

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-16 03:38:05]

https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
File chrome/browser/download/drag_download_item_views.cc (right):

https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
chrome/browser/download/drag_download_item_views.cc:38: if (!root_window ||
!aura::client::GetDragDropClient(root_window))
Under what circumstances would there be no root or drag drop client? I assumed
this function is called during event dispatching, in which I would expect a
root.

[Evan Stade, 2017-03-16 14:14:50]

https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
File chrome/browser/download/drag_download_item_views.cc (right):

https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
chrome/browser/download/drag_download_item_views.cc:38: if (!root_window ||
!aura::client::GetDragDropClient(root_window))
On 2017/03/16 03:38:05, sky wrote:
> Under what circumstances would there be no root or drag drop client? I assumed
> this function is called during event dispatching, in which I would expect a
> root.

I have no idea, I just moved the check from below to make it an earlier return.

[sky, 2017-03-16 16:30:08]

On 2017/03/16 14:14:50, Evan Stade wrote:
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> File chrome/browser/download/drag_download_item_views.cc (right):
> 
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> chrome/browser/download/drag_download_item_views.cc:38: if (!root_window ||
> !aura::client::GetDragDropClient(root_window))
> On 2017/03/16 03:38:05, sky wrote:
> > Under what circumstances would there be no root or drag drop client? I
assumed
> > this function is called during event dispatching, in which I would expect a
> > root.
> 
> I have no idea, I just moved the check from below to make it an earlier
return.

You made me look at the crash. The stack indicates the drag is started from an
async message, which explains why there may be no root. I think the checks for
parent belong in MdDownloadsDOMHandler::HandleDrag as presumably we should only
be starting a drag if the webcontents is still visible/shown.

[Evan Stade, 2017-03-16 16:56:44]

On 2017/03/16 16:30:08, sky wrote:
> On 2017/03/16 14:14:50, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > File chrome/browser/download/drag_download_item_views.cc (right):
> > 
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > chrome/browser/download/drag_download_item_views.cc:38: if (!root_window ||
> > !aura::client::GetDragDropClient(root_window))
> > On 2017/03/16 03:38:05, sky wrote:
> > > Under what circumstances would there be no root or drag drop client? I
> assumed
> > > this function is called during event dispatching, in which I would expect
a
> > > root.
> > 
> > I have no idea, I just moved the check from below to make it an earlier
> return.
> 
> You made me look at the crash. The stack indicates the drag is started from an
> async message, which explains why there may be no root. I think the checks for
> parent belong in MdDownloadsDOMHandler::HandleDrag as presumably we should
only
> be starting a drag if the webcontents is still visible/shown.

The crash fix is explained here:
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...

The rest of the change is just improvements I noticed along the way.

[Evan Stade, 2017-03-16 17:06:28]

On 2017/03/16 16:56:44, Evan Stade wrote:
> On 2017/03/16 16:30:08, sky wrote:
> > On 2017/03/16 14:14:50, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > > File chrome/browser/download/drag_download_item_views.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > > chrome/browser/download/drag_download_item_views.cc:38: if (!root_window
||
> > > !aura::client::GetDragDropClient(root_window))
> > > On 2017/03/16 03:38:05, sky wrote:
> > > > Under what circumstances would there be no root or drag drop client? I
> > assumed
> > > > this function is called during event dispatching, in which I would
expect
> a
> > > > root.
> > > 
> > > I have no idea, I just moved the check from below to make it an earlier
> > return.
> > 
> > You made me look at the crash. The stack indicates the drag is started from
an
> > async message, which explains why there may be no root. I think the checks
for
> > parent belong in MdDownloadsDOMHandler::HandleDrag as presumably we should
> only
> > be starting a drag if the webcontents is still visible/shown.
> 
> The crash fix is explained here:
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> 
> The rest of the change is just improvements I noticed along the way.

here's the CL where this was originally added:
https://chromiumcodereview.appspot.com/10541150

there aren't a lot of details to explain why it was crashing, but it's different
from the crash which caused us to revert this change.

[Evan Stade, 2017-03-18 00:51:24]

On 2017/03/16 17:06:28, Evan Stade wrote:
> On 2017/03/16 16:56:44, Evan Stade wrote:
> > The crash fix is explained here:
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > 
> > The rest of the change is just improvements I noticed along the way.
> 
> here's the CL where this was originally added:
> https://chromiumcodereview.appspot.com/10541150
> 
> there aren't a lot of details to explain why it was crashing, but it's
different
> from the crash which caused us to revert this change.

ping sky

[sky, 2017-03-20 15:45:28]

On 2017/03/16 16:56:44, Evan Stade wrote:
> On 2017/03/16 16:30:08, sky wrote:
> > On 2017/03/16 14:14:50, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > > File chrome/browser/download/drag_download_item_views.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > > chrome/browser/download/drag_download_item_views.cc:38: if (!root_window
||
> > > !aura::client::GetDragDropClient(root_window))
> > > On 2017/03/16 03:38:05, sky wrote:
> > > > Under what circumstances would there be no root or drag drop client? I
> > assumed
> > > > this function is called during event dispatching, in which I would
expect
> a
> > > > root.
> > > 
> > > I have no idea, I just moved the check from below to make it an earlier
> > return.
> > 
> > You made me look at the crash. The stack indicates the drag is started from
an
> > async message, which explains why there may be no root. I think the checks
for
> > parent belong in MdDownloadsDOMHandler::HandleDrag as presumably we should
> only
> > be starting a drag if the webcontents is still visible/shown.
> 
> The crash fix is explained here:
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...

You mean the comment "I have no idea, I just moved the check from below to make
it an earlier return."? That's not an explanation. I still think the fix belongs
in MdDownloadsDOMHandler::HandleDrag, not here. Specifically if you put the
check that the view is still showing there, then you don't need a similar check
for the mac side of things.

> The rest of the change is just improvements I noticed along the way.

[Evan Stade, 2017-03-20 15:56:47]

On 2017/03/20 15:45:28, sky wrote:
> On 2017/03/16 16:56:44, Evan Stade wrote:
> > On 2017/03/16 16:30:08, sky wrote:
> > > On 2017/03/16 14:14:50, Evan Stade wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > > > File chrome/browser/download/drag_download_item_views.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> > > > chrome/browser/download/drag_download_item_views.cc:38: if (!root_window
> ||
> > > > !aura::client::GetDragDropClient(root_window))
> > > > On 2017/03/16 03:38:05, sky wrote:
> > > > > Under what circumstances would there be no root or drag drop client? I
> > > assumed
> > > > > this function is called during event dispatching, in which I would
> expect
> > a
> > > > > root.
> > > > 
> > > > I have no idea, I just moved the check from below to make it an earlier
> > > return.
> > > 
> > > You made me look at the crash. The stack indicates the drag is started
from
> an
> > > async message, which explains why there may be no root. I think the checks
> for
> > > parent belong in MdDownloadsDOMHandler::HandleDrag as presumably we should
> > only
> > > be starting a drag if the webcontents is still visible/shown.
> > 
> > The crash fix is explained here:
> >
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
> 
> You mean the comment "I have no idea, I just moved the check from below to
make
> it an earlier return."?

No, I meant the one that starts "key to fixing the crash"
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...

[sky, 2017-03-20 16:39:47]

Yes, I understand that moving the check up fixes the crash, but I feel
the check should be higher in the stack, at least in
MdDownloadsDOMHandler::HandleDrag, but perhaps further up.

On Mon, Mar 20, 2017 at 8:56 AM,  <estade@chromium.org> wrote:
> On 2017/03/20 15:45:28, sky wrote:
>> On 2017/03/16 16:56:44, Evan Stade wrote:
>> > On 2017/03/16 16:30:08, sky wrote:
>> > > On 2017/03/16 14:14:50, Evan Stade wrote:
>> > > >
>> > >
>> >
>>
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
>> > > > File chrome/browser/download/drag_download_item_views.cc (right):
>> > > >
>> > > >
>> > >
>> >
>>
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
>> > > > chrome/browser/download/drag_download_item_views.cc:38: if
>> > > > (!root_window
>> ||
>> > > > !aura::client::GetDragDropClient(root_window))
>> > > > On 2017/03/16 03:38:05, sky wrote:
>> > > > > Under what circumstances would there be no root or drag drop
>> > > > > client? I
>> > > assumed
>> > > > > this function is called during event dispatching, in which I would
>> expect
>> > a
>> > > > > root.
>> > > >
>> > > > I have no idea, I just moved the check from below to make it an
>> > > > earlier
>> > > return.
>> > >
>> > > You made me look at the crash. The stack indicates the drag is started
> from
>> an
>> > > async message, which explains why there may be no root. I think the
>> > > checks
>> for
>> > > parent belong in MdDownloadsDOMHandler::HandleDrag as presumably we
>> > > should
>> > only
>> > > be starting a drag if the webcontents is still visible/shown.
>> >
>> > The crash fix is explained here:
>> >
>>
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
>>
>> You mean the comment "I have no idea, I just moved the check from below to
> make
>> it an earlier return."?
>
> No, I meant the one that starts "key to fixing the crash"
>
https://codereview.chromium.org/2750253002/diff/20001/chrome/browser/download...
>
>
> https://codereview.chromium.org/2750253002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2017-03-20 17:22:13]

No, the crash is fixed by using GetTopLevelWidgetForNativeView in place of
GetWidgetForNativeView.

On Mar 20, 2017 10:39 AM, "Scott Violet" <sky@chromium.org> wrote:

Yes, I understand that moving the check up fixes the crash, but I feel
the check should be higher in the stack, at least in
MdDownloadsDOMHandler::HandleDrag, but perhaps further up.

On Mon, Mar 20, 2017 at 8:56 AM,  <estade@chromium.org> wrote:
> On 2017/03/20 15:45:28, sky wrote:
>> On 2017/03/16 16:56:44, Evan Stade wrote:
>> > On 2017/03/16 16:30:08, sky wrote:
>> > > On 2017/03/16 14:14:50, Evan Stade wrote:
>> > > >
>> > >
>> >
>>
> https://codereview.chromium.org/2750253002/diff/20001/chrome
/browser/download/drag_download_item_views.cc
>> > > > File chrome/browser/download/drag_download_item_views.cc (right):
>> > > >
>> > > >
>> > >
>> >
>>
> https://codereview.chromium.org/2750253002/diff/20001/chrome
/browser/download/drag_download_item_views.cc#newcode38
>> > > > chrome/browser/download/drag_download_item_views.cc:38: if
>> > > > (!root_window
>> ||
>> > > > !aura::client::GetDragDropClient(root_window))
>> > > > On 2017/03/16 03:38:05, sky wrote:
>> > > > > Under what circumstances would there be no root or drag drop
>> > > > > client? I
>> > > assumed
>> > > > > this function is called during event dispatching, in which I
would
>> expect
>> > a
>> > > > > root.
>> > > >
>> > > > I have no idea, I just moved the check from below to make it an
>> > > > earlier
>> > > return.
>> > >
>> > > You made me look at the crash. The stack indicates the drag is
started
> from
>> an
>> > > async message, which explains why there may be no root. I think the
>> > > checks
>> for
>> > > parent belong in MdDownloadsDOMHandler::HandleDrag as presumably we
>> > > should
>> > only
>> > > be starting a drag if the webcontents is still visible/shown.
>> >
>> > The crash fix is explained here:
>> >
>>
> https://codereview.chromium.org/2750253002/diff/20001/chrome
/browser/download/drag_download_item_views.cc#newcode47
>>
>> You mean the comment "I have no idea, I just moved the check from below
to
> make
>> it an earlier return."?
>
> No, I meant the one that starts "key to fixing the crash"
> https://codereview.chromium.org/2750253002/diff/20001/chrome
/browser/download/drag_download_item_views.cc#newcode47
>
>
> https://codereview.chromium.org/2750253002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-03-20 20:01:46]

I was actually talking about the root_window checks. But I think checking for
the dnd client is always necessary. Ok, LGTM

[Evan Stade, 2017-03-20 22:51:20]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-03-20 22:52:12]

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-03-20 23:53:58]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490050280630120,
"parent_rev": "54c5f4ac7707c8e5d5341eae008fa5f303f7656c", "commit_rev":
"9c4b2005a536042afa175da68877b8f3d241380c"}

[commit-bot: I haz the power, 2017-03-20 23:54:44]

Description was changed from

==========
Reland 4e4eae4cbe6136b538a: Make download item drags look like bookmark
drags.

This also looks the same as what you get when you drag the location icon.

BUG=700161
==========

to

==========
Reland 4e4eae4cbe6136b538a: Make download item drags look like bookmark
drags.

This also looks the same as what you get when you drag the location icon.

BUG=700161

Review-Url: https://codereview.chromium.org/2750253002
Cr-Commit-Position: refs/heads/master@{#458246}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c4b2005a536042afa175da68877...
==========

[commit-bot: I haz the power, 2017-03-20 23:54:45]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9c4b2005a536042afa175da68877...