Implement browser action context menu on linux.

BUG=30656
TEST=manual (right click on browser actions and test the actions)

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

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

11 files changed, 89 insertions, 58 deletions

diff --git a/chrome/browser/back_forward_menu_model.h b/chrome/browser/back_forward_menu_model.h
index 44ecb69..18c73d5 100644
--- a/chrome/browser/back_forward_menu_model.h
+++ b/chrome/browser/back_forward_menu_model.h
@@ -118,9 +118,9 @@ class BackForwardMenuModel : public menus::MenuModel {
   // How many chapter-stops (max) to show in the back/forward dropdown list.
   static const int kMaxChapterStops;
 
-  // Converts a menu item id, as passed in through one of the menu delegate
-  // functions and converts it into an absolute index into the
-  // NavigationEntryList vector. |index| can point to a separator, or the
+  // Takes a menu item index as passed in through one of the menu delegate
+  // functions and converts it into an index into the NavigationEntryList
+  // vector. |index| can point to a separator, or the
   // "Show Full History" link in which case this function returns -1.
   int MenuIndexToNavEntryIndex(int index) const;
 
diff --git a/chrome/browser/extensions/extension_action_context_menu_model.cc b/chrome/browser/extensions/extension_action_context_menu_model.cc
index 3f935c9..7c32719 100644
--- a/chrome/browser/extensions/extension_action_context_menu_model.cc
+++ b/chrome/browser/extensions/extension_action_context_menu_model.cc
@@ -23,10 +23,9 @@ enum MenuEntries {
 };
 
 ExtensionActionContextMenuModel::ExtensionActionContextMenuModel(
-    Extension* extension, ExtensionInstallUI::Delegate* delegate)
+    Extension* extension)
   : ALLOW_THIS_IN_INITIALIZER_LIST(SimpleMenuModel(this)),
