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

ui/views/controls/menu/menu_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/menu/menu_host.h"
 
 #include "base/auto_reset.h"
 #include "base/debug/trace_event.h"
 #include "ui/events/gestures/gesture_recognizer.h"
 #include "ui/gfx/path.h"
@@ skipping 117 matching lines @@
 void MenuHost::OnOwnerClosing() {
   if (destroying_)
     return;
 
   MenuController* menu_controller =
       submenu_->GetMenuItem()->GetMenuController();
   if (menu_controller && !menu_controller->drag_in_progress())
     menu_controller->CancelAll();
 }
 
+void MenuHost::OnDragWillStart() {
+  MenuController* menu_controller =
+      submenu_->GetMenuItem()->GetMenuController();
+  if (menu_controller)

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

The if should be a DCHECK

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

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()?

[sky, 2014/07/16 19:19:00]

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. 

+    menu_controller->OnDragWillStart();
+}
+
+void MenuHost::OnDragComplete(View* dragged_view) {
+  MenuController* menu_controller =
+      submenu_->GetMenuItem()->GetMenuController();
+  if (destroying_ || !menu_controller)
+    return;
+
+  bool should_close = true;
+  // If the view came from outside menu code (i.e., not a MenuItemView), we
+  // should consult the MenuDelegate to determine whether or not to close on
+  // exit.
+  if (dragged_view && dragged_view->id() != MenuItemView::kMenuItemViewID) {

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

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]

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.

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

Done.

+    MenuDelegate* menu_delegate = submenu_->GetMenuItem()->GetDelegate();
+    should_close =
+      menu_delegate ? menu_delegate->ShouldCloseOnDragComplete() : should_close;
+  }
+  menu_controller->OnDragComplete(should_close);
+
+  // We may have lost capture in the drag and drop, but are remaining open.
+  // Return capture so we get MouseCaptureLost events.
+  if (!should_close)
+    native_widget_private()->SetCapture();
+}
+
 }  // namespace views