Remove Menu2Delegate interface and fold methods onto Menu2Model instead.

Enhance SimpleMenuModel::Delegate interface to support additional options required by the above.
Make MenuHostWindow process WM_MENUCOMMAND and WM_MENUSELECT - the first so we can determine which index was selected, the second so we can track highlight changes within a menu.
Convert Tab's context menu back to the windows native menu. This will make my life easier when I upgrade chrome_menu.cc to the new API if only the bookmarks menus/context menus are using it.

BUG=none
TEST=page, app menus again, context menu for tabs. open a bunch of tabs, open this page: http://bengoodger.dreamhosters.com/software/chrome/cxmenutest.html, close that tab then right click on the menu quickly. should not crash!

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

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

9 files changed, 137 insertions, 50 deletions

diff --git a/views/controls/menu/menu_2.cc b/views/controls/menu/menu_2.cc
index 7d2fa8a..8ce63b6 100644
--- a/views/controls/menu/menu_2.cc
+++ b/views/controls/menu/menu_2.cc
@@ -35,9 +35,8 @@ bool Menu2Model::GetModelAndIndexForCommandId(int command_id,
 ////////////////////////////////////////////////////////////////////////////////
 // Menu2, public:
 
-Menu2::Menu2(Menu2Model* model, Menu2Delegate* delegate)
+Menu2::Menu2(Menu2Model* model)
     : model_(model),
-      delegate_(delegate),
       ALLOW_THIS_IN_INITIALIZER_LIST(
           wrapper_(MenuWrapper::CreateWrapper(this))) {
   Rebuild();
@@ -51,6 +50,10 @@ void Menu2::RunMenuAt(const gfx::Point& point, Alignment alignment) {
   wrapper_->RunMenuAt(point, alignment);
 }
 
+void Menu2::CancelMenu() {
+  wrapper_->CancelMenu();
+}
+
 void Menu2::Rebuild() {
   wrapper_->Rebuild();
 }
diff --git a/views/controls/menu/menu_2.h b/views/controls/menu/menu_2.h
index b50efd7..a131ee5 100644
--- a/views/controls/menu/menu_2.h
+++ b/views/controls/menu/menu_2.h
@@ -87,6 +87,13 @@ class Menu2Model {
   // Returns the model for the submenu at the specified index.
   virtual Menu2Model* GetSubmenuModelAt(int index) const = 0;
 
+  // Called when the highlighted menu item changes to the item at the specified
+  // index.
+  virtual void HighlightChangedTo(int index) = 0;
+
+  // Called when the item at the specified index has been activated.
+  virtual void ActivatedAt(int index) = 0;
+
   // Retrieves the model and index that contains a specific command id. Returns
   // true if an item with the specified command id is found. |model| is inout,
   // and specifies the model to start searching from.
@@ -94,18 +101,10 @@ class Menu2Model {
                                            int* index);
 };
 
-// The Menu2Delegate is an interface implemented by an object that performs
-// tasks that the Menu2 cannot itself.
-class Menu2Delegate {
- public:
-  // Executes the command with the specified identifier.
-  virtual void ExecuteCommand(Menu2Model* model, int command_id) = 0;
-};
-
 // A menu. Populated from a model, and relies on a delegate to execute commands.
 class Menu2 {
  public:
-  Menu2(Menu2Model* model, Menu2Delegate* delegate);
+  explicit Menu2(Menu2Model* model);
   virtual ~Menu2() {}
 
   // How the menu is aligned relative to the point it is shown at.
@@ -118,6 +117,9 @@ class Menu2 {
   // on the platform and type of menu in use.
   void RunMenuAt(const gfx::Point& point, Alignment alignment);
 
+  // Cancels the active menu.
+  void CancelMenu();
+
   // Called when the model supplying data to this menu has changed, and the menu
   // must be rebuilt.
   void Rebuild();
@@ -131,11 +133,9 @@ class Menu2 {
 
   // Accessors.
   Menu2Model* model() const { return model_; }
-  Menu2Delegate* delegate() const { return delegate_; }
 
  private:
   Menu2Model* model_;
-  Menu2Delegate* delegate_;
 
   // The object that actually implements the menu.
   MenuWrapper* wrapper_;
diff --git a/views/controls/menu/menu_wrapper.h b/views/controls/menu/menu_wrapper.h
index f418703..3786002 100644
--- a/views/controls/menu/menu_wrapper.h
+++ b/views/controls/menu/menu_wrapper.h
@@ -24,6 +24,9 @@ class MenuWrapper {
   // on the platform.
   virtual void RunMenuAt(const gfx::Point& point, int alignment) = 0;
 
+  // Cancels the active menu.
+  virtual void CancelMenu() = 0;
+
   // Called when the model supplying data to this menu has changed, and the menu
   // must be rebuilt.
   virtual void Rebuild() = 0;
diff --git a/views/controls/menu/native_menu_gtk.cc b/views/controls/menu/native_menu_gtk.cc
index 4221e6b..c81f04b 100644
--- a/views/controls/menu/native_menu_gtk.cc
+++ b/views/controls/menu/native_menu_gtk.cc
@@ -59,11 +59,7 @@ namespace views {
 ////////////////////////////////////////////////////////////////////////////////
 // NativeMenuGtk, public:
 
-NativeMenuGtk::NativeMenuGtk(Menu2Model* model,
-                             Menu2Delegate* delegate)
-    : model_(model),
-      delegate_(delegate),
-      menu_(NULL) {
+NativeMenuGtk::NativeMenuGtk(Menu2Model* model) : model_(model), menu_(NULL) {
 }
 
 NativeMenuGtk::~NativeMenuGtk() {
@@ -80,6 +76,10 @@ void NativeMenuGtk::RunMenuAt(const gfx::Point& point, int alignment) {
                  gtk_get_current_event_time());
 }
 
+void NativeMenuGtk::CancelMenu() {
+  NOTIMPLEMENTED();
+}
+
 void NativeMenuGtk::Rebuild() {
   ResetMenu();
 
@@ -144,7 +144,7 @@ void NativeMenuGtk::AddMenuItemAt(int index,
   if (type == Menu2Model::TYPE_SUBMENU) {
     // TODO(beng): we're leaking these objects right now... consider some other
     //             arrangement.
-    Menu2* submenu = new Menu2(model_->GetSubmenuModelAt(index), delegate_);
+    Menu2* submenu = new Menu2(model_->GetSubmenuModelAt(index));
     g_object_set_data(G_OBJECT(menu_item), "submenu", submenu);
     gtk_menu_item_set_submenu(GTK_MENU_ITEM(menu_item),
                               submenu->GetNativeMenu());
@@ -212,7 +212,7 @@ void NativeMenuGtk::OnActivate(GtkMenuItem* menu_item) {
                                                          "position"));
   if (model_->IsEnabledAt(position) &&
       MenuTypeCanExecute(model_->GetTypeAt(position))) {
-    delegate_->ExecuteCommand(model_, model_->GetCommandIdAt(position));
+    model_->ActivatedAt(position);
   }
 }
 
@@ -227,7 +227,7 @@ void NativeMenuGtk::CallActivate(GtkMenuItem* menu_item,
 
 // static
 MenuWrapper* MenuWrapper::CreateWrapper(Menu2* menu) {
-  return new NativeMenuGtk(menu->model(), menu->delegate());
+  return new NativeMenuGtk(menu->model());
 }
 
 }  // namespace views
diff --git a/views/controls/menu/native_menu_gtk.h b/views/controls/menu/native_menu_gtk.h
index 1c311b5..b9fcb45d 100644
--- a/views/controls/menu/native_menu_gtk.h
+++ b/views/controls/menu/native_menu_gtk.h
@@ -12,18 +12,17 @@
 namespace views {
 
 class Menu2Model;
-class Menu2Delegate;
 
 // A Gtk implementation of MenuWrapper.
 // TODO(beng): rename to MenuGtk once the old class is dead.
 class NativeMenuGtk : public MenuWrapper {
  public:
-  NativeMenuGtk(Menu2Model* model,
-                Menu2Delegate* delegate);
+  explicit NativeMenuGtk(Menu2Model* model);
   virtual ~NativeMenuGtk();
 
   // Overridden from MenuWrapper:
   virtual void RunMenuAt(const gfx::Point& point, int alignment);
+  virtual void CancelMenu();
   virtual void Rebuild();
   virtual void UpdateStates();
   virtual gfx::NativeMenu GetNativeMenu() const;
@@ -47,7 +46,6 @@ class NativeMenuGtk : public MenuWrapper {
   static void CallActivate(GtkMenuItem* menu_item, NativeMenuGtk* native_menu);
 
   Menu2Model* model_;
-  Menu2Delegate* delegate_;
 
   GtkWidget* menu_;
 
diff --git a/views/controls/menu/native_menu_win.cc b/views/controls/menu/native_menu_win.cc
index fa2a5f7..bbdd46a 100644
--- a/views/controls/menu/native_menu_win.cc
+++ b/views/controls/menu/native_menu_win.cc
@@ -23,7 +23,8 @@ struct NativeMenuWin::ItemData {
   scoped_ptr<Menu2> submenu;
 };
 
-// TODO(beng): bring over owner draw from old menu system.
+// A window that receives messages from Windows relevant to the native menu
+// structure we have constructed in NativeMenuWin.
 class NativeMenuWin::MenuHostWindow {
  public:
   MenuHostWindow() {
@@ -59,11 +60,65 @@ class NativeMenuWin::MenuHostWindow {
     registered = true;
   }
 
+  NativeMenuWin* GetNativeMenuWinFromHMENU(HMENU hmenu) const {
+    MENUINFO mi = {0};
+    mi.cbSize = sizeof(mi);
+    mi.fMask = MIM_MENUDATA;
+    GetMenuInfo(hmenu, &mi);
+    return reinterpret_cast<NativeMenuWin*>(mi.dwMenuData);
+  }
+
+  // Converts the WPARAM value passed to WM_MENUSELECT into an index
+  // corresponding to the menu item that was selected.
+  int GetMenuItemIndexFromWPARAM(HMENU menu, WPARAM w_param) const {
+    int count = GetMenuItemCount(menu);
+    // For normal command menu items, Windows passes a command id as the LOWORD
+    // of WPARAM for WM_MENUSELECT. We need to walk forward through the menu
+    // items to find an item with a matching ID. Ugh!
+    for (int i = 0; i < count; ++i) {
+      MENUITEMINFO mii = {0};
+      mii.cbSize = sizeof(mii);
+      mii.fMask = MIIM_ID;
+      GetMenuItemInfo(menu, i, MF_BYPOSITION, &mii);
+      if (mii.wID == w_param)
+        return i;
+    }
+    // If we didn't find a matching command ID, this means a submenu has been
+    // selected instead, and rather than passing a command ID in
+    // LOWORD(w_param), Windows has actually passed us a position, so we just
+    // return it.
+    return w_param;
+  }
+
+  // Called when the user selects a specific item.
+  void OnMenuCommand(int position, HMENU menu) {
+    GetNativeMenuWinFromHMENU(menu)->model_->ActivatedAt(position);
+  }
+
+  // Called as the user moves their mouse or arrows through the contents of the
+  // menu.
+  void OnMenuSelect(WPARAM w_param, HMENU menu) {
+    int position = GetMenuItemIndexFromWPARAM(menu, w_param);
+    if (position >= 0)
+      GetNativeMenuWinFromHMENU(menu)->model_->HighlightChangedTo(position);
+  }
+
   bool ProcessWindowMessage(HWND window,
                             UINT message,
                             WPARAM w_param,
                             LPARAM l_param,
                             LRESULT* l_result) {
+    switch (message) {
+      case WM_MENUCOMMAND:
+        OnMenuCommand(w_param, reinterpret_cast<HMENU>(l_param));
+        *l_result = 0;
+        return true;
+      case WM_MENUSELECT:
+        OnMenuSelect(LOWORD(w_param), reinterpret_cast<HMENU>(l_param));
+        *l_result = 0;
+        return true;
+      // TODO(beng): bring over owner draw from old menu system.
+    }
     return false;
   }
 
@@ -97,11 +152,8 @@ const wchar_t* NativeMenuWin::MenuHostWindow::kMenuHostWindowKey =
 ////////////////////////////////////////////////////////////////////////////////
 // NativeMenuWin, public:
 
-NativeMenuWin::NativeMenuWin(Menu2Model* model,
-                             Menu2Delegate* delegate,
-                             HWND system_menu_for)
+NativeMenuWin::NativeMenuWin(Menu2Model* model, HWND system_menu_for)
     : model_(model),
-      delegate_(delegate),
       menu_(NULL),
       owner_draw_(false),
       system_menu_for_(system_menu_for),
@@ -118,17 +170,16 @@ NativeMenuWin::~NativeMenuWin() {
 void NativeMenuWin::RunMenuAt(const gfx::Point& point, int alignment) {
   CreateHostWindow();
   UpdateStates();
-  UINT flags = TPM_LEFTBUTTON | TPM_RETURNCMD | TPM_RECURSE;
+  UINT flags = TPM_LEFTBUTTON | TPM_RECURSE;
   flags |= GetAlignmentFlags(alignment);
-  UINT selected_command_id = TrackPopupMenuEx(menu_, flags, point.x(),
-                                              point.y(), host_window_->hwnd(),
-                                              NULL);
-  if (selected_command_id > 0) {
-    // Locate the correct delegate and model to notify about the selection.
-    // See comment in GetMenuForCommandId for details.
-    NativeMenuWin* menu = GetMenuForCommandId(selected_command_id);
-    menu->delegate_->ExecuteCommand(menu->model_, selected_command_id);
-  }
+  // Command dispatch is done through WM_MENUCOMMAND, handled by the host
+  // window.
+  TrackPopupMenuEx(menu_, flags, point.x(), point.y(), host_window_->hwnd(),
+                   NULL);
+}
+
+void NativeMenuWin::CancelMenu() {
+  EndMenu();
 }
 
 void NativeMenuWin::Rebuild() {
@@ -190,8 +241,7 @@ void NativeMenuWin::AddMenuItemAt(int menu_index, int model_index) {
   ItemData* item_data = new ItemData;
   Menu2Model::ItemType type = model_->GetTypeAt(model_index);
   if (type == Menu2Model::TYPE_SUBMENU) {
-    item_data->submenu.reset(new Menu2(model_->GetSubmenuModelAt(model_index),
-                                       delegate_));
+    item_data->submenu.reset(new Menu2(model_->GetSubmenuModelAt(model_index)));
     mii.fMask |= MIIM_SUBMENU;
     mii.hSubMenu = item_data->submenu->GetNativeMenu();
   } else {
@@ -312,10 +362,22 @@ void NativeMenuWin::ResetNativeMenu() {
     if (menu_)
       DestroyMenu(menu_);
     menu_ = CreatePopupMenu();
+    // Rather than relying on the return value of TrackPopupMenuEx, which is
+    // always a command identifier, instead we tell the menu to notify us via
+    // our host window and the WM_MENUCOMMAND message.
+    MENUINFO mi = {0};
+    mi.cbSize = sizeof(mi);
+    mi.fMask = MIM_STYLE | MIM_MENUDATA;
+    mi.dwStyle = MNS_NOTIFYBYPOS;
+    mi.dwMenuData = reinterpret_cast<ULONG_PTR>(this);
+    SetMenuInfo(menu_, &mi);
   }
 }
 
 void NativeMenuWin::CreateHostWindow() {
+  // This only gets called from RunMenuAt, and as such there is only ever one
+  // host window per menu hierarchy, no matter how many NativeMenuWin objects
+  // exist wrapping submenus.
   if (!host_window_.get())
     host_window_.reset(new MenuHostWindow());
 }
@@ -340,7 +402,7 @@ int SystemMenuModel::GetFirstItemIndex(gfx::NativeMenu native_menu) const {
 
 // static
 MenuWrapper* MenuWrapper::CreateWrapper(Menu2* menu) {
-  return new NativeMenuWin(menu->model(), menu->delegate(), NULL);
+  return new NativeMenuWin(menu->model(), NULL);
 }
 
 }  // namespace views
diff --git a/views/controls/menu/native_menu_win.h b/views/controls/menu/native_menu_win.h
index df0aa63..c22313a 100644
--- a/views/controls/menu/native_menu_win.h
+++ b/views/controls/menu/native_menu_win.h
@@ -13,7 +13,6 @@
 
 namespace views {
 
-class Menu2Delegate;
 class Menu2Model;
 
 // A Windows implementation of MenuWrapper.
@@ -23,13 +22,12 @@ class NativeMenuWin : public MenuWrapper {
   // Construct a NativeMenuWin, with a model and delegate. If |system_menu_for|
   // is non-NULL, the NativeMenuWin wraps the system menu for that window.
   // The caller owns the model and the delegate.
-  NativeMenuWin(Menu2Model* model,
-                Menu2Delegate* delegate,
-                HWND system_menu_for);
+  NativeMenuWin(Menu2Model* model, HWND system_menu_for);
   virtual ~NativeMenuWin();
 
   // Overridden from MenuWrapper:
   virtual void RunMenuAt(const gfx::Point& point, int alignment);
+  virtual void CancelMenu();
   virtual void Rebuild();
   virtual void UpdateStates();
   virtual gfx::NativeMenu GetNativeMenu() const;
@@ -90,7 +88,6 @@ class NativeMenuWin : public MenuWrapper {
 
   // Our attached model and delegate.
   Menu2Model* model_;
-  Menu2Delegate* delegate_;
 
   HMENU menu_;
 
@@ -105,6 +102,7 @@ class NativeMenuWin : public MenuWrapper {
 
   // The window that receives notifications from the menu.
   class MenuHostWindow;
+  friend MenuHostWindow;
   scoped_ptr<MenuHostWindow> host_window_;
 
   // The HWND this menu is the system menu for, or NULL if the menu is not a
diff --git a/views/controls/menu/simple_menu_model.cc b/views/controls/menu/simple_menu_model.cc
index 243b144..1ddc092 100644
--- a/views/controls/menu/simple_menu_model.cc
+++ b/views/controls/menu/simple_menu_model.cc
@@ -122,6 +122,16 @@ bool SimpleMenuModel::IsEnabledAt(int index) const {
   return delegate_->IsCommandIdEnabled(command_id);
 }
 
+void SimpleMenuModel::HighlightChangedTo(int index) {
+  if (delegate_)
+    delegate_->CommandIdHighlighted(GetCommandIdAt(index));
+}
+
+void SimpleMenuModel::ActivatedAt(int index) {
+  if (delegate_)
+    delegate_->ExecuteCommand(GetCommandIdAt(index));
+}
+
 Menu2Model* SimpleMenuModel::GetSubmenuModelAt(int index) const {
   return items_.at(FlipIndex(index)).submenu;
 }
diff --git a/views/controls/menu/simple_menu_model.h b/views/controls/menu/simple_menu_model.h
index dbab73f..051de76 100644
--- a/views/controls/menu/simple_menu_model.h
+++ b/views/controls/menu/simple_menu_model.h
@@ -30,8 +30,19 @@ class SimpleMenuModel : public Menu2Model {
         views::Accelerator* accelerator) = 0;
 
     // Some command ids have labels that change over time.
-    virtual bool IsLabelForCommandIdDynamic(int command_id) const = 0;
-    virtual std::wstring GetLabelForCommandId(int command_id) const = 0;
+    virtual bool IsLabelForCommandIdDynamic(int command_id) const {
+      return false;
+    }
+    virtual std::wstring GetLabelForCommandId(int command_id) const {
+      return std::wstring();
+    }
+
+    // Notifies the delegate that the item with the specified command id was
+    // visually highlighted within the menu.
+    virtual void CommandIdHighlighted(int command_id) {}
+
+    // Performs the action associated with the specified command id.
+    virtual void ExecuteCommand(int command_id) = 0;
   };
 
   // The Delegate can be NULL, though if it is items can't be checked or
@@ -63,6 +74,8 @@ class SimpleMenuModel : public Menu2Model {
   virtual int GetGroupIdAt(int index) const;
   virtual bool GetIconAt(int index, SkBitmap* icon) const;
   virtual bool IsEnabledAt(int index) const;
+  virtual void HighlightChangedTo(int index);
+  virtual void ActivatedAt(int index);
   virtual Menu2Model* GetSubmenuModelAt(int index) const;
 
  protected: