Context Menus now uses the JSON Schema Compiler.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/context_menu/context_menu_api.h"
 
 #include <string>
 
 #include "base/values.h"
 #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";

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

see how many of these constants you can delete.

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

Done.

 
 const char kCannotFindItemError[] = "Cannot find menu item with id *";
 const char kOnclickDisallowedError[] = "Extensions using event pages cannot "
     "pass an onclick parameter to chrome.contextMenus.create. Instead, use "
     "the chrome.contextMenus.onClicked event.";
 const char kCheckedError[] =
     "Only items with type \"radio\" or \"checkbox\" can be checked";
 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[] =
     "All menu items except for separators must have a title";
 
 std::string GetIDString(const extensions::MenuItem::Id& id) {
   if (id.uid == 0)
     return id.string_uid;
   else
     return base::IntToString(id.uid);
 }
 
 }  // namespace
 
+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.

+
 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;
+bool CreateContextMenuFunction::RunImpl() {
+  MenuItem::Id id(profile()->IsOffTheRecord(), extension_id());
+  scoped_ptr<Create::Params> params(Create::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
 
-  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;
-}
-
-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));
+  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();
 
   if (menu_manager->GetItemById(id)) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(kDuplicateIDError,
                                                      GetIDString(id));
     return false;
   }
 
   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());
   }
 
   if (!success)
     return false;
 
   menu_manager->WriteToStorage(GetExtension());
   return true;
 }
 
 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();
   MenuItem* item = manager->GetItemById(item_id);
   if (!item || item->extension_id() != extension_id()) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(
         kCannotFindItemError, GetIDString(item_id));
     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;
     }
-    item->set_type(type);
+    if (type != item->type()) {
+      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;
     }
     item->set_title(title);
   }
 
   // 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))
+  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(
+          document_url_patterns.get(), target_url_patterns.get(), &error_)) {
     return false;
-  if (parent && !manager->ChangeParent(item->id(), &parent->id()))
-    return false;
-
-  if (!item->PopulateURLPatterns(
-          *properties, kDocumentUrlPatternsKey, kTargetUrlPatternsKey, &error_))
-    return false;
+  }
 
   // There is no need to call ItemUpdated if ChangeParent is called because
   // all sanitation is taken care of in ChangeParent.
   if (!parent && radioItemUpdated && !manager->ItemUpdated(item->id()))
     return false;
 
   manager->WriteToStorage(GetExtension());
   return true;
 }
 
 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();
 
   MenuItem* item = manager->GetItemById(id);
   // Ensure one extension can't remove another's menu items.
   if (!item || item->extension_id() != extension_id()) {
     error_ = ExtensionErrorUtils::FormatErrorMessage(
         kCannotFindItemError, GetIDString(id));
     return false;
   }
 
   if (!manager->RemoveContextMenuItem(id))
     return false;
   manager->WriteToStorage(GetExtension());
   return true;
 }
 
 bool RemoveAllContextMenusFunction::RunImpl() {
   ExtensionService* service = profile()->GetExtensionService();
   MenuManager* manager = service->menu_manager();
   manager->RemoveAllContextItems(GetExtension()->id());
   manager->WriteToStorage(GetExtension());
   return true;
 }
 
 }  // namespace extensions