-    extension_(extension),
-    delegate_(delegate) {
+    extension_(extension) {
   AddItem(NAME, UTF8ToUTF16(extension->name()));
   AddSeparator();
   AddItemWithStringId(CONFIGURE, IDS_EXTENSIONS_OPTIONS);
@@ -90,7 +89,7 @@ void ExtensionActionContextMenuModel::ExecuteCommand(int command_id) {
                             &uninstall_icon);
 
       ExtensionInstallUI client(profile);
-      client.ConfirmUninstall(delegate_, extension_, uninstall_icon.get());
+      client.ConfirmUninstall(this, extension_, uninstall_icon.get());
       break;
     }
     case MANAGE: {
@@ -103,3 +102,10 @@ void ExtensionActionContextMenuModel::ExecuteCommand(int command_id) {
      break;
   }
 }
+
+void ExtensionActionContextMenuModel::InstallUIProceed() {
+  // TODO(finnur): GetLastActive returns NULL in unit tests.
+  Browser* browser = BrowserList::GetLastActive();
+  std::string id = extension_->id();
+  browser->profile()->GetExtensionsService()->UninstallExtension(id, false);
+}
diff --git a/chrome/browser/extensions/extension_action_context_menu_model.h b/chrome/browser/extensions/extension_action_context_menu_model.h
index 62bfdc6..3dbf8bf 100644
--- a/chrome/browser/extensions/extension_action_context_menu_model.h
+++ b/chrome/browser/extensions/extension_action_context_menu_model.h
@@ -14,10 +14,10 @@ class Extension;
 // page actions).
 class ExtensionActionContextMenuModel
     : public menus::SimpleMenuModel,
-      public menus::SimpleMenuModel::Delegate {
+      public menus::SimpleMenuModel::Delegate,
+      public ExtensionInstallUI::Delegate {
  public:
-  ExtensionActionContextMenuModel(Extension* extension,
-                                  ExtensionInstallUI::Delegate* delegate);
+  explicit ExtensionActionContextMenuModel(Extension* extension);
   ~ExtensionActionContextMenuModel();
 
   // SimpleMenuModel behavior overrides.
@@ -27,13 +27,14 @@ class ExtensionActionContextMenuModel
                                           menus::Accelerator* accelerator);
   virtual void ExecuteCommand(int command_id);
 
+  // ExtensionInstallUI::Delegate overrides.
+  virtual void InstallUIProceed();
+  virtual void InstallUIAbort() {}
+
  private:
   // The extension we are displaying the context menu for.
   Extension* extension_;
 
-  // The delegate that handles the extension Uninstall operation.
-  ExtensionInstallUI::Delegate* delegate_;
-
   DISALLOW_COPY_AND_ASSIGN(ExtensionActionContextMenuModel);
 };
 
diff --git a/chrome/browser/gtk/back_forward_button_gtk.cc b/chrome/browser/gtk/back_forward_button_gtk.cc
index d06cc33..e58ede15 100644
--- a/chrome/browser/gtk/back_forward_button_gtk.cc
+++ b/chrome/browser/gtk/back_forward_button_gtk.cc
@@ -80,14 +80,6 @@ void BackForwardButtonGtk::StoppedShowing() {
   button_->UnsetPaintOverride();
 }
 
-bool BackForwardButtonGtk::IsCommandEnabled(int command_id) const {
-  return menu_model_->IsEnabledAt(command_id);
-}
-
-void BackForwardButtonGtk::ExecuteCommand(int command_id) {
-  menu_model_->ActivatedAt(command_id);
-}
-
 bool BackForwardButtonGtk::AlwaysShowImages() const {
   return true;
 }
diff --git a/chrome/browser/gtk/back_forward_button_gtk.h b/chrome/browser/gtk/back_forward_button_gtk.h
index 24e2a76..2e91c6d 100644
--- a/chrome/browser/gtk/back_forward_button_gtk.h
+++ b/chrome/browser/gtk/back_forward_button_gtk.h
@@ -24,8 +24,6 @@ class BackForwardButtonGtk : MenuGtk::Delegate {
 
   // MenuGtk::Delegate implementation.
   virtual void StoppedShowing();
-  virtual bool IsCommandEnabled(int command_id) const;
-  virtual void ExecuteCommand(int command_id);
   bool AlwaysShowImages() const;
 
   GtkWidget* widget() { return button_->widget(); }
diff --git a/chrome/browser/gtk/browser_actions_toolbar_gtk.cc b/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
index 91782f2..cc43f3e 100644
--- a/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
+++ b/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
@@ -9,12 +9,14 @@
 #include "app/gfx/canvas_paint.h"
 #include "app/gfx/gtk_util.h"
 #include "chrome/browser/browser.h"
+#include "chrome/browser/extensions/extension_action_context_menu_model.h"
 #include "chrome/browser/extensions/extension_browser_event_router.h"
 #include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/browser/extensions/image_loading_tracker.h"
 #include "chrome/browser/gtk/extension_popup_gtk.h"
 #include "chrome/browser/gtk/gtk_chrome_button.h"
 #include "chrome/browser/gtk/gtk_theme_provider.h"
+#include "chrome/browser/gtk/menu_gtk.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/common/extensions/extension.h"
@@ -49,7 +51,8 @@ GtkTargetEntry GetDragTargetEntry() {
 }  // namespace
 
 class BrowserActionButton : public NotificationObserver,
-                            public ImageLoadingTracker::Observer {
+                            public ImageLoadingTracker::Observer,
+                            public MenuGtk::Delegate {
  public:
   BrowserActionButton(BrowserActionsToolbarGtk* toolbar,
                       Extension* extension)
@@ -75,8 +78,10 @@ class BrowserActionButton : public NotificationObserver,
                     Extension::kBrowserActionIconMaxSize));
     }
 
+    g_signal_connect(button_.get(), "button-press-event",
+                     G_CALLBACK(OnButtonPress), this);
     g_signal_connect(button_.get(), "clicked",
-                     G_CALLBACK(OnButtonClicked), this);
+                     G_CALLBACK(OnClicked), this);
     g_signal_connect_after(button_.get(), "expose-event",
                            G_CALLBACK(OnExposeEvent), this);
     g_signal_connect(button_.get(), "drag-begin",
@@ -165,7 +170,25 @@ class BrowserActionButton : public NotificationObserver,
             toolbar_->browser()->profile())->UseGtkTheme());
   }
 
