diff options
Diffstat (limited to 'chrome/browser')
6 files changed, 189 insertions, 190 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_; |