Fully merge the implementations of the browserAction and pageAction APIs

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

Index: chrome/browser/extensions/api/extension_action/extension_actions_api.cc
diff --git a/chrome/browser/extensions/api/extension_action/extension_actions_api.cc b/chrome/browser/extensions/api/extension_action/extension_actions_api.cc
index aa18536f2de0bfc98266260356baf0b22fa183fe..1eee6dc13a81946bf58e15fc489ac92ef9242f0a 100644
--- a/chrome/browser/extensions/api/extension_action/extension_actions_api.cc
+++ b/chrome/browser/extensions/api/extension_action/extension_actions_api.cc
@@ -6,14 +6,36 @@
 
 #include <string>
 
-#include "base/values.h"
 #include "base/string_number_conversions.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/api/extension_action/extension_page_actions_api_constants.h"

[Aaron Boodman, 2012/04/26 05:30:54]

There's no reason for these separate constants files anymore. Can you merge them
together in a separate change?

[not at google - send to devlin, 2012/04/26 06:01:57]

Merge which files?

[Aaron Boodman, 2012/04/26 06:25:36]

Nevermind, I didn't read this carefully.

+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_tab_helper.h"
+#include "chrome/browser/extensions/extension_tab_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_action.h"
+#include "chrome/common/extensions/extension_error_utils.h"
 #include "chrome/common/render_messages.h"
+#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/notification_service.h"
+
+namespace {
+
+// Errors.
+const char kNoExtensionActionError[] =
+    "This extension has no action specified.";
+const char kNoTabError[] = "No tab with id: *.";
+const char kIconIndexOutOfBounds[] = "Page action icon index out of bounds.";
+
+}
 
 ExtensionActionFunction::ExtensionActionFunction()
     : details_(NULL),
       tab_id_(ExtensionAction::kDefaultTabId),
+      contents_(NULL),
       extension_action_(NULL) {
 }
 
@@ -21,55 +43,65 @@ ExtensionActionFunction::~ExtensionActionFunction() {
 }
 
 bool ExtensionActionFunction::RunImpl() {
-  base::Value* arg;
-  args_->Get(0, &arg);
-  if (arg->IsType(base::Value::TYPE_DICTIONARY)) {
-    EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details_));
-    EXTENSION_FUNCTION_VALIDATE(details_ != NULL);
-    if (details_->HasKey("tabId"))
-      EXTENSION_FUNCTION_VALIDATE(details_->GetInteger("tabId", &tab_id_));
+  extension_action_ = GetExtension()->browser_action();
+  if (!extension_action_)
+    extension_action_ = GetExtension()->page_action();
+  EXTENSION_FUNCTION_VALIDATE(extension_action_);
+
+  // There may or may not be details (depends on the function).
+  // The tabId might appear in details (if it exists) or as the first
+  // argument besides the action type (depends on the function).
+  {
+    base::Value* arg = NULL;
+    EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &arg));
+    if (arg->GetType() == Value::TYPE_INTEGER) {

[Aaron Boodman, 2012/04/26 05:30:54]

Nit: There's IsType().

[not at google - send to devlin, 2012/04/26 06:01:57]

I'm innocent!

I'm going to switch. I like switching.

+      CHECK(arg->GetAsInteger(&tab_id_));
+    } else if (arg->GetType() == Value::TYPE_DICTIONARY) {
+      details_ = static_cast<base::DictionaryValue*>(arg);
+      if (details_->HasKey("tabId"))
+        EXTENSION_FUNCTION_VALIDATE(details_->GetInteger("tabId", &tab_id_));
+    } else {
+      EXTENSION_FUNCTION_VALIDATE(false);

[Aaron Boodman, 2012/04/26 05:30:54]

You could simplify this slightly by making the previous branch an else and then
making this EXTENSION_FUNCTION_VALIDATE(IsType(DICTIONARY)).

[not at google - send to devlin, 2012/04/26 06:01:57]

Oh, I made it a switch. I think it looks nice now.

+    }
   }
-  return true;
-}
 
