Allow menus to stay open during a child view's drag and drop

Allow a menu to stay open when a child View (not a MenuItemView) is doing a drag-and-drop operation.

We need this in order to allow the wrench menu to stay open during browser action drag-and-drop when the extension action redesign switch is enabled.

BUG=393038
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283656

[Devlin, 2014-07-09 15:47:47]

Scott, please take a look.  This is obviously incomplete (there's no adjusted
calculations for dragging, etc), but it allows dragging in the overflow in the
wrench menu.  I experimented with a lot of different ways (I swear this is the
longest it's ever taken me to write 30 lines of code!), and this one actually
works fairly well, and, to me, strikes me as a conceptually-correct,
non-intrusive way to do it.  But, I'm a bit too novice at views code to know if
this is "simple and elegant" or simply "a really dumb hack". =)

Background on the original problem:
Since the WrenchMenu is implemented as a MenuItemView and the overflow menu is
implemented as a regular view, dragging doesn't play nice between them - the
WrenchMenu doesn't realize it's in a drag-and-drop operation, and receives a
MouseCaptureLost event and closes.

[sky, 2014-07-09 16:09:16]

https://codereview.chromium.org/378333003/diff/1/chrome/browser/ui/views/tool...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/378333003/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:468: if
(in_overflow_mode()) {
This is too fragile. The menu code should change so that you don't have to do
anything special here, dnd from a view on the menu just works.

I'm not sure of the ideal route to make that happen, I would have to do a bit
more investigation. I'm just getting back from vacation and so rather behind. I
can take a look later in the week or next week if you get stuck.

[Devlin, 2014-07-09 18:03:41]

https://codereview.chromium.org/378333003/diff/1/chrome/browser/ui/views/tool...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/378333003/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:468: if
(in_overflow_mode()) {
On 2014/07/09 16:09:16, sky wrote:
> This is too fragile. The menu code should change so that you don't have to do
> anything special here, dnd from a view on the menu just works.

Okay - let's try this approach.  It seems to fit the requirement of "just
working" from the view's perspective. :)

> I'm not sure of the ideal route to make that happen, I would have to do a bit
> more investigation. I'm just getting back from vacation and so rather behind.
I
> can take a look later in the week or next week if you get stuck.

I'll keep plugging away for a bit, and see what I can come up with.  If nothing
else, the background will be useful.  If you can keep sparing a few minutes here
and there to lemme know if I'm on the right track, that'd be great. :)

[Devlin, 2014-07-09 18:28:44]

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:795:
GetTopLevelWidget()->set_child_drag_in_progress(false);
This still has the problem that, since capture was lost, we don't receive new
CaptureLost events, but I'll address that once we know whether it's worth
continuing with this approach or not.

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.h...
ui/views/widget/widget.h:867: bool child_drag_in_progress_;
This name isn't strictly true, since only the top-level widget will know about
this, but I think that's all that's needed (since it is the top-level widget
which closes the menu when it loses focus).  We could do this for each parent,
though, without much trouble.

[sky, 2014-07-10 15:05:00]

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:791:
GetTopLevelWidget()->set_child_drag_in_progress(true);
You shouldn't need a new field to determine if a drag is in progress. You can
use dragged_view_ set on 790.

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:795:
GetTopLevelWidget()->set_child_drag_in_progress(false);
On 2014/07/09 18:28:44, Devlin wrote:
> This still has the problem that, since capture was lost, we don't receive new
> CaptureLost events, but I'll address that once we know whether it's worth
> continuing with this approach or not.

More specifically the menu is left in a weird state. I suspect we want to leave
the menu open and working normally, is that correct? If so, then you'll need to
open some methods on Widget that are called before/after this function so that
MenuController can do the right thing.

[Devlin, 2014-07-10 15:33:00]

Sorry about having so many questions on this one... still a views newbie. 
Hopefully subsequent patches will have more code and fewer issues. :)

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:791:
GetTopLevelWidget()->set_child_drag_in_progress(true);
On 2014/07/10 15:05:00, sky wrote:
> You shouldn't need a new field to determine if a drag is in progress. You can
> use dragged_view_ set on 790.

