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..0c4cf9c105dd5b0c2490e8967781fb9da9a3a0be 100644
--- a/chrome/browser/extensions/api/context_menu/context_menu_api.cc
+++ b/chrome/browser/extensions/api/context_menu/context_menu_api.cc
@@ -10,8 +10,11 @@
 #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 {
 
@@ -53,151 +56,35 @@ std::string GetIDString(const extensions::MenuItem::Id& id) {
 
 }  // namespace
 
-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;
+namespace Create = extensions::api::context_menus::Create;
+namespace Remove = extensions::api::context_menus::Remove;
+namespace Update = extensions::api::context_menus::Update;

[not at google - send to devlin, 2012/07/26 04:16:20]

move these inside the extensions namespace, then you don't need the extensions::
out the front of every one.

[chebert, 2012/07/27 01:28:24]

Done.

 
-  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 extensions {
 
 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;
+    EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &properties));
+    EXTENSION_FUNCTION_VALIDATE(properties != NULL);

[not at google - send to devlin, 2012/07/26 04:16:20]

this will never be NULL because properties wasn't initialized to NULL. Check is
unnecessary anyway (and properties should be initialized to NULL).

It's a pity to do all the Json schema compiler stuff but still have to validate
the dictionary. Ah well.

You *could* add an optional generated_id to context_menus.json but put "nodoc"
on it. I kinda like that...

[chebert, 2012/07/27 01:28:24]

This gets generated in the custom_bindings which adds the id to the dictionary. 
would that work to just put a generated_id into the contexts? 

On 2012/07/26 04:16:20, kalman wrote:

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

Yeah that's what I meant, but I thought of something much better. I'll write it
in a comment in the new version of the change.

     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,44 +95,128 @@ 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;
+  if (params->create_properties.contexts.get()) {
+    for (size_t i = 0; i < params->create_properties.contexts->size(); ++i) {
+      Create::Params::CreateProperties::ContextsElement context =
+          params->create_properties.contexts->at(i);

[not at google - send to devlin, 2012/07/26 04:16:20]

switch plz.

[chebert, 2012/07/27 01:28:24]

Done.

+      if (context == Create::Params::CreateProperties::CONTEXTS_ELEMENT_ALL) {
+        contexts.Add(MenuItem::ALL);
+      } else if (context ==
+          Create::Params::CreateProperties::CONTEXTS_ELEMENT_PAGE) {
+        contexts.Add(MenuItem::PAGE);
+      } else if (context ==
+          Create::Params::CreateProperties::CONTEXTS_ELEMENT_SELECTION) {
+        contexts.Add(MenuItem::SELECTION);
+      } else if (context ==
+          Create::Params::CreateProperties::CONTEXTS_ELEMENT_LINK) {
+        contexts.Add(MenuItem::LINK);
+      } else if (context ==
+          Create::Params::CreateProperties::CONTEXTS_ELEMENT_EDITABLE) {
+        contexts.Add(MenuItem::EDITABLE);
+      } else if (context ==
+          Create::Params::CreateProperties::CONTEXTS_ELEMENT_IMAGE) {
+        contexts.Add(MenuItem::IMAGE);
+      } else if (context ==
+          Create::Params::CreateProperties::CONTEXTS_ELEMENT_VIDEO) {
+        contexts.Add(MenuItem::VIDEO);
+      } else if (context ==
+          Create::Params::CreateProperties::CONTEXTS_ELEMENT_AUDIO) {
+        contexts.Add(MenuItem::AUDIO);
+      } else if (context ==
+          Create::Params::CreateProperties::CONTEXTS_ELEMENT_FRAME) {
+        contexts.Add(MenuItem::FRAME);
+      } else {
+        return false;
+      }
+    }
+  }
 
   MenuItem::Type type;
