More cleanup of extensions context menu API.

This changes the contextMenus.create method to synchronously return an id
instead of aysynchronously in the callback. This meant moving the generating of
id's from a globally unique integer in C++ code to an extension-unique integer
in javascript. The C++ unique id thus becomes a pair of <extension_id,int>.

Also a couple of small drive-by cleanups while I was in some of the files.


BUG=48198
TEST=The context menu API should work normally, given the changes described 
above.


Review URL: http://codereview.chromium.org/2911007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@52384 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 283 insertions, 250 deletions

diff --git a/chrome/browser/extensions/extension_context_menu_api.cc b/chrome/browser/extensions/extension_context_menu_api.cc
index f5ecca4..ea65e0e 100644
--- a/chrome/browser/extensions/extension_context_menu_api.cc
+++ b/chrome/browser/extensions/extension_context_menu_api.cc
@@ -4,22 +4,30 @@
 
 #include "chrome/browser/extensions/extension_context_menu_api.h"
 
+#include <string>
+
 #include "base/values.h"
 #include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/browser/profile.h"
+#include "chrome/common/extensions/extension_error_utils.h"
 
 const wchar_t kCheckedKey[] = L"checked";
 const wchar_t kContextsKey[] = L"contexts";
+const wchar_t kGeneratedIdKey[] = L"generatedId";
 const wchar_t kParentIdKey[] = L"parentId";
 const wchar_t kTitleKey[] = L"title";
 const wchar_t kTypeKey[] = L"type";
 
-const char kTitleNeededError[] =
-    "All menu items except for separators must have a title";
+const char kCannotFindItemError[] = "Cannot find menu item with id *";
 const char kCheckedError[] =
     "Only items with type \"radio\" or \"checkbox\" can be checked";
+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";
+
 
 bool ExtensionContextMenuFunction::ParseContexts(
     const DictionaryValue& properties,
@@ -53,7 +61,8 @@ bool ExtensionContextMenuFunction::ParseContexts(
     } else if (value == "audio") {
       tmp_result.Add(ExtensionMenuItem::AUDIO);
     } else {
-      error_ = "Invalid value for " + WideToASCII(key);
+      error_ = ExtensionErrorUtils::FormatErrorMessage(kInvalidValueError,
+                                                       WideToASCII(key));
       return false;
     }
   }
@@ -84,7 +93,8 @@ bool ExtensionContextMenuFunction::ParseType(
   } else if (type_string == "separator") {
     *result = ExtensionMenuItem::SEPARATOR;
   } else {
-    error_ = "Invalid type string '" + type_string + "'";
+    error_ = ExtensionErrorUtils::FormatErrorMessage(kInvalidTypeStringError,
+                                                     type_string);
     return false;
   }
   return true;
@@ -103,7 +113,7 @@ bool ExtensionContextMenuFunction::ParseChecked(
     return false;
   if (checked && type != ExtensionMenuItem::CHECKBOX &&
       type != ExtensionMenuItem::RADIO) {
-    error_ = "Only checkbox and radio type items can be checked";
+    error_ = kCheckedError;
     return false;
   }
   return true;
@@ -115,14 +125,14 @@ bool ExtensionContextMenuFunction::GetParent(
     ExtensionMenuItem** result) {
   if (!properties.HasKey(kParentIdKey))
     return true;
-  int parent_id = 0;
+  ExtensionMenuItem::Id parent_id(extension_id(), 0);
   if (properties.HasKey(kParentIdKey) &&
-      !properties.GetInteger(kParentIdKey, &parent_id))
+      !properties.GetInteger(kParentIdKey, &parent_id.second))
     return false;
 
   ExtensionMenuItem* parent = manager.GetItemById(parent_id);
   if (!parent) {
-    error_ = "Cannot find menu item with id " + IntToString(parent_id);
+    error_ = "Cannot find menu item with id " + IntToString(parent_id.second);
     return false;
   }
   if (parent->type() != ExtensionMenuItem::NORMAL) {
@@ -138,6 +148,9 @@ bool CreateContextMenuFunction::RunImpl() {
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &properties));
   EXTENSION_FUNCTION_VALIDATE(properties != NULL);
 
+  ExtensionMenuItem::Id id(extension_id(), 0);
+  EXTENSION_FUNCTION_VALIDATE(properties->GetInteger(kGeneratedIdKey,
+                                                     &id.second));
   std::string title;
   if (properties->HasKey(kTitleKey) &&
       !properties->GetString(kTitleKey, &title))
@@ -164,45 +177,44 @@ bool CreateContextMenuFunction::RunImpl() {
     return false;
 
   scoped_ptr<ExtensionMenuItem> item(
-      new ExtensionMenuItem(extension_id(), title, checked, type, contexts));
+      new ExtensionMenuItem(id, title, checked, type, contexts));
 
-  int id = 0;
+  bool success = true;
   if (properties->HasKey(kParentIdKey)) {
-    int parent_id = 0;
-    if (!properties->GetInteger(kParentIdKey, &parent_id))
-      return false;
+    ExtensionMenuItem::Id parent_id(extension_id(), 0);
+    EXTENSION_FUNCTION_VALIDATE(properties->GetInteger(kParentIdKey,
+                                                       &parent_id.second));
     ExtensionMenuItem* parent = menu_manager->GetItemById(parent_id);
     if (!parent) {
-      error_ = "Cannot find menu item with id " + IntToString(parent_id);
+      error_ = ExtensionErrorUtils::FormatErrorMessage(
+          kCannotFindItemError, IntToString(parent_id.second));
       return false;
     }
     if (parent->type() != ExtensionMenuItem::NORMAL) {
       error_ = kParentsMustBeNormalError;
       return false;
     }
-    id = menu_manager->AddChildItem(parent_id, item.release());
+    success = menu_manager->AddChildItem(parent_id, item.release());
   } else {
-    id = menu_manager->AddContextItem(GetExtension(), item.release());
+    success = menu_manager->AddContextItem(GetExtension(), item.release());
   }
 
-  if (id <= 0)
+  if (!success)
     return false;
 
-  if (has_callback())
-    result_.reset(Value::CreateIntegerValue(id));
-
   return true;
 }
 
 bool UpdateContextMenuFunction::RunImpl() {
-  int item_id = 0;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &item_id));
+  ExtensionMenuItem::Id item_id(extension_id(), 0);
+  EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &item_id.second));
 
   ExtensionsService* service = profile()->GetExtensionsService();
   ExtensionMenuManager* manager = service->menu_manager();
   ExtensionMenuItem* item = manager->GetItemById(item_id);
   if (!item || item->extension_id() != extension_id()) {
-    error_ = "Invalid menu item id: " + IntToString(item_id);
+    error_ = ExtensionErrorUtils::FormatErrorMessage(
+        kCannotFindItemError, IntToString(item_id.second));
     return false;
   }
 
