Initial skeleton for System Indicator API

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 c9c0b846c3a06060d89f27a2335328f93ac870cd..6817e491a3c897cfeaad4b59f41a205a2e312958 100644
--- a/chrome/browser/extensions/api/extension_action/extension_actions_api.cc
+++ b/chrome/browser/extensions/api/extension_action/extension_actions_api.cc
@@ -283,15 +283,19 @@ ExtensionActionFunction::~ExtensionActionFunction() {
 }
 
 bool ExtensionActionFunction::RunImpl() {
-  if (base::StringPiece(name()).starts_with("scriptBadge.")) {
-    extension_action_ = extensions::ExtensionActionManager::Get(profile_)->
-        GetScriptBadge(*GetExtension());
+  extensions::ExtensionActionManager* manager =
+      extensions::ExtensionActionManager::Get(profile_);
+  const extensions::Extension* extension = GetExtension();
+  base::StringPiece functionName(name());

[not at google - send to devlin, 2012/11/26 19:44:54]

chromium is unix_style not camelCase.

however, name() is just a simple accessor:
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions...

so using StartsWith(name(), "...") from string_util.h is probably more
appropriate than constructing a new StringPiece.

[dewittj, 2012/11/27 01:53:55]

Done. StartsWithASCII added.

+
+  if (functionName.starts_with("scriptBadge.")) {
+    extension_action_ = manager->GetScriptBadge(*extension);
+  } else if (functionName.starts_with("systemIndicator.")) {
+    extension_action_ = manager->GetSystemIndicator(*extension);
   } else {
-    extension_action_ = extensions::ExtensionActionManager::Get(profile_)->
-        GetBrowserAction(*GetExtension());
+    extension_action_ = manager->GetBrowserAction(*extension);
     if (!extension_action_) {
-      extension_action_ = extensions::ExtensionActionManager::Get(profile_)->
-          GetPageAction(*GetExtension());
+      extension_action_ = manager->GetPageAction(*extension);
     }
   }
   if (!extension_action_) {
@@ -302,52 +306,11 @@ bool ExtensionActionFunction::RunImpl() {
     return false;
   }
 
-  // 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* first_arg = NULL;
-    EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &first_arg));
-
-    switch (first_arg->GetType()) {
-      case Value::TYPE_INTEGER:
-        CHECK(first_arg->GetAsInteger(&tab_id_));
-        break;
-
-      case Value::TYPE_DICTIONARY: {
-        details_ = static_cast<base::DictionaryValue*>(first_arg);
-        base::Value* tab_id_value = NULL;
-        if (details_->Get("tabId", &tab_id_value)) {
-          switch (tab_id_value->GetType()) {
-            case Value::TYPE_NULL:
-              // Fine, equivalent to it being not-there, and tabId is optional.
-              break;
-            case Value::TYPE_INTEGER:
-              CHECK(tab_id_value->GetAsInteger(&tab_id_));
-              break;
-            default:
-              // Boom.
-              EXTENSION_FUNCTION_VALIDATE(false);
-          }
-        }
-        break;
-      }
-
-      case Value::TYPE_NULL:
-        // The tabId might be an optional argument.
-        break;
-
-      default:
-        EXTENSION_FUNCTION_VALIDATE(false);
-    }
-  }
+  // Populates the tab_id_ and details_ members.
+  EXTENSION_FUNCTION_VALIDATE(ExtractDataFromArguments());
 
   // Find the WebContents that contains this tab id if one is required.
-  if (tab_id_ == ExtensionAction::kDefaultTabId) {
-    EXTENSION_FUNCTION_VALIDATE(
-        extensions::ExtensionActionManager::Get(profile_)->
-        GetBrowserAction(*GetExtension()));
-  } else {
+  if (tab_id_ != ExtensionAction::kDefaultTabId) {
     ExtensionTabUtil::GetTabById(
         tab_id_, profile(), include_incognito(), NULL, NULL, &contents_, NULL);
     if (!contents_) {
@@ -355,11 +318,69 @@ bool ExtensionActionFunction::RunImpl() {
           kNoTabError, base::IntToString(tab_id_));
       return false;
     }
-  }
+  } else {
+    // Only browser actions and system indicators have a default tabId.
+    const extensions::Extension::ActionInfo::Type type_browser =
+        extensions::Extension::ActionInfo::TYPE_BROWSER;
+    const extensions::Extension::ActionInfo::Type type_system_indicator =
+        extensions::Extension::ActionInfo::TYPE_SYSTEM_INDICATOR;

[not at google - send to devlin, 2012/11/26 19:44:54]

this is just making it more verbose?

[dewittj, 2012/11/27 01:53:55]

This is making it fit in the 80 column limit.  Replaced with a block-local
typedef.

+    extensions::Extension::ActionInfo::Type action_type =
+        extension_action_->action_type();
 
+    EXTENSION_FUNCTION_VALIDATE(
+        action_type == type_browser || action_type == type_system_indicator);
+  }
   return RunExtensionAction();
 }
 
+bool ExtensionActionFunction::ExtractDataFromArguments() {
+  // There may or may not be details (depends on the function).
+  // The tabId might appear in details (if it exists), as the first
+  // argument besides the action type (depends on the function), or be omitted
+  // entirely.
+  base::Value* first_arg = NULL;
+  if (!args_->Get(0, &first_arg)) {
+    return true;
+  }

[not at google - send to devlin, 2012/11/26 19:44:54]

generally leave out {} if it's just a 1 line body.

[dewittj, 2012/11/27 01:53:55]

Done.

+
+  switch (first_arg->GetType()) {
+    case Value::TYPE_INTEGER:
+      CHECK(first_arg->GetAsInteger(&tab_id_));
+      break;
+
+    case Value::TYPE_DICTIONARY: {
+      // Found the details argument.
+      details_ = static_cast<base::DictionaryValue*>(first_arg);
+      // Still need to check for the tabId within details.
+      base::Value* tab_id_value = NULL;
+      if (details_->Get("tabId", &tab_id_value)) {
+        switch (tab_id_value->GetType()) {
+          case Value::TYPE_NULL:
+            // OK; tabId is optional, leave it default.
+            return true;
+          case Value::TYPE_INTEGER:
+            CHECK(tab_id_value->GetAsInteger(&tab_id_));
+            return true;
+          default:
+            // Boom.
+            return false;
+        }
+      }
+      // Not found; tabId is optional, leave it default.
+      break;
+    }
+
+    case Value::TYPE_NULL:
+      // The tabId might be an optional argument.
+      break;
+
+    default:
+      return false;
+  }
+
+  return true;
+}
+
 void ExtensionActionFunction::NotifyChange() {
   switch (extension_action_->action_type()) {
     case extensions::Extension::ActionInfo::TYPE_BROWSER:
@@ -375,6 +396,9 @@ void ExtensionActionFunction::NotifyChange() {
     case extensions::Extension::ActionInfo::TYPE_SCRIPT_BADGE:
       NotifyLocationBarChange();
       return;
+    case extensions::Extension::ActionInfo::TYPE_SYSTEM_INDICATOR:
+      NotifyStatusTrayChange();
+      return;
   }
   NOTREACHED();
 }
@@ -391,6 +415,11 @@ void ExtensionActionFunction::NotifyLocationBarChange() {
       location_bar_controller()->NotifyChange();
 }
 
+void ExtensionActionFunction::NotifyStatusTrayChange() {
+  // TODO(dewittj) Implement status tray behavior here.
+  // See http://crbug.com/142450.
+}
+
 // static
 bool ExtensionActionFunction::ParseCSSColorString(
     const std::string& color_string,