Allow an empty list of page actions.

BUG=26050
TEST=Updated existing unit tests ExtensionTest.InitFromValue*, manual testing on Linux.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35802
Review URL: http://codereview.chromium.org/523132

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@35903 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 39 insertions, 17 deletions

diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 1ccaeec..7f9783d 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -1079,6 +1079,8 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
   }
 
   // Initialize page action (optional).
+  DictionaryValue* page_action_value = NULL;
+
   if (source.HasKey(keys::kPageActions)) {
     ListValue* list_value;
     if (!source.GetList(keys::kPageActions, &list_value)) {
@@ -1086,28 +1088,29 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
       return false;
     }
 
-    if (list_value->GetSize() != 1u) {
-      *error = errors::kInvalidPageActionsListSize;
-      return false;
-    }
+    size_t list_value_length = list_value->GetSize();
 
-    DictionaryValue* page_action_value;
-    if (!list_value->GetDictionary(0, &page_action_value)) {
-      *error = errors::kInvalidPageAction;
+    if (list_value_length == 0u) {
+      // A list with zero items is allowed, and is equivalent to not having
+      // a page_actions key in the manifest.  Don't set |page_action_value|.
+    } else if (list_value_length == 1u) {
+      if (!list_value->GetDictionary(0, &page_action_value)) {
+        *error = errors::kInvalidPageAction;
+        return false;
+      }
+    } else {  // list_value_length > 1u.
+      *error = errors::kInvalidPageActionsListSize;
       return false;
     }
-
-    page_action_.reset(
-        LoadExtensionActionHelper(page_action_value, error));
-    if (!page_action_.get())
-      return false;  // Failed to parse page action definition.
   } else if (source.HasKey(keys::kPageAction)) {
-    DictionaryValue* page_action_value;
     if (!source.GetDictionary(keys::kPageAction, &page_action_value)) {
       *error = errors::kInvalidPageAction;
       return false;
     }
+  }
 
+  // If page_action_value is not NULL, then there was a valid page action.
+  if (page_action_value) {
     page_action_.reset(
         LoadExtensionActionHelper(page_action_value, error));
     if (!page_action_.get())
@@ -1117,7 +1120,7 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
   // Initialize browser action (optional).
   if (source.HasKey(keys::kBrowserAction)) {
     // Restrict extensions to one UI surface.
-    if (source.HasKey(keys::kPageAction) || source.HasKey(keys::kPageActions)) {
+    if (page_action_.get()) {
       *error = errors::kOneUISurfaceOnly;
       return false;
     }
diff --git a/chrome/common/extensions/extension_constants.cc b/chrome/common/extensions/extension_constants.cc
index 825fc2e..0f59e5f 100644
--- a/chrome/common/extensions/extension_constants.cc
+++ b/chrome/common/extensions/extension_constants.cc
@@ -122,7 +122,7 @@ const char* kInvalidPageActionIconPath =
 const char* kInvalidPageActionsList =
     "Invalid value for 'page_actions'.";
 const char* kInvalidPageActionsListSize =
-    "Invalid value for 'page_actions'. There can be only one.";
+    "Invalid value for 'page_actions'. There can be at most one page action.";
 const char* kInvalidPageActionId =
     "Required value 'id' is missing or invalid.";
 const char* kInvalidPageActionDefaultTitle =
diff --git a/chrome/common/extensions/extension_unittest.cc b/chrome/common/extensions/extension_unittest.cc
index a294c23..1cdc590 100644
--- a/chrome/common/extensions/extension_unittest.cc
+++ b/chrome/common/extensions/extension_unittest.cc
@@ -223,17 +223,29 @@ TEST(ExtensionTest, InitFromValueInvalid) {
   EXPECT_FALSE(extension.InitFromValue(*input_value, true, &error));
   EXPECT_TRUE(MatchPatternASCII(error, errors::kInvalidPrivacyBlacklistsPath));
 
-  // Test invalid UI surface count (both page action and browser action).
+  // Multiple page actions are not allowed.
   input_value.reset(static_cast<DictionaryValue*>(valid_value->DeepCopy()));
   DictionaryValue* action = new DictionaryValue;
   action->SetString(keys::kPageActionId, "MyExtensionActionId");
   action->SetString(keys::kName, "MyExtensionActionName");
   ListValue* action_list = new ListValue;
   action_list->Append(action->DeepCopy());
+  action_list->Append(action);
+  input_value->Set(keys::kPageActions, action_list);
+  EXPECT_FALSE(extension.InitFromValue(*input_value, true, &error));
+  EXPECT_STREQ(errors::kInvalidPageActionsListSize, error.c_str());
+
+  // Test invalid UI surface count (both page action and browser action).
+  input_value.reset(static_cast<DictionaryValue*>(valid_value->DeepCopy()));
+  action = new DictionaryValue;
+  action->SetString(keys::kPageActionId, "MyExtensionActionId");
+  action->SetString(keys::kName, "MyExtensionActionName");
+  action_list = new ListValue;
+  action_list->Append(action->DeepCopy());
   input_value->Set(keys::kPageActions, action_list);
   input_value->Set(keys::kBrowserAction, action);
   EXPECT_FALSE(extension.InitFromValue(*input_value, true, &error));
-  EXPECT_STREQ(error.c_str(), errors::kOneUISurfaceOnly);
+  EXPECT_STREQ(errors::kOneUISurfaceOnly, error.c_str());
 
   // Test invalid options page url.
   input_value.reset(static_cast<DictionaryValue*>(valid_value->DeepCopy()));
@@ -295,6 +307,13 @@ TEST(ExtensionTest, InitFromValueValid) {
   EXPECT_EQ("chrome-extension", extension.options_url().scheme());
   EXPECT_EQ("/options.html", extension.options_url().path());
 
+  // Test that an empty list of page actions does not stop a browser action
+  // from being loaded.
+  ListValue* empty_list = new ListValue;
+  input_value.Set(keys::kPageActions, empty_list);
+  EXPECT_TRUE(extension.InitFromValue(input_value, false, &error));
+  EXPECT_EQ("", error);
+
 #if !defined(OS_MACOSX)
   // TODO(aa): The version isn't stamped into the unit test binary on mac.
   // Test with a minimum_chrome_version.