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..6d1b7ca7f183276795c196ca7fcf9e039d65cb00 100644
--- a/chrome/browser/extensions/api/extension_action/extension_action_api.cc
+++ b/chrome/browser/extensions/api/extension_action/extension_action_api.cc
@@ -4,8 +4,6 @@
 
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 
-#include <string>
-
 #include "base/base64.h"
 #include "base/lazy_instance.h"
 #include "base/strings/string_number_conversions.h"
@@ -17,6 +15,7 @@
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_function_registry.h"
+#include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
@@ -59,6 +58,7 @@ const char kNoTabError[] = "No tab with id: *.";
 const char kNoPageActionError[] =
     "This extension has no page action specified.";
 const char kUrlNotActiveError[] = "This url is no longer active: *.";
+const char kInternalError[] = "Internal error.";
 
 struct IconRepresentationInfo {
   // Size as a string that will be used to retrieve representation value from
@@ -209,6 +209,7 @@ ExtensionActionAPI::ExtensionActionAPI(Profile* profile) {
   registry->RegisterFunction<BrowserActionGetPopupFunction>();
   registry->RegisterFunction<BrowserActionEnableFunction>();
   registry->RegisterFunction<BrowserActionDisableFunction>();
+  registry->RegisterFunction<BrowserActionOpenPopupFunction>();
 
   // Page Actions
   registry->RegisterFunction<EnablePageActionsFunction>();
@@ -805,6 +806,58 @@ bool ExtensionActionGetBadgeBackgroundColorFunction::RunExtensionAction() {
   return true;
 }
 
+BrowserActionOpenPopupFunction::BrowserActionOpenPopupFunction()
+    : response_sent_(false) {
+}
+
+bool BrowserActionOpenPopupFunction::RunImpl() {
+  ExtensionToolbarModel* model = extensions::ExtensionSystem::Get(profile_)->
+      extension_service()->toolbar_model();
+  if (!model) {
+    error_ = kInternalError;
+    return false;
+  }
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
+      content::Source<Profile>(profile_));
+  model->ShowBrowserActionPopup(extension_);
+
+  // Set a timeout for waiting for the notification that the popup is loaded.
+  // Waiting is required so that the popup view can be retrieved by the custom
+  // bindings for response callback.
+  base::MessageLoopForUI::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&BrowserActionOpenPopupFunction::OpenPopupTimedOut, this),
+      base::TimeDelta::FromSeconds(1));

[Matt Perry, 2013/10/16 20:34:35]

Why add a timeout? The popup can take a while to load. DID_STOP_LOADING will
wait until the page is fully loaded, so a second may not be enough.

[justinlin, 2013/10/16 21:16:23]

The timeout is mostly needed because there isn't a good way to get notified if
the popup did not show for some reason, so this is to handle when opening a
popup fails. It's not super ideal, but it should be correct most of the time. It
may fire before the NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING notification,
but 1 second should be enough for the popup to be created already.

Oh, also the PostTask is also needed to keep a ref to this function, otherwise
it gets deleted before we can observe anything, added that comment.

[Matt Perry, 2013/10/16 21:32:50]

I believe you're guaranteed to get a DID_STOP_LOADING message even if the load
fails. Did you find that not to be the case?

You can always manually AddRef/Release to keep the ExtensionFunction alive.

[justinlin, 2013/10/16 21:42:01]

I mean if the extension doesn't try loading at all because of any of the logic
in the platform-specific code (i.e. if there was already a popup or the
extension had no button).

[Matt Perry, 2013/10/16 21:45:21]

Can you add logic to detect that instead? Maybe ShowBrowserActionPopup should
return a bool indicating whether it was successful.

[justinlin, 2013/10/17 04:14:52]

Yea, I considered it, but it seemed a bit weird to iterate over the observer
list. It's not that bad afterall, and this definitely makes a better user
experience. I think it's still possible that the popup should be shown, but it
for some reason does not end up showing (create the view host, but something
makes it decide not to load afterall), hence we may still need the timeout, but
it's set to a bit longer now.

+  return true;
+}
+
+void BrowserActionOpenPopupFunction::OpenPopupTimedOut() {
+  if (response_sent_)
+    return;
+
+  DVLOG(1) << "chrome.browserAction.openPopup did not show a popup.";
+  // Custom binding will still try to get the popup, but may fail.
+  SendResponse(false);
+  response_sent_ = true;
+}
+
+void BrowserActionOpenPopupFunction::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK_EQ(type, chrome::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING);
+  if (response_sent_)
+    return;
+
+  ExtensionHost* host = content::Details<ExtensionHost>(details).ptr();
+  if (host->extension()->id() != extension_->id())

[Matt Perry, 2013/10/16 20:34:35]

You should also check that the host type is popup.

[justinlin, 2013/10/16 21:16:23]

Done.

+    return;
+
+  SendResponse(true);
+  response_sent_ = true;
+  registrar_.RemoveAll();
+}
+
 //
 // ScriptBadgeGetAttentionFunction
 //