Make a ShowExtensionActionPopup function

chrome/browser/extensions/api/extension_action/extension_action_api.cc

Index: chrome/browser/extensions/api/extension_action/extension_action_api.cc
diff --git a/chrome/browser/extensions/api/extension_action/extension_action_api.cc b/chrome/browser/extensions/api/extension_action/extension_action_api.cc
index c5bd072967886b92167df700657358b75713e9b8..87ad253f1114033bdf304fd1bde21e8218b8c4e1 100644
--- a/chrome/browser/extensions/api/extension_action/extension_action_api.cc
+++ b/chrome/browser/extensions/api/extension_action/extension_action_api.cc
@@ -36,6 +36,7 @@
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/state_store.h"
 #include "extensions/common/error_utils.h"
+#include "extensions/common/feature_switch.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia.h"
@@ -80,7 +81,6 @@ const char kNoTabError[] = "No tab with id: *.";
 const char kOpenPopupError[] =
     "Failed to show popup either because there is an existing popup or another "
     "error occurred.";
-const char kInternalError[] = "Internal error.";
 
 struct IconRepresentationInfo {
   // Size as a string that will be used to retrieve representation value from
@@ -353,13 +353,9 @@ ExtensionAction::ShowAction ExtensionActionAPI::ExecuteExtensionAction(
 
   int tab_id = SessionTabHelper::IdForTab(web_contents);
 
-  ExtensionActionManager* action_manager =
-      ExtensionActionManager::Get(static_cast<Profile*>(browser_context_));
-  // TODO(devlin): Make a ExtensionActionManager::GetExtensionAction method.
   ExtensionAction* extension_action =
-      action_manager->GetBrowserAction(*extension);
-  if (!extension_action)
-    extension_action = action_manager->GetPageAction(*extension);
+      ExtensionActionManager::Get(browser_context_)->GetExtensionAction(
+          *extension);
 
   // Anything that calls this should have a page or browser action.
   DCHECK(extension_action);
@@ -387,6 +383,29 @@ ExtensionAction::ShowAction ExtensionActionAPI::ExecuteExtensionAction(
   return ExtensionAction::ACTION_NONE;
 }
 
+bool ExtensionActionAPI::ShowExtensionActionPopup(
+    const Extension* extension,
+    Browser* browser,
+    bool grant_active_tab_permissions) {
+  ExtensionAction* extension_action =
+      ExtensionActionManager::Get(browser_context_)->GetExtensionAction(
+          *extension);
+  if (!extension_action)
+    return false;
+
+  if (extension_action->action_type() == ActionInfo::TYPE_PAGE &&
+      !FeatureSwitch::extension_action_redesign()->IsEnabled()) {
+    // We show page actions in the location bar unless the new toolbar is
+    // enabled.
+    return browser->window()->GetLocationBar()->ShowPageActionPopup(
+        extension, grant_active_tab_permissions);
+  } else {
+    return ExtensionToolbarModel::Get(browser->profile())->
+        ShowExtensionActionPopup(
+            extension, browser, grant_active_tab_permissions);
+  }
+}
+
 void ExtensionActionAPI::NotifyChange(ExtensionAction* extension_action,
                                       content::WebContents* web_contents,
                                       content::BrowserContext* context) {
@@ -833,13 +852,19 @@ BrowserActionOpenPopupFunction::BrowserActionOpenPopupFunction()
 }
 
 bool BrowserActionOpenPopupFunction::RunAsync() {
-  ExtensionToolbarModel* model = ExtensionToolbarModel::Get(GetProfile());
-  if (!model) {
-    error_ = kInternalError;
-    return false;
-  }
-
-  if (!model->ShowBrowserActionPopup(extension_.get())) {
+  // We only allow the a popup in the active window.

[Finnur, 2014/08/27 11:53:02]

s/the a/the/

[Devlin, 2014/08/27 15:43:22]

Done.

+  Browser* browser = chrome::FindLastActiveWithProfile(

[Finnur, 2014/08/27 11:53:02]

Hmmm... is there no way around looking up the browser object?

[Devlin, 2014/08/27 15:43:22]

Sadly, I don't think so, no :(  I think there are a few flaws of the
chrome.browserAction.showPopup() function, one of which is that it doesn't take
a tab id and simply tries to open in the active browser, whichever that happens
to be.  Since it can be called from any context, I think the only way to find
the active browser is to look it up directly.

This is pretty similar to the old behavior, in which we'd ask every single
browser (indirectly, via a BrowserActionsContainer) to display the popup, and it
would iff it was the active browser.

Of course, I'm constantly discovering new functions here.  Is there an easier
way to find the active Browser?

+                         GetProfile(), chrome::GetActiveDesktop());
+
+  // If there's no active browser, or the Toolbar isn't visible, abort.
+  // Otherwise, try to open a popup in the active browser.
+  // TODO(justinlin): Remove toolbar check when http://crbug.com/308645 is
+  // fixed.
+  if (!browser ||
+      !browser->window()->IsActive() ||
+      !browser->window()->IsToolbarVisible() ||
+      !ExtensionActionAPI::Get(GetProfile())->ShowExtensionActionPopup(
+          extension_.get(), browser, false)) {
     error_ = kOpenPopupError;
     return false;
   }