Implement initial chrome.browserAction.openPopup API.

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 98e73ee428297faf8f31e47801af00e6872994d5..bab21847ab3d71e93a56e9973891875c80f83309 100644
--- a/chrome/browser/extensions/api/extension_action/extension_action_api.cc
+++ b/chrome/browser/extensions/api/extension_action/extension_action_api.cc
@@ -4,7 +4,7 @@
 
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 
-#include <string>
+#include <set>

[mark a. foltz, 2013/10/11 20:25:30]

Did you mean to remove #include <string>?

[justinlin, 2013/10/16 07:06:48]

Yea, it's already in the .h, actually we don't need set here..

 
 #include "base/base64.h"
 #include "base/lazy_instance.h"
@@ -25,6 +25,7 @@
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
+#include "chrome/common/extensions/features/simple_feature.h"
 #include "chrome/common/render_messages.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
@@ -32,6 +33,7 @@
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia.h"
 
+using chrome::VersionInfo;
 using content::WebContents;
 
 namespace page_actions_keys = extension_page_actions_api_constants;
@@ -60,6 +62,16 @@ const char kNoPageActionError[] =
     "This extension has no page action specified.";
 const char kUrlNotActiveError[] = "This url is no longer active: *.";
 
+// Whitelisted extensions for the openPopup API.
+const char* whitelisted_extensions[] = {

[mark a. foltz, 2013/10/11 20:25:30]

kOpenPopupWhitelistedExtensions

[justinlin, 2013/10/16 07:06:48]

moved to _api_features.json

+  "enhhojjnijigcajfphajepfemndkmdlo",  // Dev
+  "pkedcjkdefgpdelpbcmbmeomcjbeemfm",  // Trusted Tester
+  "fmfcbgogabcbclcofgocippekhfcmgfj",  // Staging
+  "hfaagokkkhdbgiakmmlclaapfelnkoah",  // Canary
+  "F155646B5D1CA545F7E1E4E20D573DFDD44C2540",  // Trusted Tester (public)
+  "16CA7A47AAE4BE49B1E75A6B960C3875E945B264"   // Release
+};
+
 struct IconRepresentationInfo {
   // Size as a string that will be used to retrieve representation value from
   // SetIcon function arguments.
@@ -209,6 +221,7 @@ ExtensionActionAPI::ExtensionActionAPI(Profile* profile) {
   registry->RegisterFunction<BrowserActionGetPopupFunction>();
   registry->RegisterFunction<BrowserActionEnableFunction>();
   registry->RegisterFunction<BrowserActionDisableFunction>();
+  registry->RegisterFunction<BrowserActionOpenPopupFunction>();
 
   // Page Actions
   registry->RegisterFunction<EnablePageActionsFunction>();
@@ -805,6 +818,37 @@ bool ExtensionActionGetBadgeBackgroundColorFunction::RunExtensionAction() {
   return true;
 }
 
+bool BrowserActionOpenPopupFunction::RunImpl() {
+  if (VersionInfo::GetChannel() != VersionInfo::CHANNEL_UNKNOWN &&

[mark a. foltz, 2013/10/11 20:25:30]

For clarity, suggest adding a private boolean method to check the whitelist

[Matt Perry, 2013/10/11 22:22:53]

We actually have a standard mechanism for this. Add browserAction.openPopup to
_api_features.json with this right restrictions/whitelists.

[justinlin, 2013/10/16 07:06:48]

Done.

[justinlin, 2013/10/16 07:06:48]

Just using _api_features.json now.

+      VersionInfo::GetChannel() != VersionInfo::CHANNEL_DEV &&
+      !SimpleFeature::IsIdInWhitelist(
+          extension_->id(),
+          std::set<std::string>(

[mark a. foltz, 2013/10/11 20:25:30]

Is this an overloaded ctor for std::set that takes a char*[]?  Not familiar with
it, but cool if it works.

+              whitelisted_extensions,
+              whitelisted_extensions + arraysize(whitelisted_extensions)))) {
+    SetResult(new base::DictionaryValue());
+    return false;
+  }
+
+  ExtensionService* service =
+      extensions::ExtensionSystem::Get(profile_)->extension_service();
+  if (!service) {
+    SetResult(new base::DictionaryValue());

[Matt Perry, 2013/10/11 22:22:53]

Don't set result on errors. Use SetError instead, with a reasonable error
message (though this is just likely to be "internal error").

[justinlin, 2013/10/16 07:06:48]

Done.

+    return false;
+  }
+
+  ExtensionToolbarModel *model = service->toolbar_model();
+  if (!model) {
+    SetResult(new base::DictionaryValue());
+    return false;
+  }
+
+  model->ShowBrowserActionPopup(extension_);
+
+  SetResult(new base::DictionaryValue());

[mark a. foltz, 2013/10/11 20:25:30]

Just call this at the top of RunImpl?

[justinlin, 2013/10/16 07:06:48]

no longer needed.

+  return true;
+}
+
 //
 // ScriptBadgeGetAttentionFunction
 //