-  if (!ParseType(*properties, MenuItem::NORMAL, &type))
-    return false;
+  if (params->create_properties.type ==
+                  Create::Params::CreateProperties::TYPE_NONE ||
+      params->create_properties.type ==
+                    Create::Params::CreateProperties::TYPE_NORMAL) {
+    type = MenuItem::NORMAL;
+  }  else if (params->create_properties.type ==
+                 Create::Params::CreateProperties::TYPE_CHECKBOX) {
+    type = MenuItem::CHECKBOX;
+  }  else if (params->create_properties.type ==
+                 Create::Params::CreateProperties::TYPE_RADIO) {
+    type = MenuItem::RADIO;
+  } else if (params->create_properties.type ==
+                 Create::Params::CreateProperties::TYPE_SEPARATOR) {
+    type = MenuItem::SEPARATOR;
+  }

[not at google - send to devlin, 2012/07/26 04:16:20]

switch here too

[chebert, 2012/07/27 01:28:24]

Done.

 
   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));
 
+  scoped_ptr<ListValue> document_url_patterns;

[not at google - send to devlin, 2012/07/26 04:16:20]

This is just converting back to the typeless data for MenuItem to parse it
again. Kinda defeats the purpose of JSON Schema Compiler.

Changing MenuItem to take the vectors directly rather than ListValues will clean
this up a lot.

[chebert, 2012/07/27 01:28:24]

Done.

+  scoped_ptr<ListValue> target_url_patterns;
+  if (params->create_properties.document_url_patterns.get()) {
+    document_url_patterns.reset(new ListValue());
+    for (size_t i = 0;
+         i < params->create_properties.document_url_patterns->size();
+         ++i) {
+      document_url_patterns->Append(Value::CreateStringValue(
+          params->create_properties.document_url_patterns->at(i)));
+    }
+  }
+  if (params->create_properties.target_url_patterns.get()) {
+    target_url_patterns.reset(new ListValue());
+    for (size_t i = 0;
+         i < params->create_properties.target_url_patterns->size();
+         ++i) {
+      document_url_patterns->Append(Value::CreateStringValue(
+          params->create_properties.target_url_patterns->at(i)));
+    }
+  }
   if (!item->PopulateURLPatterns(
-          *properties, kDocumentUrlPatternsKey, kTargetUrlPatternsKey, &error_))
+          document_url_patterns.get(), target_url_patterns.get(), &error_)) {
     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 !=

[not at google - send to devlin, 2012/07/26 04:16:20]

can this be switched over too?

This looks very boilerplate-y.

[chebert, 2012/07/27 01:28:24]

Switchy...

On 2012/07/26 04:16:20, kalman wrote:

+          Create::Params::CreateProperties::PARENT_ID_NONE) {
+    MenuItem::Id parent_id(profile()->IsOffTheRecord(), extension_id());
+    if (params->create_properties.parent_id_type ==
+          Create::Params::CreateProperties::PARENT_ID_INTEGER) {
+      parent_id.uid = *params->create_properties.parent_id_integer;
+    } else if (params->create_properties.parent_id_type ==
+                   Create::Params::CreateProperties::PARENT_ID_STRING) {
+      parent_id.string_uid = *params->create_properties.parent_id_string;
+    }
+    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 +232,13 @@ 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());
+  if (params->id_type == Update::Params::ID_STRING)
+    item_id.string_uid = *params->id_string;
+  else if (params->id_type == Update::Params::ID_INTEGER)
+    item_id.uid = *params->id_integer;

[not at google - send to devlin, 2012/07/26 04:16:20]

switch

[chebert, 2012/07/27 01:28:24]

Done.

 
   ExtensionService* service = profile()->GetExtensionService();
   MenuManager* manager = service->menu_manager();
