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 acc80a448deea86d23d7e2236628014c73c7dfa7..ae5547f1a8c92f6ca7593ff4ea22a3fe66245863 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[] =
@@ -55,149 +46,32 @@ std::string GetIDString(const extensions::MenuItem::Id& id) {
 
 namespace extensions {
 
-bool ExtensionContextMenuFunction::ParseContexts(
-    const DictionaryValue& properties,
-    const char* key,
-    MenuItem::ContextList* result) {
-  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;
-    }
-  }
-  *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;
-  }
-
-  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;
-}
-
-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;
-  }
-  return true;
-}
-
-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());
-  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);
-  if (!parent) {
-    error_ = ExtensionErrorUtils::FormatErrorMessage(
-        kCannotFindItemError, GetIDString(parent_id));
-    return false;
-  }
-  if (parent->type() != MenuItem::NORMAL) {
-    error_ = kParentsMustBeNormalError;
-    return false;
-  }
-  *result = parent;
-  return true;
-}
+namespace Create = api::context_menus::Create;
+namespace Remove = api::context_menus::Remove;
+namespace Update = api::context_menus::Update;
 
 bool CreateContextMenuFunction::RunImpl() {
-  DictionaryValue* properties;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &properties));
-  EXTENSION_FUNCTION_VALIDATE(properties != NULL);
-
   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;
     }
+
+    DictionaryValue* properties = NULL;
+    EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &properties));
     EXTENSION_FUNCTION_VALIDATE(properties->GetInteger(kGeneratedIdKey,
                                                        &id.uid));

[not at google - send to devlin, 2012/07/30 15:36:53]

yep, ignore previous comment. this is fine.

Could you add a comment though, like "generatedId is added by
context_menus_custom_bindings.js".

[chebert, 2012/07/30 22:45:19]

Done.

   }
 
   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,44 +82,112 @@ 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);

[not at google - send to devlin, 2012/07/30 15:36:53]

I had a go at seeing whether JSC can do this, but not without more work than I'm
willing to do. However, I believe that C++ can, if you define a templated
function for this parsing, something like:

template <typename PropertyWithEnum>
MenuItem::ContextList GetMenuItemEnum(PropertyWithEnum::ContextEnum value) {
  switch (value) {
    case PropertyWithEnum::CONTEXT_ELEMENTS_ALL:
      return MenuItem::ALL;
...
  }
  return MenuItem::PAGE;
}

(you will probably have to play a bit to make that compile)

Then call that from here and below.

[chebert, 2012/07/30 22:45:19]

Done.

-  if (!ParseContexts(*properties, kContextsKey, &contexts))
-    return false;
+  if (params->create_properties.contexts.get()) {
+    for (size_t i = 0; i < params->create_properties.contexts->size(); ++i) {
+      switch (params->create_properties.contexts->at(i)) {
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_ALL:
+          contexts.Add(MenuItem::ALL);
+          break;
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_PAGE:
+          contexts.Add(MenuItem::PAGE);
+          break;
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_SELECTION:
+          contexts.Add(MenuItem::SELECTION);
+          break;
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_LINK:
+          contexts.Add(MenuItem::LINK);
+          break;
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_EDITABLE:
+          contexts.Add(MenuItem::EDITABLE);
+          break;
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_IMAGE:
+          contexts.Add(MenuItem::IMAGE);
+          break;
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_VIDEO:
+          contexts.Add(MenuItem::VIDEO);
+          break;
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_AUDIO:
+          contexts.Add(MenuItem::AUDIO);
+          break;
+        case Create::Params::CreateProperties::CONTEXTS_ELEMENT_FRAME:
+          contexts.Add(MenuItem::FRAME);
+          break;
+        default:
+          return false;
+      }
+    }
+  }
 
   MenuItem::Type type;
-  if (!ParseType(*properties, MenuItem::NORMAL, &type))
-    return false;
+  switch (params->create_properties.type) {
+    case Create::Params::CreateProperties::TYPE_NONE:

[not at google - send to devlin, 2012/07/30 15:36:53]

same comment as above re enum

[chebert, 2012/07/30 22:45:19]

Done.

+    case Create::Params::CreateProperties::TYPE_NORMAL:
+      type = MenuItem::NORMAL;
+      break;
+    case Create::Params::CreateProperties::TYPE_CHECKBOX:
+      type = MenuItem::CHECKBOX;
+      break;
+    case Create::Params::CreateProperties::TYPE_RADIO:
+      type = MenuItem::RADIO;
+      break;
+    case Create::Params::CreateProperties::TYPE_SEPARATOR:
+      type = MenuItem::SEPARATOR;
+      break;
+  }
 
   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_)) {

[not at google - send to devlin, 2012/07/30 15:36:53]

nit: &error_ on next line

[chebert, 2012/07/30 22:45:19]

Done.

     return false;
+  }
 
   bool success = true;
