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

1 files changed, 32 insertions, 39 deletions

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_;