From 18cbf31cbcefcaae2c7e302b5f0a856dea51b30c Mon Sep 17 00:00:00 2001 From: "pam@chromium.org" Date: Wed, 28 Oct 2009 22:06:56 +0000 Subject: Neither name nor title are required for page- or browser-actions. Update loader and unit tests. BUG=25482 TEST=covered by unit tests Review URL: http://codereview.chromium.org/307048 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@30392 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/common/extensions/extension.cc | 17 +++++-- chrome/common/extensions/extension_constants.cc | 4 +- chrome/common/extensions/extension_constants.h | 1 + chrome/common/extensions/extension_unittest.cc | 65 +++++++++++++++---------- 4 files changed, 55 insertions(+), 32 deletions(-) (limited to 'chrome/common/extensions') diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc index db05c4b..ab2ff76 100644 --- a/chrome/common/extensions/extension.cc +++ b/chrome/common/extensions/extension.cc @@ -344,12 +344,19 @@ ExtensionAction* Extension::LoadExtensionActionHelper( result->set_default_icon_path(default_icon); } - // Read the page action |default_title|. + // Read the page action title from |default_title| if present, |name| if not + // (both optional). std::string title; - if (!extension_action->GetString(keys::kName, &title) && - !extension_action->GetString(keys::kPageActionDefaultTitle, &title)) { - *error = errors::kInvalidPageActionDefaultTitle; - return NULL; + if (extension_action->HasKey(keys::kPageActionDefaultTitle)) { + if (!extension_action->GetString(keys::kPageActionDefaultTitle, &title)) { + *error = errors::kInvalidPageActionDefaultTitle; + return NULL; + } + } else if (extension_action->HasKey(keys::kName)) { + if (!extension_action->GetString(keys::kName, &title)) { + *error = errors::kInvalidPageActionName; + return NULL; + } } result->SetTitle(ExtensionAction::kDefaultTabId, title); diff --git a/chrome/common/extensions/extension_constants.cc b/chrome/common/extensions/extension_constants.cc index db7c8b7..8259009 100644 --- a/chrome/common/extensions/extension_constants.cc +++ b/chrome/common/extensions/extension_constants.cc @@ -98,6 +98,8 @@ const char* kInvalidName = "Required value 'name' is missing or invalid."; const char* kInvalidPageAction = "Invalid value for 'page_action'."; +const char* kInvalidPageActionName = + "Invalid value for 'page_action.name'."; const char* kInvalidPageActionIconPath = "Invalid value for 'page_action.default_icon'."; const char* kInvalidPageActionsList = @@ -107,7 +109,7 @@ const char* kInvalidPageActionsListSize = const char* kInvalidPageActionId = "Required value 'id' is missing or invalid."; const char* kInvalidPageActionDefaultTitle = - "Required value 'default_title' is missing or invalid."; + "Invalid value for 'default_title'."; const char* kInvalidPageActionPopup = "Invalid type for page action popup."; const char* kInvalidPageActionPopupHeight = diff --git a/chrome/common/extensions/extension_constants.h b/chrome/common/extensions/extension_constants.h index 5c8a087..7f44bf6 100644 --- a/chrome/common/extensions/extension_constants.h +++ b/chrome/common/extensions/extension_constants.h @@ -90,6 +90,7 @@ namespace extension_manifest_errors { extern const char* kInvalidToolstrips; extern const char* kInvalidVersion; extern const char* kInvalidPageAction; + extern const char* kInvalidPageActionName; extern const char* kInvalidPageActionsList; extern const char* kInvalidPageActionsListSize; extern const char* kInvalidPageActionIconPath; diff --git a/chrome/common/extensions/extension_unittest.cc b/chrome/common/extensions/extension_unittest.cc index 9e95e8b..9cdb1da 100644 --- a/chrome/common/extensions/extension_unittest.cc +++ b/chrome/common/extensions/extension_unittest.cc @@ -300,19 +300,13 @@ TEST(ExtensionTest, LoadPageActionHelper) { scoped_ptr action; DictionaryValue input; - // First try with an empty dictionary. We should get nothing back. + // First try with an empty dictionary. ASSERT_TRUE(extension.LoadExtensionActionHelper( - &input, &error_msg) == NULL); - ASSERT_STRNE("", error_msg.c_str()); - error_msg = ""; - - // Now try the same, but as a browser action. Ensure same results. - ASSERT_TRUE(extension.LoadExtensionActionHelper( - &input, &error_msg) == NULL); - ASSERT_STRNE("", error_msg.c_str()); + &input, &error_msg) != NULL); + ASSERT_STREQ("", error_msg.c_str()); error_msg = ""; - // Now setup some values to use in the page action. + // Now setup some values to use in the action. const std::string id("MyExtensionActionId"); const std::string name("MyExtensionActionName"); std::string img1("image1.png"); @@ -327,11 +321,11 @@ TEST(ExtensionTest, LoadPageActionHelper) { input.Set(keys::kPageActionIcons, icons); // Parse and read back the values from the object. - action.reset(extension.LoadExtensionActionHelper( - &input, &error_msg)); + action.reset(extension.LoadExtensionActionHelper(&input, &error_msg)); ASSERT_TRUE(NULL != action.get()); ASSERT_STREQ("", error_msg.c_str()); ASSERT_STREQ(id.c_str(), action->id().c_str()); + // No title, so fall back to name. ASSERT_STREQ(name.c_str(), action->GetTitle(1).c_str()); ASSERT_EQ(2u, action->icon_paths()->size()); ASSERT_STREQ(img1.c_str(), action->icon_paths()->at(0).c_str()); @@ -339,8 +333,7 @@ TEST(ExtensionTest, LoadPageActionHelper) { // Explicitly set the same type and parse again. input.SetString(keys::kType, values::kPageActionTypeTab); - action.reset(extension.LoadExtensionActionHelper( - &input, &error_msg)); + action.reset(extension.LoadExtensionActionHelper(&input, &error_msg)); ASSERT_TRUE(NULL != action.get()); ASSERT_STREQ("", error_msg.c_str()); @@ -351,27 +344,24 @@ TEST(ExtensionTest, LoadPageActionHelper) { // First remove id key. copy.reset(static_cast(input.DeepCopy())); copy->Remove(keys::kPageActionId, NULL); - action.reset(extension.LoadExtensionActionHelper( - copy.get(), &error_msg)); + action.reset(extension.LoadExtensionActionHelper(copy.get(), &error_msg)); ASSERT_TRUE(NULL != action.get()); ASSERT_STREQ("", error_msg.c_str()); error_msg = ""; - // Then remove the name key. + // Then remove the name key. It's optional, so no error. copy.reset(static_cast(input.DeepCopy())); copy->Remove(keys::kName, NULL); - action.reset(extension.LoadExtensionActionHelper( - copy.get(), &error_msg)); - ASSERT_TRUE(NULL == action.get()); - ASSERT_TRUE(MatchPattern(error_msg.c_str(), - errors::kInvalidPageActionDefaultTitle)); + action.reset(extension.LoadExtensionActionHelper(copy.get(), &error_msg)); + ASSERT_TRUE(NULL != action.get()); + ASSERT_STREQ("", action->GetTitle(1).c_str()); + ASSERT_STREQ("", error_msg.c_str()); error_msg = ""; // Then remove the icon paths key. copy.reset(static_cast(input.DeepCopy())); copy->Remove(keys::kPageActionIcons, NULL); - action.reset(extension.LoadExtensionActionHelper( - copy.get(), &error_msg)); + action.reset(extension.LoadExtensionActionHelper(copy.get(), &error_msg)); ASSERT_TRUE(NULL != action.get()); error_msg = ""; @@ -387,12 +377,35 @@ TEST(ExtensionTest, LoadPageActionHelper) { input.SetString(keys::kPageActionDefaultIcon, kIcon); // Parse and read back the values from the object. - action.reset(extension.LoadExtensionActionHelper( - &input, &error_msg)); + action.reset(extension.LoadExtensionActionHelper(&input, &error_msg)); ASSERT_TRUE(action.get()); ASSERT_STREQ("", error_msg.c_str()); ASSERT_EQ(kTitle, action->GetTitle(1)); ASSERT_EQ(0u, action->icon_paths()->size()); + + // Invalid title should give an error even with a valid name. + input.Clear(); + input.SetInteger(keys::kPageActionDefaultTitle, 42); + input.SetString(keys::kName, name); + action.reset(extension.LoadExtensionActionHelper(&input, &error_msg)); + ASSERT_TRUE(NULL == action.get()); + ASSERT_STREQ(errors::kInvalidPageActionDefaultTitle, error_msg.c_str()); + error_msg = ""; + + // Invalid name should give an error only with no title. + input.SetString(keys::kPageActionDefaultTitle, kTitle); + input.SetInteger(keys::kName, 123); + action.reset(extension.LoadExtensionActionHelper(&input, &error_msg)); + ASSERT_TRUE(NULL != action.get()); + ASSERT_EQ(kTitle, action->GetTitle(1)); + ASSERT_STREQ("", error_msg.c_str()); + error_msg = ""; + + input.Remove(keys::kPageActionDefaultTitle, NULL); + action.reset(extension.LoadExtensionActionHelper(&input, &error_msg)); + ASSERT_TRUE(NULL == action.get()); + ASSERT_STREQ(errors::kInvalidPageActionName, error_msg.c_str()); + error_msg = ""; } TEST(ExtensionTest, IdIsValid) { -- cgit v1.1