From a11fa3437fdbb89ebe1e3a61cb4fc1de1ae4a352 Mon Sep 17 00:00:00 2001 From: "asargent@chromium.org" Date: Fri, 9 Jul 2010 16:56:00 +0000 Subject: Some cleanup of the extensions context menu API. This CL contains the following: -Use lower case names for enum values in the create/update properties (eg 'page' instead of 'PAGE') -Make the top-level API name plural (contextMenus instead of contextMenu) -Don't fire onclick handlers for a parent menu item when one of its children is clicked on. -Remove the enabledContexts property for now, to eventually be replaced with a way to programmatically enable/disable. There are a few more things in the bug that I'll be doing in subsequent CL's. BUG=48198 TEST=Extensions using the context menu API should work with the changes described above. Review URL: http://codereview.chromium.org/2887013 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@51970 0039d316-1c4b-4281-b951-d872f2087c98 --- .../extensions/extension_context_menu_api.cc | 45 ++++++++-------------- .../extensions/extension_context_menu_api.h | 8 ++-- .../browser/extensions/extension_menu_manager.cc | 6 +-- chrome/browser/extensions/extension_menu_manager.h | 14 ++----- .../extensions/extension_menu_manager_unittest.cc | 7 ++-- 5 files changed, 28 insertions(+), 52 deletions(-) (limited to 'chrome/browser/extensions') diff --git a/chrome/browser/extensions/extension_context_menu_api.cc b/chrome/browser/extensions/extension_context_menu_api.cc index 1c63dd1..f5ecca4 100644 --- a/chrome/browser/extensions/extension_context_menu_api.cc +++ b/chrome/browser/extensions/extension_context_menu_api.cc @@ -8,7 +8,6 @@ #include "chrome/browser/extensions/extensions_service.h" #include "chrome/browser/profile.h" -const wchar_t kEnabledContextsKey[] = L"enabledContexts"; const wchar_t kCheckedKey[] = L"checked"; const wchar_t kContextsKey[] = L"contexts"; const wchar_t kParentIdKey[] = L"parentId"; @@ -18,9 +17,9 @@ const wchar_t kTypeKey[] = L"type"; const char kTitleNeededError[] = "All menu items except for separators must have a title"; const char kCheckedError[] = - "Only items with type RADIO or CHECKBOX can be checked"; + "Only items with type \"radio\" or \"checkbox\" can be checked"; const char kParentsMustBeNormalError[] = - "Parent items must have type NORMAL"; + "Parent items must have type \"normal\""; bool ExtensionContextMenuFunction::ParseContexts( const DictionaryValue& properties, @@ -37,21 +36,21 @@ bool ExtensionContextMenuFunction::ParseContexts( if (!list->GetString(i, &value)) return false; - if (value == "ALL") { + if (value == "all") { tmp_result.Add(ExtensionMenuItem::ALL); - } else if (value == "PAGE") { + } else if (value == "page") { tmp_result.Add(ExtensionMenuItem::PAGE); - } else if (value == "SELECTION") { + } else if (value == "selection") { tmp_result.Add(ExtensionMenuItem::SELECTION); - } else if (value == "LINK") { + } else if (value == "link") { tmp_result.Add(ExtensionMenuItem::LINK); - } else if (value == "EDITABLE") { + } else if (value == "editable") { tmp_result.Add(ExtensionMenuItem::EDITABLE); - } else if (value == "IMAGE") { + } else if (value == "image") { tmp_result.Add(ExtensionMenuItem::IMAGE); - } else if (value == "VIDEO") { + } else if (value == "video") { tmp_result.Add(ExtensionMenuItem::VIDEO); - } else if (value == "AUDIO") { + } else if (value == "audio") { tmp_result.Add(ExtensionMenuItem::AUDIO); } else { error_ = "Invalid value for " + WideToASCII(key); @@ -76,13 +75,13 @@ bool ExtensionContextMenuFunction::ParseType( if (!properties.GetString(kTypeKey, &type_string)) return false; - if (type_string == "NORMAL") { + if (type_string == "normal") { *result = ExtensionMenuItem::NORMAL; - } else if (type_string == "CHECKBOX") { + } else if (type_string == "checkbox") { *result = ExtensionMenuItem::CHECKBOX; - } else if (type_string == "RADIO") { + } else if (type_string == "radio") { *result = ExtensionMenuItem::RADIO; - } else if (type_string == "SEPARATOR") { + } else if (type_string == "separator") { *result = ExtensionMenuItem::SEPARATOR; } else { error_ = "Invalid type string '" + type_string + "'"; @@ -104,7 +103,7 @@ bool ExtensionContextMenuFunction::ParseChecked( return false; if (checked && type != ExtensionMenuItem::CHECKBOX && type != ExtensionMenuItem::RADIO) { - error_ = "Only CHECKBOX and RADIO type items can be checked"; + error_ = "Only checkbox and radio type items can be checked"; return false; } return true; @@ -151,10 +150,6 @@ bool CreateContextMenuFunction::RunImpl() { if (!ParseContexts(*properties, kContextsKey, &contexts)) return false; - ExtensionMenuItem::ContextList enabled_contexts = contexts; - if (!ParseContexts(*properties, kEnabledContextsKey, &enabled_contexts)) - return false; - ExtensionMenuItem::Type type; if (!ParseType(*properties, ExtensionMenuItem::NORMAL, &type)) return false; @@ -169,8 +164,7 @@ bool CreateContextMenuFunction::RunImpl() { return false; scoped_ptr item( - new ExtensionMenuItem(extension_id(), title, checked, type, contexts, - enabled_contexts)); + new ExtensionMenuItem(extension_id(), title, checked, type, contexts)); int id = 0; if (properties->HasKey(kParentIdKey)) { @@ -253,13 +247,6 @@ bool UpdateContextMenuFunction::RunImpl() { if (contexts != item->contexts()) item->set_contexts(contexts); - // Enabled contexts. - ExtensionMenuItem::ContextList enabled_contexts(item->contexts()); - if (!ParseContexts(*properties, kEnabledContextsKey, &enabled_contexts)) - return false; - if (enabled_contexts != item->enabled_contexts()) - item->set_enabled_contexts(enabled_contexts); - // Parent id. ExtensionMenuItem* parent = NULL; if (!GetParent(*properties, *menu_manager, &parent)) diff --git a/chrome/browser/extensions/extension_context_menu_api.h b/chrome/browser/extensions/extension_context_menu_api.h index c9c3380..3362aae 100644 --- a/chrome/browser/extensions/extension_context_menu_api.h +++ b/chrome/browser/extensions/extension_context_menu_api.h @@ -46,25 +46,25 @@ class ExtensionContextMenuFunction : public SyncExtensionFunction { class CreateContextMenuFunction : public ExtensionContextMenuFunction { ~CreateContextMenuFunction() {} virtual bool RunImpl(); - DECLARE_EXTENSION_FUNCTION_NAME("experimental.contextMenu.create") + DECLARE_EXTENSION_FUNCTION_NAME("experimental.contextMenus.create") }; class UpdateContextMenuFunction : public ExtensionContextMenuFunction { ~UpdateContextMenuFunction() {} virtual bool RunImpl(); - DECLARE_EXTENSION_FUNCTION_NAME("experimental.contextMenu.update") + DECLARE_EXTENSION_FUNCTION_NAME("experimental.contextMenus.update") }; class RemoveContextMenuFunction : public ExtensionContextMenuFunction { ~RemoveContextMenuFunction() {} virtual bool RunImpl(); - DECLARE_EXTENSION_FUNCTION_NAME("experimental.contextMenu.remove") + DECLARE_EXTENSION_FUNCTION_NAME("experimental.contextMenus.remove") }; class RemoveAllContextMenusFunction : public ExtensionContextMenuFunction { ~RemoveAllContextMenusFunction() {} virtual bool RunImpl(); - DECLARE_EXTENSION_FUNCTION_NAME("experimental.contextMenu.removeAll") + DECLARE_EXTENSION_FUNCTION_NAME("experimental.contextMenus.removeAll") }; #endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_CONTEXT_MENU_API_H__ diff --git a/chrome/browser/extensions/extension_menu_manager.cc b/chrome/browser/extensions/extension_menu_manager.cc index 42e3842..fc02637 100644 --- a/chrome/browser/extensions/extension_menu_manager.cc +++ b/chrome/browser/extensions/extension_menu_manager.cc @@ -27,15 +27,13 @@ ExtensionMenuItem::ExtensionMenuItem(const std::string& extension_id, std::string title, bool checked, Type type, - const ContextList& contexts, - const ContextList& enabled_contexts) + const ContextList& contexts) : extension_id_(extension_id), title_(title), id_(0), type_(type), checked_(checked), contexts_(contexts), - enabled_contexts_(enabled_contexts), parent_id_(0) {} ExtensionMenuItem::~ExtensionMenuItem() { @@ -439,7 +437,7 @@ void ExtensionMenuManager::ExecuteCommand(Profile* profile, std::string json_args; base::JSONWriter::Write(&args, false, &json_args); - std::string event_name = "contextMenu/" + item->extension_id(); + std::string event_name = "contextMenus/" + item->extension_id(); service->DispatchEventToRenderers(event_name, json_args, profile->IsOffTheRecord(), GURL()); } diff --git a/chrome/browser/extensions/extension_menu_manager.h b/chrome/browser/extensions/extension_menu_manager.h index c3f82c5..98752f8 100644 --- a/chrome/browser/extensions/extension_menu_manager.h +++ b/chrome/browser/extensions/extension_menu_manager.h @@ -32,8 +32,7 @@ class ExtensionMenuItem { // A list of ExtensionMenuItem's. typedef std::vector List; - // For context menus, these are the contexts where an item can appear and - // potentially be enabled. + // For context menus, these are the contexts where an item can appear. enum Context { ALL = 1, PAGE = 2, @@ -53,7 +52,7 @@ class ExtensionMenuItem { SEPARATOR }; - // A list of Contexts for an item (where it should be shown/enabled). + // A list of Contexts for an item. class ContextList { public: ContextList() : value_(0) {} @@ -85,8 +84,7 @@ class ExtensionMenuItem { }; ExtensionMenuItem(const std::string& extension_id, std::string title, - bool checked, Type type, const ContextList& contexts, - const ContextList& enabled_contexts); + bool checked, Type type, const ContextList& contexts); virtual ~ExtensionMenuItem(); // Simple accessor methods. @@ -97,14 +95,12 @@ class ExtensionMenuItem { int parent_id() const { return parent_id_; } int child_count() const { return children_.size(); } ContextList contexts() const { return contexts_; } - ContextList enabled_contexts() const { return enabled_contexts_; } Type type() const { return type_; } bool checked() const { return checked_; } // Simple mutator methods. void set_title(std::string new_title) { title_ = new_title; } void set_contexts(ContextList contexts) { contexts_ = contexts; } - void set_enabled_contexts(ContextList contexts) { contexts_ = contexts; } void set_type(Type type) { type_ = type; } // Returns the title with any instances of %s replaced by |selection|. @@ -155,10 +151,6 @@ class ExtensionMenuItem { // In what contexts should the item be shown? ContextList contexts_; - // In what contexts should the item be enabled (i.e. not greyed out). This - // should be a subset of contexts_. - ContextList enabled_contexts_; - // If this item is a child of another item, the unique id of its parent. If // this is a top-level item with no parent, this will be 0. int parent_id_; diff --git a/chrome/browser/extensions/extension_menu_manager_unittest.cc b/chrome/browser/extensions/extension_menu_manager_unittest.cc index a03702a..f0960cb 100644 --- a/chrome/browser/extensions/extension_menu_manager_unittest.cc +++ b/chrome/browser/extensions/extension_menu_manager_unittest.cc @@ -36,9 +36,8 @@ class ExtensionMenuManagerTest : public testing::Test { static ExtensionMenuItem* CreateTestItem(Extension* extension) { ExtensionMenuItem::Type type = ExtensionMenuItem::NORMAL; ExtensionMenuItem::ContextList contexts(ExtensionMenuItem::ALL); - ExtensionMenuItem::ContextList enabled_contexts = contexts; - return new ExtensionMenuItem(extension->id(), "test", false, type, contexts, - enabled_contexts); + return new ExtensionMenuItem(extension->id(), "test", false, type, + contexts); } // Creates and returns a test Extension. The caller does *not* own the return @@ -363,7 +362,7 @@ TEST_F(ExtensionMenuManagerTest, ExecuteCommand) { // Use the magic of googlemock to save a parameter to our mock's // DispatchEventToRenderers method into event_args. std::string event_args; - std::string expected_event_name = "contextMenu/" + item->extension_id(); + std::string expected_event_name = "contextMenus/" + item->extension_id(); EXPECT_CALL(*mock_message_service.get(), DispatchEventToRenderers(expected_event_name, _, profile.IsOffTheRecord(), -- cgit v1.1