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

5 files changed, 28 insertions, 52 deletions

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<ExtensionMenuItem> 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<ExtensionMenuItem*> 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(),