-bool ExtensionActionFunction::SetIcon() {
-  base::BinaryValue* binary = NULL;
-  EXTENSION_FUNCTION_VALIDATE(details_->GetBinary("imageData", &binary));
-  IPC::Message bitmap_pickle(binary->GetBuffer(), binary->GetSize());
-  PickleIterator iter(bitmap_pickle);
-  SkBitmap bitmap;
-  EXTENSION_FUNCTION_VALIDATE(
-      IPC::ReadParam(&bitmap_pickle, &iter, &bitmap));
-  extension_action_->SetIcon(tab_id_, bitmap);
-  return true;
-}
+  // Find the TabContentsWrapper that contains this tab id if one is required.
+  if (tab_id_ == ExtensionAction::kDefaultTabId) {
+    EXTENSION_FUNCTION_VALIDATE(GetExtension()->browser_action());
+  } else {
+    ExtensionTabUtil::GetTabById(
+        tab_id_, profile(), include_incognito(), NULL, NULL, &contents_, NULL);
+    if (!contents_) {
+      error_ = ExtensionErrorUtils::FormatErrorMessage(
+          kNoTabError, base::IntToString(tab_id_));
+      return false;
+    }
+  }
 
-bool ExtensionActionFunction::SetTitle() {
-  std::string title;
-  EXTENSION_FUNCTION_VALIDATE(details_->GetString("title", &title));
-  extension_action_->SetTitle(tab_id_, title);
-  return true;
+  return RunExtensionAction();
 }
 
-bool ExtensionActionFunction::SetPopup() {
-  std::string popup_string;
-  EXTENSION_FUNCTION_VALIDATE(details_->GetString("popup", &popup_string));
-
-  GURL popup_url;
-  if (!popup_string.empty())
-    popup_url = GetExtension()->GetResourceURL(popup_string);
+void ExtensionActionFunction::NotifyChange() {

[Aaron Boodman, 2012/04/26 05:30:54]

Nit: I would make this abstract and have the subclasses implement it, since they
are different.

[not at google - send to devlin, 2012/04/26 06:01:57]

Yeah that's the thing, it's what I'm trying to avoid.

When the pageAction API can be called despite the extension having a
browser_action rather than a page_action (as in my next patch) it's this code
that prevents us needing a CommandLine check. I like that.

[Aaron Boodman, 2012/04/26 06:25:36]

I see! OK.

+  if (GetExtension()->browser_action())
+    NotifyBrowserActionChange();
+  else if (GetExtension()->page_action())
+    NotifyPageActionChange();
+  else
+    NOTREACHED();
+}
 
-  extension_action_->SetPopupUrl(tab_id_, popup_url);
-  return true;
+void ExtensionActionFunction::NotifyBrowserActionChange() {
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_UPDATED,
+      content::Source<ExtensionAction>(extension_action_),
+      content::NotificationService::NoDetails());
 }
 