@@ -274,26 +249,35 @@ 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;
-  if (type != item->type()) {
-    if (type == MenuItem::RADIO || item->type() == MenuItem::RADIO) {
-      radioItemUpdated = true;
+  if (params->update_properties.type !=
+                  Update::Params::UpdateProperties::TYPE_NONE) {

[not at google - send to devlin, 2012/07/26 04:16:20]

switch

[chebert, 2012/07/27 01:28:24]

Done.

+    MenuItem::Type type;
+    if (params->update_properties.type ==
+                    Update::Params::UpdateProperties::TYPE_NORMAL) {
+      type = MenuItem::NORMAL;
+    }  else if (params->update_properties.type ==
+                   Update::Params::UpdateProperties::TYPE_CHECKBOX) {
+      type = MenuItem::CHECKBOX;
+    }  else if (params->update_properties.type ==
+                   Update::Params::UpdateProperties::TYPE_RADIO) {
+      type = MenuItem::RADIO;
+    } else if (params->update_properties.type ==
+                   Update::Params::UpdateProperties::TYPE_SEPARATOR) {
+      type = MenuItem::SEPARATOR;
+    }
+    if (type != item->type()) {
+      if (type == MenuItem::RADIO || item->type() == MenuItem::RADIO) {
+        radioItemUpdated = true;
+      }
+      item->set_type(type);
     }
-    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 +285,116 @@ 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;
+    checked = *params->update_properties.checked;

[not at google - send to devlin, 2012/07/26 04:16:20]

combine with line above?

[chebert, 2012/07/27 01:28:24]

Done.

+    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 =

[not at google - send to devlin, 2012/07/26 04:16:20]

switch.

Didn't we talk about pulling the enum into a type, then being able to share all
the enums? We could pull all this switch stuff into a helper method then.

[chebert, 2012/07/27 01:28:24]

I want to do that, but I looked into that, and I think it is more than I have
time for right now.

In order to do that I would need to change:
the json schema compiler
all the .json api files
all the functions that currently use enums

On 2012/07/26 04:16:20, kalman wrote:

+          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 !=

[not at google - send to devlin, 2012/07/26 04:16:20]

switch or something

[chebert, 2012/07/27 01:28:24]

Done.

+          Update::Params::UpdateProperties::PARENT_ID_NONE) {
+    MenuItem::Id parent_id(profile()->IsOffTheRecord(), extension_id());
+    if (params->update_properties.parent_id_type ==
+            Update::Params::UpdateProperties::PARENT_ID_STRING) {
+      parent_id.string_uid = *params->update_properties.parent_id_string;
+    } else if (params->update_properties.parent_id_type ==
+                 Update::Params::UpdateProperties::PARENT_ID_INTEGER) {
+      parent_id.uid = *params->update_properties.parent_id_integer;
+    }
+    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.
+  scoped_ptr<ListValue> document_url_patterns;
+  scoped_ptr<ListValue> target_url_patterns;
+  if (params->update_properties.document_url_patterns.get()) {

[not at google - send to devlin, 2012/07/26 04:16:20]

same deal with passing through the vectors

[chebert, 2012/07/27 01:28:24]

Done.

+    document_url_patterns.reset(new ListValue());
+    for (size_t i = 0;
+         i < params->update_properties.document_url_patterns->size();
+         ++i) {
+      document_url_patterns->Append(Value::CreateStringValue(
+          params->update_properties.document_url_patterns->at(i)));
+    }
+  }
+  if (params->update_properties.target_url_patterns.get()) {
+    target_url_patterns.reset(new ListValue());
+    for (size_t i = 0;
+         i < params->update_properties.target_url_patterns->size();
+         ++i) {
+      document_url_patterns->Append(Value::CreateStringValue(
+          params->update_properties.target_url_patterns->at(i)));
+    }
+  }
   if (!item->PopulateURLPatterns(
-          *properties, kDocumentUrlPatternsKey, kTargetUrlPatternsKey, &error_))
+          document_url_patterns.get(), 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 +407,15 @@ 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());
+
+  if (params->menu_item_id_type == Remove::Params::MENU_ITEM_ID_STRING)
+    id.string_uid = *params->menu_item_id_string;
+  else if (params->menu_item_id_type == Remove::Params::MENU_ITEM_ID_INTEGER)

[not at google - send to devlin, 2012/07/26 04:16:20]

switch

[chebert, 2012/07/27 01:28:24]

Done.

+    id.uid = *params->menu_item_id_integer;
+
   ExtensionService* service = profile()->GetExtensionService();
   MenuManager* manager = service->menu_manager();