Revert "Revert "[Extensions] Added visibility property to chrome.contextMenu items API""
This reverts commit e83bf884e74a3a911191788bca9d1137631810a4.
Reason for revert:
Per discussion with dschuyler@. FindIt (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) mistook the Win7 webkit build failure to be caused by CL 553578. CL 553578 adds a manifest.json, and there's a MANIFEST.json in the failure log. See the Heuristic Hints on the page.
However, the failure (https://chromium-swarm.appspot.com/task?id=3822bb5befc13310&refresh=10&show_raw=1) is referring to a different manifest.json (third_party\WebKit\LayoutTests\external\WPT_BASE_MANIFEST.json) and the webkit layout script is eventually able to generate that particular manifest.json. The Win7 Webkit layout test is failing because the script is "Unable to find test driver" for content_shell.exe and content_shell_crash_service.exe, which is unrelated to CL 553578.
Original change's description:
> 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}
[email protected],[email protected],[email protected],[email protected]
Change-Id: I7324bd25d6e1271caa752c442453c6328ac4ffd0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 463476
Reviewed-on: https://chromium-review.googlesource.com/627259
Reviewed-by: Dave Schuyler <[email protected]>
Commit-Queue: catmullings <[email protected]>
Cr-Commit-Position: refs/heads/master@{#496464}diff --git a/chrome/browser/extensions/context_menu_matcher.cc b/chrome/browser/extensions/context_menu_matcher.cc
index fdda6a9..7b6a229c 100644
--- a/chrome/browser/extensions/context_menu_matcher.cc
+++ b/chrome/browser/extensions/context_menu_matcher.cc
@@ -74,8 +74,7 @@
// 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.
- if (*index == 0 && menu_model_->GetItemCount())
- menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
+ bool prepend_separator = *index == 0 && menu_model_->GetItemCount();
// 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
@@ -85,6 +84,8 @@
// 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,
@@ -98,9 +99,19 @@
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,
@@ -160,6 +171,20 @@
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)
@@ -230,7 +255,7 @@
bool is_action_menu_top_level) {
MenuItem::Type last_type = MenuItem::NORMAL;
int radio_group_id = 1;
- int num_items = 0;
+ int num_visible_items = 0;
for (auto i = items.begin(); i != items.end(); ++i) {
MenuItem* item = *i;
@@ -249,11 +274,12 @@
// 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_items >= top_level_limit))
+ (is_action_menu_top_level && num_visible_items >= top_level_limit))
return;
++(*index);
- ++num_items;
+ if (item->visible())
+ ++num_visible_items;
extension_item_map_[menu_id] = item->id();
base::string16 title = item->TitleWithReplacement(selection_text,
