Implement initial chrome.browserAction.openPopup API.

chrome/browser/extensions/extension_toolbar_model.h

Index: chrome/browser/extensions/extension_toolbar_model.h
diff --git a/chrome/browser/extensions/extension_toolbar_model.h b/chrome/browser/extensions/extension_toolbar_model.h
index df01de5d17281de69b7072048f8efca77b573e26..f9312381f1f83e1a6a831d636fdd08a70eefed81 100644
--- a/chrome/browser/extensions/extension_toolbar_model.h
+++ b/chrome/browser/extensions/extension_toolbar_model.h
@@ -48,6 +48,11 @@ class ExtensionToolbarModel : public content::NotificationObserver {
     virtual void BrowserActionMoved(const extensions::Extension* extension,
                                     int index) {}
 
+    // The browser action button for |extension| should show the popup now if it
+    // is in the active window.
+    virtual void BrowserActionShowPopup(const extensions::Extension* extension)
+        {}
+
     // Called when the model has finished loading.
     virtual void ModelLoaded() {}
 
@@ -62,7 +67,13 @@ class ExtensionToolbarModel : public content::NotificationObserver {
   // Executes the browser action for an extension and returns the action that
   // the UI should perform in response.
   // |popup_url_out| will be set if the extension should show a popup, with
-  // the URL that should be shown, if non-NULL.
+  // the URL that should be shown, if non-NULL. |should_grant| controls whether
+  // the browser action should grant the page tab permissions. The showPopup API
+  // does not grant permissions.
+  Action ExecuteBrowserAction(const extensions::Extension* extension,
+                              Browser* browser,
+                              GURL* popup_url_out,
+                              bool should_grant);

[Finnur, 2013/10/16 15:01:26]

These kinds of optional params (should_grant) are against the style guide. You
should add this param to the function below and update all the callers.

[justinlin, 2013/10/16 19:31:29]

Hmm.. according to this, this should be the preferred way to add an argument
that can be overriden, but is seldom used:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

I believe SimpleFeature does something similar as well. I can change all callers
too, but I think it makes this call more confusing since almost everytime except
through this new extension API you'll want the granting behavior. WDYT?

[Finnur, 2013/10/17 14:58:34]

The public style guide seems to be down (my coworker also tried).

I looked up Default Argument on the internal style guide and I don't see it as
the preferred way.

You have two options: 
1) Add the param to the original function and update all the callers.
2) Add a second function with a different (but similar name) that redirects.

1 is cleaner, I believe, but if we have a ton of callers that all pass in false
except one (which I don't think is the case) then I guess 2 is preferred. Having
said that, you'd probably want ExecuteBrowserAction and
ExecuteBrowserActionAndGrant...etc, so you'd end up updating all the call sites
anyway.

[justinlin, 2013/10/17 18:32:53]

Done.

   Action ExecuteBrowserAction(const extensions::Extension* extension,
                               Browser* browser,
                               GURL* popup_url_out);
@@ -84,6 +95,8 @@ class ExtensionToolbarModel : public content::NotificationObserver {
   int OriginalIndexToIncognito(int original_index);
 
   void OnExtensionToolbarPrefChange();
+  // Tells observers to display a popup without granting tab permissions.

[Finnur, 2013/10/16 15:01:26]

nit: Line break before this line.

[justinlin, 2013/10/16 19:31:29]

Done.

+  void ShowBrowserActionPopup(const extensions::Extension* extension);
 
  private:
   // content::NotificationObserver implementation.