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

ui/views/controls/menu/menu_host.cc

Index: ui/views/controls/menu/menu_host.cc
diff --git a/ui/views/controls/menu/menu_host.cc b/ui/views/controls/menu/menu_host.cc
index 2716ec8bb782c1a46288c0ab449d80a2a96e2caf..40d874927d132779960df84136f8ae3d1ac7be32 100644
--- a/ui/views/controls/menu/menu_host.cc
+++ b/ui/views/controls/menu/menu_host.cc
@@ -135,4 +135,34 @@ void MenuHost::OnOwnerClosing() {
     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