@@ -251,22 +263,23 @@ bool UpdateContextMenuFunction::RunImpl() {
   ExtensionMenuItem* parent = NULL;
   if (!GetParent(*properties, *menu_manager, &parent))
     return false;
-  if (parent && !menu_manager->ChangeParent(item->id(), parent->id()))
+  if (parent && !menu_manager->ChangeParent(item->id(), &parent->id()))
     return false;
 
   return true;
 }
 
 bool RemoveContextMenuFunction::RunImpl() {
-  int id = 0;
-  EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &id));
+  ExtensionMenuItem::Id id(extension_id(), 0);
+  EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &id.second));
   ExtensionsService* service = profile()->GetExtensionsService();
   ExtensionMenuManager* manager = service->menu_manager();
 
   ExtensionMenuItem* item = manager->GetItemById(id);
   // Ensure one extension can't remove another's menu items.
   if (!item || item->extension_id() != extension_id()) {
-    error_ = StringPrintf("no menu item with id %d is registered", id);
+    error_ = ExtensionErrorUtils::FormatErrorMessage(
+        kCannotFindItemError, IntToString(id.second));
     return false;
   }
 
diff --git a/chrome/browser/extensions/extension_menu_manager.cc b/chrome/browser/extensions/extension_menu_manager.cc
index fc02637..1bc55c7 100644
--- a/chrome/browser/extensions/extension_menu_manager.cc
+++ b/chrome/browser/extensions/extension_menu_manager.cc
@@ -8,6 +8,7 @@
 
 #include "app/resource_bundle.h"
 #include "base/logging.h"
+#include "base/stl_util-inl.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
@@ -24,13 +25,13 @@
 #include "skia/ext/image_operations.h"
 #include "webkit/glue/context_menu.h"
 