-  static void OnButtonClicked(GtkWidget* widget, BrowserActionButton* action) {
+  static gboolean OnButtonPress(GtkWidget* widget,
+                                GdkEvent* event,
+                                BrowserActionButton* action) {
+    if (event->button.button != 3)
+      return FALSE;
+
+    if (!action->context_menu_model_.get()) {
+      action->context_menu_model_.reset(
+          new ExtensionActionContextMenuModel(action->extension_));
+    }
+
+    action->context_menu_.reset(
+        new MenuGtk(action, action->context_menu_model_.get()));
+
+    action->context_menu_->Popup(widget, event);
+    return TRUE;
+  }
+
+  static void OnClicked(GtkWidget* widget, BrowserActionButton* action) {
     if (action->extension_->browser_action()->has_popup()) {
       ExtensionPopupGtk::Show(
           action->extension_->browser_action()->popup_url(),
@@ -224,6 +247,10 @@ class BrowserActionButton : public NotificationObserver,
 
   NotificationRegistrar registrar_;
 
+  // The context menu view and model for this extension action.
+  scoped_ptr<MenuGtk> context_menu_;
+  scoped_ptr<ExtensionActionContextMenuModel> context_menu_model_;
+
   friend class BrowserActionsToolbarGtk;
 };
 
diff --git a/chrome/browser/gtk/download_item_gtk.cc b/chrome/browser/gtk/download_item_gtk.cc
index 01f7c22..38034fc 100644
--- a/chrome/browser/gtk/download_item_gtk.cc
+++ b/chrome/browser/gtk/download_item_gtk.cc
@@ -109,12 +109,7 @@ class DownloadShelfContextMenuGtk : public DownloadShelfContextMenu,
   }
 
   virtual void ExecuteCommand(int id) {
-    // We delay executing the command so the menu will popdown before it is
-    // executed. This way if the command ends up destroying us the current event
-    // will have released its ref beforehand.
-    MessageLoop::current()->PostTask(FROM_HERE,
-        method_factory_.NewRunnableMethod(
-            &DownloadShelfContextMenuGtk::DoCommand, id));
+    ExecuteItemCommand(id);
   }
 
   virtual void StoppedShowing() {
@@ -122,10 +117,6 @@ class DownloadShelfContextMenuGtk : public DownloadShelfContextMenu,
     gtk_widget_queue_draw(download_item_->menu_button_);
   }
 
-  void DoCommand(int id) {
-    ExecuteItemCommand(id);
-  }
-
  private:
   // The menu we show on Popup(). We keep a pointer to it for a couple reasons:
   //  * we don't want to have to recreate the menu every time it's popped up.
diff --git a/chrome/browser/gtk/menu_gtk.cc b/chrome/browser/gtk/menu_gtk.cc
index 75605b5..f6e4f12 100644
--- a/chrome/browser/gtk/menu_gtk.cc
+++ b/chrome/browser/gtk/menu_gtk.cc
@@ -101,8 +101,7 @@ void MenuGtk::Popup(GtkWidget* widget, GdkEvent* event) {
   DCHECK(event->type == GDK_BUTTON_PRESS)
       << "Non-button press event sent to RunMenuAt";
 
-  GdkEventButton* event_button = reinterpret_cast<GdkEventButton*>(event);
-  Popup(widget, event_button->button, event_button->time);
+  Popup(widget, event->button.button, event->button.time);
 }
 
 void MenuGtk::Popup(GtkWidget* widget, gint button_type, guint32 timestamp) {
@@ -278,8 +277,14 @@ void MenuGtk::OnMenuItemActivated(GtkMenuItem* menuitem, MenuGtk* menu) {
   }
 
   // The menu item can still be activated by hotkeys even if it is disabled.
-  if (menu->delegate_->IsCommandEnabled(id))
-    menu->delegate_->ExecuteCommand(id);
+  if (menu->IsCommandEnabled(id)) {
+    // We delay execution because the command may destroy us, and we want to let
+    // the current stack unwind so the event handling code will release its
+    // reference on our GtkMenu* and the OwnedWidgetGtk won't complain during
+    // destruction.
+    MessageLoop::current()->PostTask(FROM_HERE,
+        menu->factory_.NewRunnableMethod(&MenuGtk::ExecuteCommand, id));
+  }
 }
 
 // static
@@ -350,6 +355,20 @@ void MenuGtk::UpdateMenu() {
   gtk_container_foreach(GTK_CONTAINER(menu_.get()), SetMenuItemInfo, this);
 }
 
+// http://crbug.com/31365
+bool MenuGtk::IsCommandEnabled(int id) {
+  return model_ ? model_->IsEnabledAt(id) :
+                  delegate_->IsCommandEnabled(id);
+}
+
+// http://crbug.com/31365
+void MenuGtk::ExecuteCommand(int id) {
+  if (model_)
+    model_->ActivatedAt(id);
+  else
+    delegate_->ExecuteCommand(id);
+}
+
 // static
 void MenuGtk::OnMenuShow(GtkWidget* widget, MenuGtk* menu) {
   MessageLoop::current()->PostTask(FROM_HERE,
@@ -400,8 +419,7 @@ void MenuGtk::SetMenuItemInfo(GtkWidget* widget, gpointer userdata) {
   }
 
   if (GTK_IS_MENU_ITEM(widget)) {
-    gtk_widget_set_sensitive(
-        widget, menu->delegate_->IsCommandEnabled(id));
+    gtk_widget_set_sensitive(widget, menu->IsCommandEnabled(id));
 
     GtkWidget* submenu = gtk_menu_item_get_submenu(GTK_MENU_ITEM(widget));
     if (submenu) {
diff --git a/chrome/browser/gtk/menu_gtk.h b/chrome/browser/gtk/menu_gtk.h
index f25c529..1b48253 100644
--- a/chrome/browser/gtk/menu_gtk.h
+++ b/chrome/browser/gtk/menu_gtk.h
@@ -30,13 +30,13 @@ class MenuGtk {
     virtual ~Delegate() { }
 
     // Returns whether the menu item for this command should be enabled.
-    virtual bool IsCommandEnabled(int command_id) const = 0;
+    virtual bool IsCommandEnabled(int command_id) const { return false; }
 
     // Returns whether this command is checked (for checkbox menu items only).
     virtual bool IsItemChecked(int command_id) const { return false; }
 
     // Executes the command.
-    virtual void ExecuteCommand(int command_id) = 0;
+    virtual void ExecuteCommand(int command_id) {}
 
     // Called when the menu stops showing. This will be called along with
     // ExecuteCommand if the user clicks an item, but will also be called when
@@ -130,6 +130,14 @@ class MenuGtk {
   // Contains implementation for OnMenuShow.
   void UpdateMenu();
 
+  // Dispatches to either |model_| (if it is non-null) or |delegate_|. The
+  // reason for this awkwardness is that we are in a transitional period where
+  // we support both MenuModel and Delegate as a menu controller.
+  // TODO(estade): remove controller functions from Delegate.
+  // http://crbug.com/31365
+  bool IsCommandEnabled(int id);
+  void ExecuteCommand(int id);
+
   // Callback for when a menu item is clicked.
   static void OnMenuItemActivated(GtkMenuItem* menuitem, MenuGtk* menu);
 
@@ -145,7 +153,8 @@ class MenuGtk {
   // Queries this object about the menu state.
   MenuGtk::Delegate* delegate_;
 
-  // If non-NULL, the MenuModel that we use to populate the GTK menu.
+  // If non-NULL, the MenuModel that we use to populate and control the GTK
+  // menu (overriding the delegate as a controller).
   menus::MenuModel* model_;
 
   // For some menu items, we want to show the accelerator, but not actually
diff --git a/chrome/browser/views/extensions/extension_action_context_menu.cc b/chrome/browser/views/extensions/extension_action_context_menu.cc
index dbf7c07..47927fc 100644
--- a/chrome/browser/views/extensions/extension_action_context_menu.cc
+++ b/chrome/browser/views/extensions/extension_action_context_menu.cc
@@ -21,14 +21,7 @@ void ExtensionActionContextMenu::Run(Extension* extension,
   extension_ = extension;
 
   context_menu_contents_.reset(
-      new ExtensionActionContextMenuModel(extension, this));
+      new ExtensionActionContextMenuModel(extension));
   context_menu_menu_.reset(new views::Menu2(context_menu_contents_.get()));
   context_menu_menu_->RunContextMenuAt(point);
 }
-
-void ExtensionActionContextMenu::InstallUIProceed() {
-  // TODO(finnur): GetLastActive returns NULL in unit tests.
-  Browser* browser = BrowserList::GetLastActive();
-  std::string id = extension_->id();
-  browser->profile()->GetExtensionsService()->UninstallExtension(id, false);
-}
diff --git a/chrome/browser/views/extensions/extension_action_context_menu.h b/chrome/browser/views/extensions/extension_action_context_menu.h
index 92ac97c..25d1949 100644
--- a/chrome/browser/views/extensions/extension_action_context_menu.h
+++ b/chrome/browser/views/extensions/extension_action_context_menu.h
@@ -12,7 +12,7 @@
 class Extension;
 
 // Displays the context menu for extension action icons (browser/page actions).
-class ExtensionActionContextMenu : public ExtensionInstallUI::Delegate {
+class ExtensionActionContextMenu {
  public:
   ExtensionActionContextMenu();
   ~ExtensionActionContextMenu();
@@ -20,10 +20,6 @@ class ExtensionActionContextMenu : public ExtensionInstallUI::Delegate {
   // Display the context menu at a given point.
   void Run(Extension* extension, const gfx::Point& point);
 
-  // ExtensionInstallUI::Delegate overrides.
-  virtual void InstallUIProceed();
-  virtual void InstallUIAbort() {}
-
  private:
   // The options menu.
   scoped_ptr<ExtensionActionContextMenuModel> context_menu_contents_;