-  if (properties->HasKey(kParentIdKey)) {
-    MenuItem* parent = NULL;
-    if (!GetParent(*properties, *menu_manager, &parent))
+  if (params->create_properties.parent_id_type !=
+          Create::Params::CreateProperties::PARENT_ID_NONE) {
+    MenuItem::Id parent_id(profile()->IsOffTheRecord(), extension_id());
+    switch (params->create_properties.parent_id_type) {
+      case Create::Params::CreateProperties::PARENT_ID_INTEGER:
+        parent_id.uid = *params->create_properties.parent_id_integer;
+        break;
+      case Create::Params::CreateProperties::PARENT_ID_STRING:
+        parent_id.string_uid = *params->create_properties.parent_id_string;
+        break;
+      default:
+        break;
+    }
+    MenuItem* parent = menu_manager->GetItemById(parent_id);
+    if (!parent) {
+      error_ = ExtensionErrorUtils::FormatErrorMessage(
+          kCannotFindItemError, GetIDString(parent_id));
+      return false;
+    }
+    if (parent->type() != MenuItem::NORMAL) {
+      error_ = kParentsMustBeNormalError;
       return false;
+    }
+
     success = menu_manager->AddChildItem(parent->id(), item.release());
   } else {
     success = menu_manager->AddContextItem(GetExtension(), item.release());
@@ -261,9 +203,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;

[not at google - send to devlin, 2012/07/30 15:36:53]

more switch with template stuff

[chebert, 2012/07/30 22:45:19]

The enums are named differently here, so a helper function probably won't help
much.

On 2012/07/30 15:36:53, kalman wrote:

+      break;
+    case Update::Params::ID_INTEGER:
+      item_id.uid = *params->id_integer;
+      break;
+  }
 
   ExtensionService* service = profile()->GetExtensionService();
   MenuManager* manager = service->menu_manager();
@@ -274,14 +224,24 @@ 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;
+  switch (params->update_properties.type) {
+    case Update::Params::UpdateProperties::TYPE_NONE:
+    case Update::Params::UpdateProperties::TYPE_NORMAL:
+      type = MenuItem::NORMAL;
+      break;
+    case Update::Params::UpdateProperties::TYPE_CHECKBOX:
+      type = MenuItem::CHECKBOX;
+      break;
+    case Update::Params::UpdateProperties::TYPE_RADIO:
+      type = MenuItem::RADIO;
+      break;
+    case Update::Params::UpdateProperties::TYPE_SEPARATOR:
+      type = MenuItem::SEPARATOR;
+      break;
+  }
+
   if (type != item->type()) {
     if (type == MenuItem::RADIO || item->type() == MenuItem::RADIO) {
       radioItemUpdated = true;
@@ -290,10 +250,9 @@ bool UpdateContextMenuFunction::RunImpl() {
   }
 
   // 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 +260,99 @@ 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()) {
+    for (size_t i = 0; i < params->update_properties.contexts->size(); ++i) {
+      Update::Params::UpdateProperties::ContextsElement context =
+          params->update_properties.contexts->at(i);
+      if (context == Update::Params::UpdateProperties::CONTEXTS_ELEMENT_ALL) {
+        contexts.Add(MenuItem::ALL);
+      } else if (context ==
+          Update::Params::UpdateProperties::CONTEXTS_ELEMENT_PAGE) {
+        contexts.Add(MenuItem::PAGE);
+      } else if (context ==
+          Update::Params::UpdateProperties::CONTEXTS_ELEMENT_SELECTION) {
+        contexts.Add(MenuItem::SELECTION);
+      } else if (context ==
+          Update::Params::UpdateProperties::CONTEXTS_ELEMENT_LINK) {
+        contexts.Add(MenuItem::LINK);
+      } else if (context ==
+          Update::Params::UpdateProperties::CONTEXTS_ELEMENT_EDITABLE) {
+        contexts.Add(MenuItem::EDITABLE);
+      } else if (context ==
+          Update::Params::UpdateProperties::CONTEXTS_ELEMENT_IMAGE) {
+        contexts.Add(MenuItem::IMAGE);
+      } else if (context ==
+          Update::Params::UpdateProperties::CONTEXTS_ELEMENT_VIDEO) {
+        contexts.Add(MenuItem::VIDEO);
+      } else if (context ==
+          Update::Params::UpdateProperties::CONTEXTS_ELEMENT_AUDIO) {
+        contexts.Add(MenuItem::AUDIO);
+      } else if (context ==
+          Update::Params::UpdateProperties::CONTEXTS_ELEMENT_FRAME) {
+        contexts.Add(MenuItem::FRAME);
+      }
+    }
+    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;
+  if (params->update_properties.parent_id_type !=
+          Update::Params::UpdateProperties::PARENT_ID_NONE) {
+    MenuItem::Id parent_id(profile()->IsOffTheRecord(), extension_id());
+    switch (params->update_properties.parent_id_type) {
+      case Update::Params::UpdateProperties::PARENT_ID_STRING:
+        parent_id.string_uid = *params->update_properties.parent_id_string;
+        break;
+      case Update::Params::UpdateProperties::PARENT_ID_INTEGER:
+        parent_id.uid = *params->update_properties.parent_id_integer;
+        break;
+      default:
+        break;

[not at google - send to devlin, 2012/07/30 15:36:53]

no point having this default branch

[chebert, 2012/07/30 22:45:19]

Done.

+    }
+    parent = manager->GetItemById(parent_id);
+    if (!parent) {
+      error_ = ExtensionErrorUtils::FormatErrorMessage(
+          kCannotFindItemError, GetIDString(parent_id));
+      return false;
+    }
+    if (parent->type() != MenuItem::NORMAL) {
+      error_ = kParentsMustBeNormalError;
+      return false;
+    }
+    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.
@@ -346,9 +365,18 @@ 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());
+
+  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;
+  }
+
   ExtensionService* service = profile()->GetExtensionService();
   MenuManager* manager = service->menu_manager();