Make BookmarkContextMenuGtk use a SimpleMenuModel. Also separate controller and view responsibilities.

This is the first step in re-merging bookmark context menus across platforms.

BUG=32830
TEST=manual

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

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

8 files changed, 79 insertions, 57 deletions

diff --git a/chrome/browser/gtk/bookmark_bar_gtk.cc b/chrome/browser/gtk/bookmark_bar_gtk.cc
index ad75450..19ef406 100644
--- a/chrome/browser/gtk/bookmark_bar_gtk.cc
+++ b/chrome/browser/gtk/bookmark_bar_gtk.cc
@@ -27,6 +27,7 @@
 #include "chrome/browser/gtk/gtk_chrome_button.h"
 #include "chrome/browser/gtk/gtk_theme_provider.h"
 #include "chrome/browser/gtk/import_dialog_gtk.h"
+#include "chrome/browser/gtk/menu_gtk.h"
 #include "chrome/browser/gtk/rounded_window.h"
 #include "chrome/browser/gtk/tabstrip_origin_provider.h"
 #include "chrome/browser/gtk/tabs/tab_strip_gtk.h"
@@ -914,10 +915,12 @@ void BookmarkBarGtk::PopupMenuForNode(GtkWidget* sender,
   }
 
   GtkWindow* window = GTK_WINDOW(gtk_widget_get_toplevel(sender));
-  current_context_menu_.reset(new BookmarkContextMenuGtk(
-                                  window, profile_, browser_, page_navigator_,
-                                  parent, nodes,
-                                  BookmarkContextMenuGtk::BOOKMARK_BAR, NULL));
+  current_context_menu_controller_.reset(
+      new BookmarkContextMenuGtk(
+          window, profile_, browser_, page_navigator_, parent, nodes,
+          BookmarkContextMenuGtk::BOOKMARK_BAR, NULL));
+  current_context_menu_.reset(
+      new MenuGtk(NULL, current_context_menu_controller_->menu_model()));
   current_context_menu_->PopupAsContext(event->time);
 }
 
diff --git a/chrome/browser/gtk/bookmark_bar_gtk.h b/chrome/browser/gtk/bookmark_bar_gtk.h
index 837905a..68a0e4a 100644
--- a/chrome/browser/gtk/bookmark_bar_gtk.h
+++ b/chrome/browser/gtk/bookmark_bar_gtk.h
@@ -28,6 +28,7 @@ class BookmarkMenuController;
 class Browser;
 class BrowserWindowGtk;
 class CustomContainerButton;
+class MenuGtk;
 class PageNavigator;
 class Profile;
 class TabstripOriginProvider;
@@ -349,7 +350,10 @@ class BookmarkBarGtk : public AnimationDelegate,
 
   // The last displayed right click menu, or NULL if no menus have been
   // displayed yet.
-  scoped_ptr<BookmarkContextMenuGtk> current_context_menu_;
+  // The controller.
+  scoped_ptr<BookmarkContextMenuGtk> current_context_menu_controller_;
+  // The view.
+  scoped_ptr<MenuGtk> current_context_menu_;
 
   // The last displayed left click menu, or NULL if no menus have been
   // displayed yet.
