Don't show popup browser actions in the wrench menu.

BUG=23834
TEST=none

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

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

7 files changed, 33 insertions, 19 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index c685b5e..6da9183 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1486,7 +1486,7 @@ void Browser::ExecuteCommand(int id) {
 
     // Go find the browser action in question.
     std::vector<ExtensionAction*> browser_actions =
-        service->GetBrowserActions();
+        service->GetBrowserActions(false);  // false means no popup actions.
     for (size_t i = 0; i < browser_actions.size(); ++i) {
       if (browser_actions[i]->command_id() == id) {
         int window_id = ExtensionTabUtil::GetWindowId(this);
@@ -2378,7 +2378,7 @@ void Browser::InitCommandState() {
   ExtensionsService* service = profile()->GetExtensionsService();
   if (service) {
     std::vector<ExtensionAction*> browser_actions =
-        service->GetBrowserActions();
+        service->GetBrowserActions(false);  // false means no popup actions.
     for (size_t i = 0; i < browser_actions.size(); ++i) {
       command_updater_.UpdateCommandEnabled(browser_actions[i]->command_id(),
                                             true);
diff --git a/chrome/browser/extensions/browser_action_test.cc b/chrome/browser/extensions/browser_action_test.cc
index f597f07..acf0c94 100644
--- a/chrome/browser/extensions/browser_action_test.cc
+++ b/chrome/browser/extensions/browser_action_test.cc
@@ -18,9 +18,10 @@ static void TestAction(Browser* browser) {
   ui_test_utils::NavigateToURL(browser,
       GURL("http://localhost:1337/files/extensions/test_file.txt"));
 
-  // Send the command.
+  // Send the command.  Note that this only works for non-popup actions, so
+  // we specify |false|.
   ExtensionsService* service = browser->profile()->GetExtensionsService();
-  browser->ExecuteCommand(service->GetBrowserActions()[0]->command_id());
+  browser->ExecuteCommand(service->GetBrowserActions(false)[0]->command_id());
 
   // Verify the command worked.
   TabContents* tab = browser->GetSelectedTabContents();
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 4f61ac4..f65e6ff 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -146,11 +146,12 @@ void ExtensionsService::Init() {
 }
 
 std::vector<ExtensionAction*> ExtensionsService::GetPageActions() const {
-  return GetExtensionActions(ExtensionAction::PAGE_ACTION);
+  return GetExtensionActions(ExtensionAction::PAGE_ACTION, true);
 }
 
-std::vector<ExtensionAction*> ExtensionsService::GetBrowserActions() const {
-  return GetExtensionActions(ExtensionAction::BROWSER_ACTION);
+std::vector<ExtensionAction*> ExtensionsService::GetBrowserActions(
+    bool include_popups) const {
+  return GetExtensionActions(ExtensionAction::BROWSER_ACTION, include_popups);
 }
 
 void ExtensionsService::InstallExtension(const FilePath& extension_path) {
@@ -365,7 +366,8 @@ void ExtensionsService::NotifyExtensionUnloaded(Extension* extension) {
 }
 
 std::vector<ExtensionAction*> ExtensionsService::GetExtensionActions(
-    ExtensionAction::ExtensionActionType action_type) const {
+    ExtensionAction::ExtensionActionType action_type,
+    bool include_popups) const {
   std::vector<ExtensionAction*> result;
 
   // TODO(finnur): Sort the icons in some meaningful way.
@@ -375,11 +377,12 @@ std::vector<ExtensionAction*> ExtensionsService::GetExtensionActions(
       const ExtensionActionMap* page_actions = &(*iter)->page_actions();
       for (ExtensionActionMap::const_iterator i(page_actions->begin());
            i != page_actions->end(); ++i) {
-        result.push_back(i->second);
+        if (include_popups || !i->second->is_popup())
+          result.push_back(i->second);
       }
     } else {
       ExtensionAction* browser_action = (*iter)->browser_action();
-      if (browser_action)
+      if (browser_action && (include_popups || !browser_action->is_popup()))
         result.push_back(browser_action);
     }
   }
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index d4a6805..e4fd701 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -108,7 +108,7 @@ class ExtensionsService
 
   // Retrieves a vector of all browser actions, irrespective of which extension
   // they belong to.
-  std::vector<ExtensionAction*> GetBrowserActions() const;
+  std::vector<ExtensionAction*> GetBrowserActions(bool include_popups) const;
 
   // Install the extension file at |extension_path|.  Will install as an
   // update if an older version is already installed.
@@ -243,9 +243,11 @@ class ExtensionsService
   void NotifyExtensionUnloaded(Extension* extension);
 
   // Retrieves a vector of all page actions or browser actions, irrespective of
-  // which extension they belong to.
+  // which extension they belong to.  If |include_popups| is false, actions that
+  // are popups are excluded.
   std::vector<ExtensionAction*> GetExtensionActions(
-      ExtensionAction::ExtensionActionType action_type) const;
+      ExtensionAction::ExtensionActionType action_type,
+      bool include_popups) const;
 
   // The profile this ExtensionsService is part of.
   Profile* profile_;
diff --git a/chrome/browser/views/browser_actions_container.cc b/chrome/browser/views/browser_actions_container.cc
index 2fba91b..45a0116 100644
--- a/chrome/browser/views/browser_actions_container.cc
+++ b/chrome/browser/views/browser_actions_container.cc
@@ -178,7 +178,7 @@ void BrowserActionImageView::Observe(NotificationType type,
 }
 
 bool BrowserActionImageView::IsPopup() {
-  return (browser_action_ && !browser_action_->popup_url().is_empty());
+  return browser_action_->is_popup();
 }
 
 bool BrowserActionImageView::Activate() {
@@ -268,8 +268,9 @@ void BrowserActionsContainer::RefreshBrowserActionViews() {
   if (!extension_service)  // The |extension_service| can be NULL in Incognito.
     return;
 
+  // Get all browser actions, including those with popups.
   std::vector<ExtensionAction*> browser_actions;
-  browser_actions = extension_service->GetBrowserActions();
+  browser_actions = extension_service->GetBrowserActions(true);
 
   DeleteBrowserActionViews();
   for (size_t i = 0; i < browser_actions.size(); ++i) {
diff --git a/chrome/browser/views/toolbar_view.cc b/chrome/browser/views/toolbar_view.cc
index dbc6fda..a3e0123c 100644
--- a/chrome/browser/views/toolbar_view.cc
+++ b/chrome/browser/views/toolbar_view.cc
@@ -1083,10 +1083,10 @@ void ToolbarView::CreateAppMenu() {
   ExtensionsService* extensions_service =
       browser_->profile()->GetExtensionsService();
   if (extensions_service && extensions_service->extensions_enabled()) {
-    const ExtensionList* extensions = extensions_service->extensions();
+    // Get a count of all non-popup browser actions to decide how to layout
+    // the Extensions menu.
     std::vector<ExtensionAction*> browser_actions =
-        browser_->profile()->GetExtensionsService()->GetBrowserActions();
-
+        browser_->profile()->GetExtensionsService()->GetBrowserActions(false);
     if (browser_actions.size() == 0) {
       app_menu_contents_->AddItemWithStringId(IDC_MANAGE_EXTENSIONS,
                                               IDS_SHOW_EXTENSIONS);
@@ -1097,6 +1097,11 @@ void ToolbarView::CreateAppMenu() {
 
       extension_menu_contents_->AddItemWithStringId(IDC_MANAGE_EXTENSIONS,
                                                     IDS_MANAGE_EXTENSIONS);
+
+      // TODO(erikkay) Even though we just got the list of all browser actions,
+      // we have to enumerate the list of extensions in order to get the action
+      // state.  It seems like we should find a way to combine these.
+      const ExtensionList* extensions = extensions_service->extensions();
       for (size_t i = 0; i < extensions->size(); ++i) {
         Extension* extension = extensions->at(i);
         if (!extension->browser_action()) {
@@ -1104,7 +1109,7 @@ void ToolbarView::CreateAppMenu() {
         } else if (extension->browser_action()->command_id() >
                    IDC_BROWSER_ACTION_LAST) {
           NOTREACHED() << "Too many browser actions.";
-        } else {
+        } else if (!extension->browser_action()->is_popup()) {
           extension_menu_contents_->AddItem(
               extension->browser_action()->command_id(),
               UTF8ToUTF16(extension->browser_action_state()->title()));
diff --git a/chrome/common/extensions/extension_action.h b/chrome/common/extensions/extension_action.h
index 70ea093..13b1fe71 100644
--- a/chrome/common/extensions/extension_action.h
+++ b/chrome/common/extensions/extension_action.h
@@ -51,6 +51,8 @@ class ExtensionAction {
   const int popup_height() const { return popup_height_; }
   void set_popup_height(int height) { popup_height_ = height; }
 
+  bool is_popup() const { return !popup_url_.is_empty(); }
+
  private:
   static int next_command_id_;