Revert "[Extensions] Added visibility property to chrome.contextMenu items API"
This reverts commit 44248851c5f19227c3fc9bcc8c60285175064a3a.
Reason for revert: <INSERT REASONING HERE>
FindIt claims that this CL is causing a build failure (with 100% certainty). More info:
https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29/builds/10594
https://chromium-swarm.appspot.com/task?id=3822bb5befc13310&refresh=10&show_raw=1
Original change's description:
> [Extensions] Added visibility property to chrome.contextMenu items API
>
> Users of the chrome.contextMenu API should be able to programmatically
> set the visiblility (shown, hidden) of context menu items on item
> creation and update.
>
> This CL introduces the |visible| property to createProperties
> (chrome.contextMenus.create) and updateProperties
> (chrome.contextMenus.udpate). Note, this change ensures that hidden
> items are not counted against
> chrome.contextMenus.ACTION_MENU_TOP_LEVEL_LIMIT.
>
> Bug: 463476
> Change-Id: I315bcf070516fe8667e51fbec7c8f539230afa23
> Reviewed-on: https://chromium-review.googlesource.com/553578
> Commit-Queue: catmullings <[email protected]>
> Reviewed-by: Istiaque Ahmed <[email protected]>
> Reviewed-by: Devlin <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#496210}
[email protected],[email protected],[email protected]
Change-Id: I25265b68f8881f1bce650a395067ce05bab4cadd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 463476
Reviewed-on: https://chromium-review.googlesource.com/626622
Reviewed-by: Dave Schuyler <[email protected]>
Commit-Queue: Dave Schuyler <[email protected]>
Cr-Commit-Position: refs/heads/master@{#496386}diff --git a/chrome/browser/extensions/api/context_menus/context_menu_apitest.cc b/chrome/browser/extensions/api/context_menus/context_menu_apitest.cc
index 216ffc2..2dcdab1 100644
--- a/chrome/browser/extensions/api/context_menus/context_menu_apitest.cc
+++ b/chrome/browser/extensions/api/context_menus/context_menu_apitest.cc
@@ -2,115 +2,17 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/extensions/extension_action.h"
#include "chrome/browser/extensions/extension_apitest.h"
#include "chrome/browser/extensions/extension_service.h"
-#include "chrome/browser/extensions/menu_manager.h"
#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/renderer_context_menu/render_view_context_menu.h"
-#include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/common/extensions/api/context_menus.h"
#include "chrome/test/base/ui_test_utils.h"
-#include "content/public/browser/render_frame_host.h"
-#include "content/public/common/context_menu_params.h"
-#include "content/public/test/browser_test_utils.h"
#include "extensions/test/result_catcher.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
-#include "ui/base/models/menu_model.h"
namespace extensions {
-class ExtensionContextMenuApiTest : public ExtensionApiTest {
- public:
- ExtensionContextMenuApiTest()
- : top_level_model_(nullptr), menu_(nullptr), top_level_index_(-1) {}
-
- void SetUpTestExtension() {
- extension_ = LoadExtension(
- test_data_dir_.AppendASCII("context_menus/item_visibility/"));
- }
-
- int CountItemsInMenu(TestRenderViewContextMenu* menu) {
- return menu->extension_items().extension_item_map_.size();
- }
-
- // Sets up the top-level model, which is the list of menu items (both related
- // and unrelated to extensions) that is passed to UI code to be displayed.
- bool SetupTopLevelMenuModel() {
- content::RenderFrameHost* frame =
- browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame();
- content::ContextMenuParams params;
- params.page_url = frame->GetLastCommittedURL();
-
- // Create context menu.
- menu_.reset(new TestRenderViewContextMenu(frame, params));
- menu_->Init();
-
- // Get menu model.
- bool valid_setup = menu_->GetMenuModelAndItemIndex(
- menu_->extension_items().ConvertToExtensionsCustomCommandId(0),
- &top_level_model_, &top_level_index_);
-
- EXPECT_GT(top_level_index(), 0);
-
- return valid_setup;
- }
-
- void CallAPI(const std::string& script) {
- content::RenderViewHost* background_page =
- GetBackgroundPage(extension_->id());
- bool error = false;
- ASSERT_TRUE(
- content::ExecuteScriptAndExtractBool(background_page, script, &error));
- ASSERT_FALSE(error);
- }
-
- // Verifies that the context menu is valid and contains the given number of
- // menu items, |numItems|.
- void VerifyNumContextMenuItems(int num_items) {
- ASSERT_TRUE(menu());
- EXPECT_EQ(num_items,
- (int)(menu_->extension_items().extension_item_map_.size()));
- }
-
- // Verifies a context menu item's visibility, title, and item type.
- void VerifyMenuItem(const std::string& title,
- ui::MenuModel* model,
- int index,
- ui::MenuModel::ItemType type,
- bool visible) {
- EXPECT_EQ(base::ASCIIToUTF16(title), model->GetLabelAt(index));
- ASSERT_EQ(type, model->GetTypeAt(index));
- EXPECT_EQ(visible, model->IsVisibleAt(index));
- }
-
- int top_level_index() { return top_level_index_; }
-
- TestRenderViewContextMenu* menu() { return menu_.get(); }
-
- const Extension* extension() { return extension_; }
-
- ui::MenuModel* top_level_model_;
-
- private:
- content::RenderViewHost* GetBackgroundPage(const std::string& extension_id) {
- return process_manager()
- ->GetBackgroundHostForExtension(extension_id)
- ->render_view_host();
- }
-
- ProcessManager* process_manager() { return ProcessManager::Get(profile()); }
-
- const Extension* extension_;
- std::unique_ptr<TestRenderViewContextMenu> menu_;
- int top_level_index_;
-
- DISALLOW_COPY_AND_ASSIGN(ExtensionContextMenuApiTest);
-};
-
// Times out on win syzyasan, http://crbug.com/166026
#if defined(SYZYASAN)
#define MAYBE_ContextMenus DISABLED_ContextMenus
@@ -130,7 +32,7 @@
ASSERT_TRUE(embedded_test_server()->Start());
ASSERT_TRUE(RunExtensionTest("context_menus/add_from_multiple_contexts"))
<< message_;
- const Extension* extension = GetSingleLoadedExtension();
+ const extensions::Extension* extension = GetSingleLoadedExtension();
ASSERT_TRUE(extension) << message_;
{
@@ -150,251 +52,4 @@
}
}
-// Tests showing a single visible menu item in the top-level menu model, which
-// includes actions like "Back", "View Page Source", "Inspect", etc.
-IN_PROC_BROWSER_TEST_F(ExtensionContextMenuApiTest, ShowOneTopLevelItem) {
- SetUpTestExtension();
- CallAPI("create({title: 'item', visible: true});");
-
- ASSERT_TRUE(SetupTopLevelMenuModel());
-
- VerifyNumContextMenuItems(1);
-
- VerifyMenuItem("item", top_level_model_, top_level_index(),
- ui::MenuModel::TYPE_COMMAND, true);
-
- // There should be no submenu model.
- EXPECT_FALSE(top_level_model_->GetSubmenuModelAt(top_level_index()));
-}
-
-// Tests hiding a menu item in the top-level menu model, which includes actions
-// like "Back", "View Page Source", "Inspect", etc.
-IN_PROC_BROWSER_TEST_F(ExtensionContextMenuApiTest, HideTopLevelItem) {
- SetUpTestExtension();
- CallAPI("create({id: 'item1', title: 'item', visible: true});");
- CallAPI("update('item1', {visible: false});");
-
- ASSERT_TRUE(SetupTopLevelMenuModel());
-
- VerifyNumContextMenuItems(1);
-
- VerifyMenuItem("item", top_level_model_, top_level_index(),
- ui::MenuModel::TYPE_COMMAND, false);
-
- // There should be no submenu model.
- EXPECT_FALSE(top_level_model_->GetSubmenuModelAt(top_level_index()));
-}
-
-// Tests hiding a parent menu item, when it is hidden and so are all of its
-// children.
-IN_PROC_BROWSER_TEST_F(ExtensionContextMenuApiTest,
- HideTopLevelSubmenuItemIfHiddenAndChildrenHidden) {
- SetUpTestExtension();
- CallAPI("create({id: 'id', title: 'parent', visible: false});");
- CallAPI("create({title: 'child1', parentId: 'id', visible: false});");
- CallAPI("create({title: 'child2', parentId: 'id', visible: false});");
-
- ASSERT_TRUE(SetupTopLevelMenuModel());
-
- VerifyNumContextMenuItems(3);
-
- VerifyMenuItem("parent", top_level_model_, top_level_index(),
- ui::MenuModel::TYPE_SUBMENU, false);
-
- // Since the extension submenu is hidden, the previous separator should not be
- // in the model.
- EXPECT_NE(ui::MenuModel::TYPE_SEPARATOR,
- top_level_model_->GetTypeAt(top_level_index() - 1));
-
- ui::MenuModel* submodel =
- top_level_model_->GetSubmenuModelAt(top_level_index());
- ASSERT_TRUE(submodel);
- EXPECT_EQ(2, submodel->GetItemCount());
-
- VerifyMenuItem("child1", submodel, 0, ui::MenuModel::TYPE_COMMAND, false);
- VerifyMenuItem("child2", submodel, 1, ui::MenuModel::TYPE_COMMAND, false);
-}
-
-// Tests hiding a parent menu item, when it is hidden and some of its children
-// are visible.
-IN_PROC_BROWSER_TEST_F(ExtensionContextMenuApiTest,
- HideTopLevelSubmenuItemIfHiddenAndSomeChildrenVisible) {
- SetUpTestExtension();
- CallAPI("create({id: 'id', title: 'parent', visible: false});");
- CallAPI("create({title: 'child1', parentId: 'id', visible: false});");
- CallAPI("create({title: 'child2', parentId: 'id', visible: true});");
-
- ASSERT_TRUE(SetupTopLevelMenuModel());
-
- VerifyNumContextMenuItems(3);
-
- VerifyMenuItem("parent", top_level_model_, top_level_index(),
- ui::MenuModel::TYPE_SUBMENU, false);
-
- // Since the extension submenu is hidden, the previous separator should not be
- // in the model.
- EXPECT_NE(ui::MenuModel::TYPE_SEPARATOR,
- top_level_model_->GetTypeAt(top_level_index() - 1));
-
- ui::MenuModel* submodel =
- top_level_model_->GetSubmenuModelAt(top_level_index());
- ASSERT_TRUE(submodel);
- EXPECT_EQ(2, submodel->GetItemCount());
-
- // Though the children's internal visibility state remains unchanged, the ui
- // code will hide the children if the parent is hidden.
- VerifyMenuItem("child1", submodel, 0, ui::MenuModel::TYPE_COMMAND, false);
- VerifyMenuItem("child2", submodel, 1, ui::MenuModel::TYPE_COMMAND, true);
-}
-
-// Tests showing a single top-level parent menu item, when it is visible, but
-// all of its child items are hidden. The child items' hidden states are tested
-// too. Recall that a top-level item can be either a parent item specified by
-// the developer or parent item labeled with the extension's name. In this case,
-// we test the former.
-IN_PROC_BROWSER_TEST_F(ExtensionContextMenuApiTest,
- ShowTopLevelItemIfAllItsChildrenAreHidden) {
- SetUpTestExtension();
- CallAPI("create({id: 'id', title: 'parent', visible: true});");
- CallAPI("create({title: 'child1', parentId: 'id', visible: false});");
- CallAPI("create({title: 'child2', parentId: 'id', visible: false});");
-
- ASSERT_TRUE(SetupTopLevelMenuModel());
-
- VerifyNumContextMenuItems(3);
-
- VerifyMenuItem("parent", top_level_model_, top_level_index(),
- ui::MenuModel::TYPE_SUBMENU, true);
-
- // Since the extension submenu is shown, the previous separator should be in
- // the model.
- EXPECT_EQ(ui::MenuModel::TYPE_SEPARATOR,
- top_level_model_->GetTypeAt(top_level_index() - 1));
-
- ui::MenuModel* submodel =
- top_level_model_->GetSubmenuModelAt(top_level_index());
- ASSERT_TRUE(submodel);
- EXPECT_EQ(2, submodel->GetItemCount());
-
- VerifyMenuItem("child1", submodel, 0, ui::MenuModel::TYPE_COMMAND, false);
- VerifyMenuItem("child2", submodel, 1, ui::MenuModel::TYPE_COMMAND, false);
-}
-
-// Tests showing a top-level parent menu item as a submenu, when some of its
-// child items are visibile. The child items' visibilities are tested too.
-// Recall that a top-level item can be either a parent item specified by the
-// developer or parent item labeled with the extension's name. In this case, we
-// test the former.
-IN_PROC_BROWSER_TEST_F(ExtensionContextMenuApiTest,
- ShowTopLevelSubmenuItemIfSomeOfChildrenAreVisible) {
- SetUpTestExtension();
- CallAPI("create({id: 'id', title: 'parent', visible: true});");
- CallAPI("create({title: 'child1', parentId: 'id', visible: true});");
- CallAPI("create({title: 'child2', parentId: 'id', visible: false});");
-
- ASSERT_TRUE(SetupTopLevelMenuModel());
-
- VerifyNumContextMenuItems(3);
-
- VerifyMenuItem("parent", top_level_model_, top_level_index(),
- ui::MenuModel::TYPE_SUBMENU, true);
-
- // Since the extension submenu is shown, the previous separator should be in
- // the model.
- EXPECT_EQ(ui::MenuModel::TYPE_SEPARATOR,
- top_level_model_->GetTypeAt(top_level_index() - 1));
-
- ui::MenuModel* submodel =
- top_level_model_->GetSubmenuModelAt(top_level_index());
- ASSERT_TRUE(submodel);
- EXPECT_EQ(2, submodel->GetItemCount());
-
- VerifyMenuItem("child1", submodel, 0, ui::MenuModel::TYPE_COMMAND, true);
- VerifyMenuItem("child2", submodel, 1, ui::MenuModel::TYPE_COMMAND, false);
-}
-
-// Tests showing a top-level parent menu item, when all of its child items are
-// hidden. Recall that a top-level item can be either a parent item specified by
-// the developer or parent item labeled with the extension's name. In this case,
-// we test the latter. This extension-named top-level item should always be
-// visible.
-IN_PROC_BROWSER_TEST_F(ExtensionContextMenuApiTest,
- ShowExtensionNamedTopLevelItemIfAllChildrenAreHidden) {
- SetUpTestExtension();
- CallAPI("create({title: 'item1', visible: false});");
- CallAPI("create({title: 'item2', visible: false});");
- CallAPI("create({title: 'item3', visible: false});");
-
- ASSERT_TRUE(SetupTopLevelMenuModel());
-
- VerifyNumContextMenuItems(3);
-
- VerifyMenuItem(extension()->name(), top_level_model_, top_level_index(),
- ui::MenuModel::TYPE_SUBMENU, true);
-
- // Since the extension submenu is shown, the previous separator should be in
- // the model.
- EXPECT_EQ(ui::MenuModel::TYPE_SEPARATOR,
- top_level_model_->GetTypeAt(top_level_index() - 1));
-
- ui::MenuModel* submodel =
- top_level_model_->GetSubmenuModelAt(top_level_index());
- ASSERT_TRUE(submodel);
- EXPECT_EQ(3, submodel->GetItemCount());
-
- VerifyMenuItem("item1", submodel, 0, ui::MenuModel::TYPE_COMMAND, false);
- VerifyMenuItem("item2", submodel, 1, ui::MenuModel::TYPE_COMMAND, false);
- VerifyMenuItem("item3", submodel, 2, ui::MenuModel::TYPE_COMMAND, false);
-}
-
-// Tests showing a top-level parent menu item, when some of its child items are
-// visible. The child items' visibilities are tested as well. Recall that a
-// top-level item can be either a parent item specified by the developer or
-// parent item labeled with the extension's name. In this case, we test the
-// latter.
-//
-// Also, this tests that hiding a parent item should hide its children even if
-// they are set as visible.
-IN_PROC_BROWSER_TEST_F(ExtensionContextMenuApiTest,
- ShowExtensionNamedTopLevelItemIfSomeChildrenAreVisible) {
- SetUpTestExtension();
- CallAPI("create({title: 'item1'});");
- CallAPI("create({title: 'item2'});");
- CallAPI("create({title: 'item3', id: 'item3', visible: false});");
- CallAPI("create({title: 'child1', visible: true, parentId: 'item3'});");
- CallAPI("create({title: 'child2', visible: true, parentId: 'item3'});");
-
- ASSERT_TRUE(SetupTopLevelMenuModel());
-
- VerifyNumContextMenuItems(5);
-
- VerifyMenuItem(extension()->name(), top_level_model_, top_level_index(),
- ui::MenuModel::TYPE_SUBMENU, true);
-
- // Since the extension submenu is shown, the previous separator should be in
- // the model.
- EXPECT_EQ(ui::MenuModel::TYPE_SEPARATOR,
- top_level_model_->GetTypeAt(top_level_index() - 1));
-
- ui::MenuModel* submodel =
- top_level_model_->GetSubmenuModelAt(top_level_index());
- ASSERT_TRUE(submodel);
- EXPECT_EQ(3, submodel->GetItemCount());
-
- VerifyMenuItem("item1", submodel, 0, ui::MenuModel::TYPE_COMMAND, true);
- VerifyMenuItem("item2", submodel, 1, ui::MenuModel::TYPE_COMMAND, true);
- VerifyMenuItem("item3", submodel, 2, ui::MenuModel::TYPE_SUBMENU, false);
-
- ui::MenuModel* item3_submodel = submodel->GetSubmenuModelAt(2);
- ASSERT_TRUE(item3_submodel);
- EXPECT_EQ(2, item3_submodel->GetItemCount());
-
- // Though the children's internal visibility state remains unchanged, the ui
- // code will hide the children if the parent is hidden.
- VerifyMenuItem("child1", item3_submodel, 0, ui::MenuModel::TYPE_COMMAND,
- true);
- VerifyMenuItem("child2", item3_submodel, 1, ui::MenuModel::TYPE_COMMAND,
- true);
-}
-
} // namespace extensions
diff --git a/chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h b/chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h
index 9b63355..7804052 100644
--- a/chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h
+++ b/chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h
@@ -118,11 +118,6 @@
return false;
}
- // Visibility state.
- bool visible = true;
- if (create_properties.visible)
- visible = *create_properties.visible;
-
// Checked state.
bool checked = false;
if (create_properties.checked.get())
@@ -134,7 +129,7 @@
enabled = *create_properties.enabled;
std::unique_ptr<MenuItem> item(
- new MenuItem(item_id, title, checked, visible, enabled, type, contexts));
+ new MenuItem(item_id, title, checked, enabled, type, contexts));
// URL Patterns.
if (!item->PopulateURLPatterns(
@@ -224,10 +219,6 @@
}
}
- // Visibility state.
- if (update_properties.visible)
- item->set_visible(*update_properties.visible);
-
// Enabled.
if (update_properties.enabled.get())
item->set_enabled(*update_properties.enabled);
diff --git a/chrome/browser/extensions/context_menu_matcher.cc b/chrome/browser/extensions/context_menu_matcher.cc
index 7b6a229c..fdda6a9 100644
--- a/chrome/browser/extensions/context_menu_matcher.cc
+++ b/chrome/browser/extensions/context_menu_matcher.cc
@@ -74,7 +74,8 @@
// If this is the first extension-provided menu item, and there are other
// items in the menu, and the last item is not a separator add a separator.
- bool prepend_separator = *index == 0 && menu_model_->GetItemCount();
+ if (*index == 0 && menu_model_->GetItemCount())
+ menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
// Extensions (other than platform apps) are only allowed one top-level slot
// (and it can't be a radio or checkbox item because we are going to put the
@@ -84,8 +85,6 @@
// Otherwise, we automatically push them into a submenu if there is more than
// one top-level item.
if (extension->is_platform_app() || is_action_menu) {
- if (prepend_separator)
- menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
RecursivelyAppendExtensionItems(items,
can_cross_incognito,
selection_text,
@@ -99,19 +98,9 @@
MenuItem::List submenu_items;
if (items.size() > 1 || items[0]->type() != MenuItem::NORMAL) {
- if (prepend_separator)
- menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
title = base::UTF8ToUTF16(extension->name());
submenu_items = items;
} else {
- // The top-level menu item, |item[0]|, is sandwiched between two menu
- // separators. If the top-level menu item is visible, its preceding
- // separator should be included in the UI model, so that both separators
- // are shown. Otherwise if the top-level menu item is hidden, the
- // preceding separator should be excluded, so that only one of the two
- // separators remain.
- if (prepend_separator && items[0]->visible())
- menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
MenuItem* item = items[0];
extension_item_map_[menu_id] = item->id();
title = item->TitleWithReplacement(selection_text,
@@ -171,20 +160,6 @@
return item->checked();
}
-bool ContextMenuMatcher::IsCommandIdVisible(int command_id) const {
- MenuItem* item = GetExtensionMenuItem(command_id);
- // The context menu code creates a top-level menu item, labeled with the
- // extension's name, that is a container of an extension's menu items. This
- // top-level menu item is not added to the context menu, so checking its
- // visibility is a special case handled below. This top-level menu item should
- // always be displayed.
- if (command_id == IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST && !item)
- return true;
- if (!item)
- return false;
- return item->visible();
-}
-
bool ContextMenuMatcher::IsCommandIdEnabled(int command_id) const {
MenuItem* item = GetExtensionMenuItem(command_id);
if (!item)
@@ -255,7 +230,7 @@
bool is_action_menu_top_level) {
MenuItem::Type last_type = MenuItem::NORMAL;
int radio_group_id = 1;
- int num_visible_items = 0;
+ int num_items = 0;
for (auto i = items.begin(); i != items.end(); ++i) {
MenuItem* item = *i;
@@ -274,12 +249,11 @@
// items will not be placed in a submenu.
const int top_level_limit = api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT;
if (menu_id >= extensions_context_custom_last ||
- (is_action_menu_top_level && num_visible_items >= top_level_limit))
+ (is_action_menu_top_level && num_items >= top_level_limit))
return;
++(*index);
- if (item->visible())
- ++num_visible_items;
+ ++num_items;
extension_item_map_[menu_id] = item->id();
base::string16 title = item->TitleWithReplacement(selection_text,
diff --git a/chrome/browser/extensions/context_menu_matcher.h b/chrome/browser/extensions/context_menu_matcher.h
index ce478e0..90f6836 100644
--- a/chrome/browser/extensions/context_menu_matcher.h
+++ b/chrome/browser/extensions/context_menu_matcher.h
@@ -64,7 +64,6 @@
const base::string16& selection_text);
bool IsCommandIdChecked(int command_id) const;
- bool IsCommandIdVisible(int command_id) const;
bool IsCommandIdEnabled(int command_id) const;
void ExecuteCommand(int command_id,
content::WebContents* web_contents,
@@ -73,7 +72,6 @@
private:
friend class ::ExtensionContextMenuBrowserTest;
- friend class ExtensionContextMenuApiTest;
bool GetRelevantExtensionTopLevelItems(
const MenuItem::ExtensionKey& extension_key,
diff --git a/chrome/browser/extensions/extension_context_menu_model.cc b/chrome/browser/extensions/extension_context_menu_model.cc
index 1b63eab0..0dcd188 100644
--- a/chrome/browser/extensions/extension_context_menu_model.cc
+++ b/chrome/browser/extensions/extension_context_menu_model.cc
@@ -176,19 +176,6 @@
return false;
}
-bool ExtensionContextMenuModel::IsCommandIdVisible(int command_id) const {
- const Extension* extension = GetExtension();
- if (!extension)
- return false;
- if (command_id >= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST &&
- command_id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST) {
- return extension_items_->IsCommandIdVisible(command_id);
- }
-
- // Standard menu items are always visible.
- return true;
-}
-
bool ExtensionContextMenuModel::IsCommandIdEnabled(int command_id) const {
const Extension* extension = GetExtension();
if (!extension)
diff --git a/chrome/browser/extensions/extension_context_menu_model.h b/chrome/browser/extensions/extension_context_menu_model.h
index 3f7c704b..a6fdefc 100644
--- a/chrome/browser/extensions/extension_context_menu_model.h
+++ b/chrome/browser/extensions/extension_context_menu_model.h
@@ -78,7 +78,6 @@
// SimpleMenuModel::Delegate:
bool IsCommandIdChecked(int command_id) const override;
- bool IsCommandIdVisible(int command_id) const override;
bool IsCommandIdEnabled(int command_id) const override;
void ExecuteCommand(int command_id, int event_flags) override;
diff --git a/chrome/browser/extensions/extension_context_menu_model_unittest.cc b/chrome/browser/extensions/extension_context_menu_model_unittest.cc
index aa521b4..93b27ce 100644
--- a/chrome/browser/extensions/extension_context_menu_model_unittest.cc
+++ b/chrome/browser/extensions/extension_context_menu_model_unittest.cc
@@ -57,10 +57,6 @@
// Label for test extension menu item.
const char* kTestExtensionItemLabel = "test-ext-item";
-std::string item_label() {
- return kTestExtensionItemLabel;
-}
-
class MenuBuilder {
public:
MenuBuilder(scoped_refptr<const Extension> extension,
@@ -86,28 +82,11 @@
extension_.get(),
base::MakeUnique<MenuItem>(id, kTestExtensionItemLabel,
false, // check`ed
- true, // visible
true, // enabled
MenuItem::NORMAL,
MenuItem::ContextList(context)));
}
- void SetItemVisibility(int item_id, bool visible) {
- MenuItem::Id id(false /* not incognito */,
- MenuItem::ExtensionKey(extension_->id()));
- id.uid = item_id;
-
- menu_manager_->GetItemById(id)->set_visible(visible);
- }
-
- void SetItemTitle(int item_id, const std::string& title) {
- MenuItem::Id id(false /* not incognito */,
- MenuItem::ExtensionKey(extension_->id()));
- id.uid = item_id;
-
- menu_manager_->GetItemById(id)->set_title(title);
- }
-
private:
scoped_refptr<const Extension> extension_;
Browser* browser_;
@@ -125,22 +104,12 @@
int num_items_found = 0;
int num_custom_found = 0;
for (int i = 0; i < model.GetItemCount(); ++i) {
- base::string16 actual_label = model.GetLabelAt(i);
+ if (expected_label == model.GetLabelAt(i))
+ ++num_items_found;
int command_id = model.GetCommandIdAt(i);
- // If the command id is not visible, it should not be counted.
- if (model.IsCommandIdVisible(command_id)) {
- // The last character of |expected_label| can be the item number (e.g
- // "test-ext-item" -> "test-ext-item1"). In checking that extensions items
- // have the same label |kTestExtensionItemLabel|, the specific item number
- // is ignored, [0, expected_label.size).
- if (base::StartsWith(actual_label, expected_label,
- base::CompareCase::SENSITIVE))
- ++num_items_found;
- if (command_id >= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST &&
- command_id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST) {
- ++num_custom_found;
- }
- }
+ if (command_id >= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST &&
+ command_id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST)
+ ++num_custom_found;
}
// The only custom extension items present on the menu should be those we
// added in the test.
@@ -148,25 +117,6 @@
return num_items_found;
}
-// Checks that the model has the extension items in the exact order specified by
-// |item_number|.
-void VerifyItems(const ExtensionContextMenuModel& model,
- std::vector<std::string> item_number) {
- size_t j = 0;
- for (int i = 0; i < model.GetItemCount(); i++) {
- int command_id = model.GetCommandIdAt(i);
- if (command_id >= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST &&
- command_id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST &&
- model.IsCommandIdVisible(command_id)) {
- ASSERT_LT(j, item_number.size());
- EXPECT_EQ(base::ASCIIToUTF16(item_label() + item_number[j]),
- model.GetLabelAt(i));
- j++;
- }
- }
- EXPECT_EQ(item_number.size(), j);
-}
-
} // namespace
class ExtensionContextMenuModelTest : public ExtensionServiceTestBase {
@@ -271,8 +221,6 @@
// Uninstallation should be, by default, enabled.
EXPECT_TRUE(menu.IsCommandIdEnabled(ExtensionContextMenuModel::UNINSTALL));
- // Uninstallation should always be visible.
- EXPECT_TRUE(menu.IsCommandIdVisible(ExtensionContextMenuModel::UNINSTALL));
TestManagementPolicyProvider policy_provider(
TestManagementPolicyProvider::PROHIBIT_MODIFY_STATUS);
@@ -381,7 +329,7 @@
builder.AddContextItem(MenuItem::PAGE_ACTION);
EXPECT_EQ(1, CountExtensionItems(*builder.BuildMenu()));
- // Create more page action items to test top-level menu item limitations.
+ // Create more page action items to test top level menu item limitations.
// We start at 1, so this should try to add the limit + 1.
for (int i = 0; i < api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT; ++i)
builder.AddContextItem(MenuItem::PAGE_ACTION);
@@ -391,127 +339,6 @@
CountExtensionItems(*builder.BuildMenu()));
}
-// Top-level extension actions, like 'browser_action' or 'page_action',
-// are subject to an item limit chrome.contextMenus.ACTION_MENU_TOP_LEVEL_LIMIT.
-// The test below ensures that:
-//
-// 1. The limit is respected for top-level items. In this case, we test
-// MenuItem::PAGE_ACTION.
-// 2. Adding more items than the limit are ignored; only items within the limit
-// are visible.
-// 3. Hiding items within the limit makes "extra" ones visible.
-// 4. Unhiding an item within the limit hides a visible "extra" one.
-TEST_F(ExtensionContextMenuModelTest,
- TestItemVisibilityAgainstItemLimitForTopLevelItems) {
- InitializeEmptyExtensionService();
- const Extension* extension =
- AddExtension("extension", manifest_keys::kPageAction, Manifest::INTERNAL);
-
- // Create a MenuManager for adding context items.
- MenuManager* manager = static_cast<MenuManager*>(
- MenuManagerFactory::GetInstance()->SetTestingFactoryAndUse(
- profile(), &MenuManagerFactory::BuildServiceInstanceForTesting));
- ASSERT_TRUE(manager);
-
- MenuBuilder builder(extension, GetBrowser(), manager);
-
- // There should be no extension items yet.
- EXPECT_EQ(0, CountExtensionItems(*builder.BuildMenu()));
-
- // Create more page action items to test top-level menu item limitations.
- for (int i = 1; i <= api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT; ++i) {
- builder.AddContextItem(MenuItem::PAGE_ACTION);
- builder.SetItemTitle(i, item_label().append(base::StringPrintf("%d", i)));
- }
-
- // We shouldn't go above the limit of top-level items.
- EXPECT_EQ(api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT,
- CountExtensionItems(*builder.BuildMenu()));
-
- // Add three more page actions items. This exceeds the top-level menu item
- // limit, so the three added should not be visible in the menu.
- builder.AddContextItem(MenuItem::PAGE_ACTION);
- builder.SetItemTitle(7, item_label() + "7");
-
- // By default, the additional page action items have their visibility set to
- // true. Test creating the eigth item such that it is hidden.
- builder.AddContextItem(MenuItem::PAGE_ACTION);
- builder.SetItemTitle(8, item_label() + "8");
- builder.SetItemVisibility(8, false);
-
- builder.AddContextItem(MenuItem::PAGE_ACTION);
- builder.SetItemTitle(9, item_label() + "9");
-
- std::unique_ptr<ExtensionContextMenuModel> model = builder.BuildMenu();
-
- // Ensure that the menu item limit is obeyed, meaning that the three
- // additional items are not visible in the menu.
- EXPECT_EQ(api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT,
- CountExtensionItems(*model));
- // Items 7 to 9 should not be visible in the model.
- VerifyItems(*model, {"1", "2", "3", "4", "5", "6"});
-
- // Hide the first two items.
- builder.SetItemVisibility(1, false);
- builder.SetItemVisibility(2, false);
- model = builder.BuildMenu();
-
- // Ensure that the menu item limit is obeyed.
- EXPECT_EQ(api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT,
- CountExtensionItems(*model));
- // Hiding the first two items in the model should make visible the "extra"
- // items -- items 7 and 9. Note, item 8 was set to hidden, so it should not
- // show in the model.
- VerifyItems(*model, {"3", "4", "5", "6", "7", "9"});
-
- // Unhide the eigth item.
- builder.SetItemVisibility(8, true);
- model = builder.BuildMenu();
-
- // Ensure that the menu item limit is obeyed.
- EXPECT_EQ(api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT,
- CountExtensionItems(*model));
- // The ninth item should be replaced with the eigth.
- VerifyItems(*model, {"3", "4", "5", "6", "7", "8"});
-
- // Unhide the first two items.
- builder.SetItemVisibility(1, true);
- builder.SetItemVisibility(2, true);
- model = builder.BuildMenu();
-
- EXPECT_EQ(api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT,
- CountExtensionItems(*model));
- // Unhiding the first two items should respect the menu item limit and
- // exclude the "extra" items -- items 7, 8, and 9 -- from the model.
- VerifyItems(*model, {"1", "2", "3", "4", "5", "6"});
-}
-
-// Tests that the standard menu items (e.g. uninstall, manage) are always
-// visible.
-TEST_F(ExtensionContextMenuModelTest,
- ExtensionContextMenuStandardItemsAlwaysVisible) {
- InitializeEmptyExtensionService();
- const Extension* extension =
- AddExtension("extension", manifest_keys::kPageAction, Manifest::INTERNAL);
-
- ExtensionContextMenuModel menu(extension, GetBrowser(),
- ExtensionContextMenuModel::VISIBLE, nullptr);
- EXPECT_TRUE(menu.IsCommandIdVisible(ExtensionContextMenuModel::NAME));
- EXPECT_TRUE(menu.IsCommandIdVisible(ExtensionContextMenuModel::CONFIGURE));
- EXPECT_TRUE(
- menu.IsCommandIdVisible(ExtensionContextMenuModel::TOGGLE_VISIBILITY));
- EXPECT_TRUE(menu.IsCommandIdVisible(ExtensionContextMenuModel::UNINSTALL));
- EXPECT_TRUE(menu.IsCommandIdVisible(ExtensionContextMenuModel::MANAGE));
- EXPECT_TRUE(
- menu.IsCommandIdVisible(ExtensionContextMenuModel::INSPECT_POPUP));
- EXPECT_TRUE(menu.IsCommandIdVisible(
- ExtensionContextMenuModel::PAGE_ACCESS_RUN_ON_CLICK));
- EXPECT_TRUE(menu.IsCommandIdVisible(
- ExtensionContextMenuModel::PAGE_ACCESS_RUN_ON_SITE));
- EXPECT_TRUE(menu.IsCommandIdVisible(
- ExtensionContextMenuModel::PAGE_ACCESS_RUN_ON_ALL_SITES));
-}
-
// Test that the "show" and "hide" menu items appear correctly in the extension
// context menu.
TEST_F(ExtensionContextMenuModelTest, ExtensionContextMenuShowAndHide) {
diff --git a/chrome/browser/extensions/menu_manager.cc b/chrome/browser/extensions/menu_manager.cc
index e62e6b7..ee7b375 100644
--- a/chrome/browser/extensions/menu_manager.cc
+++ b/chrome/browser/extensions/menu_manager.cc
@@ -60,7 +60,6 @@
const char kTargetURLPatternsKey[] = "target_url_patterns";
const char kTitleKey[] = "title";
const char kTypeKey[] = "type";
-const char kVisibleKey[] = "visible";
void SetIdKeyValue(base::DictionaryValue* properties,
const char* key,
@@ -124,7 +123,6 @@
MenuItem::MenuItem(const Id& id,
const std::string& title,
bool checked,
- bool visible,
bool enabled,
Type type,
const ContextList& contexts)
@@ -132,7 +130,6 @@
title_(title),
type_(type),
checked_(checked),
- visible_(visible),
enabled_(enabled),
contexts_(contexts) {}
@@ -212,7 +209,6 @@
if (type_ == CHECKBOX || type_ == RADIO)
value->SetBoolean(kCheckedKey, checked_);
value->SetBoolean(kEnabledKey, enabled_);
- value->SetBoolean(kVisibleKey, visible_);
value->Set(kContextsKey, contexts_.ToValue());
if (parent_id_) {
DCHECK_EQ(0, parent_id_->uid);
@@ -246,12 +242,6 @@
!value.GetBoolean(kCheckedKey, &checked)) {
return nullptr;
}
- // The ability to toggle a menu item's visibility was introduced in M62, so it
- // is expected that the kVisibleKey will not be present in older menu items in
- // storage. Thus, we do not return nullptr if the kVisibleKey is not found.
- // TODO(catmullings): Remove this in M65 when all prefs should be migrated.
- bool visible = true;
- value.GetBoolean(kVisibleKey, &visible);
bool enabled = true;
if (!value.GetBoolean(kEnabledKey, &enabled))
return nullptr;
@@ -262,8 +252,8 @@
if (!contexts.Populate(*contexts_value))
return nullptr;
- std::unique_ptr<MenuItem> result = base::MakeUnique<MenuItem>(
- id, title, checked, visible, enabled, type, contexts);
+ std::unique_ptr<MenuItem> result =
+ base::MakeUnique<MenuItem>(id, title, checked, enabled, type, contexts);
std::vector<std::string> document_url_patterns;
if (!GetStringList(value, kDocumentURLPatternsKey, &document_url_patterns))
diff --git a/chrome/browser/extensions/menu_manager.h b/chrome/browser/extensions/menu_manager.h
index 6a5c6e6..502fb85b 100644
--- a/chrome/browser/extensions/menu_manager.h
+++ b/chrome/browser/extensions/menu_manager.h
@@ -163,7 +163,6 @@
MenuItem(const Id& id,
const std::string& title,
bool checked,
- bool visible,
bool enabled,
Type type,
const ContextList& contexts);
@@ -182,7 +181,6 @@
const ContextList& contexts() const { return contexts_; }
Type type() const { return type_; }
bool checked() const { return checked_; }
- bool visible() const { return visible_; }
bool enabled() const { return enabled_; }
const URLPatternSet& document_url_patterns() const {
return document_url_patterns_;
@@ -195,7 +193,6 @@
void set_title(const std::string& new_title) { title_ = new_title; }
void set_contexts(ContextList contexts) { contexts_ = contexts; }
void set_type(Type type) { type_ = type; }
- void set_visible(bool visible) { visible_ = visible; }
void set_enabled(bool enabled) { enabled_ = enabled; }
void set_document_url_patterns(const URLPatternSet& patterns) {
document_url_patterns_ = patterns;
@@ -255,9 +252,6 @@
// This should only be true for items of type CHECKBOX or RADIO.
bool checked_;
- // If the item is visible (shown or hidden) in the menu.
- bool visible_;
-
// If the item is enabled or not.
bool enabled_;
diff --git a/chrome/browser/extensions/menu_manager_unittest.cc b/chrome/browser/extensions/menu_manager_unittest.cc
index 434762c..973852c 100644
--- a/chrome/browser/extensions/menu_manager_unittest.cc
+++ b/chrome/browser/extensions/menu_manager_unittest.cc
@@ -70,8 +70,7 @@
const MenuItem::ExtensionKey key(extension->id());
MenuItem::Id id(incognito, key);
id.uid = next_id_++;
- return base::MakeUnique<MenuItem>(id, "test", false, true, true, type,
- contexts);
+ return base::MakeUnique<MenuItem>(id, "test", false, true, type, contexts);
}
// Returns a test item with the given string ID.
@@ -82,8 +81,7 @@
const MenuItem::ExtensionKey key(extension->id());
MenuItem::Id id(false, key);
id.string_uid = string_id;
- return base::MakeUnique<MenuItem>(id, "test", false, true, true, type,
- contexts);
+ return base::MakeUnique<MenuItem>(id, "test", false, true, type, contexts);
}
// Creates and returns a test Extension. The caller does *not* own the return
@@ -227,7 +225,6 @@
int type = MenuItem::CHECKBOX;
std::string title("TITLE");
bool checked = true;
- bool visible = true;
bool enabled = true;
MenuItem::ContextList contexts;
contexts.Add(MenuItem::PAGE);
@@ -249,7 +246,6 @@
value.SetInteger("type", type);
value.SetString("title", title);
value.SetBoolean("checked", checked);
- value.SetBoolean("visible", visible);
value.SetBoolean("enabled", enabled);
value.SetInteger("contexts", contexts_value);
std::string error;
@@ -271,7 +267,6 @@
EXPECT_EQ(title, item->title());
EXPECT_EQ(checked, item->checked());
EXPECT_EQ(item->checked(), item->checked());
- EXPECT_EQ(visible, item->visible());
EXPECT_EQ(enabled, item->enabled());
EXPECT_EQ(contexts, item->contexts());