The issue there is that dragged_view_ is typically for the direct widget.  In
this particular case, the widget associated with the dragging view will always
be the top level menu widget, but it seemed like that was a coincidence, rather
than a guarantee.  Will that be the case in all scenarios we should care about? 
If so, then this becomes a very nice one line fix to at least prevent the menu
from closing. :)

https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:795:
GetTopLevelWidget()->set_child_drag_in_progress(false);
On 2014/07/10 15:05:00, sky wrote:
> On 2014/07/09 18:28:44, Devlin wrote:
> > This still has the problem that, since capture was lost, we don't receive
new
> > CaptureLost events, but I'll address that once we know whether it's worth
> > continuing with this approach or not.
> 
> More specifically the menu is left in a weird state. I suspect we want to
leave
> the menu open and working normally, is that correct? If so, then you'll need
to
> open some methods on Widget that are called before/after this function so that
> MenuController can do the right thing.

Yeah, I would assume that we want to leave the menu open and working normally. 
A few notes:
- This would actually be slightly different than current behavior for
drag-and-drop in a menu, since that typically closes the menu as a result of
MenuController::OnDragExited (easy example: move around bookmarks in the wrench
menu).  Personally, I think this is a bit jarring, and for something where we
don't offer another UI surface (e.g., extension actions), it seems like we
shouldn't be closing after a drag-and-drop.
- Simply returning the capture to the widget with SetCapture() fixes half the
problem, in that it does mean that capture lost events will once again get sent
(so that clicking outside the menu will close it), but the menu doesn't seem to
be running entirely normally.

[Devlin, 2014-07-10 20:16:40]

On 2014/07/10 15:33:00, Devlin wrote:
> Sorry about having so many questions on this one... still a views newbie. 
> Hopefully subsequent patches will have more code and fewer issues. :)
> 
> https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.cc
> File ui/views/widget/widget.cc (right):
> 
>
https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.c...
> ui/views/widget/widget.cc:791:
> GetTopLevelWidget()->set_child_drag_in_progress(true);
> On 2014/07/10 15:05:00, sky wrote:
> > You shouldn't need a new field to determine if a drag is in progress. You
can
> > use dragged_view_ set on 790.
> 
> The issue there is that dragged_view_ is typically for the direct widget.  In
> this particular case, the widget associated with the dragging view will always
> be the top level menu widget, but it seemed like that was a coincidence,
rather
> than a guarantee.  Will that be the case in all scenarios we should care
about? 
> If so, then this becomes a very nice one line fix to at least prevent the menu
> from closing. :)
> 
>
https://codereview.chromium.org/378333003/diff/20001/ui/views/widget/widget.c...
> ui/views/widget/widget.cc:795:
> GetTopLevelWidget()->set_child_drag_in_progress(false);
> On 2014/07/10 15:05:00, sky wrote:
> > On 2014/07/09 18:28:44, Devlin wrote:
> > > This still has the problem that, since capture was lost, we don't receive
> new
> > > CaptureLost events, but I'll address that once we know whether it's worth
> > > continuing with this approach or not.
> > 
> > More specifically the menu is left in a weird state. I suspect we want to
> leave
> > the menu open and working normally, is that correct? If so, then you'll need
> to
> > open some methods on Widget that are called before/after this function so
that
> > MenuController can do the right thing.
> 
> Yeah, I would assume that we want to leave the menu open and working normally.

> A few notes:
> - This would actually be slightly different than current behavior for
> drag-and-drop in a menu, since that typically closes the menu as a result of
> MenuController::OnDragExited (easy example: move around bookmarks in the
wrench
> menu).  Personally, I think this is a bit jarring, and for something where we
> don't offer another UI surface (e.g., extension actions), it seems like we
> shouldn't be closing after a drag-and-drop.
> - Simply returning the capture to the widget with SetCapture() fixes half the
> problem, in that it does mean that capture lost events will once again get
sent
> (so that clicking outside the menu will close it), but the menu doesn't seem
to
> be running entirely normally.

