Implement initial chrome.browserAction.openPopup API.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/api/extension_action/extension_page_actions_api_constants.h"
 #include "chrome/browser/extensions/event_router.h"
 #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"
 #include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/state_store.h"
 #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/render_messages.h"
 #include "content/public/browser/navigation_entry.h"
@@ skipping 22 matching lines @@
 // Whether the browser action is visible in the toolbar.
 const char kBrowserActionVisible[] = "browser_action_visible";
 
 // Errors.
 const char kNoExtensionActionError[] =
     "This extension has no action specified.";
 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
   // SetIcon function arguments.
   const char* size_string;
   // Scale factor for which the represantion should be used.
   ui::ScaleFactor scale;
 };
 
 const IconRepresentationInfo kIconSizes[] = {
@@ skipping 130 matching lines @@
   registry->RegisterFunction<BrowserActionSetTitleFunction>();
   registry->RegisterFunction<BrowserActionSetBadgeTextFunction>();
   registry->RegisterFunction<BrowserActionSetBadgeBackgroundColorFunction>();
   registry->RegisterFunction<BrowserActionSetPopupFunction>();
   registry->RegisterFunction<BrowserActionGetTitleFunction>();
   registry->RegisterFunction<BrowserActionGetBadgeTextFunction>();
   registry->RegisterFunction<BrowserActionGetBadgeBackgroundColorFunction>();
   registry->RegisterFunction<BrowserActionGetPopupFunction>();
   registry->RegisterFunction<BrowserActionEnableFunction>();
   registry->RegisterFunction<BrowserActionDisableFunction>();
+  registry->RegisterFunction<BrowserActionOpenPopupFunction>();
 
   // Page Actions
   registry->RegisterFunction<EnablePageActionsFunction>();
   registry->RegisterFunction<DisablePageActionsFunction>();
   registry->RegisterFunction<PageActionShowFunction>();
   registry->RegisterFunction<PageActionHideFunction>();
   registry->RegisterFunction<PageActionSetIconFunction>();
   registry->RegisterFunction<PageActionSetTitleFunction>();
   registry->RegisterFunction<PageActionSetPopupFunction>();
   registry->RegisterFunction<PageActionGetTitleFunction>();
@@ skipping 576 matching lines @@
   list->Append(
       new base::FundamentalValue(static_cast<int>(SkColorGetG(color))));
   list->Append(
       new base::FundamentalValue(static_cast<int>(SkColorGetB(color))));
   list->Append(
       new base::FundamentalValue(static_cast<int>(SkColorGetA(color))));
   SetResult(list);
   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
 //
 
 ScriptBadgeGetAttentionFunction::~ScriptBadgeGetAttentionFunction() {}
 
 bool ScriptBadgeGetAttentionFunction::RunExtensionAction() {
   tab_helper().location_bar_controller()->GetAttentionFor(extension_id());
   return true;
 }
@@ skipping 66 matching lines @@
   return true;
 }
 
 bool EnablePageActionsFunction::RunImpl() {
   return SetPageActionEnabled(true);
 }
 
 bool DisablePageActionsFunction::RunImpl() {
   return SetPageActionEnabled(false);
 }