-ExtensionMenuItem::ExtensionMenuItem(const std::string& extension_id,
+ExtensionMenuItem::ExtensionMenuItem(const Id& id,
                                      std::string title,
-                                     bool checked, Type type,
+                                     bool checked,
+                                     Type type,
                                      const ContextList& contexts)
-    : extension_id_(extension_id),
+    : id_(id),
       title_(title),
-      id_(0),
       type_(type),
       checked_(checked),
       contexts_(contexts),
@@ -40,7 +41,7 @@ ExtensionMenuItem::~ExtensionMenuItem() {
   STLDeleteElements(&children_);
 }
 
-bool ExtensionMenuItem::RemoveChild(int child_id) {
+bool ExtensionMenuItem::RemoveChild(const Id& child_id) {
   ExtensionMenuItem* child = ReleaseChild(child_id, true);
   if (child) {
     delete child;
@@ -50,7 +51,7 @@ bool ExtensionMenuItem::RemoveChild(int child_id) {
   }
 }
 
-ExtensionMenuItem* ExtensionMenuItem::ReleaseChild(int child_id,
+ExtensionMenuItem* ExtensionMenuItem::ReleaseChild(const Id& child_id,
                                                    bool recursive) {
   for (List::iterator i = children_.begin(); i != children_.end(); ++i) {
     ExtensionMenuItem* child = NULL;
@@ -67,12 +68,12 @@ ExtensionMenuItem* ExtensionMenuItem::ReleaseChild(int child_id,
   return NULL;
 }
 
-std::set<int> ExtensionMenuItem::RemoveAllDescendants() {
-  std::set<int> result;
+std::set<ExtensionMenuItem::Id> ExtensionMenuItem::RemoveAllDescendants() {
+  std::set<Id> result;
   for (List::iterator i = children_.begin(); i != children_.end(); ++i) {
     ExtensionMenuItem* child = *i;
     result.insert(child->id());
-    std::set<int> removed = child->RemoveAllDescendants();
+    std::set<Id> removed = child->RemoveAllDescendants();
     result.insert(removed.begin(), removed.end());
   }
   STLDeleteElements(&children_);
@@ -96,13 +97,12 @@ bool ExtensionMenuItem::SetChecked(bool checked) {
 }
 
 void ExtensionMenuItem::AddChild(ExtensionMenuItem* item) {
-  item->parent_id_ = id_;
+  item->parent_id_.reset(new Id(id_));
   children_.push_back(item);
 }
 
 ExtensionMenuManager::ExtensionMenuManager()
-    : next_item_id_(1),
-      ALLOW_THIS_IN_INITIALIZER_LIST(image_tracker_(this)) {
+    : ALLOW_THIS_IN_INITIALIZER_LIST(image_tracker_(this)) {
   registrar_.Add(this, NotificationType::EXTENSION_UNLOADED,
                  NotificationService::AllSources());
 }
@@ -132,18 +132,16 @@ const ExtensionMenuItem::List* ExtensionMenuManager::MenuItems(
   return NULL;
 }
 
-int ExtensionMenuManager::AddContextItem(Extension* extension,
-                                         ExtensionMenuItem* item) {
+bool ExtensionMenuManager::AddContextItem(Extension* extension,
+                                          ExtensionMenuItem* item) {
   const std::string& extension_id = item->extension_id();
-  // The item must have a non-empty extension id.
-  if (extension_id.empty())
-    return 0;
+  // The item must have a non-empty extension id, and not have already been
+  // added.
+  if (extension_id.empty() || ContainsKey(items_by_id_, item->id()))
+    return false;
 
   DCHECK_EQ(extension->id(), extension_id);
 
-  DCHECK_EQ(0, item->id());
-  item->set_id(next_item_id_++);
-
   bool first_item = !ContainsKey(context_items_, extension_id);
   context_items_[extension_id].push_back(item);
   items_by_id_[item->id()] = item;
@@ -164,32 +162,31 @@ int ExtensionMenuManager::AddContextItem(Extension* extension,
     }
   }
 
-  return item->id();
+  return true;
 }
 
-int ExtensionMenuManager::AddChildItem(int parent_id,
-                                       ExtensionMenuItem* child) {
+bool ExtensionMenuManager::AddChildItem(const ExtensionMenuItem::Id& parent_id,
+                                        ExtensionMenuItem* child) {
   ExtensionMenuItem* parent = GetItemById(parent_id);
   if (!parent || parent->type() != ExtensionMenuItem::NORMAL ||
-      parent->extension_id() != child->extension_id())
-    return 0;
-  child->set_id(next_item_id_++);
+      parent->extension_id() != child->extension_id() ||
+      ContainsKey(items_by_id_, child->id()))
+    return false;
   parent->AddChild(child);
   items_by_id_[child->id()] = child;
-  return child->id();
+  return true;
 }
 
-bool ExtensionMenuManager::DescendantOf(ExtensionMenuItem* item,
-                                        int ancestor_id) {
-  DCHECK_GT(ancestor_id, 0);
-
-  // Work our way up the tree until we find the ancestor or 0.
-  int id = item->parent_id();
-  while (id > 0) {
-    DCHECK(id != item->id());  // Catch circular graphs.
-    if (id == ancestor_id)
+bool ExtensionMenuManager::DescendantOf(
+    ExtensionMenuItem* item,
+    const ExtensionMenuItem::Id& ancestor_id) {
+  // Work our way up the tree until we find the ancestor or NULL.
+  ExtensionMenuItem::Id* id = item->parent_id();
+  while (id != NULL) {
+    DCHECK(*id != item->id());  // Catch circular graphs.
+    if (*id == ancestor_id)
       return true;
-    ExtensionMenuItem* next = GetItemById(id);
+    ExtensionMenuItem* next = GetItemById(*id);
     if (!next) {
       NOTREACHED();
       return false;
@@ -199,17 +196,20 @@ bool ExtensionMenuManager::DescendantOf(ExtensionMenuItem* item,
   return false;
 }
 
-bool ExtensionMenuManager::ChangeParent(int child_id, int parent_id) {
+bool ExtensionMenuManager::ChangeParent(
+    const ExtensionMenuItem::Id& child_id,
+    const ExtensionMenuItem::Id* parent_id) {
   ExtensionMenuItem* child = GetItemById(child_id);
-  ExtensionMenuItem* new_parent = GetItemById(parent_id);
-  if (child_id == parent_id || !child || (!new_parent && parent_id > 0) ||
+  ExtensionMenuItem* new_parent = parent_id ? GetItemById(*parent_id) : NULL;
+  if ((parent_id && (child_id == *parent_id)) || !child ||
+      (!new_parent && parent_id != NULL) ||
       (new_parent && (DescendantOf(new_parent, child_id) ||
                       child->extension_id() != new_parent->extension_id())))
     return false;
 
-  int old_parent_id = child->parent_id();
-  if (old_parent_id > 0) {
-    ExtensionMenuItem* old_parent = GetItemById(old_parent_id);
+  ExtensionMenuItem::Id* old_parent_id = child->parent_id();
+  if (old_parent_id != NULL) {
+    ExtensionMenuItem* old_parent = GetItemById(*old_parent_id);
     if (!old_parent) {
       NOTREACHED();
       return false;
@@ -220,7 +220,6 @@ bool ExtensionMenuManager::ChangeParent(int child_id, int parent_id) {
   } else {
     // This is a top-level item, so we need to pull it out of our list of
     // top-level items.
-    DCHECK_EQ(0, old_parent_id);
     MenuItemMap::iterator i = context_items_.find(child->extension_id());
     if (i == context_items_.end()) {
       NOTREACHED();
@@ -240,12 +239,13 @@ bool ExtensionMenuManager::ChangeParent(int child_id, int parent_id) {
     new_parent->AddChild(child);
   } else {
     context_items_[child->extension_id()].push_back(child);
-    child->parent_id_ = 0;
+    child->parent_id_.reset(NULL);
   }
   return true;
 }
 
-bool ExtensionMenuManager::RemoveContextMenuItem(int id) {
+bool ExtensionMenuManager::RemoveContextMenuItem(
+    const ExtensionMenuItem::Id& id) {
   if (!ContainsKey(items_by_id_, id))
     return false;
 
@@ -289,9 +289,9 @@ void ExtensionMenuManager::RemoveAllContextItems(std::string extension_id) {
     items_by_id_.erase(item->id());
 
     // Remove descendants from this item and erase them from the lookup cache.
-    std::set<int> removed_ids = item->RemoveAllDescendants();
-    for (std::set<int>::const_iterator j = removed_ids.begin();
-        j != removed_ids.end(); ++j) {
+    std::set<ExtensionMenuItem::Id> removed_ids = item->RemoveAllDescendants();
+    std::set<ExtensionMenuItem::Id>::const_iterator j;
+    for (j = removed_ids.begin(); j != removed_ids.end(); ++j) {
       items_by_id_.erase(*j);
     }
   }
@@ -302,8 +302,10 @@ void ExtensionMenuManager::RemoveAllContextItems(std::string extension_id) {
     extension_icons_.erase(extension_id);
 }
 
-ExtensionMenuItem* ExtensionMenuManager::GetItemById(int id) const {
-  std::map<int, ExtensionMenuItem*>::const_iterator i = items_by_id_.find(id);
+ExtensionMenuItem* ExtensionMenuManager::GetItemById(
+    const ExtensionMenuItem::Id& id) const {
+  std::map<ExtensionMenuItem::Id, ExtensionMenuItem*>::const_iterator i =
+      items_by_id_.find(id);
   if (i != items_by_id_.end())
     return i->second;
   else
@@ -315,7 +317,7 @@ void ExtensionMenuManager::RadioItemSelected(ExtensionMenuItem* item) {
   // parent. Otherwise get a handle to the top-level list.
   const ExtensionMenuItem::List* list = NULL;
   if (item->parent_id()) {
-    ExtensionMenuItem* parent = GetItemById(item->parent_id());
+    ExtensionMenuItem* parent = GetItemById(*item->parent_id());
     if (!parent) {
       NOTREACHED();
       return;
@@ -367,10 +369,11 @@ static void AddURLProperty(DictionaryValue* dictionary,
     dictionary->SetString(key, url.possibly_invalid_spec());
 }
 
-void ExtensionMenuManager::ExecuteCommand(Profile* profile,
-                                          TabContents* tab_contents,
-                                          const ContextMenuParams& params,
-                                          int menuItemId) {
+void ExtensionMenuManager::ExecuteCommand(
+    Profile* profile,
+    TabContents* tab_contents,
+    const ContextMenuParams& params,
+    const ExtensionMenuItem::Id& menuItemId) {
   ExtensionMessageService* service = profile->GetExtensionMessageService();
   if (!service)
     return;
@@ -385,9 +388,9 @@ void ExtensionMenuManager::ExecuteCommand(Profile* profile,
   ListValue args;
 
   DictionaryValue* properties = new DictionaryValue();
-  properties->SetInteger(L"menuItemId", item->id());
+  properties->SetInteger(L"menuItemId", item->id().second);
   if (item->parent_id())
-    properties->SetInteger(L"parentMenuItemId", item->parent_id());
+    properties->SetInteger(L"parentMenuItemId", item->parent_id()->second);
 
   switch (params.media_type) {
     case WebKit::WebContextMenuData::MediaTypeImage:
diff --git a/chrome/browser/extensions/extension_menu_manager.h b/chrome/browser/extensions/extension_menu_manager.h
index 98752f8..9e407d0 100644
--- a/chrome/browser/extensions/extension_menu_manager.h
+++ b/chrome/browser/extensions/extension_menu_manager.h
@@ -11,8 +11,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
-#include "base/linked_ptr.h"
-#include "base/stl_util-inl.h"
+#include "base/scoped_ptr.h"
 #include "base/string16.h"
 #include "chrome/browser/extensions/image_loading_tracker.h"
 #include "chrome/common/notification_observer.h"
@@ -83,16 +82,20 @@ class ExtensionMenuItem {
     uint32 value_;  // A bitmask of Context values.
   };
 
-  ExtensionMenuItem(const std::string& extension_id, std::string title,
-                    bool checked, Type type, const ContextList& contexts);
+  // An Id is a pair of |extension id|, |int| where the |int| is unique per
+  // extension.
+  typedef std::pair<std::string, int> Id;
+
+  ExtensionMenuItem(const Id& id, std::string title, bool checked, Type type,
+                    const ContextList& contexts);
   virtual ~ExtensionMenuItem();
 
   // Simple accessor methods.
-  const std::string& extension_id() const { return extension_id_; }
+  const std::string& extension_id() const { return id_.first; }
   const std::string& title() const { return title_; }
   const List& children() { return children_; }
-  int id() const { return id_; }
-  int parent_id() const { return parent_id_; }
+  const Id& id() const { return id_; }
+  Id* parent_id() const { return parent_id_.get(); }
   int child_count() const { return children_.size(); }
   ContextList contexts() const { return contexts_; }
   Type type() const { return type_; }
@@ -112,37 +115,28 @@ class ExtensionMenuItem {
  protected:
   friend class ExtensionMenuManager;
 
-  // This is protected because the ExtensionMenuManager is in charge of
-  // assigning unique ids.
-  virtual void set_id(int id) {
-    id_ = id;
-  }
-
   // Takes ownership of |item| and sets its parent_id_.
   void AddChild(ExtensionMenuItem* item);
 
   // Removes child menu item with the given id, returning true if the item was
   // found and removed, or false otherwise.
-  bool RemoveChild(int child_id);
+  bool RemoveChild(const Id& child_id);
 
   // Takes the child item from this parent. The item is returned and the caller
   // then owns the pointer.
-  ExtensionMenuItem* ReleaseChild(int child_id, bool recursive);
+  ExtensionMenuItem* ReleaseChild(const Id& child_id, bool recursive);
 
   // Recursively removes all descendant items (children, grandchildren, etc.),
   // returning the ids of the removed items.
-  std::set<int> RemoveAllDescendants();
+  std::set<Id> RemoveAllDescendants();
 
  private:
-  // The extension that added this item.
-  std::string extension_id_;
+  // The unique id for this item.
+  Id id_;
 
   // What gets shown in the menu for this item.
   std::string title_;
 
-  // A unique id for this item. The value 0 means "not yet assigned".
-  int id_;
-
   Type type_;
 
   // This should only be true for items of type CHECKBOX or RADIO.
@@ -152,8 +146,8 @@ class ExtensionMenuItem {
   ContextList contexts_;
 
   // If this item is a child of another item, the unique id of its parent. If
-  // this is a top-level item with no parent, this will be 0.
-  int parent_id_;
+  // this is a top-level item with no parent, this will be NULL.
+  scoped_ptr<Id> parent_id_;
 
   // Any children this item may have.
   List children_;
@@ -181,36 +175,37 @@ class ExtensionMenuManager
 
   // Adds a top-level menu item for an extension, requiring the |extension|
   // pointer so it can load the icon for the extension. Takes ownership of
-  // |item|. Returns the id assigned to the item. Has the side-effect of
-  // incrementing the next_item_id_ member.
-  int AddContextItem(Extension* extension, ExtensionMenuItem* item);
+  // |item|. Returns a boolean indicating success or failure.
+  bool AddContextItem(Extension* extension, ExtensionMenuItem* item);
 
   // Add an item as a child of another item which has been previously added, and
-  // takes ownership of |item|. Returns the id assigned to the item, or 0 on
-  // error.  Has the side-effect of incrementing the next_item_id_ member.
-  int AddChildItem(int parent_id, ExtensionMenuItem* child);
+  // takes ownership of |item|. Returns a boolean indicating success or failure.
+  bool AddChildItem(const ExtensionMenuItem::Id& parent_id,
+                    ExtensionMenuItem* child);
 
   // Makes existing item with |child_id| a child of the item with |parent_id|.
   // If the child item was already a child of another parent, this will remove
   // it from that parent first. It is an error to try and move an item to be a
-  // child of one of its own descendants.
-  bool ChangeParent(int child_id, int parent_id);
+  // child of one of its own descendants. It is legal to pass NULL for
+  // |parent_id|, which means the item should be moved to the top-level.
+  bool ChangeParent(const ExtensionMenuItem::Id& child_id,
+                    const ExtensionMenuItem::Id* parent_id);
 
   // Removes a context menu item with the given id (whether it is a top-level
   // item or a child of some other item), returning true if the item was found
   // and removed or false otherwise.
-  bool RemoveContextMenuItem(int id);
+  bool RemoveContextMenuItem(const ExtensionMenuItem::Id& id);
 
   // Removes all items for the given extension id.
   void RemoveAllContextItems(std::string extension_id);
 
   // Returns the item with the given |id| or NULL.
-  ExtensionMenuItem* GetItemById(int id) const;
+  ExtensionMenuItem* GetItemById(const ExtensionMenuItem::Id& id) const;
 
   // Called when a menu item is clicked on by the user.
   void ExecuteCommand(Profile* profile, TabContents* tab_contents,
                       const ContextMenuParams& params,
-                      int menuItemId);
+                      const ExtensionMenuItem::Id& menuItemId);
 
   // Implements the NotificationObserver interface.
   virtual void Observe(NotificationType type, const NotificationSource& source,
@@ -231,7 +226,8 @@ class ExtensionMenuManager
   void RadioItemSelected(ExtensionMenuItem* item);
 
   // Returns true if item is a descendant of an item with id |ancestor_id|.
-  bool DescendantOf(ExtensionMenuItem* item, int ancestor_id);
+  bool DescendantOf(ExtensionMenuItem* item,
+                    const ExtensionMenuItem::Id& ancestor_id);
 
   // Makes sure we've done one-time initialization of the default extension icon
   // default_icon_.
@@ -247,10 +243,7 @@ class ExtensionMenuManager
   // This lets us make lookup by id fast. It maps id to ExtensionMenuItem* for
   // all items the menu manager knows about, including all children of top-level
   // items.
-  std::map<int, ExtensionMenuItem*> items_by_id_;
-
-  // The id we will assign to the next item that gets added.
-  int next_item_id_;
+  std::map<ExtensionMenuItem::Id, ExtensionMenuItem*> items_by_id_;
 
   NotificationRegistrar registrar_;
 
diff --git a/chrome/browser/extensions/extension_menu_manager_unittest.cc b/chrome/browser/extensions/extension_menu_manager_unittest.cc
index f0960cb..e3f82e8 100644
--- a/chrome/browser/extensions/extension_menu_manager_unittest.cc
+++ b/chrome/browser/extensions/extension_menu_manager_unittest.cc
@@ -29,15 +29,15 @@ using testing::SaveArg;
 // Base class for tests.
 class ExtensionMenuManagerTest : public testing::Test {
  public:
-  ExtensionMenuManagerTest() {}
+  ExtensionMenuManagerTest() : next_id_(1) {}
   ~ExtensionMenuManagerTest() {}
 
   // Returns a test item.
-  static ExtensionMenuItem* CreateTestItem(Extension* extension) {
+  ExtensionMenuItem* CreateTestItem(Extension* extension) {
     ExtensionMenuItem::Type type = ExtensionMenuItem::NORMAL;
     ExtensionMenuItem::ContextList contexts(ExtensionMenuItem::ALL);
-    return new ExtensionMenuItem(extension->id(), "test", false, type,
-                                 contexts);
+    ExtensionMenuItem::Id id(extension->id(), next_id_++);
+    return new ExtensionMenuItem(id, "test", false, type, contexts);
   }
 
   // Creates and returns a test Extension. The caller does *not* own the return
@@ -52,6 +52,7 @@ class ExtensionMenuManagerTest : public testing::Test {
   ExtensionMenuManager manager_;
   ScopedVector<Extension> extensions_;
   TestExtensionPrefs prefs_;
+  int next_id_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ExtensionMenuManagerTest);
@@ -64,9 +65,8 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
   // Add a new item, make sure you can get it back.
   ExtensionMenuItem* item1 = CreateTestItem(extension);
   ASSERT_TRUE(item1 != NULL);
-  int id1 = manager_.AddContextItem(extension, item1);
-  ASSERT_GT(id1, 0);
-  ASSERT_EQ(item1, manager_.GetItemById(id1));
+  ASSERT_TRUE(manager_.AddContextItem(extension, item1));
+  ASSERT_EQ(item1, manager_.GetItemById(item1->id()));
   const ExtensionMenuItem::List* items =
       manager_.MenuItems(item1->extension_id());
   ASSERT_EQ(1u, items->size());
@@ -74,10 +74,8 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
 
   // Add a second item, make sure it comes back too.
   ExtensionMenuItem* item2 = CreateTestItem(extension);
-  int id2 = manager_.AddContextItem(extension, item2);
-  ASSERT_GT(id2, 0);
-  ASSERT_NE(id1, id2);
-  ASSERT_EQ(item2, manager_.GetItemById(id2));
+  ASSERT_TRUE(manager_.AddContextItem(extension, item2));
+  ASSERT_EQ(item2, manager_.GetItemById(item2->id()));
   items = manager_.MenuItems(item2->extension_id());
   ASSERT_EQ(2u, items->size());
   ASSERT_EQ(item1, items->at(0));
@@ -85,9 +83,9 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
 
   // Try adding item 3, then removing it.
   ExtensionMenuItem* item3 = CreateTestItem(extension);
+  ExtensionMenuItem::Id id3 = item3->id();
   std::string extension_id = item3->extension_id();
-  int id3 = manager_.AddContextItem(extension, item3);
-  ASSERT_GT(id3, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension, item3));
   ASSERT_EQ(item3, manager_.GetItemById(id3));
   ASSERT_EQ(3u, manager_.MenuItems(extension_id)->size());
   ASSERT_TRUE(manager_.RemoveContextMenuItem(id3));
@@ -95,7 +93,8 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
   ASSERT_EQ(2u, manager_.MenuItems(extension_id)->size());
 
   // Make sure removing a non-existent item returns false.
-  ASSERT_FALSE(manager_.RemoveContextMenuItem(5));
+  ExtensionMenuItem::Id id(extension->id(), id3.second + 50);
+  ASSERT_FALSE(manager_.RemoveContextMenuItem(id));
 }
 
 // Test adding/removing child items.
@@ -114,18 +113,19 @@ TEST_F(ExtensionMenuManagerTest, ChildFunctions) {
   scoped_ptr<ExtensionMenuItem> item3(CreateTestItem(extension3));
 
   // Add in the first two items.
-  int id1 = manager_.AddContextItem(extension1, item1);
-  int id2 = manager_.AddContextItem(extension2, item2);
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
+  ASSERT_TRUE(manager_.AddContextItem(extension2, item2));
 
-  ASSERT_NE(id1, id2);
+  ExtensionMenuItem::Id id1 = item1->id();
+  ExtensionMenuItem::Id id2 = item2->id();
 
   // Try adding item3 as a child of item2 - this should fail because item3 has
   // a different extension id.
-  ASSERT_EQ(0, manager_.AddChildItem(id2, item3.get()));
+  ASSERT_FALSE(manager_.AddChildItem(id2, item3.get()));
 
   // Add item2_child as a child of item2.
-  int id2_child = manager_.AddChildItem(id2, item2_child);
-  ASSERT_GT(id2_child, 0);
+  ExtensionMenuItem::Id id2_child = item2_child->id();
+  ASSERT_TRUE(manager_.AddChildItem(id2, item2_child));
   ASSERT_EQ(1, item2->child_count());
   ASSERT_EQ(0, item1->child_count());
   ASSERT_EQ(item2_child, manager_.GetItemById(id2_child));
@@ -134,8 +134,8 @@ TEST_F(ExtensionMenuManagerTest, ChildFunctions) {
   ASSERT_EQ(item1, manager_.MenuItems(item1->extension_id())->at(0));
 
   // Add item2_grandchild as a child of item2_child, then remove it.
-  int id2_grandchild = manager_.AddChildItem(id2_child, item2_grandchild);
-  ASSERT_GT(id2_grandchild, 0);
+  ExtensionMenuItem::Id id2_grandchild = item2_grandchild->id();
+  ASSERT_TRUE(manager_.AddChildItem(id2_child, item2_grandchild));
   ASSERT_EQ(1, item2->child_count());
   ASSERT_EQ(1, item2_child->child_count());
   ASSERT_TRUE(manager_.RemoveContextMenuItem(id2_grandchild));
@@ -160,10 +160,8 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   ExtensionMenuItem* item1 = CreateTestItem(extension1);
   ExtensionMenuItem* item2 = CreateTestItem(extension1);
 
-  int id1 = manager_.AddContextItem(extension1, item1);
-  ASSERT_GT(id1, 0);
-  int id2 = manager_.AddContextItem(extension1, item2);
-  ASSERT_GT(id2, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item2));
 
   const ExtensionMenuItem::List* items =
       manager_.MenuItems(item1->extension_id());
@@ -175,18 +173,17 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   // it to be a child of item2.
   ExtensionMenuItem* item3 = CreateTestItem(extension1);
 
-  int id3 = manager_.AddChildItem(id1, item3);
-  ASSERT_GT(id3, 0);
+  ASSERT_TRUE(manager_.AddChildItem(item1->id(), item3));
   ASSERT_EQ(1, item1->child_count());
   ASSERT_EQ(item3, item1->children()[0]);
 
-  ASSERT_TRUE(manager_.ChangeParent(id3, id2));
+  ASSERT_TRUE(manager_.ChangeParent(item3->id(), &item2->id()));
   ASSERT_EQ(0, item1->child_count());
   ASSERT_EQ(1, item2->child_count());
   ASSERT_EQ(item3, item2->children()[0]);
 
   // Move item2 to be a child of item1.
-  ASSERT_TRUE(manager_.ChangeParent(id2, id1));
+  ASSERT_TRUE(manager_.ChangeParent(item2->id(), &item1->id()));
   ASSERT_EQ(1, item1->child_count());
   ASSERT_EQ(item2, item1->children()[0]);
   ASSERT_EQ(1, item2->child_count());
@@ -199,13 +196,13 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   ASSERT_EQ(item1, items->at(0));
 
   // Move item3 back to being a child of item1, so it's now a sibling of item2.
-  ASSERT_TRUE(manager_.ChangeParent(id3, id1));
+  ASSERT_TRUE(manager_.ChangeParent(item3->id(), &item1->id()));
   ASSERT_EQ(2, item1->child_count());
   ASSERT_EQ(item2, item1->children()[0]);
   ASSERT_EQ(item3, item1->children()[1]);
 
   // Try switching item3 to be the parent of item1 - this should fail.
-  ASSERT_FALSE(manager_.ChangeParent(id1, id3));
+  ASSERT_FALSE(manager_.ChangeParent(item1->id(), &item3->id()));
   ASSERT_EQ(0, item3->child_count());
   ASSERT_EQ(2, item1->child_count());
   ASSERT_EQ(item2, item1->children()[0]);
@@ -215,7 +212,7 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   ASSERT_EQ(item1, items->at(0));
 
   // Move item2 to be a top-level item.
-  ASSERT_TRUE(manager_.ChangeParent(id2, 0));
+  ASSERT_TRUE(manager_.ChangeParent(item2->id(), NULL));
   items = manager_.MenuItems(item1->extension_id());
   ASSERT_EQ(2u, items->size());
   ASSERT_EQ(item1, items->at(0));
@@ -226,13 +223,12 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   // Make sure you can't move a node to be a child of another extension's item.
   Extension* extension2 = AddExtension("2222");
   ExtensionMenuItem* item4 = CreateTestItem(extension2);
-  int id4 = manager_.AddContextItem(extension2, item4);
-  ASSERT_GT(id4, 0);
-  ASSERT_FALSE(manager_.ChangeParent(id4, id1));
-  ASSERT_FALSE(manager_.ChangeParent(id1, id4));
+  ASSERT_TRUE(manager_.AddContextItem(extension2, item4));
+  ASSERT_FALSE(manager_.ChangeParent(item4->id(), &item1->id()));
+  ASSERT_FALSE(manager_.ChangeParent(item1->id(), &item4->id()));
 
   // Make sure you can't make an item be it's own parent.
-  ASSERT_FALSE(manager_.ChangeParent(id1, id1));
+  ASSERT_FALSE(manager_.ChangeParent(item1->id(), &item1->id()));
 }
 
 // Tests that we properly remove an extension's menu item when that extension is
@@ -246,17 +242,16 @@ TEST_F(ExtensionMenuManagerTest, ExtensionUnloadRemovesMenuItems) {
 
   // Create an ExtensionMenuItem and put it into the manager.
   ExtensionMenuItem* item1 = CreateTestItem(extension1);
+  ExtensionMenuItem::Id id1 = item1->id();
   ASSERT_EQ(extension1->id(), item1->extension_id());
-  int id1 = manager_.AddContextItem(extension1, item1);
-  ASSERT_GT(id1, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
   ASSERT_EQ(1u, manager_.MenuItems(extension1->id())->size());
 
   // Create a menu item with a different extension id and add it to the manager.
   Extension* extension2 = AddExtension("2222");
   ExtensionMenuItem* item2 = CreateTestItem(extension2);
   ASSERT_NE(item1->extension_id(), item2->extension_id());
-  int id2 = manager_.AddContextItem(extension2, item2);
-  ASSERT_GT(id2, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension2, item2));
 
   // Notify that the extension was unloaded, and make sure the right item is
   // gone.
@@ -266,7 +261,7 @@ TEST_F(ExtensionMenuManagerTest, ExtensionUnloadRemovesMenuItems) {
   ASSERT_EQ(NULL, manager_.MenuItems(extension1->id()));
   ASSERT_EQ(1u, manager_.MenuItems(extension2->id())->size());
   ASSERT_TRUE(manager_.GetItemById(id1) == NULL);
-  ASSERT_TRUE(manager_.GetItemById(id2) != NULL);
+  ASSERT_TRUE(manager_.GetItemById(item2->id()) != NULL);
 }
 
 // A mock message service for tests of ExtensionMenuManager::ExecuteCommand.
@@ -305,17 +300,14 @@ TEST_F(ExtensionMenuManagerTest, RemoveAll) {
   ExtensionMenuItem* item1 = CreateTestItem(extension1);
   ExtensionMenuItem* item2 = CreateTestItem(extension1);
   ExtensionMenuItem* item3 = CreateTestItem(extension1);
-  int id1 = manager_.AddContextItem(extension1, item1);
-  int id2 = manager_.AddContextItem(extension1, item2);
-  EXPECT_GT(id1, 0);
-  EXPECT_GT(id2, 0);
-  int id3 = manager_.AddChildItem(id1, item3);
-  EXPECT_GT(id3, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item2));
+  ASSERT_TRUE(manager_.AddChildItem(item1->id(), item3));
 
   // Add one top-level item for extension 2.
   Extension* extension2 = AddExtension("2222");
   ExtensionMenuItem* item4 = CreateTestItem(extension2);
-  manager_.AddContextItem(extension2, item4);
+  ASSERT_TRUE(manager_.AddContextItem(extension2, item4));
 
   EXPECT_EQ(2u, manager_.MenuItems(extension1->id())->size());
   EXPECT_EQ(1u, manager_.MenuItems(extension2->id())->size());
@@ -348,8 +340,8 @@ TEST_F(ExtensionMenuManagerTest, ExecuteCommand) {
 
   Extension* extension = AddExtension("test");
   ExtensionMenuItem* item = CreateTestItem(extension);
-  int id = manager_.AddContextItem(extension, item);
-  ASSERT_GT(id, 0);
+  ExtensionMenuItem::Id id = item->id();
+  ASSERT_TRUE(manager_.AddContextItem(extension, item));
 
   EXPECT_CALL(profile, GetExtensionMessageService())
       .Times(1)
@@ -387,7 +379,7 @@ TEST_F(ExtensionMenuManagerTest, ExecuteCommand) {
 
   int tmp_id = 0;
   ASSERT_TRUE(info->GetInteger(L"menuItemId", &tmp_id));
-  ASSERT_EQ(id, tmp_id);
+  ASSERT_EQ(id.second, tmp_id);
 
   std::string tmp;
   ASSERT_TRUE(info->GetString(L"mediaType", &tmp));
diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
index f513891..7bb1c5c9 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.cc
+++ b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -2,7 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <functional>
+#include <algorithm>
+#include <set>
 
 #include "chrome/browser/tab_contents/render_view_context_menu.h"
 
@@ -21,7 +22,6 @@
 #include "chrome/browser/debugger/devtools_manager.h"
 #include "chrome/browser/debugger/devtools_window.h"
 #include "chrome/browser/download/download_manager.h"
-#include "chrome/browser/extensions/extension_menu_manager.h"
 #include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/browser/fonts_languages_window.h"
 #include "chrome/browser/metrics/user_metrics.h"
@@ -67,12 +67,6 @@ bool RenderViewContextMenu::IsSyncResourcesURL(const GURL& url) {
 
 static const int kSpellcheckRadioGroup = 1;
 
-// For extensions that have multiple top level menu items, we automatically
-// create a submenu item and push the top level menu items into it. This special
-// value takes the place of the ExtensionMenuItem's internal ID for the submenu
-// item inside the extension_item_map_ member variable.
-static const int kExtensionTopLevelItem = -1;
-
 RenderViewContextMenu::RenderViewContextMenu(
     TabContents* tab_contents,
     const ContextMenuParams& params)
@@ -178,7 +172,6 @@ void RenderViewContextMenu::AppendExtensionItems(
   ExtensionMenuItem::List submenu_items;
   if (items.size() > 1 || items[0]->type() != ExtensionMenuItem::NORMAL) {
     title = UTF8ToUTF16(extension->name());
-    extension_item_map_[menu_id] = kExtensionTopLevelItem;
     submenu_items = items;
   } else {
     ExtensionMenuItem* item = items[0];
@@ -667,7 +660,8 @@ void RenderViewContextMenu::AppendBidiSubMenu() {
 ExtensionMenuItem* RenderViewContextMenu::GetExtensionMenuItem(int id) const {
   ExtensionMenuManager* manager =
       profile_->GetExtensionsService()->menu_manager();
-  std::map<int, int>::const_iterator i = extension_item_map_.find(id);
+  std::map<int, ExtensionMenuItem::Id>::const_iterator i =
+      extension_item_map_.find(id);
   if (i != extension_item_map_.end()) {
     ExtensionMenuItem* item = manager->GetItemById(i->second);
     if (item)
@@ -700,7 +694,9 @@ bool RenderViewContextMenu::IsCommandIdEnabled(int id) const {
   // Extension items.
   if (id >= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST &&
       id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST) {
-    return ContainsKey(extension_item_map_, id);
+    // In the future we may add APIs for extensions to disable items, but for
+    // now all items are implicitly enabled.
+    return true;
   }
 
   switch (id) {
@@ -978,7 +974,8 @@ void RenderViewContextMenu::ExecuteCommand(int id) {
       id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST) {
     ExtensionMenuManager* manager =
         profile_->GetExtensionsService()->menu_manager();
-    std::map<int, int>::const_iterator i = extension_item_map_.find(id);
+    std::map<int, ExtensionMenuItem::Id>::const_iterator i =
+        extension_item_map_.find(id);
     if (i != extension_item_map_.end()) {
       manager->ExecuteCommand(profile_, source_tab_contents_, params_,
                               i->second);
diff --git a/chrome/browser/tab_contents/render_view_context_menu.h b/chrome/browser/tab_contents/render_view_context_menu.h
index 2874aee..836c56d 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.h
+++ b/chrome/browser/tab_contents/render_view_context_menu.h
@@ -13,6 +13,7 @@
 #include "base/string16.h"
 #include "base/scoped_vector.h"
 #include "chrome/common/page_transition_types.h"
+#include "chrome/browser/extensions/extension_menu_manager.h"
 #include "webkit/glue/context_menu.h"
 #include "webkit/glue/window_open_disposition.h"
 
@@ -127,7 +128,7 @@ class RenderViewContextMenu : public menus::SimpleMenuModel::Delegate {
 
   // Maps the id from a context menu item to the ExtensionMenuItem's internal
   // id.
-  std::map<int, int> extension_item_map_;
+  std::map<int, ExtensionMenuItem::Id> extension_item_map_;
 
   menus::SimpleMenuModel spellcheck_submenu_model_;
   menus::SimpleMenuModel bidi_submenu_model_;
diff --git a/chrome/common/extensions/api/extension_api.json b/chrome/common/extensions/api/extension_api.json
index 0dc0fd6..1805e9b 100644
--- a/chrome/common/extensions/api/extension_api.json
+++ b/chrome/common/extensions/api/extension_api.json
@@ -2508,7 +2508,11 @@
       {
         "name": "create",
         "type": "function",
-        "description": "An API to add items to context menus.",
+        "description": "Creates a new context menu item. Note that if an error occurs during creation, you may not find out until the creation callback fires (the details will be in chrome.extension.lastError).",
+        "returns": {
+          "type": "integer",
+          "description": "The id of the newly created item."
+        },
         "parameters": [
           {
             "type": "object",
@@ -2527,7 +2531,7 @@
               "checked": {
                 "type": "boolean",
                 "optional": true,
-                "description": " For items of type checkbox or radio, should this be selected (radio) or checked (checkbox)? Only one radio item can be selected at a time in a given group of radio items, with the last one to have checked == true winning."
+                "description": "This only applies to the initial state of a checkbox or radio item. It indicates if the item should initially be checked/selected. Only one radio item can be selected at a time in a given group of radio items."
               },
               "contexts": {
                 "type": "array",
@@ -2552,13 +2556,8 @@
             "type": "function",
             "name": "callback",
             "optional": true,
-            "parameters": [
-              {
-                "name": "menuItemId",
-                "type": "integer",
-                "description": "The id of the newly created context menu item."
-              }
-            ]
+            "description": "Called when the item has been created in the browser. If there were any problems creating the item, details will be avilable in chrome.extension.lastError.",
+            "parameters": []
           }
         ]
       },
diff --git a/chrome/common/extensions/docs/experimental.contextMenus.html b/chrome/common/extensions/docs/experimental.contextMenus.html
index be67403..5994d51 100644
--- a/chrome/common/extensions/docs/experimental.contextMenus.html
+++ b/chrome/common/extensions/docs/experimental.contextMenus.html
@@ -312,7 +312,7 @@ see the <a href="experimental.html">chrome.experimental.* APIs</a> page.
               <a name="method-create"></a> <!-- method-anchor -->
               <h4>create</h4>
 
-              <div class="summary"><span style="display: none; ">void</span>
+              <div class="summary"><span>integer</span>
                   <!-- Note: intentionally longer 80 columns -->
                   <span>chrome.experimental.contextMenus.create</span>(<span class="null"><span style="display: none; ">, </span><span>object</span>
                       <var><span>createProperties</span></var></span><span class="optional"><span>, </span><span>function</span>
@@ -320,7 +320,7 @@ see the <a href="experimental.html">chrome.experimental.* APIs</a> page.
 
               <div class="description">
                 <p class="todo" style="display: none; ">Undocumented.</p>
-                <p>An API to add items to context menus.</p>
+                <p>Creates a new context menu item. Note that if an error occurs during creation, you may not find out until the creation callback fires (the details will be in chrome.extension.lastError).</p>
 
                 <!-- PARAMETERS -->
                 <h4>Parameters</h4>
@@ -507,7 +507,7 @@ see the <a href="experimental.html">chrome.experimental.* APIs</a> page.
           <dd class="todo" style="display: none; ">
             Undocumented.
           </dd>
-          <dd> For items of type checkbox or radio, should this be selected (radio) or checked (checkbox)? Only one radio item can be selected at a time in a given group of radio items, with the last one to have checked == true winning.</dd>
+          <dd>This only applies to the initial state of a checkbox or radio item. It indicates if the item should initially be checked/selected. Only one radio item can be selected at a time in a given group of radio items.</dd>
           <dd style="display: none; ">
             This parameter was added in version
             <b><span></span></b>.
@@ -727,12 +727,10 @@ see the <a href="experimental.html">chrome.experimental.* APIs</a> page.
 
               </em>
           </dt>
-          <dd class="todo">
+          <dd class="todo" style="display: none; ">
             Undocumented.
           </dd>
-          <dd style="display: none; ">
-            Description of this parameter from the json schema.
-          </dd>
+          <dd>Called when the item has been created in the browser. If there were any problems creating the item, details will be avilable in chrome.extension.lastError.</dd>
           <dd style="display: none; ">
             This parameter was added in version
             <b><span></span></b>.
@@ -757,34 +755,12 @@ see the <a href="experimental.html">chrome.experimental.* APIs</a> page.
                 </dl>
 
                 <!-- RETURNS -->
-                <h4 style="display: none; ">Returns</h4>
+                <h4>Returns</h4>
                 <dl>
-                  <div style="display: none; ">
-                    <div>
-                    </div>
-                  </div>
-                </dl>
-
-                <!-- CALLBACK -->
-                <div>
                   <div>
-                  <h4>Callback function</h4>
-                  <p style="display: none; ">
-                    The callback <em>parameter</em> should specify a function
-                    that looks like this:
-                  </p>
-                  <p>
-                    If you specify the <em>callback</em> parameter, it should
-                    specify a function that looks like this:
-                  </p>
-
-                  <!-- Note: intentionally longer 80 columns -->
-                  <pre>function(<span>integer menuItemId</span>) <span class="subdued">{...}</span>);</pre>
-                  <dl>
                     <div>
-                      <div>
           <dt>
-            <var>menuItemId</var>
+            <var style="display: none; ">paramName</var>
               <em>
 
                 <!-- TYPE -->
@@ -812,7 +788,7 @@ see the <a href="experimental.html">chrome.experimental.* APIs</a> page.
           <dd class="todo" style="display: none; ">
             Undocumented.
           </dd>
-          <dd>The id of the newly created context menu item.</dd>
+          <dd>The id of the newly created item.</dd>
           <dd style="display: none; ">
             This parameter was added in version
             <b><span></span></b>.
@@ -833,6 +809,28 @@ see the <a href="experimental.html">chrome.experimental.* APIs</a> page.
             </dl>
           </dd>
         </div>
+                  </div>
+                </dl>
+
+                <!-- CALLBACK -->
+                <div>
+                  <div>
+                  <h4>Callback function</h4>
+                  <p style="display: none; ">
+                    The callback <em>parameter</em> should specify a function
+                    that looks like this:
+                  </p>
+                  <p>
+                    If you specify the <em>callback</em> parameter, it should
+                    specify a function that looks like this:
+                  </p>
+
+                  <!-- Note: intentionally longer 80 columns -->
+                  <pre>function(<span></span>) <span class="subdued">{...}</span>);</pre>
+                  <dl>
+                    <div style="display: none; ">
+                      <div>
+                      </div>
                     </div>
                   </dl>
                   </div>
diff --git a/chrome/renderer/resources/extension_process_bindings.js b/chrome/renderer/resources/extension_process_bindings.js
index 9614959..5e7697b 100644
--- a/chrome/renderer/resources/extension_process_bindings.js
+++ b/chrome/renderer/resources/extension_process_bindings.js
@@ -263,12 +263,16 @@ var chrome = chrome || {};
   }
 
   function setupHiddenContextMenuEvent(extensionId) {
+    chromeHidden.contextMenus = {};
+    chromeHidden.contextMenus.nextId = 1;
+    chromeHidden.contextMenus.handlers = {};
     var eventName = "contextMenus/" + extensionId;
-    chromeHidden.contextMenuEvent = new chrome.Event(eventName);
-    chromeHidden.contextMenuHandlers = {};
-    chromeHidden.contextMenuEvent.addListener(function() {
-      var menuItemId = arguments[0].menuItemId;
-      var onclick = chromeHidden.contextMenuHandlers[menuItemId];
+    chromeHidden.contextMenus.event = new chrome.Event(eventName);
+    chromeHidden.contextMenus.event.addListener(function() {
+      // An extension context menu item has been clicked on - fire the onclick
+      // if there is one.
+      var id = arguments[0].menuItemId;
+      var onclick = chromeHidden.contextMenus.handlers[id];
       if (onclick) {
         onclick.apply(onclick, arguments);
       }
@@ -591,27 +595,57 @@ var chrome = chrome || {};
           details, this.name, this.definition.parameters, "page action");
     };
 
+    apiFunctions["experimental.contextMenus.create"].handleRequest =
+        function() {
+      var args = arguments;
+      var id = chromeHidden.contextMenus.nextId++;
+      args[0].generatedId = id;
+      sendRequest(this.name, args, this.definition.parameters,
+                  this.customCallback);
+      return id;
+    };
+
     apiFunctions["experimental.contextMenus.create"].customCallback =
         function(name, request, response) {
-      if (chrome.extension.lastError || !response) {
+      if (chrome.extension.lastError) {
         return;
       }
 
+      var id = request.args[0].generatedId;
+
       // Set up the onclick handler if we were passed one in the request.
       var onclick = request.args.length ? request.args[0].onclick : null;
       if (onclick) {
-        var menuItemId = chromeHidden.JSON.parse(response);
-        chromeHidden.contextMenuHandlers[menuItemId] = onclick;
+        chromeHidden.contextMenus.handlers[id] = onclick;
       }
     };
 
     apiFunctions["experimental.contextMenus.remove"].customCallback =
         function(name, request, response) {
-      // Remove any onclick handler we had registered for this menu item.
-      if (request.args.length > 0) {
-        var menuItemId = request.args[0];
-        delete chromeHidden.contextMenuHandlers[menuItemId];
+      if (chrome.extension.lastError) {
+        return;
+      }
+      var id = request.args[0];
+      delete chromeHidden.contextMenus.handlers[id];
+    };
+
+    apiFunctions["experimental.contextMenus.update"].customCallback =
+        function(name, request, response) {
+      if (chrome.extension.lastError) {
+        return;
+      }
+      var id = request.args[0];
+      if (request.args[1].onclick) {
+        chromeHidden.contextMenus.handlers[id] = request.args[1].onclick;
+      }
+    };
+
+    apiFunctions["experimental.contextMenus.removeAll"].customCallback =
+        function(name, request, response) {
+      if (chrome.extension.lastError) {
+        return;
       }
+      chromeHidden.contextMenus.handlers = {};
     };
 
     apiFunctions["tabs.captureVisibleTab"].updateArguments = function() {
diff --git a/chrome/test/data/extensions/api_test/context_menus/test.js b/chrome/test/data/extensions/api_test/context_menus/test.js
index 14cc1d7..65ca7d9 100644
--- a/chrome/test/data/extensions/api_test/context_menus/test.js
+++ b/chrome/test/data/extensions/api_test/context_menus/test.js
@@ -21,14 +21,16 @@ var tests = [
   },
 
   function remove() {
-    chrome.contextMenus.create({"title":"1"}, function(id) {
+    var id;
+    id = chrome.contextMenus.create({"title":"1"}, function() {
       assertNoLastError();
       chrome.contextMenus.remove(id, chrome.test.callbackPass());
     });
   },
 
   function update() {
-    chrome.contextMenus.create({"title":"update test"}, function(id) {
+    var id;
+    id = chrome.contextMenus.create({"title":"update test"}, function() {
       assertNoLastError();
       chrome.contextMenus.update(id, {"title": "test2"},
                                 chrome.test.callbackPass());
@@ -36,9 +38,9 @@ var tests = [
   },
 
   function removeAll() {
-    chrome.contextMenus.create({"title":"1"}, function(id) {
+    chrome.contextMenus.create({"title":"1"}, function() {
       assertNoLastError();
-      chrome.contextMenus.create({"title":"2"}, function(id2) {
+      chrome.contextMenus.create({"title":"2"}, function() {
         assertNoLastError();
         chrome.contextMenus.removeAll(chrome.test.callbackPass());
       });
@@ -46,10 +48,11 @@ var tests = [
   },
 
   function hasParent() {
-    chrome.contextMenus.create({"title":"parent"}, function(id) {
+    var id;
+    id = chrome.contextMenus.create({"title":"parent"}, function() {
       assertNoLastError();
       chrome.contextMenus.create({"title":"child", "parentId":id},
-                                function(id2) {
+                                function() {
         assertNoLastError();
         chrome.test.succeed();
       });
diff --git a/chrome/test/data/extensions/context_menus/test.js b/chrome/test/data/extensions/context_menus/test.js
index 9cab126..778b42c 100644
--- a/chrome/test/data/extensions/context_menus/test.js
+++ b/chrome/test/data/extensions/context_menus/test.js
@@ -26,7 +26,7 @@ function onclick(info) {
 
 window.onload = function() {
   chrome.experimental.contextMenus.create({"title":"Extension Item 1",
-                                          "onclick": onclick}, function(id) {
+                                          "onclick": onclick}, function() {
     if (!chrome.extension.lastError) {
       navigateCurrentTab(chrome.extension.getURL("test.html"));
     }