Next pseudo-version up.
- Tentatively uses dragged_view_ presence instead of another parameter.
- Maybe fixed capture by restoring it for the menu after a drag is complete. 
Not sure if this is the right way to do it, though.

Clicking on another part of the menu after performing a drag doesn't immediately
work.  Interestingly, though, it looks like this is an existing bug to do with
clicking on a View within a menu and then trying to click on something else, and
is not limited to this.  Both on trunk and dev (I don't have a stable lying
around here, but I can find one if it'd help), the following works (or rather,
doesn't):
- Open wrench menu.
- Click on a View button, like zoom in ("+").
- Try to click "New tab" (or any other menu item).
- Nothing happens!
The View stole "focus" from the menu.  Clicking new tab again will work, because
the first click returned "focus".  Similarly, if you open up a submenu (even by
just hovering), focus will return (so clicking zoom, followed by mousing on
tools, followed by clicking on extensions, works).
All of this done on Linux.

Do you know if this is a known bug, or if it should be filed?

[sky, 2014-07-11 17:45:34]

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:115: !dragged_view()) {
Document why we check dragged_view(). I also midly prefer putting as much on one
line as possible, in other words merge 113 and 114.

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:142: // During a view's drag, we may have
lost the capture. Reset it so that we
Add a delegate method as to what should happen when the drag is completed. The
default implementation should return a value indicating the menu should close.
WrenchMenu would return otherwise. This code should just call through to the
MenuController and let it do the right thing.

https://codereview.chromium.org/378333003/diff/40001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/378333003/diff/40001/ui/views/widget/widget.h...
ui/views/widget/widget.h:798: virtual void OnDragComplete();
You should move the code in MenuController::StartDrag so that state is set
correctly if a drag is initiated outside of the menu code. I believe this means
adding a method like OnDragWillStart() here having MenuHost override and forward
to MenuController. MenuController's OnDragWillStart would set drag_in_progress_.
A similar flow should happen at the end of the drag to either reset capture or
close menus.

[Devlin, 2014-07-11 18:43:51]

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:115: !dragged_view()) {
On 2014/07/11 17:45:33, sky wrote:
> Document why we check dragged_view(). I also midly prefer putting as much on
one
> line as possible, in other words merge 113 and 114.

If we forward a OnDragWillStart() down to MenuController, I don't think we need
to check dragged view anymore, because drag_in_progress will actually be true
here.  Right?

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:142: // During a view's drag, we may have
lost the capture. Reset it so that we
On 2014/07/11 17:45:33, sky wrote:
> Add a delegate method as to what should happen when the drag is completed. The
> default implementation should return a value indicating the menu should close.
> WrenchMenu would return otherwise. This code should just call through to the
> MenuController and let it do the right thing.

Done.  However, this has a side-effect: other drags in the wrench menu (e.g.,
bookmarks) now behave differently than they used to.  Is this okay?

https://codereview.chromium.org/378333003/diff/40001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/378333003/diff/40001/ui/views/widget/widget.h...
ui/views/widget/widget.h:798: virtual void OnDragComplete();
On 2014/07/11 17:45:34, sky wrote:
> You should move the code in MenuController::StartDrag so that state is set
> correctly if a drag is initiated outside of the menu code. I believe this
means
> adding a method like OnDragWillStart() here having MenuHost override and
forward
> to MenuController. MenuController's OnDragWillStart would set
drag_in_progress_.
> A similar flow should happen at the end of the drag to either reset capture or
> close menus.

Done (though not much of the code in StartDrag() is relevant, since the drag is
initiated and handled through the other View).

[sky, 2014-07-11 19:15:20]

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:115: !dragged_view()) {
On 2014/07/11 18:43:51, Devlin wrote:
> On 2014/07/11 17:45:33, sky wrote:
> > Document why we check dragged_view(). I also midly prefer putting as much on
> one
> > line as possible, in other words merge 113 and 114.
> 
> If we forward a OnDragWillStart() down to MenuController, I don't think we
need
> to check dragged view anymore, because drag_in_progress will actually be true
> here.  Right?

Yes, I believe you are right.

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:142: // During a view's drag, we may have
lost the capture. Reset it so that we
On 2014/07/11 18:43:51, Devlin wrote:
> On 2014/07/11 17:45:33, sky wrote:
> > Add a delegate method as to what should happen when the drag is completed.
The
> > default implementation should return a value indicating the menu should
close.
> > WrenchMenu would return otherwise. This code should just call through to the
> > MenuController and let it do the right thing.
> 
> Done.  However, this has a side-effect: other drags in the wrench menu (e.g.,
> bookmarks) now behave differently than they used to.  Is this okay?

Good point. I don't believe the bookmark code does the right thing in the cast,
so you should cancel on a drag from bookmarks. The delegate should only be
consulted on drags initiated outside of menu code.

https://codereview.chromium.org/378333003/diff/40001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/378333003/diff/40001/ui/views/widget/widget.h...
ui/views/widget/widget.h:798: virtual void OnDragComplete();
On 2014/07/11 18:43:51, Devlin wrote:
> On 2014/07/11 17:45:34, sky wrote:
> > You should move the code in MenuController::StartDrag so that state is set
> > correctly if a drag is initiated outside of the menu code. I believe this
> means
> > adding a method like OnDragWillStart() here having MenuHost override and
> forward
> > to MenuController. MenuController's OnDragWillStart would set
> drag_in_progress_.
> > A similar flow should happen at the end of the drag to either reset capture
or
> > close menus.
> 
> Done (though not much of the code in StartDrag() is relevant, since the drag
is
> initiated and handled through the other View).

I wanted to make sure the state (which you're right, is only one boolean) is set
consistently for both cases. That's why moving to the method that is always
invoked is ideal.

[Devlin, 2014-07-11 20:28:15]

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/378333003/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_host.cc:142: // During a view's drag, we may have
lost the capture. Reset it so that we
On 2014/07/11 19:15:20, sky wrote:
> On 2014/07/11 18:43:51, Devlin wrote:
> > On 2014/07/11 17:45:33, sky wrote:
> > > Add a delegate method as to what should happen when the drag is completed.
> The
> > > default implementation should return a value indicating the menu should
> close.
> > > WrenchMenu would return otherwise. This code should just call through to
the
> > > MenuController and let it do the right thing.
> > 
> > Done.  However, this has a side-effect: other drags in the wrench menu
(e.g.,
> > bookmarks) now behave differently than they used to.  Is this okay?
> 
> Good point. I don't believe the bookmark code does the right thing in the
cast,
> so you should cancel on a drag from bookmarks. The delegate should only be
> consulted on drags initiated outside of menu code.

Tentatively done (see comment on new patch set).

https://codereview.chromium.org/378333003/diff/40001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/378333003/diff/40001/ui/views/widget/widget.h...
ui/views/widget/widget.h:798: virtual void OnDragComplete();
On 2014/07/11 19:15:20, sky wrote:
> On 2014/07/11 18:43:51, Devlin wrote:
> > On 2014/07/11 17:45:34, sky wrote:
> > > You should move the code in MenuController::StartDrag so that state is set
> > > correctly if a drag is initiated outside of the menu code. I believe this
> > means
> > > adding a method like OnDragWillStart() here having MenuHost override and
> > forward
> > > to MenuController. MenuController's OnDragWillStart would set
> > drag_in_progress_.
> > > A similar flow should happen at the end of the drag to either reset
capture
> or
> > > close menus.
> > 
> > Done (though not much of the code in StartDrag() is relevant, since the drag
> is
> > initiated and handled through the other View).
> 
> I wanted to make sure the state (which you're right, is only one boolean) is
set
> consistently for both cases. That's why moving to the method that is always
> invoked is ideal.

Yep, makes a lot of sense. :)  Just wanted to make sure that the only state that
applied was the bool, and that I wasn't missing something.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:155: if (dragged_view && dragged_view->id()
!= MenuItemView::kMenuItemViewID) {
I think this should work for ensuring the delegate is only consulted in the case
when the drag was initiated outside of menu code, but two concerns:
- Subclasses of MenuItemView don't seem to modify the id, but I'm not sure it's
wise to rely on that.  On the other hand, we *do* rely on it for things like
SubmenuView::GetMenuItemAt(), so maybe it's not a big deal.
- Is it safe to check |dragged_view| here, even if the view was removed?  It
seems like "yes" from the current example (the view is removed, and nothing has
crashed), but a sanity check is welcome. :)

[sky, 2014-07-14 15:19:05]

You also need to add test coverage, which means an interactive ui test. The
existing tests for menus are in bookmark_bar_view_test. You won't be able to add
a test for your change there though as all those tests use bookmarkbarview,
which you don't want.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:796: void
MenuController::OnDragWillStart() {
Position should match that of header.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:797: drag_in_progress_ = true;
Before this add a DCHECK(!drag_in_progress_).

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:801: drag_in_progress_ = false;
And DCHECK(drag_in_progress_)

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:802: if (GetActiveInstance() == this
&& showing_ && should_close) {
move the showing_ and should_close checks first.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_delegate.h (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_delegate.h:136: // true.
Document this is only invoked for menus originating outside of those on menu
items. Also, move this right above SelectionChanged() so that it's grouped with
the other dnd functions.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:141: if (menu_controller)
The if should be a DCHECK

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:155: if (dragged_view && dragged_view->id()
!= MenuItemView::kMenuItemViewID) {
On 2014/07/11 20:28:15, Devlin wrote:
> I think this should work for ensuring the delegate is only consulted in the
case
> when the drag was initiated outside of menu code, but two concerns:
> - Subclasses of MenuItemView don't seem to modify the id, but I'm not sure
it's
> wise to rely on that.  On the other hand, we *do* rely on it for things like
> SubmenuView::GetMenuItemAt(), so maybe it's not a big deal.
> - Is it safe to check |dragged_view| here, even if the view was removed?  It
> seems like "yes" from the current example (the view is removed, and nothing
has
> crashed), but a sanity check is welcome. :)

It would be safer to make MenuController know it initiated the drag. This way
you don't have to worry about any lifetime issues or whether the view is a
menuitem.

https://codereview.chromium.org/378333003/diff/100001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/widget/widget....
ui/views/widget/widget.h:800: // |view| is the view that was dragged. This may
be NULL.
I don't think you should pass in view here, it may have been deleted.

[Devlin, 2014-07-16 16:00:45]

Still working on the test - drag and drop seems to be a difficult area,
exacerbated by the fact that, apparently, they don't work on Linux.  Trying to
wrangle up a Windows box so I can iterate a bit faster.

Everything else, though, should be done. :)

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:796: void
MenuController::OnDragWillStart() {
On 2014/07/14 15:19:05, sky wrote:
> Position should match that of header.

Whoops!  Done.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:797: drag_in_progress_ = true;
On 2014/07/14 15:19:05, sky wrote:
> Before this add a DCHECK(!drag_in_progress_).

Done.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:801: drag_in_progress_ = false;
On 2014/07/14 15:19:05, sky wrote:
> And DCHECK(drag_in_progress_)

Done.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:802: if (GetActiveInstance() == this
&& showing_ && should_close) {
On 2014/07/14 15:19:05, sky wrote:
> move the showing_ and should_close checks first.

Done.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_delegate.h (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_delegate.h:136: // true.
On 2014/07/14 15:19:05, sky wrote:
> Document this is only invoked for menus originating outside of those on menu
> items. Also, move this right above SelectionChanged() so that it's grouped
with
> the other dnd functions.

Done.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:141: if (menu_controller)
On 2014/07/14 15:19:05, sky wrote:
> The if should be a DCHECK

Done.  For my own edification, why should it be DCHECK here, but if (...) most
other places (OnMouseCaptureLost, CreateRootView, OnOwnerClosing, etc)? 
Similarly, should it be a DCHECK in the newly-added OnDragComplete()?

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:155: if (dragged_view && dragged_view->id()
!= MenuItemView::kMenuItemViewID) {
On 2014/07/14 15:19:05, sky wrote:
> On 2014/07/11 20:28:15, Devlin wrote:
> > I think this should work for ensuring the delegate is only consulted in the
> case
> > when the drag was initiated outside of menu code, but two concerns:
> > - Subclasses of MenuItemView don't seem to modify the id, but I'm not sure
> it's
> > wise to rely on that.  On the other hand, we *do* rely on it for things like
> > SubmenuView::GetMenuItemAt(), so maybe it's not a big deal.
> > - Is it safe to check |dragged_view| here, even if the view was removed?  It
> > seems like "yes" from the current example (the view is removed, and nothing
> has
> > crashed), but a sanity check is welcome. :)
> 
> It would be safer to make MenuController know it initiated the drag. This way
> you don't have to worry about any lifetime issues or whether the view is a
> menuitem.

Done.

https://codereview.chromium.org/378333003/diff/100001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/widget/widget....
ui/views/widget/widget.h:800: // |view| is the view that was dragged. This may
be NULL.
On 2014/07/14 15:19:05, sky wrote:
> I don't think you should pass in view here, it may have been deleted.

Done.

[sky, 2014-07-16 19:19:01]

I didn't look at your test as you said it's still a work in the progress. The
rest is fine. I'll approve once you straighten out the test.

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:141: if (menu_controller)
On 2014/07/16 16:00:45, Devlin wrote:
> On 2014/07/14 15:19:05, sky wrote:
> > The if should be a DCHECK
> 
> Done.  For my own edification, why should it be DCHECK here, but if (...) most
> other places (OnMouseCaptureLost, CreateRootView, OnOwnerClosing, etc)? 
> Similarly, should it be a DCHECK in the newly-added OnDragComplete()?

MenuHost may outlive MenuController, hence the necessity to use DCHECKs in some
places. In this particular case I can't think of a reason why we would be
starting the drag with no menu controller.

[Devlin, 2014-07-16 19:42:51]

On 2014/07/16 19:19:01, sky wrote:
> I didn't look at your test as you said it's still a work in the progress. The
> rest is fine. I'll approve once you straighten out the test.
> 
>
https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
> File ui/views/controls/menu/menu_host.cc (right):
> 
>
https://codereview.chromium.org/378333003/diff/100001/ui/views/controls/menu/...
> ui/views/controls/menu/menu_host.cc:141: if (menu_controller)
> On 2014/07/16 16:00:45, Devlin wrote:
> > On 2014/07/14 15:19:05, sky wrote:
> > > The if should be a DCHECK
> > 
> > Done.  For my own edification, why should it be DCHECK here, but if (...)
most
> > other places (OnMouseCaptureLost, CreateRootView, OnOwnerClosing, etc)? 
> > Similarly, should it be a DCHECK in the newly-added OnDragComplete()?
> 
> MenuHost may outlive MenuController, hence the necessity to use DCHECKs in
some
> places. In this particular case I can't think of a reason why we would be
> starting the drag with no menu controller.

Okay, tests added and pass on a local windows machine.  Please take a look. :)

[sky, 2014-07-16 21:16:50]

LGTM

[Devlin, 2014-07-16 21:31:07]

On 2014/07/16 21:16:50, sky wrote:
> LGTM

Thanks for all your help on this one. =)  I know it was a lot of questions for
such a small change.

[Devlin, 2014-07-16 21:32:21]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-07-16 21:33:44]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/378333003/...

[commit-bot: I haz the power, 2014-07-17 06:17:42]

Change committed as 283656