Add option of string item IDs (given at create() time) to contextMenus API.
This will be (but is not yet) required for event pages.
BUG=123366
TEST=automated tests
Review URL: https://chromiumcodereview.appspot.com/10391125
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@137323 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/extensions/api/context_menu/context_menu_api.cc b/chrome/browser/extensions/api/context_menu/context_menu_api.cc
index 83e48763..695abb7 100644
--- a/chrome/browser/extensions/api/context_menu/context_menu_api.cc
+++ b/chrome/browser/extensions/api/context_menu/context_menu_api.cc
@@ -18,6 +18,7 @@
const char kDocumentUrlPatternsKey[] = "documentUrlPatterns";
const char kEnabledKey[] = "enabled";
const char kGeneratedIdKey[] = "generatedId";
+const char kIdKey[] = "id";
const char kParentIdKey[] = "parentId";
const char kTargetUrlPatternsKey[] = "targetUrlPatterns";
const char kTitleKey[] = "title";
@@ -26,6 +27,8 @@
const char kCannotFindItemError[] = "Cannot find menu item with id *";
const char kCheckedError[] =
"Only items with type \"radio\" or \"checkbox\" can be checked";
+const char kDuplicateIDError[] =
+ "Cannot create item with duplicate id *";
const char kInvalidURLPatternError[] = "Invalid url pattern '*'";
const char kInvalidValueError[] = "Invalid value for *";
const char kInvalidTypeStringError[] = "Invalid type string '*'";
@@ -36,6 +39,17 @@
namespace extensions {
+namespace {
+
+std::string GetIDString(const ExtensionMenuItem::Id& id) {
+ if (id.uid == 0)
+ return id.string_uid;
+ else
+ return base::IntToString(id.uid);
+}
+
+} // namespace
+
bool ExtensionContextMenuFunction::ParseContexts(
const DictionaryValue& properties,
const char* key,
@@ -180,21 +194,33 @@
return true;
}
+bool ExtensionContextMenuFunction::ParseID(
+ const Value* value,
+ ExtensionMenuItem::Id* result) {
+ if (value->GetAsInteger(&result->uid) ||
+ value->GetAsString(&result->string_uid)) {
+ return true;
+ } else {
+ return false;
+ }
+}
+
bool ExtensionContextMenuFunction::GetParent(
const DictionaryValue& properties,
const ExtensionMenuManager& manager,
ExtensionMenuItem** result) {
if (!properties.HasKey(kParentIdKey))
return true;
- ExtensionMenuItem::Id parent_id(profile(), extension_id(), 0);
- if (properties.HasKey(kParentIdKey) &&
- !properties.GetInteger(kParentIdKey, &parent_id.uid))
+ ExtensionMenuItem::Id parent_id(profile(), extension_id());
+ Value* parent_id_value = NULL;
+ if (properties.Get(kParentIdKey, &parent_id_value) &&
+ !ParseID(parent_id_value, &parent_id))
return false;
ExtensionMenuItem* parent = manager.GetItemById(parent_id);
if (!parent) {
- error_ = "Cannot find menu item with id " +
- base::IntToString(parent_id.uid);
+ error_ = ExtensionErrorUtils::FormatErrorMessage(
+ kCannotFindItemError, GetIDString(parent_id));
return false;
}
if (parent->type() != ExtensionMenuItem::NORMAL) {
@@ -210,9 +236,15 @@
EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &properties));
EXTENSION_FUNCTION_VALIDATE(properties != NULL);
- ExtensionMenuItem::Id id(profile(), extension_id(), 0);
- EXTENSION_FUNCTION_VALIDATE(properties->GetInteger(kGeneratedIdKey,
- &id.uid));
+ ExtensionMenuItem::Id id(profile(), extension_id());
+ if (properties->HasKey(kIdKey)) {
+ EXTENSION_FUNCTION_VALIDATE(properties->GetString(kIdKey,
+ &id.string_uid));
+ } else {
+ EXTENSION_FUNCTION_VALIDATE(properties->GetInteger(kGeneratedIdKey,
+ &id.uid));
+ }
+
std::string title;
if (properties->HasKey(kTitleKey) &&
!properties->GetString(kTitleKey, &title))
@@ -221,6 +253,12 @@
ExtensionMenuManager* menu_manager =
profile()->GetExtensionService()->menu_manager();
+ if (menu_manager->GetItemById(id)) {
+ error_ = ExtensionErrorUtils::FormatErrorMessage(kDuplicateIDError,
+ GetIDString(id));
+ return false;
+ }
+
ExtensionMenuItem::ContextList contexts(ExtensionMenuItem::PAGE);
if (!ParseContexts(*properties, kContextsKey, &contexts))
return false;
@@ -250,20 +288,10 @@
bool success = true;
if (properties->HasKey(kParentIdKey)) {
- ExtensionMenuItem::Id parent_id(profile(), extension_id(), 0);
- EXTENSION_FUNCTION_VALIDATE(properties->GetInteger(kParentIdKey,
- &parent_id.uid));
- ExtensionMenuItem* parent = menu_manager->GetItemById(parent_id);
- if (!parent) {
- error_ = ExtensionErrorUtils::FormatErrorMessage(
- kCannotFindItemError, base::IntToString(parent_id.uid));
+ ExtensionMenuItem* parent = NULL;
+ if (!GetParent(*properties, *menu_manager, &parent))
return false;
- }
- if (parent->type() != ExtensionMenuItem::NORMAL) {
- error_ = kParentsMustBeNormalError;
- return false;
- }
- success = menu_manager->AddChildItem(parent_id, item.release());
+ success = menu_manager->AddChildItem(parent->id(), item.release());
} else {
success = menu_manager->AddContextItem(GetExtension(), item.release());
}
@@ -276,15 +304,17 @@
bool UpdateContextMenuFunction::RunImpl() {
bool radioItemUpdated = false;
- ExtensionMenuItem::Id item_id(profile(), extension_id(), 0);
- EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &item_id.uid));
+ ExtensionMenuItem::Id item_id(profile(), extension_id());
+ Value* id_value = NULL;
+ EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &id_value));
+ EXTENSION_FUNCTION_VALIDATE(ParseID(id_value, &item_id));
ExtensionService* service = profile()->GetExtensionService();
ExtensionMenuManager* manager = service->menu_manager();
ExtensionMenuItem* item = manager->GetItemById(item_id);
if (!item || item->extension_id() != extension_id()) {
error_ = ExtensionErrorUtils::FormatErrorMessage(
- kCannotFindItemError, base::IntToString(item_id.uid));
+ kCannotFindItemError, GetIDString(item_id));
return false;
}
@@ -358,8 +388,10 @@
}
bool RemoveContextMenuFunction::RunImpl() {
- ExtensionMenuItem::Id id(profile(), extension_id(), 0);
- EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &id.uid));
+ ExtensionMenuItem::Id id(profile(), extension_id());
+ Value* id_value = NULL;
+ EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &id_value));
+ EXTENSION_FUNCTION_VALIDATE(ParseID(id_value, &id));
ExtensionService* service = profile()->GetExtensionService();
ExtensionMenuManager* manager = service->menu_manager();
@@ -367,7 +399,7 @@
// Ensure one extension can't remove another's menu items.
if (!item || item->extension_id() != extension_id()) {
error_ = ExtensionErrorUtils::FormatErrorMessage(
- kCannotFindItemError, base::IntToString(id.uid));
+ kCannotFindItemError, GetIDString(id));
return false;
}
diff --git a/chrome/browser/extensions/api/context_menu/context_menu_api.h b/chrome/browser/extensions/api/context_menu/context_menu_api.h
index 2f88137..6117e3e 100644
--- a/chrome/browser/extensions/api/context_menu/context_menu_api.h
+++ b/chrome/browser/extensions/api/context_menu/context_menu_api.h
@@ -52,6 +52,11 @@
bool SetURLPatterns(const base::DictionaryValue& properties,
ExtensionMenuItem* item);
+ // Helper to read an ID from the Value*. The ID can be either a string or
+ // integer.
+ bool ParseID(const Value* value,
+ ExtensionMenuItem::Id* result);
+
// If the parentId key was specified in properties, this will try looking up
// an ExtensionMenuItem with that id and set it into |result|. Returns false
// on error, with an explanation written into error_. Note that if the
diff --git a/chrome/browser/extensions/api/context_menu/context_menu_apitest.cc b/chrome/browser/extensions/api/context_menu/context_menu_apitest.cc
index 973a299..094df01 100644
--- a/chrome/browser/extensions/api/context_menu/context_menu_apitest.cc
+++ b/chrome/browser/extensions/api/context_menu/context_menu_apitest.cc
@@ -15,6 +15,7 @@
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, ContextMenus) {
ASSERT_TRUE(RunExtensionTest("context_menus/basics")) << message_;
ASSERT_TRUE(RunExtensionTest("context_menus/no_perms")) << message_;
+ ASSERT_TRUE(RunExtensionTest("context_menus/item_ids")) << message_;
}
// crbug.com/51436 -- creating context menus from multiple script contexts
diff --git a/chrome/browser/extensions/extension_menu_manager.cc b/chrome/browser/extensions/extension_menu_manager.cc
index d12989c..8f320056 100644
--- a/chrome/browser/extensions/extension_menu_manager.cc
+++ b/chrome/browser/extensions/extension_menu_manager.cc
@@ -25,6 +25,19 @@
using content::WebContents;
+namespace {
+
+void SetIdKeyValue(base::DictionaryValue* properties,
+ const char* key,
+ ExtensionMenuItem::Id id) {
+ if (id.uid == 0)
+ properties->SetString(key, id.string_uid);
+ else
+ properties->SetInteger(key, id.uid);
+}
+
+} // namespace
+
ExtensionMenuItem::ExtensionMenuItem(const Id& id,
const std::string& title,
bool checked,
@@ -408,9 +421,9 @@
ListValue args;
DictionaryValue* properties = new DictionaryValue();
- properties->SetInteger("menuItemId", item->id().uid);
+ SetIdKeyValue(properties, "menuItemId", item->id());
if (item->parent_id())
- properties->SetInteger("parentMenuItemId", item->parent_id()->uid);
+ SetIdKeyValue(properties, "parentMenuItemId", item->id());
switch (params.media_type) {
case WebKit::WebContextMenuData::MediaTypeImage:
@@ -464,6 +477,7 @@
event_router->DispatchEventToExtension(
item->extension_id(), event_name, json_args, profile, GURL(),
ExtensionEventRouter::USER_GESTURE_ENABLED);
+ // TODO(yoz): dispatch another event onClicked.
}
void ExtensionMenuManager::SanitizeRadioList(
@@ -543,13 +557,13 @@
}
ExtensionMenuItem::Id::Id()
- : profile(NULL), uid(0) {
+ : profile(NULL), extension_id(""), uid(0), string_uid("") {
}
ExtensionMenuItem::Id::Id(Profile* profile,
- const std::string& extension_id,
- int uid)
- : profile(profile), extension_id(extension_id), uid(uid) {
+ const std::string& extension_id)
+ : profile(profile), extension_id(extension_id), uid(0),
+ string_uid("") {
}
ExtensionMenuItem::Id::~Id() {
@@ -558,7 +572,8 @@
bool ExtensionMenuItem::Id::operator==(const Id& other) const {
return (profile == other.profile &&
extension_id == other.extension_id &&
- uid == other.uid);
+ uid == other.uid &&
+ string_uid == other.string_uid);
}
bool ExtensionMenuItem::Id::operator!=(const Id& other) const {
@@ -571,8 +586,12 @@
if (profile == other.profile) {
if (extension_id < other.extension_id)
return true;
- if (extension_id == other.extension_id)
- return uid < other.uid;
+ if (extension_id == other.extension_id) {
+ if (uid < other.uid)
+ return true;
+ if (uid == other.uid)
+ return string_uid < other.string_uid;
+ }
}
return false;
}
diff --git a/chrome/browser/extensions/extension_menu_manager.h b/chrome/browser/extensions/extension_menu_manager.h
index 299fa05..30a8b35 100644
--- a/chrome/browser/extensions/extension_menu_manager.h
+++ b/chrome/browser/extensions/extension_menu_manager.h
@@ -40,7 +40,10 @@
// An Id uniquely identifies a context menu item registered by an extension.
struct Id {
Id();
- Id(Profile* profile, const std::string& extension_id, int uid);
+ // Since the unique ID (uid or string_uid) is parsed from API arguments,
+ // the normal usage is to set the uid or string_uid immediately after
+ // construction.
+ Id(Profile* profile, const std::string& extension_id);
~Id();
bool operator==(const Id& other) const;
@@ -49,7 +52,9 @@
Profile* profile;
std::string extension_id;
+ // Only one of uid or string_uid will be defined.
int uid;
+ std::string string_uid;
};
// For context menus, these are the contexts where an item can appear.
diff --git a/chrome/browser/extensions/extension_menu_manager_unittest.cc b/chrome/browser/extensions/extension_menu_manager_unittest.cc
index cae9a9b..a506545 100644
--- a/chrome/browser/extensions/extension_menu_manager_unittest.cc
+++ b/chrome/browser/extensions/extension_menu_manager_unittest.cc
@@ -45,7 +45,18 @@
ExtensionMenuItem* CreateTestItem(Extension* extension) {
ExtensionMenuItem::Type type = ExtensionMenuItem::NORMAL;
ExtensionMenuItem::ContextList contexts(ExtensionMenuItem::ALL);
- ExtensionMenuItem::Id id(NULL, extension->id(), next_id_++);
+ ExtensionMenuItem::Id id(NULL, extension->id());
+ id.uid = next_id_++;
+ return new ExtensionMenuItem(id, "test", false, true, type, contexts);
+ }
+
+ // Returns a test item with the given string ID.
+ ExtensionMenuItem* CreateTestItemWithID(Extension* extension,
+ const std::string& string_id) {
+ ExtensionMenuItem::Type type = ExtensionMenuItem::NORMAL;
+ ExtensionMenuItem::ContextList contexts(ExtensionMenuItem::ALL);
+ ExtensionMenuItem::Id id(NULL, extension->id());
+ id.string_uid = string_id;
return new ExtensionMenuItem(id, "test", false, true, type, contexts);
}
@@ -87,7 +98,7 @@
ASSERT_EQ(item1, items->at(0));
// Add a second item, make sure it comes back too.
- ExtensionMenuItem* item2 = CreateTestItem(extension);
+ ExtensionMenuItem* item2 = CreateTestItemWithID(extension, "id2");
ASSERT_TRUE(manager_.AddContextItem(extension, item2));
ASSERT_EQ(item2, manager_.GetItemById(item2->id()));
items = manager_.MenuItems(item2->extension_id());
@@ -107,8 +118,19 @@
ASSERT_EQ(2u, manager_.MenuItems(extension_id)->size());
// Make sure removing a non-existent item returns false.
- ExtensionMenuItem::Id id(NULL, extension->id(), id3.uid + 50);
+ ExtensionMenuItem::Id id(NULL, extension->id());
+ id.uid = id3.uid + 50;
ASSERT_FALSE(manager_.RemoveContextMenuItem(id));
+
+ // Make sure adding an item with the same string ID returns false.
+ scoped_ptr<ExtensionMenuItem> item2too(
+ CreateTestItemWithID(extension, "id2"));
+ ASSERT_FALSE(manager_.AddContextItem(extension, item2too.get()));
+
+ // But the same string ID should not collide with another extension.
+ Extension* extension2 = AddExtension("test2");
+ ExtensionMenuItem* item2other = CreateTestItemWithID(extension2, "id2");
+ ASSERT_TRUE(manager_.AddContextItem(extension2, item2other));
}
// Test adding/removing child items.
@@ -119,7 +141,7 @@
ExtensionMenuItem* item1 = CreateTestItem(extension1);
ExtensionMenuItem* item2 = CreateTestItem(extension2);
- ExtensionMenuItem* item2_child = CreateTestItem(extension2);
+ ExtensionMenuItem* item2_child = CreateTestItemWithID(extension2, "2child");
ExtensionMenuItem* item2_grandchild = CreateTestItem(extension2);
// This third item we expect to fail inserting, so we use a scoped_ptr to make
@@ -173,8 +195,8 @@
// Set up 5 items to add.
ExtensionMenuItem* item1 = CreateTestItem(extension);
ExtensionMenuItem* item2 = CreateTestItem(extension);
- ExtensionMenuItem* item3 = CreateTestItem(extension);
- ExtensionMenuItem* item4 = CreateTestItem(extension);
+ ExtensionMenuItem* item3 = CreateTestItemWithID(extension, "id3");
+ ExtensionMenuItem* item4 = CreateTestItemWithID(extension, "id4");
ExtensionMenuItem* item5 = CreateTestItem(extension);
ExtensionMenuItem* item6 = CreateTestItem(extension);
ExtensionMenuItem::Id item1_id = item1->id();
@@ -370,7 +392,6 @@
public:
MockTestingProfile() {}
MOCK_METHOD0(GetExtensionEventRouter, ExtensionEventRouter*());
- MOCK_CONST_METHOD0(IsOffTheRecord, bool());
private:
DISALLOW_COPY_AND_ASSIGN(MockTestingProfile);
@@ -414,11 +435,14 @@
Extension* extension1 = AddExtension("1111");
ExtensionMenuItem* item1 = CreateTestItem(extension1);
ExtensionMenuItem* item2 = CreateTestItem(extension1);
+ ExtensionMenuItem* item3 = CreateTestItemWithID(extension1, "id3");
ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
ASSERT_TRUE(manager_.AddContextItem(extension1, item2));
+ ASSERT_TRUE(manager_.AddContextItem(extension1, item3));
ASSERT_FALSE(manager_.context_items_.empty());
+ manager_.RemoveContextMenuItem(item3->id());
manager_.RemoveContextMenuItem(item1->id());
manager_.RemoveContextMenuItem(item2->id());