diff --git a/chrome/browser/gtk/bookmark_context_menu_gtk.cc b/chrome/browser/gtk/bookmark_context_menu_gtk.cc
index 0ad068d..4c20ff3 100644
--- a/chrome/browser/gtk/bookmark_context_menu_gtk.cc
+++ b/chrome/browser/gtk/bookmark_context_menu_gtk.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -214,10 +214,11 @@ BookmarkContextMenuGtk::BookmarkContextMenuGtk(
       selection_(selection),
       model_(profile->GetBookmarkModel()),
       configuration_(configuration),
-      delegate_(delegate) {
+      delegate_(delegate),
+      model_changed_(false) {
   DCHECK(profile_);
   DCHECK(model_->IsLoaded());
-  CreateMenuObject();
+  menu_model_.reset(new menus::SimpleMenuModel(this));
 
   if (configuration != BOOKMARK_MANAGER_ORGANIZE_MENU) {
     if (selection.size() == 1 && selection[0]->is_url()) {
@@ -281,15 +282,14 @@ BookmarkContextMenuGtk::~BookmarkContextMenuGtk() {
     model_->RemoveObserver(this);
 }
 
-void BookmarkContextMenuGtk::PopupAsContext(guint32 event_time) {
-  menu_->PopupAsContext(event_time);
-}
-
 void BookmarkContextMenuGtk::DelegateDestroyed() {
   delegate_ = NULL;
 }
 
-void BookmarkContextMenuGtk::ExecuteCommandById(int id) {
+void BookmarkContextMenuGtk::ExecuteCommand(int id) {
+  if (model_changed_)
+    return;
+
   if (delegate_)
     delegate_->WillExecuteCommand();
 
@@ -433,12 +433,12 @@ void BookmarkContextMenuGtk::ExecuteCommandById(int id) {
   }
 }
 
-bool BookmarkContextMenuGtk::IsItemChecked(int id) const {
+bool BookmarkContextMenuGtk::IsCommandIdChecked(int id) const {
   DCHECK(id == IDS_BOOMARK_BAR_ALWAYS_SHOW);
   return profile_->GetPrefs()->GetBoolean(prefs::kShowBookmarkBar);
 }
 
-bool BookmarkContextMenuGtk::IsCommandEnabled(int id) const {
+bool BookmarkContextMenuGtk::IsCommandIdEnabled(int id) const {
   bool is_root_node =
       (selection_.size() == 1 &&
        selection_[0]->GetParent() == model_->root_node());
@@ -486,6 +486,12 @@ bool BookmarkContextMenuGtk::IsCommandEnabled(int id) const {
   return true;
 }
 
+bool BookmarkContextMenuGtk::GetAcceleratorForCommandId(
+    int command_id,
+    menus::Accelerator* accelerator) {
+  return false;
+}
+
 void BookmarkContextMenuGtk::BookmarkModelBeingDeleted(BookmarkModel* model) {
   ModelChanged();
 }
@@ -522,27 +528,23 @@ void BookmarkContextMenuGtk::BookmarkNodeChildrenReordered(
 }
 
 void BookmarkContextMenuGtk::ModelChanged() {
-  menu_->Cancel();
-}
-
-void BookmarkContextMenuGtk::CreateMenuObject() {
-  menu_.reset(new MenuGtk(this));
+  model_changed_ = true;
 }
 
 void BookmarkContextMenuGtk::AppendItem(int id) {
-  menu_->AppendMenuItemWithLabel(id, l10n_util::GetStringUTF8(id));
+  menu_model_->AddItem(id, l10n_util::GetStringUTF16(id));
 }
 
 void BookmarkContextMenuGtk::AppendItem(int id, int localization_id) {
-  menu_->AppendMenuItemWithLabel(id, l10n_util::GetStringUTF8(localization_id));
+  menu_model_->AddItemWithStringId(id, localization_id);
 }
 
 void BookmarkContextMenuGtk::AppendSeparator() {
-  menu_->AppendSeparator();
+  menu_model_->AddSeparator();
 }
 
 void BookmarkContextMenuGtk::AppendCheckboxItem(int id) {
-  menu_->AppendCheckMenuItemWithLabel(id, l10n_util::GetStringUTF8(id));
+  menu_model_->AddCheckItemWithStringId(id, id);
 }
 
 BookmarkModel* BookmarkContextMenuGtk::RemoveModelObserver() {
diff --git a/chrome/browser/gtk/bookmark_context_menu_gtk.h b/chrome/browser/gtk/bookmark_context_menu_gtk.h
index a266840..a625611 100644
--- a/chrome/browser/gtk/bookmark_context_menu_gtk.h
+++ b/chrome/browser/gtk/bookmark_context_menu_gtk.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -8,10 +8,10 @@
 #include <vector>
 
 #include "app/gfx/native_widget_types.h"
+#include "app/menus/simple_menu_model.h"
 #include "base/basictypes.h"
 #include "base/scoped_ptr.h"
 #include "chrome/browser/bookmarks/bookmark_model_observer.h"
-#include "chrome/browser/gtk/menu_gtk.h"
 
 class Browser;
 class PageNavigator;
@@ -21,7 +21,7 @@ class Profile;
 // bookmark bar, items on the bookmark bar, submenus of the bookmark bar and
 // the bookmark manager.
 class BookmarkContextMenuGtk : public BookmarkModelObserver,
-                               public MenuGtk::Delegate {
+                               public menus::SimpleMenuModel::Delegate {
  public:
   // Used to configure what the context menu shows.
   enum ConfigurationType {
@@ -60,19 +60,18 @@ class BookmarkContextMenuGtk : public BookmarkModelObserver,
                          Delegate* delegate);
   virtual ~BookmarkContextMenuGtk();
 
-  // Pops up this menu. This call doesn't block.
-  void PopupAsContext(guint32 event_time);
-
-  // Returns the menu.
-  GtkWidget* menu() const { return menu_->widget(); }
+  menus::MenuModel* menu_model() const { return menu_model_.get(); }
 
   // Should be called by the delegate when it is no longer valid.
   void DelegateDestroyed();
 
-  // Menu::Delegate / MenuGtk::Delegate methods.
-  virtual void ExecuteCommandById(int id);
-  virtual bool IsItemChecked(int id) const;
-  virtual bool IsCommandEnabled(int id) const;
+  // Menu::Delegate methods.
+  virtual bool IsCommandIdChecked(int command_id) const;
+  virtual bool IsCommandIdEnabled(int command_id) const;
+  virtual bool GetAcceleratorForCommandId(
+      int command_id,
+      menus::Accelerator* accelerator);
+  virtual void ExecuteCommand(int command_id);
 
  private:
   // BookmarkModelObserver method. Any change to the model results in closing
@@ -101,9 +100,6 @@ class BookmarkContextMenuGtk : public BookmarkModelObserver,
   // Invoked from the various bookmark model observer methods. Closes the menu.
   void ModelChanged();
 
-  // Builds the platform specific menu object.
-  void CreateMenuObject();
-
   // Adds a IDS_* style command to the menu.
   void AppendItem(int id);
   // Adds a IDS_* style command to the menu with a different localized string.
@@ -133,7 +129,12 @@ class BookmarkContextMenuGtk : public BookmarkModelObserver,
   BookmarkModel* model_;
   ConfigurationType configuration_;
   Delegate* delegate_;
-  scoped_ptr<MenuGtk> menu_;
+  scoped_ptr<menus::SimpleMenuModel> menu_model_;
+
+  // Tracks whether the model has changed. For the most part the model won't
+  // change while a context menu is showing, but if it does, we'd better not
+  // try to execute any commands.
+  bool model_changed_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkContextMenuGtk);
 };
diff --git a/chrome/browser/gtk/bookmark_manager_gtk.cc b/chrome/browser/gtk/bookmark_manager_gtk.cc
index 7f4947f..9c77346 100644
--- a/chrome/browser/gtk/bookmark_manager_gtk.cc
+++ b/chrome/browser/gtk/bookmark_manager_gtk.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/gtk/bookmark_tree_model.h"
 #include "chrome/browser/gtk/bookmark_utils_gtk.h"
+#include "chrome/browser/gtk/menu_gtk.h"
 #include "chrome/browser/importer/importer.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/sync_ui_util.h"
@@ -627,14 +628,18 @@ void BookmarkManagerGtk::ResetOrganizeMenu(bool left) {
 
   // We DeleteSoon on the old one to give any reference holders (e.g.
   // the event that caused this reset) a chance to release their refs.
-  BookmarkContextMenuGtk* old_menu = organize_menu_.release();
+  MenuGtk* old_menu = organize_menu_.release();
   if (old_menu)
     MessageLoop::current()->DeleteSoon(FROM_HERE, old_menu);
 
-  organize_menu_.reset(new BookmarkContextMenuGtk(GTK_WINDOW(window_), profile_,
-      NULL, NULL, parent, nodes,
-      BookmarkContextMenuGtk::BOOKMARK_MANAGER_ORGANIZE_MENU, NULL));
-  gtk_menu_item_set_submenu(GTK_MENU_ITEM(organize_), organize_menu_->menu());
+  organize_menu_controller_.reset(
+      new BookmarkContextMenuGtk(GTK_WINDOW(window_), profile_,
+        NULL, NULL, parent, nodes,
+        BookmarkContextMenuGtk::BOOKMARK_MANAGER_ORGANIZE_MENU, NULL));
+  organize_menu_.reset(
+      new MenuGtk(NULL, organize_menu_controller_->menu_model()));
+  gtk_menu_item_set_submenu(GTK_MENU_ITEM(organize_),
+                            organize_menu_->widget());
 }
 
 void BookmarkManagerGtk::BuildLeftStore() {
@@ -1333,9 +1338,9 @@ gboolean BookmarkManagerGtk::OnTreeViewKeyPress(
   if (command == -1)
     return FALSE;
 
-  if (bm->organize_menu_.get() &&
-      bm->organize_menu_->IsCommandEnabled(command)) {
-    bm->organize_menu_->ExecuteCommandById(command);
+  if (bm->organize_menu_controller_.get() &&
+      bm->organize_menu_controller_->IsCommandIdEnabled(command)) {
+    bm->organize_menu_controller_->ExecuteCommand(command);
     return TRUE;
   }
 
diff --git a/chrome/browser/gtk/bookmark_manager_gtk.h b/chrome/browser/gtk/bookmark_manager_gtk.h
index 7d21d28..24db624 100644
--- a/chrome/browser/gtk/bookmark_manager_gtk.h
+++ b/chrome/browser/gtk/bookmark_manager_gtk.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.TIT
+// Copyright (c) 2010 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.
 
@@ -21,6 +21,7 @@
 
 class BookmarkModel;
 class BookmarkTableModel;
+class MenuGtk;
 class Profile;
 
 class BookmarkManagerGtk : public BookmarkModelObserver,
@@ -338,7 +339,10 @@ class BookmarkManagerGtk : public BookmarkModelObserver,
   // The Organize menu item.
   GtkWidget* organize_;
   // The submenu the item pops up.
-  scoped_ptr<BookmarkContextMenuGtk> organize_menu_;
+  // The controller.
+  scoped_ptr<BookmarkContextMenuGtk> organize_menu_controller_;
+  // The view.
+  scoped_ptr<MenuGtk> organize_menu_;
   // Whether the menu refers to the left selection.
   bool organize_is_for_left_;
 
diff --git a/chrome/browser/gtk/bookmark_menu_controller_gtk.cc b/chrome/browser/gtk/bookmark_menu_controller_gtk.cc
index 874ce96..e3bdcb3 100644
--- a/chrome/browser/gtk/bookmark_menu_controller_gtk.cc
+++ b/chrome/browser/gtk/bookmark_menu_controller_gtk.cc
@@ -106,9 +106,8 @@ BookmarkMenuController::BookmarkMenuController(Browser* browser,
 }
 
 BookmarkMenuController::~BookmarkMenuController() {
-
-  if (context_menu_.get())
-    context_menu_->DelegateDestroyed();
+  if (context_menu_controller_.get())
+    context_menu_controller_->DelegateDestroyed();
   profile_->GetBookmarkModel()->RemoveObserver(this);
   gtk_menu_popdown(GTK_MENU(menu_));
 }
@@ -251,17 +250,19 @@ gboolean BookmarkMenuController::OnButtonPressed(
     std::vector<const BookmarkNode*> nodes;
     if (node)
       nodes.push_back(node);
-    controller->context_menu_.reset(
+    controller->context_menu_controller_.reset(
         new BookmarkContextMenuGtk(
             controller->parent_window_, controller->profile_,
             controller->browser_, controller->page_navigator_, parent, nodes,
             BookmarkContextMenuGtk::BOOKMARK_BAR, controller));
+    controller->context_menu_.reset(
+        new MenuGtk(NULL, controller->context_menu_controller_->menu_model()));
 
     // Our bookmark folder menu loses the grab to the context menu. When the
     // context menu is hidden, re-assert our grab.
     GtkWidget* grabbing_menu = gtk_grab_get_current();
     g_object_ref(grabbing_menu);
-    g_signal_connect(controller->context_menu_->menu(), "hide",
+    g_signal_connect(controller->context_menu_->widget(), "hide",
                      G_CALLBACK(OnContextMenuHide), grabbing_menu);
 
     controller->context_menu_->PopupAsContext(event->time);
diff --git a/chrome/browser/gtk/bookmark_menu_controller_gtk.h b/chrome/browser/gtk/bookmark_menu_controller_gtk.h
index dbd0c3e..fdf098a 100644
--- a/chrome/browser/gtk/bookmark_menu_controller_gtk.h
+++ b/chrome/browser/gtk/bookmark_menu_controller_gtk.h
@@ -21,6 +21,7 @@ class Profiler;
 class PageNavigator;
 class BookmarkModel;
 class BookmarkNode;
+class MenuGtk;
 
 class BookmarkMenuController : public BaseBookmarkModelObserver,
                                public BookmarkContextMenuGtk::Delegate {
@@ -123,8 +124,9 @@ class BookmarkMenuController : public BaseBookmarkModelObserver,
   // nodes of type URL.
   std::map<const BookmarkNode*, GtkWidget*> node_to_menu_widget_map_;
 
-  // Owns our right click context menu.
-  scoped_ptr<BookmarkContextMenuGtk> context_menu_;
+  // The controller and view for the right click context menu.
+  scoped_ptr<BookmarkContextMenuGtk> context_menu_controller_;
+  scoped_ptr<MenuGtk> context_menu_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkMenuController);
 };