Moves ownership of MenuItemView to MenuRunner as well as responbility

for showing (running) the menu entirely to MenuRunner. This way I can
guarantee if MenuRunner is deleted, the related menu classes aren't
deleted until the nested message loop completes.

BUG=57890
TEST=thorougly test all menus in chrome: bookmarks, wrench menu,
context menu on page, extension menus...
R=ben@chromium.org

Review URL: http://codereview.chromium.org/7720012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@98179 0039d316-1c4b-4281-b951-d872f2087c98

16 files changed, 423 insertions, 249 deletions

diff --git a/views/controls/button/button_dropdown.cc b/views/controls/button/button_dropdown.cc
index 16c0313..44b79d3 100644
--- a/views/controls/button/button_dropdown.cc
+++ b/views/controls/button/button_dropdown.cc
@@ -13,6 +13,7 @@
 #include "ui/base/models/menu_model.h"
 #include "views/controls/menu/menu_item_view.h"
 #include "views/controls/menu/menu_model_adapter.h"
+#include "views/controls/menu/menu_runner.h"
 #include "views/widget/widget.h"
 
 namespace views {
@@ -155,20 +156,21 @@ void ButtonDropDown::ShowDropDownMenu(gfx::NativeView window) {
   if (model_) {
     MenuModelAdapter menu_delegate(model_);
     menu_delegate.set_triggerable_event_flags(triggerable_event_flags());
-    MenuItemView menu(&menu_delegate);
-    menu_delegate.BuildMenu(&menu);
-
-    menu.RunMenuAt(GetWidget(), NULL,
-                   gfx::Rect(menu_position, gfx::Size(0, 0)),
-                   views::MenuItemView::TOPLEFT,
-                   true);
+    MenuRunner runner(menu_delegate.CreateMenu());
+    if (runner.RunMenuAt(GetWidget(), NULL,
+                         gfx::Rect(menu_position, gfx::Size(0, 0)),
+                         MenuItemView::TOPLEFT,
+                         MenuRunner::HAS_MNEMONICS) == MenuRunner::MENU_DELETED)
+      return;
   } else {
     MenuDelegate menu_delegate;
-    MenuItemView menu(&menu_delegate);
-    menu.RunMenuAt(GetWidget(), NULL,
-                   gfx::Rect(menu_position, gfx::Size(0, 0)),
-                   views::MenuItemView::TOPLEFT,
-                   true);
+    MenuItemView* menu = new MenuItemView(&menu_delegate);
+    MenuRunner runner(menu);
+    if (runner.RunMenuAt(GetWidget(), NULL,
+                         gfx::Rect(menu_position, gfx::Size(0, 0)),
+                         views::MenuItemView::TOPLEFT,
+                         MenuRunner::HAS_MNEMONICS) == MenuRunner::MENU_DELETED)
+      return;
   }
 
   // Need to explicitly clear mouse handler so that events get sent
diff --git a/views/controls/combobox/native_combobox_views.cc b/views/controls/combobox/native_combobox_views.cc
index 6a7b318..4696615 100644
--- a/views/controls/combobox/native_combobox_views.cc
+++ b/views/controls/combobox/native_combobox_views.cc
@@ -18,6 +18,7 @@
 #include "views/border.h"
 #include "views/controls/combobox/combobox.h"
 #include "views/controls/focusable_border.h"
+#include "views/controls/menu/menu_runner.h"
 #include "views/controls/menu/submenu_view.h"
 #include "views/widget/root_view.h"
 #include "views/widget/widget.h"
@@ -163,7 +164,9 @@ void NativeComboboxViews::UpdateFromModel() {
   int max_width = 0;
   const gfx::Font &font = GetFont();
 
-  dropdown_list_menu_.reset(new MenuItemView(this));
+  MenuItemView* menu = new MenuItemView(this);
+  // MenuRunner owns |menu|.
+  dropdown_list_menu_runner_.reset(new MenuRunner(menu));
 
   int num_items = combobox_->model()->GetItemCount();
   for (int i = 0; i < num_items; ++i) {
@@ -173,10 +176,10 @@ void NativeComboboxViews::UpdateFromModel() {
     // text is displayed correctly in right-to-left UIs.
     base::i18n::AdjustStringForLocaleDirection(&text);
 
-    dropdown_list_menu_->AppendMenuItem(i + kFirstMenuItemId, UTF16ToWide(text),
-         MenuItemView::NORMAL);
-     max_width = std::max(max_width, font.GetStringWidth(text));
-   }
+    menu->AppendMenuItem(i + kFirstMenuItemId, UTF16ToWide(text),
+                         MenuItemView::NORMAL);
+    max_width = std::max(max_width, font.GetStringWidth(text));
+  }
 
   content_width_ = max_width;
   content_height_ = font.GetFontSize();
@@ -342,11 +345,11 @@ void NativeComboboxViews::PaintText(gfx::Canvas* canvas) {
 
 void NativeComboboxViews::ShowDropDownMenu() {
 
-  if (!dropdown_list_menu_.get())
+  if (!dropdown_list_menu_runner_.get())
     UpdateFromModel();
 
   // Extend the menu to the width of the combobox.
-  SubmenuView* submenu = dropdown_list_menu_->CreateSubmenu();
+  SubmenuView* submenu = dropdown_list_menu_runner_->GetMenu()->CreateSubmenu();
   submenu->set_minimum_preferred_width(size().width());
 
   gfx::Rect lb = GetLocalBounds();
@@ -358,8 +361,10 @@ void NativeComboboxViews::ShowDropDownMenu() {
   gfx::Rect bounds(menu_position, lb.size());
 
   dropdown_open_ = true;
-  dropdown_list_menu_->RunMenuAt(NULL, NULL, bounds, MenuItemView::TOPLEFT,
-                                 true);
+  if (dropdown_list_menu_runner_->RunMenuAt(
+          NULL, NULL, bounds, MenuItemView::TOPLEFT,
+          MenuRunner::HAS_MNEMONICS) == MenuRunner::MENU_DELETED)
+    return;
   dropdown_open_ = false;
 
   // Need to explicitly clear mouse handler so that events get sent
diff --git a/views/controls/combobox/native_combobox_views.h b/views/controls/combobox/native_combobox_views.h
index f4cb56b..5722e96 100644
--- a/views/controls/combobox/native_combobox_views.h
+++ b/views/controls/combobox/native_combobox_views.h
@@ -19,6 +19,7 @@ namespace views {
 
 class KeyEvent;
 class FocusableBorder;
+class MenuRunner;
 
 // A views/skia only implementation of NativeComboboxWrapper.
 // No platform specific code is used.
@@ -82,8 +83,8 @@ class NativeComboboxViews : public views::View,
   // The reference to the border class. The object is owned by View::border_.
   FocusableBorder* text_border_;
 
-  // Context menu and its content list for the combobox.
-  scoped_ptr<views::MenuItemView> dropdown_list_menu_;
+  // Responsible for showing the context menu.
+  scoped_ptr<MenuRunner> dropdown_list_menu_runner_;
 
   // Is the drop down list showing
   bool dropdown_open_;
diff --git a/views/controls/menu/menu_controller.cc b/views/controls/menu/menu_controller.cc
index c44aa9a..cb53602 100644
--- a/views/controls/menu/menu_controller.cc
+++ b/views/controls/menu/menu_controller.cc
@@ -14,6 +14,7 @@
 #include "ui/gfx/canvas_skia.h"
 #include "ui/gfx/screen.h"
 #include "views/controls/button/menu_button.h"
+#include "views/controls/menu/menu_controller_delegate.h"
 #include "views/controls/menu/menu_scroll_view_container.h"
 #include "views/controls/menu/submenu_view.h"
 #include "views/drag_utils.h"
@@ -369,7 +370,7 @@ void MenuController::Cancel(ExitType type) {
   // If the menu has already been destroyed, no further cancellation is
   // needed.  We especially don't want to set the |exit_type_| to a lesser
   // value.
-  if (exit_type_ == EXIT_DESTROYED)
+  if (exit_type_ == EXIT_DESTROYED || exit_type_ == type)
     return;
 
   if (!showing_) {
@@ -391,7 +392,9 @@ void MenuController::Cancel(ExitType type) {
     // triggers deleting us.
     DCHECK(selected);
     showing_ = false;
-    selected->GetRootMenuItem()->DropMenuClosed(true);
+    delegate_->DropMenuClosed(
+        internal::MenuControllerDelegate::NOTIFY_DELEGATE,
+        selected->GetRootMenuItem());
     // WARNING: the call to MenuClosed deletes us.
     return;
   }
@@ -716,8 +719,11 @@ int MenuController::OnPerformDrop(SubmenuView* source,
   if (drop_target->id() == MenuItemView::kEmptyMenuItemViewID)
     drop_target = drop_target->GetParentMenuItem();
 
-  if (!IsBlockingRun())
-    item->GetRootMenuItem()->DropMenuClosed(false);
+  if (!IsBlockingRun()) {
+    delegate_->DropMenuClosed(
+        internal::MenuControllerDelegate::DONT_NOTIFY_DELEGATE,
+        item->GetRootMenuItem());
+  }
 
   // WARNING: the call to MenuClosed deletes us.
 
@@ -808,11 +814,6 @@ void MenuController::SetSelection(MenuItemView* menu_item,
   }
 }
 
-// static
-void MenuController::SetActiveInstance(MenuController* controller) {
-  active_instance_ = controller;
-}
-
 #if defined(OS_WIN)
 bool MenuController::Dispatch(const MSG& msg) {
   DCHECK(blocking_run_);
@@ -994,7 +995,8 @@ bool MenuController::OnKeyDown(int key_code
   return true;
 }
 
-MenuController::MenuController(bool blocking)
+MenuController::MenuController(bool blocking,
+                               internal::MenuControllerDelegate* delegate)
     : blocking_run_(blocking),
       showing_(false),
       exit_type_(EXIT_NONE),
@@ -1008,11 +1010,15 @@ MenuController::MenuController(bool blocking)
       valid_drop_coordinates_(false),
       showing_submenu_(false),
       menu_button_(NULL),
-      active_mouse_view_(NULL) {
+      active_mouse_view_(NULL),
+      delegate_(delegate) {
+  active_instance_ = this;
 }
 
 MenuController::~MenuController() {
   DCHECK(!showing_);
+  if (active_instance_ == this)
+    active_instance_ = NULL;
   StopShowTimer();
   StopCancelAllTimer();
 }
@@ -1095,6 +1101,8 @@ bool MenuController::ShowSiblingMenu(SubmenuView* source,
   if (!alt_menu || (state_.item && state_.item->GetRootMenuItem() == alt_menu))
     return false;
 
+  delegate_->SiblingMenuCreated(alt_menu);
+
   if (!button) {
     // If the delegate returns a menu, they must also return a button.
     NOTREACHED();
diff --git a/views/controls/menu/menu_controller.h b/views/controls/menu/menu_controller.h
index cf2fa49..35b7a78 100644
--- a/views/controls/menu/menu_controller.h
+++ b/views/controls/menu/menu_controller.h
@@ -32,6 +32,11 @@ class MouseEvent;
 class SubmenuView;
 class View;
 
+namespace internal {
+class MenuControllerDelegate;
+class MenuRunnerImpl;
+}
+
 // MenuController -------------------------------------------------------------
 
 // MenuController is used internally by the various menu classes to manage
@@ -39,10 +44,6 @@ class View;
 // forwarded to the MenuController from SubmenuView and MenuHost.
 class VIEWS_EXPORT MenuController : public MessageLoop::Dispatcher {
  public:
-  friend class MenuHostRootView;
-  friend class MenuItemView;
-  friend class SubmenuView;
-
   // Enumeration of how the menu should exit.
   enum ExitType {
     // Don't exit.
@@ -122,6 +123,11 @@ class VIEWS_EXPORT MenuController : public MessageLoop::Dispatcher {
   void OnWidgetActivationChanged();
 
  private:
+  friend class internal::MenuRunnerImpl;
+  friend class MenuHostRootView;
+  friend class MenuItemView;
+  friend class SubmenuView;
+
   class MenuScrollTask;
 
   struct SelectByCharDetails;
@@ -209,9 +215,6 @@ class VIEWS_EXPORT MenuController : public MessageLoop::Dispatcher {
   // to show/hide submenus and update state_.
   void SetSelection(MenuItemView* menu_item, int types);
 
-  // Sets the active MenuController.
-  static void SetActiveInstance(MenuController* controller);
-
 #if defined(OS_WIN)
   // Dispatcher method. This returns true if the menu was canceled, or
   // if the message is such that the menu should be closed.
@@ -231,8 +234,9 @@ class VIEWS_EXPORT MenuController : public MessageLoop::Dispatcher {
   bool OnKeyDown(int key_code);
 #endif
 
-  // Creates a MenuController. If blocking is true, Run blocks the caller
-  explicit MenuController(bool blocking);
+  // Creates a MenuController. If |blocking| is true a nested message loop is
+  // started in |Run|.
+  MenuController(bool blocking, internal::MenuControllerDelegate* delegate);
 
   virtual ~MenuController();
 
@@ -494,6 +498,8 @@ class VIEWS_EXPORT MenuController : public MessageLoop::Dispatcher {
   // UpdateActiveMouseView for details.
   View* active_mouse_view_;
 
+  internal::MenuControllerDelegate* delegate_;
+
   DISALLOW_COPY_AND_ASSIGN(MenuController);
 };
 
diff --git a/views/controls/menu/menu_controller_delegate.h b/views/controls/menu/menu_controller_delegate.h
new file mode 100644
index 0000000..db68579
--- /dev/null
+++ b/views/controls/menu/menu_controller_delegate.h
@@ -0,0 +1,41 @@
+// Copyright (c) 2011 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.
+
+#ifndef VIEWS_CONTROLS_MENU_MENU_CONTROLLER_DELEGATE_H_
+#define VIEWS_CONTROLS_MENU_MENU_CONTROLLER_DELEGATE_H_
+#pragma once
+
+namespace views {
+
+class MenuItemView;
+
+// This is internal as there should be no need for usage of this class outside
+// of views.
+namespace internal {
+
+// Used by MenuController to notify of interesting events that are intended for
+// the class using MenuController. This is implemented by MenuRunnerImpl.
+class MenuControllerDelegate {
+ public:
+  enum NotifyType {
+    NOTIFY_DELEGATE,
+    DONT_NOTIFY_DELEGATE
+  };
+
+  // Invoked when MenuController closes a menu and the MenuController was
+  // configured for drop (MenuRunner::FOR_DROP).
+  virtual void DropMenuClosed(NotifyType type, MenuItemView* menu) = 0;
+
+  // Invoked when the MenuDelegate::GetSiblingMenu() returns non-NULL.
+  virtual void SiblingMenuCreated(MenuItemView* menu) = 0;
+
+ protected:
+  virtual ~MenuControllerDelegate() {}
+};
+
+}  // namespace internal
+
+}  // namespace view
+
+#endif  // VIEWS_CONTROLS_MENU_MENU_CONTROLLER_DELEGATE_H_
diff --git a/views/controls/menu/menu_item_view.cc b/views/controls/menu/menu_item_view.cc
index df4a2a8..8181793 100644
--- a/views/controls/menu/menu_item_view.cc
+++ b/views/controls/menu/menu_item_view.cc
@@ -19,10 +19,6 @@
 #include "views/controls/menu/menu_separator.h"
 #include "views/controls/menu/submenu_view.h"
 
-#if defined(OS_WIN)
-#include "base/win/win_util.h"
-#endif
-
 namespace views {
 
 namespace {
@@ -100,16 +96,6 @@ MenuItemView::MenuItemView(MenuDelegate* delegate)
   Init(NULL, 0, SUBMENU, delegate);
 }
 
-MenuItemView::~MenuItemView() {
-  // TODO(sky): ownership is bit wrong now. In particular if a nested message
-  // loop is running deletion can't be done, otherwise the stack gets
-  // thoroughly screwed. The destructor should be made private, and
-  // MenuController should be the only place handling deletion of the menu.
-  // (57890).
-  delete submenu_;
-  STLDeleteElements(&removed_items_);
-}
-
 void MenuItemView::ChildPreferredSizeChanged(View* child) {
   pref_size_.SetSize(0, 0);
   PreferredSizeChanged();
@@ -192,86 +178,6 @@ string16 MenuItemView::GetAccessibleNameForMenuItem(
   return accessible_name;
 }
 
-void MenuItemView::RunMenuAt(Widget* parent,
-                             MenuButton* button,
-                             const gfx::Rect& bounds,
-                             AnchorPosition anchor,
-                             bool has_mnemonics) {
-  // Show mnemonics if the button has focus or alt is pressed.
-  bool show_mnemonics = button ? button->HasFocus() : false;
-#if defined(OS_WIN)
-  // We don't currently need this on gtk as showing the menu gives focus to the
-  // button first.
-  if (!show_mnemonics)
-    show_mnemonics = base::win::IsAltPressed();
-#endif
-  PrepareForRun(has_mnemonics, show_mnemonics);
-  int mouse_event_flags;
-
-  MenuController* controller = MenuController::GetActiveInstance();
-  if (controller && !controller->IsBlockingRun()) {
-    // A menu is already showing, but it isn't a blocking menu. Cancel it.
-    // We can get here during drag and drop if the user right clicks on the
-    // menu quickly after the drop.
-    controller->Cancel(MenuController::EXIT_ALL);
-    controller = NULL;
-  }
-  bool owns_controller = false;
-  if (!controller) {
-    // No menus are showing, show one.
-    controller = new MenuController(true);
-    MenuController::SetActiveInstance(controller);
-    owns_controller = true;
-  } else {
-    // A menu is already showing, use the same controller.
-
-    // Don't support blocking from within non-blocking.
-    DCHECK(controller->IsBlockingRun());
-  }
-
-  controller_ = controller;
-
-  // Run the loop.
-  MenuItemView* result =
-      controller->Run(parent, button, this, bounds, anchor, &mouse_event_flags);
-
-  RemoveEmptyMenus();
-
-  controller_ = NULL;
-
-  if (owns_controller) {
-    // We created the controller and need to delete it.
-    if (MenuController::GetActiveInstance() == controller)
-      MenuController::SetActiveInstance(NULL);
-    delete controller;
-  }
-  // Make sure all the windows we created to show the menus have been
-  // destroyed.
-  DestroyAllMenuHosts();
-  if (result && delegate_)
-    delegate_->ExecuteCommand(result->GetCommand(), mouse_event_flags);
-}
-
-void MenuItemView::RunMenuForDropAt(Widget* parent,
-                                    const gfx::Rect& bounds,
-                                    AnchorPosition anchor) {
-  PrepareForRun(false, false);
-
-  // If there is a menu, hide it so that only one menu is shown during dnd.
-  MenuController* current_controller = MenuController::GetActiveInstance();
-  if (current_controller) {
-    current_controller->Cancel(MenuController::EXIT_ALL);
-  }
-
-  // Always create a new controller for non-blocking.
-  controller_ = new MenuController(false);
-
-  // Set the instance, that way it can be canceled by another menu.
-  MenuController::SetActiveInstance(controller_);
-
-  controller_->Run(parent, NULL, this, bounds, anchor, NULL);
-}
-
 void MenuItemView::Cancel() {
   if (controller_ && !canceled_) {
     canceled_ = true;
@@ -558,6 +464,11 @@ MenuItemView::MenuItemView(MenuItemView* parent,
   Init(parent, command, type, NULL);
 }
 
+MenuItemView::~MenuItemView() {
+  delete submenu_;
+  STLDeleteElements(&removed_items_);
+}
+
 std::string MenuItemView::GetClassName() const {
   return kViewClassName;
 }
@@ -612,23 +523,6 @@ void MenuItemView::Init(MenuItemView* parent,
     SetEnabled(root_delegate->IsCommandEnabled(command));
 }
 
-void MenuItemView::DropMenuClosed(bool notify_delegate) {
-  DCHECK(controller_);
-  DCHECK(!controller_->IsBlockingRun());
-  if (MenuController::GetActiveInstance() == controller_)
-    MenuController::SetActiveInstance(NULL);
-  delete controller_;
-  controller_ = NULL;
-
-  RemoveEmptyMenus();
-
-  if (notify_delegate && delegate_) {
-    // Our delegate is null when invoked from the destructor.
-    delegate_->DropMenuClosed(this);
-  }
-  // WARNING: its possible the delegate deleted us at this point.
-}
-
 void MenuItemView::PrepareForRun(bool has_mnemonics, bool show_mnemonics) {
   // Currently we only support showing the root.
   DCHECK(!parent_menu_item_);
diff --git a/views/controls/menu/menu_item_view.h b/views/controls/menu/menu_item_view.h
index af8f0f0..6b91c8d 100644
--- a/views/controls/menu/menu_item_view.h
+++ b/views/controls/menu/menu_item_view.h
@@ -33,13 +33,16 @@ class MenuModel;
 
 namespace views {
 
+namespace internal {
+class MenuRunnerImpl;
+}
+
 class MenuButton;
+struct MenuConfig;
 class MenuController;
 class MenuDelegate;
 class SubmenuView;
 
-struct MenuConfig;
-
 // MenuItemView --------------------------------------------------------------
 
 // MenuItemView represents a single menu item with a label and optional icon.
@@ -59,12 +62,8 @@ struct MenuConfig;
 // focus from the hosting window child views do not actually get focus. Instead
 // |SetHotTracked| is used as the user navigates around.
 //
-// There are two ways to show a MenuItemView:
-// 1. Use RunMenuAt. This blocks the caller, executing the selected command
-//    on success.
-// 2. Use RunMenuForDropAt. This is intended for use during a drop session
-//    and does NOT block the caller. Instead the delegate is notified when the
-//    menu closes via the DropMenuClosed method.
+// To show the menu use MenuRunner. See MenuRunner for details on how to run
+// (show) the menu as well as for details on the life time of the menu.
 
 class VIEWS_EXPORT MenuItemView : public View {
  public:
@@ -111,8 +110,6 @@ class VIEWS_EXPORT MenuItemView : public View {
   // shown to the user, rather its use as the parent for all menu items.
   explicit MenuItemView(MenuDelegate* delegate);
 
-  virtual ~MenuItemView();
-
   // Overridden from View:
   virtual bool GetTooltipText(const gfx::Point& p,
                               std::wstring* tooltip) OVERRIDE;
@@ -130,20 +127,6 @@ class VIEWS_EXPORT MenuItemView : public View {
   static string16 GetAccessibleNameForMenuItem(
       const string16& item_text, const string16& accelerator_text);
 
-  // Run methods. See description above class for details. Both Run methods take
-  // a rectangle, which is used to position the menu. |has_mnemonics| indicates
-  // whether the items have mnemonics. Mnemonics are identified by way of the
-  // character following the '&'.  The anchor position is specified for non-RTL
-  // languages; the opposite value will be used for RTL.
-  void RunMenuAt(Widget* parent,
-                 MenuButton* button,
-                 const gfx::Rect& bounds,
-                 AnchorPosition anchor,
-                 bool has_mnemonics);
-  void RunMenuForDropAt(Widget* parent,
-                        const gfx::Rect& bounds,
-                        AnchorPosition anchor);
-
   // Hides and cancels the menu. This does nothing if the menu is not open.
   void Cancel();
 
@@ -288,6 +271,7 @@ class VIEWS_EXPORT MenuItemView : public View {
 
   // Returns the delegate. This returns the delegate of the root menu item.
   MenuDelegate* GetDelegate();
+  void set_delegate(MenuDelegate* delegate) { delegate_ = delegate; }
 
   // Returns the root parent, or this if this has no parent.
   MenuItemView* GetRootMenuItem();
@@ -334,11 +318,16 @@ class VIEWS_EXPORT MenuItemView : public View {
   // Creates a MenuItemView. This is used by the various AddXXX methods.
   MenuItemView(MenuItemView* parent, int command, Type type);
 
+  // MenuRunner owns MenuItemView and should be the only one deleting it.
+  virtual ~MenuItemView();
+
   virtual void ChildPreferredSizeChanged(View* child) OVERRIDE;
 
   virtual std::string GetClassName() const OVERRIDE;
 
  private:
+  friend class internal::MenuRunnerImpl;  // For access to ~MenuItemView.
+
   // Calculates all sizes that we can from the OS.
   //
   // This is invoked prior to Running a menu.
@@ -350,10 +339,6 @@ class VIEWS_EXPORT MenuItemView : public View {
             MenuItemView::Type type,
             MenuDelegate* delegate);
 
-  // Invoked by the MenuController when the menu closes as the result of
-  // drag and drop run.
-  void DropMenuClosed(bool notify_delegate);
-
   // The RunXXX methods call into this to set up the necessary state before
   // running.
   void PrepareForRun(bool has_mnemonics, bool show_mnemonics);
@@ -418,13 +403,14 @@ class VIEWS_EXPORT MenuItemView : public View {
     actual_menu_position_ = actual_menu_position;
   }
 
+  void set_controller(MenuController* controller) { controller_ = controller; }
+
   // The delegate. This is only valid for the root menu item. You shouldn't
   // use this directly, instead use GetDelegate() which walks the tree as
   // as necessary.
   MenuDelegate* delegate_;
 
-  // Returns the controller for the run operation, or NULL if the menu isn't
-  // showing.
+  // The controller for the run operation, or NULL if the menu isn't showing.
   MenuController* controller_;
 
   // Used to detect when Cancel was invoked.
diff --git a/views/controls/menu/menu_model_adapter.cc b/views/controls/menu/menu_model_adapter.cc
index e2c895d..850b6b5 100644
--- a/views/controls/menu/menu_model_adapter.cc
+++ b/views/controls/menu/menu_model_adapter.cc
@@ -44,6 +44,12 @@ void MenuModelAdapter::BuildMenu(MenuItemView* menu) {
   menu->ChildrenChanged();
 }
 
+MenuItemView* MenuModelAdapter::CreateMenu() {
+  MenuItemView* item = new MenuItemView(this);
+  BuildMenu(item);
+  return item;
+}
+
 // MenuModelAdapter, MenuDelegate implementation:
 
 void MenuModelAdapter::ExecuteCommand(int id) {
diff --git a/views/controls/menu/menu_model_adapter.h b/views/controls/menu/menu_model_adapter.h
index a1a90a2..ba50ef4 100644
--- a/views/controls/menu/menu_model_adapter.h
+++ b/views/controls/menu/menu_model_adapter.h
@@ -30,6 +30,10 @@ class VIEWS_EXPORT MenuModelAdapter : public MenuDelegate {
   // (including submenus).
   virtual void BuildMenu(MenuItemView* menu);
 
+  // Convenience for creating and populating a menu. The caller owns the
+  // returned MenuItemView.
+  MenuItemView* CreateMenu();
+
   void set_triggerable_event_flags(int triggerable_event_flags) {
     triggerable_event_flags_ = triggerable_event_flags;
   }
diff --git a/views/controls/menu/menu_model_adapter_unittest.cc b/views/controls/menu/menu_model_adapter_unittest.cc
index 035ec16..f797d40 100644
--- a/views/controls/menu/menu_model_adapter_unittest.cc
+++ b/views/controls/menu/menu_model_adapter_unittest.cc
@@ -7,6 +7,7 @@
 #include "ui/base/models/menu_model_delegate.h"
 #include "views/controls/menu/menu_item_view.h"
 #include "views/controls/menu/menu_model_adapter.h"
+#include "views/controls/menu/menu_runner.h"
 #include "views/controls/menu/submenu_view.h"
 #include "views/test/views_test_base.h"
 
@@ -200,9 +201,11 @@ TEST_F(MenuModelAdapterTest, BasicTest) {
   views::MenuModelAdapter delegate(&model);
 
   // Create menu.  Build menu twice to check that rebuilding works properly.
-  scoped_ptr<views::MenuItemView> menu(new views::MenuItemView(&delegate));
-  delegate.BuildMenu(menu.get());
-  delegate.BuildMenu(menu.get());
+  MenuItemView* menu = new views::MenuItemView(&delegate);
+  // MenuRunner takes ownership of menu.
+  scoped_ptr<MenuRunner> menu_runner(new MenuRunner(menu));
+  delegate.BuildMenu(menu);
+  delegate.BuildMenu(menu);
   EXPECT_TRUE(menu->HasSubmenu());
 
   // Check top level menu items.
@@ -297,7 +300,7 @@ TEST_F(MenuModelAdapterTest, BasicTest) {
   // Check that selecting the root item is safe.  The MenuModel does
   // not care about the root so MenuModelAdapter should do nothing
   // (not hit the NOTREACHED check) when the root is selected.
-  static_cast<views::MenuDelegate*>(&delegate)->SelectionChanged(menu.get());
+  static_cast<views::MenuDelegate*>(&delegate)->SelectionChanged(menu);
 }
 
 }  // namespace views
diff --git a/views/controls/menu/menu_runner.cc b/views/controls/menu/menu_runner.cc
index 1941d69a..89cb114 100644
--- a/views/controls/menu/menu_runner.cc
+++ b/views/controls/menu/menu_runner.cc
@@ -4,30 +4,69 @@
 
 #include "views/controls/menu/menu_runner.h"
 
+#include <set>
+
+#include "views/controls/button/menu_button.h"
+#include "views/controls/menu/menu_controller.h"
+#include "views/controls/menu/menu_controller_delegate.h"
+#include "views/controls/menu/menu_delegate.h"
+
+#if defined(OS_WIN)
+#include "base/win/win_util.h"
+#endif
+
 namespace views {
 
-// Manages the menu. To destroy a Holder invoke Release(). Release() deletes
-// immediately if the menu isn't showing. If the menu is showing Release()
-// cancels the menu and when the nested RunMenuAt() call returns, deletes itself
-// and the menu.
-class MenuRunner::Holder {
+namespace internal {
+
+// Manages the menu. To destroy a MenuRunnerImpl invoke Release(). Release()
+// deletes immediately if the menu isn't showing. If the menu is showing
+// Release() cancels the menu and when the nested RunMenuAt() call returns
+// deletes itself and the menu.
+class MenuRunnerImpl : public internal::MenuControllerDelegate {
  public:
-  explicit Holder(MenuItemView* menu);
+  explicit MenuRunnerImpl(MenuItemView* menu);
+
+  MenuItemView* menu() { return menu_; }
 
   // See description above class for details.
   void Release();
 
   // Runs the menu.
-  void RunMenuAt(Widget* parent,
-                 MenuButton* button,
-                 const gfx::Rect& bounds,
-                 MenuItemView::AnchorPosition anchor,
-                 bool has_mnemonics);
+  MenuRunner::RunResult RunMenuAt(Widget* parent,
+                                  MenuButton* button,
+                                  const gfx::Rect& bounds,
+                                  MenuItemView::AnchorPosition anchor,
+                                  int32 types) WARN_UNUSED_RESULT;
+
+  void Cancel();
+
+  // MenuControllerDelegate:
+  virtual void DropMenuClosed(NotifyType type, MenuItemView* menu) OVERRIDE;
+  virtual void SiblingMenuCreated(MenuItemView* menu) OVERRIDE;
 
  private:
-  ~Holder();
+  ~MenuRunnerImpl();
+
+  // Cleans up after the menu is no longer showing. |result| is the menu that
+  // the user selected, or NULL if nothing was selected.
+  MenuRunner::RunResult MenuDone(MenuItemView* result, int mouse_event_flags);
+
+  // Returns true if mnemonics should be shown in the menu.
+  bool ShouldShowMnemonics(MenuButton* button);
+
+  // The menu. We own this. We don't use scoped_ptr as the destructor is
+  // protected and we're a friend.
+  MenuItemView* menu_;
+
+  // Any sibling menus. Does not include |menu_|. We own these too.
+  std::set<MenuItemView*> sibling_menus_;
 
-  scoped_ptr<MenuItemView> menu_;
+  // Created and set as the delegate of the MenuItemView if Release() is
+  // invoked.  This is done to make sure the delegate isn't notified after
+  // Release() is invoked. We do this as we assume the delegate is no longer
+  // valid if MenuRunner has been deleted.
+  scoped_ptr<MenuDelegate> empty_delegate_;
 
   // Are we in run waiting for it to return?
   bool running_;
@@ -35,63 +74,198 @@ class MenuRunner::Holder {
   // Set if |running_| and Release() has been invoked.
   bool delete_after_run_;
 
-  DISALLOW_COPY_AND_ASSIGN(Holder);
+  // Are we running for a drop?
+  bool for_drop_;
+
+  // The controller.
+  MenuController* controller_;
+
+  // Do we own the controller?
+  bool owns_controller_;
+
+  DISALLOW_COPY_AND_ASSIGN(MenuRunnerImpl);
 };
 
-MenuRunner::Holder::Holder(MenuItemView* menu)
+MenuRunnerImpl::MenuRunnerImpl(MenuItemView* menu)
     : menu_(menu),
       running_(false),
-      delete_after_run_(false) {
+      delete_after_run_(false),
+      for_drop_(false),
+      controller_(NULL),
+      owns_controller_(false) {
 }
 
-void MenuRunner::Holder::Release() {
+void MenuRunnerImpl::Release() {
   if (running_) {
+    if (delete_after_run_)
+      return;  // We already canceled.
+
     // The menu is running a nested message loop, we can't delete it now
     // otherwise the stack would be in a really bad state (many frames would
     // have deleted objects on them). Instead cancel the menu, when it returns
     // Holder will delete itself.
     delete_after_run_ = true;
+
+    // Swap in a different delegate. That way we know the original MenuDelegate
+    // won't be notified later on (when it's likely already been deleted).
+    if (!empty_delegate_.get())
+      empty_delegate_.reset(new MenuDelegate());
+    menu_->set_delegate(empty_delegate_.get());
+
     menu_->Cancel();
   } else {
     delete this;
   }
 }
 
-void MenuRunner::Holder::RunMenuAt(Widget* parent,
-                                   MenuButton* button,
-                                   const gfx::Rect& bounds,
-                                   MenuItemView::AnchorPosition anchor,
-                                   bool has_mnemonics) {
+MenuRunner::RunResult MenuRunnerImpl::RunMenuAt(
+    Widget* parent,
+    MenuButton* button,
+    const gfx::Rect& bounds,
+    MenuItemView::AnchorPosition anchor,
+    int32 types) {
   if (running_) {
     // Ignore requests to show the menu while it's already showing. MenuItemView
     // doesn't handle this very well (meaning it crashes).
-    return;
+    return MenuRunner::NORMAL_EXIT;
+  }
+
+  MenuController* controller = MenuController::GetActiveInstance();
+  if (controller) {
+    if ((types & MenuRunner::IS_NESTED) != 0) {
+      if (!controller->IsBlockingRun()) {
+        controller->CancelAll();
+        controller = NULL;
+      }
+    } else {
+      // There's some other menu open and we're not nested. Cancel the menu.
+      controller->CancelAll();
+      if ((types & MenuRunner::FOR_DROP) == 0) {
+        // We can't open another menu, otherwise the message loop would become
+        // twice nested. This isn't necessarily a problem, but generally isn't
+        // expected.
+        return MenuRunner::NORMAL_EXIT;
+      }
+      // Drop menus don't block the message loop, so it's ok to create a new
+      // MenuController.
+      controller = NULL;
+    }
   }
   running_ = true;
-  menu_->RunMenuAt(parent, button, bounds, anchor, has_mnemonics);
+  for_drop_ = (types & MenuRunner::FOR_DROP) != 0;
+  bool has_mnemonics = (types & MenuRunner::HAS_MNEMONICS) != 0 && !for_drop_;
+  menu_->PrepareForRun(has_mnemonics,
+                       !for_drop_ && ShouldShowMnemonics(button));
+  int mouse_event_flags = 0;
+  owns_controller_ = false;
+  if (!controller) {
+    // No menus are showing, show one.
+    controller = new MenuController(!for_drop_, this);
+    owns_controller_ = true;
+  }
+  controller_ = controller;
+  menu_->set_controller(controller_);
+
+  // Run the loop.
+  MenuItemView* result = controller->Run(parent, button, menu_, bounds, anchor,
+                                         &mouse_event_flags);
+
+  if (for_drop_) {
+    // Drop menus return immediately. We finish processing in DropMenuClosed.
+    return MenuRunner::NORMAL_EXIT;
+  }
+
+  return MenuDone(result, mouse_event_flags);
+}
+
+void MenuRunnerImpl::Cancel() {
+  if (running_)
+    controller_->Cancel(MenuController::EXIT_ALL);
+}
+
+void MenuRunnerImpl::DropMenuClosed(NotifyType type, MenuItemView* menu) {
+  MenuDone(NULL, 0);
+
+  if (type == NOTIFY_DELEGATE && menu->GetDelegate()) {
+    // Delegate is null when invoked from the destructor.
+    menu->GetDelegate()->DropMenuClosed(menu);
+  }
+}
+
+void MenuRunnerImpl::SiblingMenuCreated(MenuItemView* menu) {
+  if (menu != menu_ && sibling_menus_.count(menu) == 0)
+    sibling_menus_.insert(menu);
+}
+
+MenuRunnerImpl::~MenuRunnerImpl() {
+  delete menu_;
+  for (std::set<MenuItemView*>::iterator i = sibling_menus_.begin();
+       i != sibling_menus_.end(); ++i)
+    delete *i;
+}
+
+MenuRunner::RunResult MenuRunnerImpl::MenuDone(MenuItemView* result,
+                                               int mouse_event_flags) {
+  menu_->RemoveEmptyMenus();
+  menu_->set_controller(NULL);
+
+  if (owns_controller_) {
+    // We created the controller and need to delete it.
+    delete controller_;
+    owns_controller_ = false;
+  }
+  controller_ = NULL;
+  // Make sure all the windows we created to show the menus have been
+  // destroyed.
+  menu_->DestroyAllMenuHosts();
   if (delete_after_run_) {
     delete this;
-    return;
+    return MenuRunner::MENU_DELETED;
   }
   running_ = false;
+  if (result && menu_->GetDelegate()) {
+    menu_->GetDelegate()->ExecuteCommand(result->GetCommand(),
+                                         mouse_event_flags);
+  }
+  return MenuRunner::NORMAL_EXIT;
 }
 
-MenuRunner::Holder::~Holder() {
+bool MenuRunnerImpl::ShouldShowMnemonics(MenuButton* button) {
+  // Show mnemonics if the button has focus or alt is pressed.
+  bool show_mnemonics = button ? button->HasFocus() : false;
+#if defined(OS_WIN)
+  // We don't currently need this on gtk as showing the menu gives focus to the
+  // button first.
+  if (!show_mnemonics)
+    show_mnemonics = base::win::IsAltPressed();
+#endif
+  return show_mnemonics;
 }
 
-MenuRunner::MenuRunner(MenuItemView* menu) : holder_(new Holder(menu)) {
+}  // namespace internal
+
+MenuRunner::MenuRunner(MenuItemView* menu)
+    : holder_(new internal::MenuRunnerImpl(menu)) {
 }
 
 MenuRunner::~MenuRunner() {
   holder_->Release();
 }
 
-void MenuRunner::RunMenuAt(Widget* parent,
-                           MenuButton* button,
-                           const gfx::Rect& bounds,
-                           MenuItemView::AnchorPosition anchor,
-                           bool has_mnemonics) {
-  holder_->RunMenuAt(parent, button, bounds, anchor, has_mnemonics);
+MenuItemView* MenuRunner::GetMenu() {
+  return holder_->menu();
+}
+
+MenuRunner::RunResult MenuRunner::RunMenuAt(Widget* parent,
+                                            MenuButton* button,
+                                            const gfx::Rect& bounds,
+                                            MenuItemView::AnchorPosition anchor,
+                                            int32 types) {
+  return holder_->RunMenuAt(parent, button, bounds, anchor, types);
+}
+
+void MenuRunner::Cancel() {
+  holder_->Cancel();
 }
 
 }  // namespace views
diff --git a/views/controls/menu/menu_runner.h b/views/controls/menu/menu_runner.h
index 1337d08..5cdb845 100644
--- a/views/controls/menu/menu_runner.h
+++ b/views/controls/menu/menu_runner.h
@@ -7,6 +7,7 @@
 #pragma once
 
 #include "base/basictypes.h"
+#include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "views/controls/menu/menu_item_view.h"
 
@@ -15,32 +16,73 @@ namespace views {
 class MenuButton;
 class Widget;
 
-// MenuRunner handles the lifetime of the root MenuItemView. MenuItemView runs a
-// nested message loop, which means care must be taken when the MenuItemView
-// needs to be deleted. MenuRunner makes sure the menu is deleted after the
-// nested message loop completes.
-//
-// MenuRunner can be deleted at any time and will correctly handle deleting the
-// underlying menu.
+namespace internal {
+class MenuRunnerImpl;
+}
+
+// MenuRunner is responsible for showing (running) the menu and additionally
+// owning the MenuItemView. RunMenuAt() runs a nested message loop. It is safe
+// to delete MenuRunner at any point, but MenuRunner internally only deletes the
+// MenuItemView *after* the nested message loop completes. If MenuRunner is
+// deleted while the menu is showing the delegate of the menu is reset. This is
+// done to ensure delegates aren't notified after they may have been deleted.
 //
-// TODO: this is a work around for 57890. If we fix it this class shouldn't be
-// needed.
+// NOTE: while you can delete a MenuRunner at any point, the nested message loop
+// won't return immediately. This means if you delete the object that owns
+// the MenuRunner while the menu is running, your object is effectively still
+// on the stack. A return value of MENU_DELETED indicated this. In most cases
+// if RunMenuAt() returns MENU_DELETED, you should return immediately.
 class VIEWS_EXPORT MenuRunner {
  public:
+  enum RunTypes {
+    // The menu has mnemonics.
+    HAS_MNEMONICS = 1 << 0,
+
+    // The menu is a nested context menu. For example, click a folder on the
+    // bookmark bar, then right click an entry to get its context menu.
+    IS_NESTED     = 1 << 1,
+
+    // Used for showing a menu during a drop operation. This does NOT block the
+    // caller, instead the delegate is notified when the menu closes via the
+    // DropMenuClosed method.
+    FOR_DROP      = 1 << 2,
+  };
+
+  enum RunResult {
+    // Indicates RunMenuAt is returning because the MenuRunner was deleted.
+    MENU_DELETED,
+
+    // Indicates RunMenuAt returned and MenuRunner was not deleted.
+    NORMAL_EXIT
+  };
+
+  // Creates a new MenuRunner. MenuRunner owns the supplied menu.
   explicit MenuRunner(MenuItemView* menu);
   ~MenuRunner();
 
-  // Runs the menu.
-  void RunMenuAt(Widget* parent,
-                 MenuButton* button,
-                 const gfx::Rect& bounds,
-                 MenuItemView::AnchorPosition anchor,
-                 bool has_mnemonics);
+  // Returns the menu.
+  MenuItemView* GetMenu();
 
- private:
-  class Holder;
+  // Takes ownership of |menu|, deleting it when MenuRunner is deleted. You
+  // only need call this if you create additional menus from
+  // MenuDelegate::GetSiblingMenu.
+  void OwnMenu(MenuItemView* menu);
+
+  // Runs the menu. |types| is a bitmask of RunTypes. If this returns
+  // MENU_DELETED the method is returning because the MenuRunner was deleted.
+  // Typically callers should NOT do any processing if this returns
+  // MENU_DELETED.
+  RunResult RunMenuAt(Widget* parent,
+                      MenuButton* button,
+                      const gfx::Rect& bounds,
+                      MenuItemView::AnchorPosition anchor,
+                      int32 types) WARN_UNUSED_RESULT;
 
-  Holder* holder_;
+  // Hides and cancels the menu. This does nothing if the menu is not open.
+  void Cancel();
+
+ private:
+  internal::MenuRunnerImpl* holder_;
 
   DISALLOW_COPY_AND_ASSIGN(MenuRunner);
 };
diff --git a/views/controls/textfield/native_textfield_views.cc b/views/controls/textfield/native_textfield_views.cc
index 468a9c9..5b43215 100644
--- a/views/controls/textfield/native_textfield_views.cc
+++ b/views/controls/textfield/native_textfield_views.cc
@@ -22,6 +22,7 @@
 #include "views/controls/focusable_border.h"
 #include "views/controls/menu/menu_item_view.h"
 #include "views/controls/menu/menu_model_adapter.h"
+#include "views/controls/menu/menu_runner.h"
 #include "views/controls/textfield/textfield.h"
 #include "views/controls/textfield/textfield_controller.h"
 #include "views/controls/textfield/textfield_views_model.h"
@@ -298,11 +299,11 @@ void NativeTextfieldViews::ShowContextMenuForView(View* source,
                                                   const gfx::Point& p,
                                                   bool is_mouse_gesture) {
   UpdateContextMenu();
-  context_menu_menu_->RunMenuAt(GetWidget(),
-                                NULL,
-                                gfx::Rect(p, gfx::Size()),
-                                views::MenuItemView::TOPLEFT,
-                                true);
+  if (context_menu_runner_->RunMenuAt(
+          GetWidget(), NULL, gfx::Rect(p, gfx::Size()),
+          views::MenuItemView::TOPLEFT, MenuRunner::HAS_MNEMONICS) ==
+      MenuRunner::MENU_DELETED)
+    return;
 }
 
 /////////////////////////////////////////////////////////////////
@@ -975,11 +976,11 @@ void NativeTextfieldViews::UpdateContextMenu() {
 
     context_menu_delegate_.reset(
         new views::MenuModelAdapter(context_menu_contents_.get()));
-    context_menu_menu_.reset(
-        new views::MenuItemView(context_menu_delegate_.get()));
+    context_menu_runner_.reset(
+        new MenuRunner(new views::MenuItemView(context_menu_delegate_.get())));
   }
 
-  context_menu_delegate_->BuildMenu(context_menu_menu_.get());
+  context_menu_delegate_->BuildMenu(context_menu_runner_->GetMenu());
 }
 
 void NativeTextfieldViews::OnTextInputTypeChanged() {
diff --git a/views/controls/textfield/native_textfield_views.h b/views/controls/textfield/native_textfield_views.h
index 1e26eb2..8849a3b 100644
--- a/views/controls/textfield/native_textfield_views.h
+++ b/views/controls/textfield/native_textfield_views.h
@@ -31,8 +31,8 @@ namespace views {
 
 class FocusableBorder;
 class KeyEvent;
-class MenuItemView;
 class MenuModelAdapter;
+class MenuRunner;
 
 // A views/skia only implementation of NativeTextfieldWrapper.
 // No platform specific code is used.
@@ -249,7 +249,7 @@ class VIEWS_EXPORT NativeTextfieldViews : public TouchSelectionClientView,
   // Context menu and its content list for the textfield.
   scoped_ptr<ui::SimpleMenuModel> context_menu_contents_;
   scoped_ptr<views::MenuModelAdapter> context_menu_delegate_;
-  scoped_ptr<views::MenuItemView> context_menu_menu_;
+  scoped_ptr<views::MenuRunner> context_menu_runner_;
 
   scoped_ptr<TouchSelectionController> touch_selection_controller_;
 
diff --git a/views/views.gyp b/views/views.gyp
index e82ea8b..6a6416d 100644
--- a/views/views.gyp
+++ b/views/views.gyp
@@ -103,6 +103,7 @@
         'controls/menu/menu_config_win.cc',
         'controls/menu/menu_controller.cc',
         'controls/menu/menu_controller.h',
+        'controls/menu/menu_controller_delegate.h',
         'controls/menu/menu_delegate.cc',
         'controls/menu/menu_delegate.h',
         'controls/menu/menu_gtk.cc',