Context Menus now uses the JSON Schema Compiler.

chrome/browser/extensions/api/context_menu/context_menu_api.cc

Index: chrome/browser/extensions/api/context_menu/context_menu_api.cc
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 15f1af22c903299bc63d92efaa71aeeb58d08711..b29681fabcfd2f5b34af002110ff8f235fad1efe 100644
--- a/chrome/browser/extensions/api/context_menu/context_menu_api.cc
+++ b/chrome/browser/extensions/api/context_menu/context_menu_api.cc
@@ -10,22 +10,15 @@
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/menu_manager.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/api/context_menus.h"
 #include "chrome/common/extensions/extension_error_utils.h"
+#include "chrome/common/extensions/url_pattern_set.h"
 
 namespace {
 
-const char kCheckedKey[] = "checked";
-const char kContextsKey[] = "contexts";
-const char kDocumentUrlPatternsKey[] = "documentUrlPatterns";
-const char kEnabledKey[] = "enabled";
 const char kGeneratedIdKey[] = "generatedId";
-const char kIdKey[] = "id";
-const char kOnclickKey[] = "onclick";
-const char kParentIdKey[] = "parentId";
-const char kTargetUrlPatternsKey[] = "targetUrlPatterns";
-const char kTitleKey[] = "title";
-const char kTypeKey[] = "type";
 
 const char kCannotFindItemError[] = "Cannot find menu item with id *";
 const char kOnclickDisallowedError[] = "Extensions using event pages cannot "
@@ -37,8 +30,6 @@ const char kDuplicateIDError[] =
     "Cannot create item with duplicate id *";
 const char kIdRequiredError[] = "Extensions using event pages must pass an "
     "id parameter to chrome.contextMenus.create";
-const char kInvalidValueError[] = "Invalid value for *";
-const char kInvalidTypeStringError[] = "Invalid type string '*'";
 const char kParentsMustBeNormalError[] =
     "Parent items must have type \"normal\"";
 const char kTitleNeededError[] =
@@ -51,153 +42,124 @@ std::string GetIDString(const extensions::MenuItem::Id& id) {
     return base::IntToString(id.uid);
 }
 
-}  // namespace
-
-namespace extensions {
-
-bool ExtensionContextMenuFunction::ParseContexts(
-    const DictionaryValue& properties,
-    const char* key,
-    MenuItem::ContextList* result) {
-  const ListValue* list = NULL;
-  if (!properties.GetList(key, &list)) {
-    return true;
-  }
-  MenuItem::ContextList tmp_result;
-
-  std::string value;
-  for (size_t i = 0; i < list->GetSize(); i++) {
-    if (!list->GetString(i, &value))
-      return false;
-
-    if (value == "all") {
-      tmp_result.Add(MenuItem::ALL);
-    } else if (value == "page") {
-      tmp_result.Add(MenuItem::PAGE);
-    } else if (value == "selection") {
-      tmp_result.Add(MenuItem::SELECTION);
-    } else if (value == "link") {
-      tmp_result.Add(MenuItem::LINK);
-    } else if (value == "editable") {
-      tmp_result.Add(MenuItem::EDITABLE);
-    } else if (value == "image") {
-      tmp_result.Add(MenuItem::IMAGE);
-    } else if (value == "video") {
-      tmp_result.Add(MenuItem::VIDEO);
-    } else if (value == "audio") {
-      tmp_result.Add(MenuItem::AUDIO);
-    } else if (value == "frame") {
-      tmp_result.Add(MenuItem::FRAME);
-    } else {
-      error_ = ExtensionErrorUtils::FormatErrorMessage(kInvalidValueError, key);
-      return false;
+template<typename PropertyWithEnumT>
+bool ParseContexts(extensions::MenuItem::ContextList& contexts,

[not at google - send to devlin, 2012/08/06 01:55:45]

Don't pass non-const refs; this should be const&

[chebert, 2012/08/06 22:43:06]

Done.

+                  PropertyWithEnumT& property) {

[not at google - send to devlin, 2012/08/06 01:55:45]

Ditto non-const refs; if you need to pass an out-param, use a pointer.

also nit: one more space indent

[chebert, 2012/08/06 22:43:06]

Done.

+  for (size_t i = 0; i < property.contexts->size(); ++i) {
+    switch (property.contexts->at(i)) {
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_ALL:
+        contexts.Add(extensions::MenuItem::ALL);
+        break;
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_PAGE:
+        contexts.Add(extensions::MenuItem::PAGE);
+        break;
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_SELECTION:
+        contexts.Add(extensions::MenuItem::SELECTION);
+        break;
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_LINK:
+        contexts.Add(extensions::MenuItem::LINK);
+        break;
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_EDITABLE:
+        contexts.Add(extensions::MenuItem::EDITABLE);
+        break;
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_IMAGE:
+        contexts.Add(extensions::MenuItem::IMAGE);
+        break;
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_VIDEO:
+        contexts.Add(extensions::MenuItem::VIDEO);
+        break;
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_AUDIO:
+        contexts.Add(extensions::MenuItem::AUDIO);
+        break;
+      case PropertyWithEnumT::CONTEXTS_ELEMENT_FRAME:
+        contexts.Add(extensions::MenuItem::FRAME);
+        break;
+      default:
+        return false;
     }
   }
-  *result = tmp_result;
   return true;
 }
 
-bool ExtensionContextMenuFunction::ParseType(
-    const DictionaryValue& properties,
-    const MenuItem::Type& default_value,
-    MenuItem::Type* result) {
-  DCHECK(result);
-  if (!properties.HasKey(kTypeKey)) {
-    *result = default_value;
-    return true;
+template<typename PropertyWithEnumT>
+extensions::MenuItem::Type ParseType(PropertyWithEnumT& property) {

[not at google - send to devlin, 2012/08/06 01:55:45]

ditto non-const refs (use pointer)

[chebert, 2012/08/06 22:43:06]

Done.

+  switch (property.type) {
+    case PropertyWithEnumT::TYPE_NONE:
+    case PropertyWithEnumT::TYPE_NORMAL:
+      return extensions::MenuItem::NORMAL;
+    case PropertyWithEnumT::TYPE_CHECKBOX:
+      return extensions::MenuItem::CHECKBOX;
+    case PropertyWithEnumT::TYPE_RADIO:
+      return extensions::MenuItem::RADIO;
+    case PropertyWithEnumT::TYPE_SEPARATOR:
+      return extensions::MenuItem::SEPARATOR;
   }
-
-  std::string type_string;
-  if (!properties.GetString(kTypeKey, &type_string))
-    return false;
-
-  if (type_string == "normal") {
-    *result = MenuItem::NORMAL;
-  } else if (type_string == "checkbox") {
-    *result = MenuItem::CHECKBOX;
-  } else if (type_string == "radio") {
-    *result = MenuItem::RADIO;
-  } else if (type_string == "separator") {
-    *result = MenuItem::SEPARATOR;
-  } else {
-    error_ = ExtensionErrorUtils::FormatErrorMessage(kInvalidTypeStringError,
-                                                     type_string);
-    return false;
-  }
-  return true;
+  return extensions::MenuItem::NORMAL;

[not at google - send to devlin, 2012/08/06 01:55:45]

All other methods have out-params; this one just returns its result. I generally
prefer methods that just return their results if possible... and all of these
values come from json-schema-compiler so if they're wrong them something else is
wrong.

So I'm saying that, if possible, change all the methods to return their results.
If not then out-params fine I guess, but we should at least be consistent.

[chebert, 2012/08/06 22:43:06]

Done.

 }
 
-bool ExtensionContextMenuFunction::ParseChecked(
-    MenuItem::Type type,
-    const DictionaryValue& properties,
-    bool default_value,
-    bool* checked) {
-  if (!properties.HasKey(kCheckedKey)) {
-    *checked = default_value;
-    return true;
-  }
-  if (!properties.GetBoolean(kCheckedKey, checked))
-    return false;
-  if (checked && type != MenuItem::CHECKBOX && type != MenuItem::RADIO) {
-    error_ = kCheckedError;
-    return false;
+template<typename PropertyWithEnumT>
+bool ParseParentId(extensions::MenuItem::Id& parent_id,

[not at google - send to devlin, 2012/08/06 01:55:45]

pass pointer, and put argument at end; out-params should go at the end of the
parameter list.

[chebert, 2012/08/06 22:43:06]

Done.

+                   PropertyWithEnumT& property) {

[not at google - send to devlin, 2012/08/06 01:55:45]

pass const&

[chebert, 2012/08/06 22:43:06]

Done.

+  switch (property.parent_id_type) {
+    case PropertyWithEnumT::PARENT_ID_NONE:
+      return false;
+    case PropertyWithEnumT::PARENT_ID_INTEGER:
+      parent_id.uid = *property.parent_id_integer;
+      break;
+    case PropertyWithEnumT::PARENT_ID_STRING:
+      parent_id.string_uid = *property.parent_id_string;
+      break;
   }
   return true;

[not at google - send to devlin, 2012/08/06 01:55:45]

why returning true, I would have expected this to be a failure? I.e. I think the
breaks should be "return true" and the fall-through case should be "return
false"?

[chebert, 2012/08/06 22:43:06]

Done.

 }
 
-bool ExtensionContextMenuFunction::ParseID(const Value* value,
-                                           MenuItem::Id* result) {
-  return (value->GetAsInteger(&result->uid) ||
-          value->GetAsString(&result->string_uid));
-}
-
-bool ExtensionContextMenuFunction::GetParent(const DictionaryValue& properties,
-                                             const MenuManager& manager,
-                                             MenuItem** result) {
-  if (!properties.HasKey(kParentIdKey))
-    return true;
-  MenuItem::Id parent_id(profile()->IsOffTheRecord(), extension_id());
-  const Value* parent_id_value = NULL;
-  if (properties.Get(kParentIdKey, &parent_id_value) &&
-      !ParseID(parent_id_value, &parent_id))
-    return false;
-
-  MenuItem* parent = manager.GetItemById(parent_id);
+extensions::MenuItem* GetParent(extensions::MenuItem::Id& parent_id,
+                                extensions::MenuManager* menu_manager,
+                                std::string* error) {
+  extensions::MenuItem* parent = menu_manager->GetItemById(parent_id);
   if (!parent) {
-    error_ = ExtensionErrorUtils::FormatErrorMessage(
+    *error = ExtensionErrorUtils::FormatErrorMessage(
         kCannotFindItemError, GetIDString(parent_id));
-    return false;
   }
-  if (parent->type() != MenuItem::NORMAL) {
-    error_ = kParentsMustBeNormalError;
-    return false;
+  if (parent->type() != extensions::MenuItem::NORMAL) {
+    *error = kParentsMustBeNormalError;
+    return NULL;
   }
-  *result = parent;
-  return true;
+
+  return parent;
 }
 
-bool CreateContextMenuFunction::RunImpl() {
-  DictionaryValue* properties;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &properties));
-  EXTENSION_FUNCTION_VALIDATE(properties != NULL);
+}  // namespace
+
+namespace extensions {
+
+namespace Create = api::context_menus::Create;
+namespace Remove = api::context_menus::Remove;
+namespace Update = api::context_menus::Update;
 
+bool CreateContextMenuFunction::RunImpl() {
   MenuItem::Id id(profile()->IsOffTheRecord(), extension_id());
-  if (properties->HasKey(kIdKey)) {
-    EXTENSION_FUNCTION_VALIDATE(properties->GetString(kIdKey,
-                                                      &id.string_uid));
+  scoped_ptr<Create::Params> params(Create::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  if (params->create_properties.id.get()) {
+    id.string_uid = *params->create_properties.id;
   } else {
     if (GetExtension()->has_lazy_background_page()) {
       error_ = kIdRequiredError;
       return false;
     }
+
+    // The Generated Id is added by context_menus_custom_bindings.js.
+    DictionaryValue* properties = NULL;
+    EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &properties));
     EXTENSION_FUNCTION_VALIDATE(properties->GetInteger(kGeneratedIdKey,
                                                        &id.uid));
   }
 
   std::string title;
-  if (properties->HasKey(kTitleKey) &&
-      !properties->GetString(kTitleKey, &title))
-    return false;
+  if (params->create_properties.title.get())
+    title = *params->create_properties.title;
 
   MenuManager* menu_manager = profile()->GetExtensionService()->menu_manager();
 
@@ -208,43 +170,49 @@ bool CreateContextMenuFunction::RunImpl() {
   }
 
   if (GetExtension()->has_lazy_background_page() &&
-      properties->HasKey(kOnclickKey)) {
+      params->create_properties.onclick.get()) {
     error_ = kOnclickDisallowedError;
     return false;
   }
 
-  MenuItem::ContextList contexts(MenuItem::PAGE);
-  if (!ParseContexts(*properties, kContextsKey, &contexts))
-    return false;
+  MenuItem::ContextList contexts;
+  if (params->create_properties.contexts.get()) {
+    if (!ParseContexts(contexts, params->create_properties))
+        return false;

[not at google - send to devlin, 2012/08/06 01:55:45]

nit: too much indent

[chebert, 2012/08/06 22:43:06]

Done.

+  } else {
+    contexts.Add(MenuItem::PAGE);
+  }
 
-  MenuItem::Type type;
-  if (!ParseType(*properties, MenuItem::NORMAL, &type))
-    return false;
+  MenuItem::Type type = ParseType(params->create_properties);
 
   if (title.empty() && type != MenuItem::SEPARATOR) {
     error_ = kTitleNeededError;
     return false;
   }
 
-  bool checked;
-  if (!ParseChecked(type, *properties, false, &checked))
-    return false;
+  bool checked = false;
+  if (params->create_properties.checked.get())
+    checked = *params->create_properties.checked;
 
   bool enabled = true;
-  if (properties->HasKey(kEnabledKey))
-    EXTENSION_FUNCTION_VALIDATE(properties->GetBoolean(kEnabledKey, &enabled));
+  if (params->create_properties.enabled.get())
+    enabled = *params->create_properties.enabled;
 
   scoped_ptr<MenuItem> item(
       new MenuItem(id, title, checked, enabled, type, contexts));
 
   if (!item->PopulateURLPatterns(
-          *properties, kDocumentUrlPatternsKey, kTargetUrlPatternsKey, &error_))
+          params->create_properties.document_url_patterns.get(),
+          params->create_properties.target_url_patterns.get(),
+          &error_)) {
     return false;
+  }
 
   bool success = true;
-  if (properties->HasKey(kParentIdKey)) {
-    MenuItem* parent = NULL;
-    if (!GetParent(*properties, *menu_manager, &parent))
+  MenuItem::Id parent_id(profile()->IsOffTheRecord(), extension_id());
+  if (ParseParentId(parent_id, params->create_properties)) {

[not at google - send to devlin, 2012/08/06 01:55:45]

This looks like "if it succeed parsing", but it will always parse (since
json-schema-compiler has already parsed it), so could you rename it to
"GetParentId".

Ditto all those other Parse functions.

[chebert, 2012/08/06 22:43:06]

Done.

+    MenuItem* parent = GetParent(parent_id, menu_manager, &error_);
+    if (!parent)
       return false;
     success = menu_manager->AddChildItem(parent->id(), item.release());
   } else {
@@ -261,9 +229,17 @@ bool CreateContextMenuFunction::RunImpl() {
 bool UpdateContextMenuFunction::RunImpl() {
   bool radioItemUpdated = false;
   MenuItem::Id item_id(profile()->IsOffTheRecord(), extension_id());
-  Value* id_value = NULL;
-  EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &id_value));
-  EXTENSION_FUNCTION_VALIDATE(ParseID(id_value, &item_id));
+  scoped_ptr<Update::Params> params(Update::Params::Create(*args_));
+
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  switch (params->id_type) {
+    case Update::Params::ID_STRING:
+      item_id.string_uid = *params->id_string;
+      break;
+    case Update::Params::ID_INTEGER:
+      item_id.uid = *params->id_integer;
+      break;
+  }
 
   ExtensionService* service = profile()->GetExtensionService();
   MenuManager* manager = service->menu_manager();
@@ -274,26 +250,19 @@ bool UpdateContextMenuFunction::RunImpl() {
     return false;
   }
 
-  DictionaryValue* properties = NULL;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &properties));
-  EXTENSION_FUNCTION_VALIDATE(properties != NULL);
-
   // Type.
-  MenuItem::Type type;
-  if (!ParseType(*properties, item->type(), &type))
-    return false;
+  MenuItem::Type type = ParseType(params->update_properties);
+
   if (type != item->type()) {
-    if (type == MenuItem::RADIO || item->type() == MenuItem::RADIO) {
+    if (type == MenuItem::RADIO || item->type() == MenuItem::RADIO)
       radioItemUpdated = true;
-    }
     item->set_type(type);
   }
 
   // Title.
-  if (properties->HasKey(kTitleKey)) {
-    std::string title;
-    EXTENSION_FUNCTION_VALIDATE(properties->GetString(kTitleKey, &title));
-    if (title.empty() && type != MenuItem::SEPARATOR) {
+  if (params->update_properties.title.get()) {
+    std::string title(*params->update_properties.title);
+    if (title.empty() && item->type() != MenuItem::SEPARATOR) {
       error_ = kTitleNeededError;
       return false;
     }
@@ -301,39 +270,49 @@ bool UpdateContextMenuFunction::RunImpl() {
   }
 
   // Checked state.
-  bool checked;
-  if (!ParseChecked(item->type(), *properties, item->checked(), &checked))
-    return false;
-  if (checked != item->checked()) {
-    if (!item->SetChecked(checked))
+  if (params->update_properties.checked.get()) {
+    bool checked = *params->update_properties.checked;
+    if (checked && item->type() != MenuItem::CHECKBOX &&
+        item->type() != MenuItem::RADIO) {
+      error_ = kCheckedError;
       return false;
-    radioItemUpdated = true;
+    }
+    if (checked != item->checked()) {
+      if (!item->SetChecked(checked)) {
+        error_ = kCheckedError;
+        return false;
+      }
+      radioItemUpdated = true;
+    }
   }
 
   // Enabled.
-  bool enabled;
-  if (properties->HasKey(kEnabledKey)) {
-    EXTENSION_FUNCTION_VALIDATE(properties->GetBoolean(kEnabledKey, &enabled));
-    item->set_enabled(enabled);
-  }
+  if (params->update_properties.enabled.get())
+    item->set_enabled(*params->update_properties.enabled);
 
   // Contexts.
-  MenuItem::ContextList contexts(item->contexts());
-  if (!ParseContexts(*properties, kContextsKey, &contexts))
-    return false;
-  if (contexts != item->contexts())
-    item->set_contexts(contexts);
+  MenuItem::ContextList contexts;
+  if (params->update_properties.contexts.get()) {
+    ParseContexts(contexts, params->update_properties);
+    if (contexts != item->contexts())
+      item->set_contexts(contexts);
+  }
 
   // Parent id.
   MenuItem* parent = NULL;
-  if (!GetParent(*properties, *manager, &parent))
-    return false;
-  if (parent && !manager->ChangeParent(item->id(), &parent->id()))
-    return false;
+  MenuItem::Id parent_id(profile()->IsOffTheRecord(), extension_id());
+  if (ParseParentId(parent_id, params->update_properties)) {
+    MenuItem* parent = GetParent(parent_id, manager, &error_);
+    if (!parent || !manager->ChangeParent(item->id(), &parent->id()))
+      return false;
+  }
 
+  // URL Patterns.
   if (!item->PopulateURLPatterns(
-          *properties, kDocumentUrlPatternsKey, kTargetUrlPatternsKey, &error_))
+          params->update_properties.document_url_patterns.get(),
+          params->update_properties.target_url_patterns.get(), &error_)) {
     return false;
+  }
 
   // There is no need to call ItemUpdated if ChangeParent is called because
   // all sanitation is taken care of in ChangeParent.
@@ -345,13 +324,21 @@ bool UpdateContextMenuFunction::RunImpl() {
 }
 
 bool RemoveContextMenuFunction::RunImpl() {
-  MenuItem::Id id(profile()->IsOffTheRecord(), extension_id());
-  Value* id_value = NULL;
-  EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &id_value));
-  EXTENSION_FUNCTION_VALIDATE(ParseID(id_value, &id));
+  scoped_ptr<Remove::Params> params(Remove::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
   ExtensionService* service = profile()->GetExtensionService();
   MenuManager* manager = service->menu_manager();
 
+  MenuItem::Id id(profile()->IsOffTheRecord(), extension_id());
+  switch (params->menu_item_id_type) {
+    case Remove::Params::MENU_ITEM_ID_STRING:
+      id.string_uid = *params->menu_item_id_string;
+      break;
+    case Remove::Params::MENU_ITEM_ID_INTEGER:
+      id.uid = *params->menu_item_id_integer;
+  }
+
   MenuItem* item = manager->GetItemById(id);
   // Ensure one extension can't remove another's menu items.
   if (!item || item->extension_id() != extension_id()) {