-bool ExtensionActionFunction::SetBadgeText() {
-  std::string badge_text;
-  EXTENSION_FUNCTION_VALIDATE(details_->GetString("text", &badge_text));
-  extension_action_->SetBadgeText(tab_id_, badge_text);
-  return true;
+void ExtensionActionFunction::NotifyPageActionChange() {
+  contents_->extension_tab_helper()->PageActionStateChanged();
 }
 
+// static
 bool ExtensionActionFunction::ParseCSSColorString(
     const std::string& color_string,
     SkColor* result) {
@@ -106,7 +138,83 @@ bool ExtensionActionFunction::ParseCSSColorString(
   return true;
 }
 
-bool ExtensionActionFunction::SetBadgeBackgroundColor() {
+bool ExtensionActionFunction::SetVisible(bool visible) {
+  EXTENSION_FUNCTION_VALIDATE(GetExtension()->page_action());

[Aaron Boodman, 2012/04/26 05:30:54]

If this has to always be page_action, why put it here?

[not at google - send to devlin, 2012/04/26 06:01:57]

Yeah, good point. I was trying to make this API work independently of what the
actual API bindings were (in extension_function_registry) since that's the
situation we are probably going to end up with, sort of.

Anyway in my next patch this changes to

if (!GetExtension()->page_action())
  return true;

since when the pageAction API gets called into a converted browser_action we'll
end up with this.

Eventually we'll add whatever the UX is for a page action that wants to show.

Anyway, I'll remove it for now.

[Aaron Boodman, 2012/04/26 06:25:36]

It's fine, after seeing the other patch, it makes sense.

+  extension_action_->SetIsVisible(tab_id_, visible);
+  NotifyChange();
+  return true;
+}
+
+bool ExtensionActionShowFunction::RunExtensionAction() {
+  return SetVisible(true);
+}
+
+bool ExtensionActionHideFunction::RunExtensionAction() {
+  return SetVisible(false);
+}
+
+bool ExtensionActionSetIconFunction::RunExtensionAction() {
+  // setIcon can take a variant argument: either a canvas ImageData, or an
+  // icon index.
+  base::BinaryValue* binary = NULL;
+  int icon_index;
+  if (details_->GetBinary("imageData", &binary)) {
+    IPC::Message bitmap_pickle(binary->GetBuffer(), binary->GetSize());
+    PickleIterator iter(bitmap_pickle);
+    SkBitmap bitmap;
+    EXTENSION_FUNCTION_VALIDATE(
+        IPC::ReadParam(&bitmap_pickle, &iter, &bitmap));
+    extension_action_->SetIcon(tab_id_, bitmap);
+  } else if (details_->GetInteger("iconIndex", &icon_index)) {
+    EXTENSION_FUNCTION_VALIDATE(GetExtension()->page_action());
+    if (icon_index < 0 ||
+        static_cast<size_t>(icon_index) >=
+            extension_action_->icon_paths()->size()) {
+      error_ = kIconIndexOutOfBounds;
+      return false;
+    }
+    extension_action_->SetIcon(tab_id_, SkBitmap());
+    extension_action_->SetIconIndex(tab_id_, icon_index);
+  } else {
+    EXTENSION_FUNCTION_VALIDATE(false);
+  }
+  NotifyChange();
+  return true;
+}
+
+bool ExtensionActionSetTitleFunction::RunExtensionAction() {
+  std::string title;
+  EXTENSION_FUNCTION_VALIDATE(details_->GetString("title", &title));
+  extension_action_->SetTitle(tab_id_, title);
+  NotifyChange();
+  return true;
+}
+
+bool ExtensionActionSetPopupFunction::RunExtensionAction() {
+  std::string popup_string;
+  EXTENSION_FUNCTION_VALIDATE(details_->GetString("popup", &popup_string));
+
+  GURL popup_url;
+  if (!popup_string.empty())
+    popup_url = GetExtension()->GetResourceURL(popup_string);
+
+  extension_action_->SetPopupUrl(tab_id_, popup_url);
+  NotifyChange();
+  return true;
+}
+
+bool ExtensionActionSetBadgeTextFunction::RunExtensionAction() {
+  EXTENSION_FUNCTION_VALIDATE(GetExtension()->browser_action());
+  std::string badge_text;
+  EXTENSION_FUNCTION_VALIDATE(details_->GetString("text", &badge_text));
+  extension_action_->SetBadgeText(tab_id_, badge_text);
+  NotifyChange();
+  return true;
+}
+
+bool ExtensionActionSetBadgeBackgroundColorFunction::RunExtensionAction() {
+  EXTENSION_FUNCTION_VALIDATE(GetExtension()->browser_action());
+
   Value* color_value = NULL;
   details_->Get("color", &color_value);
   SkColor color = 0;
@@ -122,7 +230,6 @@ bool ExtensionActionFunction::SetBadgeBackgroundColor() {
 
     color = SkColorSetARGB(color_array[3], color_array[0],
                            color_array[1], color_array[2]);
-
   } else if (color_value->IsType(Value::TYPE_STRING)) {
     std::string color_string;
     EXTENSION_FUNCTION_VALIDATE(details_->GetString("color", &color_string));
@@ -131,28 +238,30 @@ bool ExtensionActionFunction::SetBadgeBackgroundColor() {
   }
 
   extension_action_->SetBadgeBackgroundColor(tab_id_, color);
-
+  NotifyChange();
   return true;
 }
 
-bool ExtensionActionFunction::GetTitle() {
+bool ExtensionActionGetTitleFunction::RunExtensionAction() {
   result_.reset(Value::CreateStringValue(extension_action_->GetTitle(tab_id_)));
   return true;
 }
 
-bool ExtensionActionFunction::GetPopup() {
+bool ExtensionActionGetPopupFunction::RunExtensionAction() {
   result_.reset(Value::CreateStringValue(
                     extension_action_->GetPopupUrl(tab_id_).spec()));
   return true;
 }
 
-bool ExtensionActionFunction::GetBadgeText() {
+bool ExtensionActionGetBadgeTextFunction::RunExtensionAction() {
+  EXTENSION_FUNCTION_VALIDATE(GetExtension()->browser_action());
   result_.reset(Value::CreateStringValue(
                     extension_action_->GetBadgeText(tab_id_)));
   return true;
 }
 
-bool ExtensionActionFunction::GetBadgeBackgroundColor() {
+bool ExtensionActionGetBadgeBackgroundColorFunction::RunExtensionAction() {
+  EXTENSION_FUNCTION_VALIDATE(GetExtension()->browser_action());
   ListValue* list = new ListValue();
   SkColor color = extension_action_->GetBadgeBackgroundColor(tab_id_);
   list->Append(Value::CreateIntegerValue(SkColorGetR(color)));