Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
359493005:
Extend contextMenus API to support browser/page actions (Closed)
DescriptionExtend contextMenus API to support browser/page actions
Screenshot:
http://i.imgur.com/RILqQqe.png
BUG=234425
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288418
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289211
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added context strings, page action implementation #
Total comments: 31
Patch Set 3 : Addressed yoyo's comments, restricted items to relevant extension #
Total comments: 41
Patch Set 4 : Minor changes #
Total comments: 10
Patch Set 5 : Moar minor changes #
Total comments: 12
Patch Set 6 : Minor changes and cleanup #
Total comments: 24
Patch Set 7 : Minor cleanup #
Total comments: 14
Patch Set 8 : Moar minor cleanup #
Total comments: 2
Patch Set 9 : Removed icons #
Total comments: 6
Patch Set 10 : Removed AppendExtensionItemsImpl, minor cleanup, comments #
Total comments: 16
Patch Set 11 : Removed submenu for action menus, removed unnecessary append logic #Patch Set 12 : Limited top level extension items #Patch Set 13 : Added unit test, created limit constant #Patch Set 14 : Small changes, formatting #
Total comments: 20
Patch Set 15 : Comments, naming, small changes #
Total comments: 11
Patch Set 16 : Webview checks #
Total comments: 4
Patch Set 17 : Small fixes #
Total comments: 40
Patch Set 18 : Testing cleanup #Patch Set 19 : Moved BuildInstance to unit test #
Total comments: 1
Patch Set 20 : TestingFactoryFunction documentation #
Total comments: 12
Patch Set 21 : Cleanup, minor unittest refactor #
Total comments: 7
Patch Set 22 : Created BuildServiceInstanceForTesting for menu manager #
Total comments: 3
Patch Set 23 : Revert loop change in render_view_context_menu.cc #
Total comments: 4
Patch Set 24 : Fix android compile error #
Total comments: 1
Patch Set 25 : Replaced static initializer #
Total comments: 1
Patch Set 26 : Rebase #Messages
Total messages: 115 (0 generated)
Hey Kalman, I went ahead and implemented the browser actions part of this request. It's not complete for page actions yet, but I figured I'd upload my progress so far. Can you take a look at it and see what you think?
Yoyo is more familiar with the context menus stuff, so I'll let him review. Awesome fast work btw, but I can't tell how the extension is actually populating these items..? there should at least be changes like adding a "browser_action" and "page_action" contexts here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
On 2014/06/26 14:44:57, kalman wrote: > Yoyo is more familiar with the context menus stuff, so I'll let him review. > > Awesome fast work btw, but I can't tell how the extension is actually populating > these items..? there should at least be changes like adding a "browser_action" > and "page_action" contexts here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... Thanks! I'll add some comments to outline some of the questions I had. These are directed to whoever is reviewing (which appears to be Yoyo).
https://codereview.chromium.org/359493005/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:128: extension_items_.ExecuteCommand(command_id, web_contents, *params); I had to remove params when I was porting over all this code, which I figure isn't an issue. Here I added a blank params to be passed into ExecuteCommand. The code appeared to accommodate for any null values and had default actions. From what I understood, params contain all the information relevant to a right click, like what was clicked on, whether there was selected text, etc. I figured since this would always be a predictable context from which the menu is created, that information is irrelevant (ie, we're always clicking on an extension icon in the top right). Let me know if this isn't the proper way to approach this. If we should in fact have params here, it should be relatively simple to inject the values we need into params before it's passed in, if any values are necessary for this context. https://codereview.chromium.org/359493005/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:267: UMA_HISTOGRAM_COUNTS("Extensions.ContextMenus_ItemCount", index); I wanted to get some clarification on that to make sure I wasn't doing it wrong here with the histogram stats, so I left it commented out. Most of this code is borrowed from RenderViewContextMenu, which was my model for a context menu that implements the ContextMenuMatcher, the functionality of which I added into ExtensionContextMenuModel to support extension menu items. Some direction on how histogram times work and how to add this in properly would be appreciated. https://codereview.chromium.org/359493005/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:278: return true; I haven't added a browser_context string yet-- instead I had it show up in the browser context menu by default (this function also needs to be renamed-- I forgot to do that when I borrowed the prototype from another class). This function is the filter for menu items, and it currently returns true by default, adding any items to the menu. The commented out section shows what will be added once I had that string to the JS.
Hey Yoyo, can I get some feedback on this patch? I just extended the functionality to support page actions, and I added in the necessary context strings in the JSON. Thanks!
One high-level comment: tabs and other browser UI surfaces don't belong to a particular extension, so any extension can add context menu items to them. Sure. But browser actions and page actions do belong to a particular extension. I think we should only let extensions add items to their own context menus... https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:34: #include "ui/gfx/text_elider.h" Is this used? https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:201: AddSeparator(ui::NORMAL_SEPARATOR); We don't want to do this if there are no extension-provided items. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:227: base::string16 *null_string = new base::string16(); This is a leak. Use EmptyString16 from string_util. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:229: // Get a list of extension id's that have context menu items, and sort by the Wait a minute. Here's my understanding of the world: ExtensionContextMenuModel is created when you right-click on an extension surface (e.g. a page action). Does it make sense for an extension to add context menu items for other extension's items? Arguably, I would say this should only add items from our own extension_. But if you have a compelling reason otherwise, please say so. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:241: if (extension && !extension->is_platform_app()) { I don't think platform apps can have extension UI surfaces that ExtensionContextMenuModel gets created on. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:263: UMA_HISTOGRAM_TIMES("Extensions.ContextMenus_BuildTime", You can look at the comments in base/metrics/histogram.h to understand the histograms here. We probably want to use new metric names, though, since these are different. How about Extensions.ActionContextMenus_BuildTime Extensions.ActionContextMenus_ItemCount ? https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:268: ExtensionContextMenuModel::ActionType ExtensionContextMenuModel::GetActionType( This is not a good style. A name like "GetActionType" suggests that it's a const function, whereas this has the side effect of setting extension_action_. It'd be better for this to be folded into InitMenu. There's no particular need to initialize action_type_ and extension_items_ in the initializer list. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/extension_context_menu_model.h (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.h:40: enum ActionType { This should definitely have a default 'none' (NO_ACTION) - there are context menu models created for non-actions, e.g. infobars. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.h:110: // Menu matcher for extensions context menu items. This should be more specific - these are the ones specified by the extension. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.h:113: void AppendExtensionItems(); Private functions should go above members. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/common/ext... File chrome/common/extensions/api/web_view_internal.json (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/common/ext... chrome/common/extensions/api/web_view_internal.json:127: "enum": ["all", "page", "frame", "selection", "link", "editable", "image", "video", "audio", "launcher", "browser_action", "page_action"] I don't think these make sense for webviews.
Thanks for the review! I appreciate the feedback. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:97: break; This is the reason I added the browser_action and page_action strings to web_views_internal. I could probably add logic in here to determine what PropertyWithEnumT is at the time this function is called, and only accommodate for these cases if it's context_menu and not web_views_internal. Seems like a messy thing to do to such a clean function though. What do you think? https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:34: #include "ui/gfx/text_elider.h" Ah, no, it isn't. I had it included because of a TruncateString call that ended up not being necessary. I'll take care of that. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:201: AddSeparator(ui::NORMAL_SEPARATOR); I'll go ahead and put the AddSeparator call into AppendExtensionItems. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:227: base::string16 *null_string = new base::string16(); Oh, wonderful. I was wondering what I should use as a null string. Thanks. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:229: // Get a list of extension id's that have context menu items, and sort by the I wasn't sure which way to do it, and since the change is minimal, I just chose to do it this way. I imagine situations in which an extension might want to interact with another extension, so its context menu item would then show up on all extensions' context menus instead of just its own. But if we don't need to accommodate for anything like that, then no problem. I can make this only add items from the extension that was clicked on. Should be a simple modification to the filter function. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:263: UMA_HISTOGRAM_TIMES("Extensions.ContextMenus_BuildTime", Sounds good to me. I'll take a look at that file and add these new metrics in. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/extension_context_menu_model.h (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.h:40: enum ActionType { I was under the impression that this class was specifically for context menus generated by right clicking on the extension icons for browser and page actions. Is this class also used for infobar context menus? I'll take another look at it. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/common/ext... File chrome/common/extensions/api/web_view_internal.json (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/common/ext... chrome/common/extensions/api/web_view_internal.json:127: "enum": ["all", "page", "frame", "selection", "link", "editable", "image", "video", "audio", "launcher", "browser_action", "page_action"] I added these here because extra logic would need to be added for differentiation if these strings are missing from web_view_internal, but present in context_menus. See my comment in context_menus_api_helpers.h. Could you briefly explain why this doesn't make sense? I'm not sure I understand webviews.
https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:268: ExtensionContextMenuModel::ActionType ExtensionContextMenuModel::GetActionType( Oops-- I meant to address this comment but I must not have saved the draft. I added this function because action_type_ needs to be initialized before extension_items_ because it's necessary for the fourth parameter, the filter function (MenuItemMatchesContext). But if I have it set up in InitMenu, then I can construct extension_items_ afterwards and this function will no longer be necessary.
https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:229: // Get a list of extension id's that have context menu items, and sort by the On 2014/06/28 00:05:41, gpdavis wrote: > I wasn't sure which way to do it, and since the change is minimal, I just chose > to do it this way. I imagine situations in which an extension might want to > interact with another extension, so its context menu item would then show up on > all extensions' context menus instead of just its own. But if we don't need to > accommodate for anything like that, then no problem. > > I can make this only add items from the extension that was clicked on. Should > be a simple modification to the filter function. Yes, please. I don't think it makes sense for an extension to act on an arbitrary other extension that you choose by its browser/page action.
On 2014/06/28 00:29:45, Yoyo Zhou (OOO till July 6) wrote: > https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... > File chrome/browser/extensions/extension_context_menu_model.cc (right): > > https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... > chrome/browser/extensions/extension_context_menu_model.cc:229: // Get a list of > extension id's that have context menu items, and sort by the > On 2014/06/28 00:05:41, gpdavis wrote: > > I wasn't sure which way to do it, and since the change is minimal, I just > chose > > to do it this way. I imagine situations in which an extension might want to > > interact with another extension, so its context menu item would then show up > on > > all extensions' context menus instead of just its own. But if we don't need > to > > accommodate for anything like that, then no problem. > > > > I can make this only add items from the extension that was clicked on. Should > > be a simple modification to the filter function. > > Yes, please. I don't think it makes sense for an extension to act on an > arbitrary other extension that you choose by its browser/page action. Indeed. Not only that, but we don't need attribution on the browser action icons either. It should be similar to the launcher's support for the context menus API.
On 2014/06/30 18:10:46, kalman wrote: > Indeed. Not only that, but we don't need attribution on the browser action icons > either. It should be similar to the launcher's support for the context menus > API. What do you mean by attribution on the browser action icons?
Went ahead and took care of most of these issues. @kalman, if yoyo is OOO, who should I have review this in the meantime? https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:34: #include "ui/gfx/text_elider.h" On 2014/06/28 00:05:41, gpdavis wrote: > Ah, no, it isn't. I had it included because of a TruncateString call that ended > up not being necessary. I'll take care of that. Done. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:201: AddSeparator(ui::NORMAL_SEPARATOR); On 2014/06/28 00:05:41, gpdavis wrote: > I'll go ahead and put the AddSeparator call into AppendExtensionItems. Done. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:227: base::string16 *null_string = new base::string16(); On 2014/06/28 00:05:41, gpdavis wrote: > Oh, wonderful. I was wondering what I should use as a null string. Thanks. Done. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:229: // Get a list of extension id's that have context menu items, and sort by the On 2014/06/28 00:29:44, Yoyo Zhou (OOO till July 6) wrote: > On 2014/06/28 00:05:41, gpdavis wrote: > > I wasn't sure which way to do it, and since the change is minimal, I just > chose > > to do it this way. I imagine situations in which an extension might want to > > interact with another extension, so its context menu item would then show up > on > > all extensions' context menus instead of just its own. But if we don't need > to > > accommodate for anything like that, then no problem. > > > > I can make this only add items from the extension that was clicked on. Should > > be a simple modification to the filter function. > > Yes, please. I don't think it makes sense for an extension to act on an > arbitrary other extension that you choose by its browser/page action. Done. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:241: if (extension && !extension->is_platform_app()) { On 2014/06/27 23:37:57, Yoyo Zhou (OOO till July 6) wrote: > I don't think platform apps can have extension UI surfaces that > ExtensionContextMenuModel gets created on. Done. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:263: UMA_HISTOGRAM_TIMES("Extensions.ContextMenus_BuildTime", On 2014/06/28 00:05:41, gpdavis wrote: > Sounds good to me. I'll take a look at that file and add these new metrics in. Done. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.cc:268: ExtensionContextMenuModel::ActionType ExtensionContextMenuModel::GetActionType( On 2014/06/28 00:27:02, gpdavis wrote: > Oops-- I meant to address this comment but I must not have saved the draft. > > I added this function because action_type_ needs to be initialized before > extension_items_ because it's necessary for the fourth parameter, the filter > function (MenuItemMatchesContext). But if I have it set up in InitMenu, then I > can construct extension_items_ afterwards and this function will no longer be > necessary. Done. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... File chrome/browser/extensions/extension_context_menu_model.h (right): https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.h:40: enum ActionType { Ah, you're right. The second constructor is called for infobars. I'll add the default action in and make sure to accommodate for this case. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.h:110: // Menu matcher for extensions context menu items. On 2014/06/27 23:37:57, Yoyo Zhou (OOO till July 6) wrote: > This should be more specific - these are the ones specified by the extension. Done. https://chromiumcodereview.appspot.com/359493005/diff/40001/chrome/browser/ex... chrome/browser/extensions/extension_context_menu_model.h:113: void AppendExtensionItems(); On 2014/06/27 23:37:57, Yoyo Zhou (OOO till July 6) wrote: > Private functions should go above members. Done.
For a change like this, it'd be really great to see some screenshots :) https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:122: content::ContextMenuParams *params = new content::ContextMenuParams(); content::ContextMenuParams* params (note asterisk placement). https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:122: content::ContextMenuParams *params = new content::ContextMenuParams(); Gah! Leak! https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:195: extension_items_.reset(new extensions::ContextMenuMatcher(profile_, nit: fix param wrapping. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:223: void ExtensionContextMenuModel::AppendExtensionItems() { The fact that 90% of this is verbatim copied code makes me wonder if we shouldn't have it in a common location... but we'll worry about that later. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:234: // Get a list of extension id's that have context menu items, and sort by the extension ids, not extension id's. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:239: for (std::set<MenuItem::ExtensionKey>::iterator i = ids.begin(); nit: I don't know why, but we seem to have the pattern of having iterators be called iter or it (I prefer iter), and only using i as an integer or size_t https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:243: service->GetExtensionById(i->extension_id, false); ExtensionService::GetExtensionById is deprecated. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:256: const std::string app_locale = g_browser_process->GetApplicationLocale(); Make this a const &, not just const. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:265: extension_key, base::EmptyString16(), &index); See comment for base::EmptyString16 where its declared. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:268: UMA_HISTOGRAM_TIMES("Extensions.ActionContextMenus_BuildTime", If you don't add these to histograms.xml, we'll never see the data. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:11: #include "chrome/browser/extensions/context_menu_matcher.h" Nit: prefer forward declaration instead of includes. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:13: #include "chrome/browser/extensions/menu_manager.h" same here. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:40: enum ActionType { Comment. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:92: void AppendExtensionItems(); Comment. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:99: static bool MenuItemMatchesExtension(ActionType type, This should probably be an anonymous function in the .cc. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/menu_manager.cc (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/menu_manager.cc:664: if (params.extension_id.length() > 0) nit: prefer "!params.extension_id.empty()" https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/menu_manager.cc:665: properties->SetString("extension_id", params.extension_id); it looks like we prefer camelCaseStyle for these keys. https://codereview.chromium.org/359493005/diff/60001/content/public/common/co... File content/public/common/context_menu_params.h (right): https://codereview.chromium.org/359493005/diff/60001/content/public/common/co... content/public/common/context_menu_params.h:144: std::string extension_id; This is a layering violation - src/content should have no knowledge of extensions.
https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:122: content::ContextMenuParams *params = new content::ContextMenuParams(); On 2014/07/01 20:54:24, Devlin wrote: > Gah! Leak! I'm ashamed of all the leaks you've found in my code. Nice catch. I'll make a private params_. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:195: extension_items_.reset(new extensions::ContextMenuMatcher(profile_, On 2014/07/01 20:54:24, Devlin wrote: > nit: fix param wrapping. Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:234: // Get a list of extension id's that have context menu items, and sort by the On 2014/07/01 20:54:24, Devlin wrote: > extension ids, not extension id's. Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:239: for (std::set<MenuItem::ExtensionKey>::iterator i = ids.begin(); On 2014/07/01 20:54:24, Devlin wrote: > nit: I don't know why, but we seem to have the pattern of having iterators be > called iter or it (I prefer iter), and only using i as an integer or size_t Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:243: service->GetExtensionById(i->extension_id, false); On 2014/07/01 20:54:23, Devlin wrote: > ExtensionService::GetExtensionById is deprecated. It is? I don't see any indication of that in the comments. What should I use instead? https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:256: const std::string app_locale = g_browser_process->GetApplicationLocale(); On 2014/07/01 20:54:24, Devlin wrote: > Make this a const &, not just const. Done. Can you explain exactly why this ought to be the case here? This code was borrowed, and I didn't touch this line. Such is the case for a lot of these nits ;) https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:265: extension_key, base::EmptyString16(), &index); On 2014/07/01 20:54:23, Devlin wrote: > See comment for base::EmptyString16 where its declared. I was following yoyo's suggestion to use this. How would I go about doing this another way without running into a memory leak? I need to pass in an empty string by reference. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:268: UMA_HISTOGRAM_TIMES("Extensions.ActionContextMenus_BuildTime", On 2014/07/01 20:54:23, Devlin wrote: > If you don't add these to histograms.xml, we'll never see the data. How exactly do I add items to this file without blowing things up? https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:11: #include "chrome/browser/extensions/context_menu_matcher.h" On 2014/07/01 20:54:24, Devlin wrote: > Nit: prefer forward declaration instead of includes. Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:13: #include "chrome/browser/extensions/menu_manager.h" On 2014/07/01 20:54:24, Devlin wrote: > same here. Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:40: enum ActionType { On 2014/07/01 20:54:24, Devlin wrote: > Comment. Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:92: void AppendExtensionItems(); On 2014/07/01 20:54:24, Devlin wrote: > Comment. Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:99: static bool MenuItemMatchesExtension(ActionType type, On 2014/07/01 20:54:24, Devlin wrote: > This should probably be an anonymous function in the .cc. Done. https://codereview.chromium.org/359493005/diff/60001/content/public/common/co... File content/public/common/context_menu_params.h (right): https://codereview.chromium.org/359493005/diff/60001/content/public/common/co... content/public/common/context_menu_params.h:144: std::string extension_id; On 2014/07/01 20:54:24, Devlin wrote: > This is a layering violation - src/content should have no knowledge of > extensions. I actually think I don't need this anymore, so I'll take it out.
https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:243: service->GetExtensionById(i->extension_id, false); On 2014/07/02 01:17:46, gpdavis wrote: > On 2014/07/01 20:54:23, Devlin wrote: > > ExtensionService::GetExtensionById is deprecated. > > It is? I don't see any indication of that in the comments. > > What should I use instead? Huh. There should be a comment there. Instead, use ExtensionRegistry::enabled_extensions().GetByID(id); https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:256: const std::string app_locale = g_browser_process->GetApplicationLocale(); On 2014/07/02 01:17:46, gpdavis wrote: > On 2014/07/01 20:54:24, Devlin wrote: > > Make this a const &, not just const. > > Done. Can you explain exactly why this ought to be the case here? This code > was borrowed, and I didn't touch this line. Such is the case for a lot of these > nits ;) I believe it :) There's no guarantee that the code existing in the codebase is the best code. The reason to have it be a const & is to avoid having to make a copy of it. Since you only persist it for the current function, you can guarantee it won't go away, and a const & is fine and saves a little bit of time. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:265: extension_key, base::EmptyString16(), &index); On 2014/07/02 01:17:46, gpdavis wrote: > On 2014/07/01 20:54:23, Devlin wrote: > > See comment for base::EmptyString16 where its declared. > > I was following yoyo's suggestion to use this. How would I go about doing this > another way without running into a memory leak? I need to pass in an empty > string by reference. Yeah, we should only use base::EmptyString[16] when we need to be sure the reference isn't gonna go anywhere. In this case, you're just passing in a string by const &, and it only does that to avoid copying it (as you want to do above). So for this, it's perfectly fine to just pass in base::string16(), as in: extension_items_->AppendExtensionItems( extension_key, base::string16(), &index); https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:268: UMA_HISTOGRAM_TIMES("Extensions.ActionContextMenus_BuildTime", On 2014/07/02 01:17:46, gpdavis wrote: > On 2014/07/01 20:54:23, Devlin wrote: > > If you don't add these to histograms.xml, we'll never see the data. > > How exactly do I add items to this file without blowing things up? Did something blow up? Huh... usually that doesn't happen ;) You should be able to just add a new histogram definition along with the rest (in alphabetical order), and it should be fine. If it's whining that it's not pretty-printed you could a) try to find it yourself out of stubbornness (my approach), or b) do the smart thing and run the pretty_print.py script in the same folder. If it's not compiling anymore, it likely just means that a bracket or quote is mismatched somewhere. If it's something else, then I dunno - I'd have to see the explosion. :) https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:47: static bool MenuItemMatchesExtension( static functions in anonymous namespace are redundant and repetitive. https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:150: extension_items_->ExecuteCommand(command_id, web_contents, params_); Are we guaranteed that extension_items_ will never, ever be NULL here? If so, can we DCHECK it? https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:115: ActionType action_type_; comment. https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:117: content::ContextMenuParams params_; No need for this to be a member variable - you only use it once.
https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:243: service->GetExtensionById(i->extension_id, false); On 2014/07/02 17:25:21, Devlin wrote: > On 2014/07/02 01:17:46, gpdavis wrote: > > On 2014/07/01 20:54:23, Devlin wrote: > > > ExtensionService::GetExtensionById is deprecated. > > > > It is? I don't see any indication of that in the comments. > > > > What should I use instead? > > Huh. There should be a comment there. > Instead, use ExtensionRegistry::enabled_extensions().GetByID(id); Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:265: extension_key, base::EmptyString16(), &index); On 2014/07/02 17:25:21, Devlin wrote: > On 2014/07/02 01:17:46, gpdavis wrote: > > On 2014/07/01 20:54:23, Devlin wrote: > > > See comment for base::EmptyString16 where its declared. > > > > I was following yoyo's suggestion to use this. How would I go about doing > this > > another way without running into a memory leak? I need to pass in an empty > > string by reference. > > Yeah, we should only use base::EmptyString[16] when we need to be sure the > reference isn't gonna go anywhere. In this case, you're just passing in a > string by const &, and it only does that to avoid copying it (as you want to do > above). So for this, it's perfectly fine to just pass in base::string16(), as > in: > extension_items_->AppendExtensionItems( > extension_key, base::string16(), &index); Done. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:268: UMA_HISTOGRAM_TIMES("Extensions.ActionContextMenus_BuildTime", On 2014/07/02 17:25:20, Devlin wrote: > On 2014/07/02 01:17:46, gpdavis wrote: > > On 2014/07/01 20:54:23, Devlin wrote: > > > If you don't add these to histograms.xml, we'll never see the data. > > > > How exactly do I add items to this file without blowing things up? > > Did something blow up? Huh... usually that doesn't happen ;) > > You should be able to just add a new histogram definition along with the rest > (in alphabetical order), and it should be fine. If it's whining that it's not > pretty-printed you could a) try to find it yourself out of stubbornness (my > approach), or b) do the smart thing and run the pretty_print.py script in the > same folder. If it's not compiling anymore, it likely just means that a bracket > or quote is mismatched somewhere. If it's something else, then I dunno - I'd > have to see the explosion. :) Well, it didn't actually blow up because I was apprehensive about adding things to it. I can't find any instances of ContextMenus_BuildTime (which was the original metric from where this code was borrowed), so I'm kind of puzzled. Not really sure how to go about adding this metric if I can't find where the old one that I'm modeling it off of is. https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:47: static bool MenuItemMatchesExtension( On 2014/07/02 17:25:21, Devlin wrote: > static functions in anonymous namespace are redundant and repetitive. Done. https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:150: extension_items_->ExecuteCommand(command_id, web_contents, params_); On 2014/07/02 17:25:21, Devlin wrote: > Are we guaranteed that extension_items_ will never, ever be NULL here? If so, > can we DCHECK it? I don't see any way it could be null. I'll DCHECK it. https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:115: ActionType action_type_; On 2014/07/02 17:25:21, Devlin wrote: > comment. I'm not sure what to say about this that isn't explained in the comment for the ActionType enum. Should I add another comment here or move the enum comment down? https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:117: content::ContextMenuParams params_; On 2014/07/02 17:25:21, Devlin wrote: > No need for this to be a member variable - you only use it once. Done.
A screenshot would also be good for this, as it introduces a UI feature. :) (Think I mentioned that in my first round of comments?) https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:268: UMA_HISTOGRAM_TIMES("Extensions.ActionContextMenus_BuildTime", On 2014/07/03 01:46:58, gpdavis wrote: > On 2014/07/02 17:25:20, Devlin wrote: > > On 2014/07/02 01:17:46, gpdavis wrote: > > > On 2014/07/01 20:54:23, Devlin wrote: > > > > If you don't add these to histograms.xml, we'll never see the data. > > > > > > How exactly do I add items to this file without blowing things up? > > > > Did something blow up? Huh... usually that doesn't happen ;) > > > > You should be able to just add a new histogram definition along with the rest > > (in alphabetical order), and it should be fine. If it's whining that it's not > > pretty-printed you could a) try to find it yourself out of stubbornness (my > > approach), or b) do the smart thing and run the pretty_print.py script in the > > same folder. If it's not compiling anymore, it likely just means that a > bracket > > or quote is mismatched somewhere. If it's something else, then I dunno - I'd > > have to see the explosion. :) > > Well, it didn't actually blow up because I was apprehensive about adding things > to it. I can't find any instances of ContextMenus_BuildTime (which was the > original metric from where this code was borrowed), so I'm kind of puzzled. Not > really sure how to go about adding this metric if I can't find where the old one > that I'm modeling it off of is. If there's no entry in histograms.xml, then we don't care about the data and aren't even gathering it. Read: clearly no one cares about this data, and we can safely remove it (and not add any analogous data) :) https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:115: ActionType action_type_; On 2014/07/03 01:46:58, gpdavis wrote: > On 2014/07/02 17:25:21, Devlin wrote: > > comment. > > I'm not sure what to say about this that isn't explained in the comment for the > ActionType enum. Should I add another comment here or move the enum comment > down? As a rule of thumb, the comment for the enum as a whole should be "This is the type of data the enum can represent" and the variable should be "This is what the enum means in this context". So if the enum is "The type of extension action an icon can have", the variable is "The type of extension action to which this context menu is attached." Sometimes its also useful to include a (very) short reason why you need it, if it's not immediately clear, but that's pretty much on a case-by-case basis. https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:47: // Matches menu items with the context menu's extension. What does "matches" mean? How about, "Returns true if the given |item| is for the given |extension| and of the given |type|." or something like that? https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:56: MenuItem::ContextList contexts = item->contexts(); While you're in the area, would you mind updating MenuItem::contexts() to return a const &? Unless there's a reason it shouldn't? https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:256: ExtensionService* service = We don't use this anymore, right? https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:273: const Extension* extension = extensions::ExtensionRegistry::Get( cache ExtensionRegistry, or better yet, enabled_extensions() before the for-loop. https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:12: #include "content/public/common/context_menu_params.h" Do we still need this? https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:22: class MenuManager; DO we still need this?
Sorry I missed that about the screenshot. Turns out you can't screenshot in ubuntu with a context menu open. I can try and find a way around that. Where would I put the screenshot, though? I don't see anywhere to upload a file. https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.cc:268: UMA_HISTOGRAM_TIMES("Extensions.ActionContextMenus_BuildTime", On 2014/07/07 20:20:57, Devlin wrote: > On 2014/07/03 01:46:58, gpdavis wrote: > > On 2014/07/02 17:25:20, Devlin wrote: > > > On 2014/07/02 01:17:46, gpdavis wrote: > > > > On 2014/07/01 20:54:23, Devlin wrote: > > > > > If you don't add these to histograms.xml, we'll never see the data. > > > > > > > > How exactly do I add items to this file without blowing things up? > > > > > > Did something blow up? Huh... usually that doesn't happen ;) > > > > > > You should be able to just add a new histogram definition along with the > rest > > > (in alphabetical order), and it should be fine. If it's whining that it's > not > > > pretty-printed you could a) try to find it yourself out of stubbornness (my > > > approach), or b) do the smart thing and run the pretty_print.py script in > the > > > same folder. If it's not compiling anymore, it likely just means that a > > bracket > > > or quote is mismatched somewhere. If it's something else, then I dunno - > I'd > > > have to see the explosion. :) > > > > Well, it didn't actually blow up because I was apprehensive about adding > things > > to it. I can't find any instances of ContextMenus_BuildTime (which was the > > original metric from where this code was borrowed), so I'm kind of puzzled. > Not > > really sure how to go about adding this metric if I can't find where the old > one > > that I'm modeling it off of is. > > If there's no entry in histograms.xml, then we don't care about the data and > aren't even gathering it. Read: clearly no one cares about this data, and we > can safely remove it (and not add any analogous data) :) Sounds good to me! I'll go ahead and remove the unused entry from the file that this code was borrowed from. Done. https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_context_menu_model.h:115: ActionType action_type_; On 2014/07/07 20:20:57, Devlin wrote: > On 2014/07/03 01:46:58, gpdavis wrote: > > On 2014/07/02 17:25:21, Devlin wrote: > > > comment. > > > > I'm not sure what to say about this that isn't explained in the comment for > the > > ActionType enum. Should I add another comment here or move the enum comment > > down? > > As a rule of thumb, the comment for the enum as a whole should be "This is the > type of data the enum can represent" and the variable should be "This is what > the enum means in this context". So if the enum is "The type of extension > action an icon can have", the variable is "The type of extension action to which > this context menu is attached." Sometimes its also useful to include a (very) > short reason why you need it, if it's not immediately clear, but that's pretty > much on a case-by-case basis. Done. https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:47: // Matches menu items with the context menu's extension. On 2014/07/07 20:20:57, Devlin wrote: > What does "matches" mean? How about, > "Returns true if the given |item| is for the given |extension| and of the given > |type|." > or something like that? Done. https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:56: MenuItem::ContextList contexts = item->contexts(); On 2014/07/07 20:20:58, Devlin wrote: > While you're in the area, would you mind updating MenuItem::contexts() to return > a const &? Unless there's a reason it shouldn't? Done. https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:256: ExtensionService* service = On 2014/07/07 20:20:57, Devlin wrote: > We don't use this anymore, right? Negative. I just put the method call into the conditional. Done. https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:273: const Extension* extension = extensions::ExtensionRegistry::Get( On 2014/07/07 20:20:57, Devlin wrote: > cache ExtensionRegistry, or better yet, enabled_extensions() before the > for-loop. Done. https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:12: #include "content/public/common/context_menu_params.h" On 2014/07/07 20:20:58, Devlin wrote: > Do we still need this? Done. https://codereview.chromium.org/359493005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:22: class MenuManager; On 2014/07/07 20:20:58, Devlin wrote: > DO we still need this? Done.
On 2014/07/09 02:13:54, gpdavis wrote: > Sorry I missed that about the screenshot. > > Turns out you can't screenshot in ubuntu with a context menu open. I can try > and find a way around that. Where would I put the screenshot, though? I don't > see anywhere to upload a file. > To take a screenshot: Use the print screen button on your keyboard. This saves a screenshot to your Pictures folder and works with a context menu open (or always has for me). I typically upload to imgur, because it's fast, easy, and free (since chrome is open source, it doesn't really matter where we upload pictures for this type of stuff). We should include support for uploading images to Rietveld, but, for some reason, we don't. The other option is to add a picture to the bug, where we do have support for attachments. Okay, on to the review...
https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:7: #include "base/metrics/histogram.h" Don't need this. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:17: #include "chrome/browser/extensions/extension_service.h" Don't need this. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:57: MenuItem::ContextList contexts = item->contexts(); Make this a const &. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:260: if (!extensions::ExtensionSystem::Get(profile_)->extension_service()) Don't need this? Still? https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:280: *iter, base::EmptyString16()); (Again), read the documentation for base::EmptyString16(). You really _don't_ need to use it here. I promise. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:294: base::TimeTicks begin = base::TimeTicks::Now(); Why do we need this? https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:295: for (size_t i = 0; i < sorted_menu_titles.size(); ++i) { prefer an iterator. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:39: // Type of action the extension icon represents. Do these actually need to be public? https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:97: ActionType GetActionType(const extensions::Extension* extension); Does this still exist? https://codereview.chromium.org/359493005/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:557: base::TimeTicks begin = base::TimeTicks::Now(); Take this out, too.
I was using Print Screen, and it would always close the context menu right before the screenshot was taken. I found out there's delay functionality, so I used that. Here's a screenshot of the browser action context menu item in the wild: http://i.imgur.com/PtaghOL.png https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:7: #include "base/metrics/histogram.h" On 2014/07/09 20:07:46, Devlin wrote: > Don't need this. Done. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:17: #include "chrome/browser/extensions/extension_service.h" On 2014/07/09 20:07:47, Devlin wrote: > Don't need this. Are you sure? It doesn't compile without it. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:57: MenuItem::ContextList contexts = item->contexts(); On 2014/07/09 20:07:47, Devlin wrote: > Make this a const &. Done. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:260: if (!extensions::ExtensionSystem::Get(profile_)->extension_service()) On 2014/07/09 20:07:47, Devlin wrote: > Don't need this? Still? Oh shoot. I just realized that the unit test that the comment refers to doesn't apply to this file. That's what you meant; I thought you meant that it didn't need to have a reference in this method, so I put it into the conditional. Removed. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:280: *iter, base::EmptyString16()); On 2014/07/09 20:07:47, Devlin wrote: > (Again), read the documentation for base::EmptyString16(). You really _don't_ > need to use it here. I promise. Oops. I don't mean to give you the impression that I'm ignoring your advice; I changed the instance on line 299 and didn't realize I had it in here as well. Went ahead and swapped it out for base::string16(). https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:294: base::TimeTicks begin = base::TimeTicks::Now(); On 2014/07/09 20:07:47, Devlin wrote: > Why do we need this? Negligence on my part. Dead code leftover from the histogram bit. I've removed it from here and from render_view_context_menu.cc I wonder why the compiler didn't warn me about the unused variable. Hm... https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:295: for (size_t i = 0; i < sorted_menu_titles.size(); ++i) { On 2014/07/09 20:07:47, Devlin wrote: > prefer an iterator. Done. Changed in render_view_context_menu as well. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:39: // Type of action the extension icon represents. On 2014/07/09 20:07:47, Devlin wrote: > Do these actually need to be public? They are necessary for MenuItemMatchesExtension, which is anonymous. Should I keep it the way it is, or make this private and move MenuItemMatchesExtension into the class? https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:97: ActionType GetActionType(const extensions::Extension* extension); On 2014/07/09 20:07:47, Devlin wrote: > Does this still exist? No, no it does not. Sorry about all the dead code; I need to pay more attention when I'm removing things that I've added. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:557: base::TimeTicks begin = base::TimeTicks::Now(); On 2014/07/09 20:07:47, Devlin wrote: > Take this out, too. Done.
Here's a screenshot of the page action as well (I modified the extension to have a page action for this screenshot): http://i.imgur.com/Qwtq1Z5.png
Close! https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:17: #include "chrome/browser/extensions/extension_service.h" On 2014/07/09 22:15:50, gpdavis wrote: > On 2014/07/09 20:07:47, Devlin wrote: > > Don't need this. > > Are you sure? It doesn't compile without it. Whoops - overly enthusiastic. :) You're right. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:39: // Type of action the extension icon represents. On 2014/07/09 22:15:50, gpdavis wrote: > On 2014/07/09 20:07:47, Devlin wrote: > > Do these actually need to be public? > > They are necessary for MenuItemMatchesExtension, which is anonymous. Should I > keep it the way it is, or make this private and move MenuItemMatchesExtension > into the class? Ah, that's right. Let's just leave it as-is for now. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:8: #include "base/strings/string_util.h" Pretty sure you don't need this. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:109: const Extension* extension = this->GetExtension(); also not you're code... but take out the "this" here, please :) https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:253: extensions::ExtensionSystem::Get(profile_)->extension_service(); Not your code, but since you're here, can you update this to use ExtensionRegistry::enabled_extensions()->GetByID()? https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:291: std::vector<base::string16>::iterator it; scope the iterator with the for-loop. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:294: const MenuItem::ExtensionKey extension_key(id); You can inline extension_key. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:557: std::vector<base::string16>::iterator it; same comment about scoping. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:560: const MenuItem::ExtensionKey extension_key(id); Since you're here... same comment about inlining extension_key. :)
https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:17: #include "chrome/browser/extensions/extension_service.h" On 2014/07/09 22:42:58, Devlin wrote: > On 2014/07/09 22:15:50, gpdavis wrote: > > On 2014/07/09 20:07:47, Devlin wrote: > > > Don't need this. > > > > Are you sure? It doesn't compile without it. > > Whoops - overly enthusiastic. :) You're right. Acknowledged. https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:39: // Type of action the extension icon represents. On 2014/07/09 22:42:58, Devlin wrote: > On 2014/07/09 22:15:50, gpdavis wrote: > > On 2014/07/09 20:07:47, Devlin wrote: > > > Do these actually need to be public? > > > > They are necessary for MenuItemMatchesExtension, which is anonymous. Should I > > keep it the way it is, or make this private and move MenuItemMatchesExtension > > into the class? > > Ah, that's right. Let's just leave it as-is for now. Acknowledged. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:8: #include "base/strings/string_util.h" On 2014/07/09 22:42:59, Devlin wrote: > Pretty sure you don't need this. Done. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:109: const Extension* extension = this->GetExtension(); On 2014/07/09 22:42:59, Devlin wrote: > also not you're code... but take out the "this" here, please :) Done. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:253: extensions::ExtensionSystem::Get(profile_)->extension_service(); On 2014/07/09 22:42:59, Devlin wrote: > Not your code, but since you're here, can you update this to use > ExtensionRegistry::enabled_extensions()->GetByID()? Done. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:291: std::vector<base::string16>::iterator it; On 2014/07/09 22:42:59, Devlin wrote: > scope the iterator with the for-loop. Done. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:294: const MenuItem::ExtensionKey extension_key(id); On 2014/07/09 22:42:59, Devlin wrote: > You can inline extension_key. Done. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:557: std::vector<base::string16>::iterator it; On 2014/07/09 22:42:59, Devlin wrote: > same comment about scoping. Done. https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:560: const MenuItem::ExtensionKey extension_key(id); On 2014/07/09 22:42:59, Devlin wrote: > Since you're here... same comment about inlining extension_key. :) Done.
On 2014/07/10 00:22:26, gpdavis wrote: > https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... > File chrome/browser/extensions/extension_context_menu_model.cc (right): > > https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... > chrome/browser/extensions/extension_context_menu_model.cc:17: #include > "chrome/browser/extensions/extension_service.h" > On 2014/07/09 22:42:58, Devlin wrote: > > On 2014/07/09 22:15:50, gpdavis wrote: > > > On 2014/07/09 20:07:47, Devlin wrote: > > > > Don't need this. > > > > > > Are you sure? It doesn't compile without it. > > > > Whoops - overly enthusiastic. :) You're right. > > Acknowledged. > > https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... > File chrome/browser/extensions/extension_context_menu_model.h (right): > > https://codereview.chromium.org/359493005/diff/120001/chrome/browser/extensio... > chrome/browser/extensions/extension_context_menu_model.h:39: // Type of action > the extension icon represents. > On 2014/07/09 22:42:58, Devlin wrote: > > On 2014/07/09 22:15:50, gpdavis wrote: > > > On 2014/07/09 20:07:47, Devlin wrote: > > > > Do these actually need to be public? > > > > > > They are necessary for MenuItemMatchesExtension, which is anonymous. Should > I > > > keep it the way it is, or make this private and move > MenuItemMatchesExtension > > > into the class? > > > > Ah, that's right. Let's just leave it as-is for now. > > Acknowledged. > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... > File chrome/browser/extensions/extension_context_menu_model.cc (right): > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... > chrome/browser/extensions/extension_context_menu_model.cc:8: #include > "base/strings/string_util.h" > On 2014/07/09 22:42:59, Devlin wrote: > > Pretty sure you don't need this. > > Done. > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... > chrome/browser/extensions/extension_context_menu_model.cc:109: const Extension* > extension = this->GetExtension(); > On 2014/07/09 22:42:59, Devlin wrote: > > also not you're code... but take out the "this" here, please :) > > Done. > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... > chrome/browser/extensions/extension_context_menu_model.cc:253: > extensions::ExtensionSystem::Get(profile_)->extension_service(); > On 2014/07/09 22:42:59, Devlin wrote: > > Not your code, but since you're here, can you update this to use > > ExtensionRegistry::enabled_extensions()->GetByID()? > > Done. > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... > chrome/browser/extensions/extension_context_menu_model.cc:291: > std::vector<base::string16>::iterator it; > On 2014/07/09 22:42:59, Devlin wrote: > > scope the iterator with the for-loop. > > Done. > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/extensio... > chrome/browser/extensions/extension_context_menu_model.cc:294: const > MenuItem::ExtensionKey extension_key(id); > On 2014/07/09 22:42:59, Devlin wrote: > > You can inline extension_key. > > Done. > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... > File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... > chrome/browser/renderer_context_menu/render_view_context_menu.cc:557: > std::vector<base::string16>::iterator it; > On 2014/07/09 22:42:59, Devlin wrote: > > same comment about scoping. > > Done. > > https://codereview.chromium.org/359493005/diff/140001/chrome/browser/renderer... > chrome/browser/renderer_context_menu/render_view_context_menu.cc:560: const > MenuItem::ExtensionKey extension_key(id); > On 2014/07/09 22:42:59, Devlin wrote: > > Since you're here... same comment about inlining extension_key. :) > > Done. (Will get to the rest of the review later today.) Talking with Ben, he makes the good point that we really don't need icons next to these - there's only ever going to be one extension acting, so it's unambiguous. If we add multiple items, it'll look really weird, too.
This mostly lg, but you need to fix the icon, and Yoyo should take another look (as he's more familiar with context menu code). https://codereview.chromium.org/359493005/diff/150008/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/150008/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:290: it != sorted_menu_titles.end(); ++it) { nit: be consistent in style. Use either it or iter, not both (my preference is iter, but it really doesn't matter). Similarly, either one condition per line for (iter = begin(); iter != end(); ++iter) {} or two lines for (iter = begin(); iter != end(); ++iter) {} The for loop here and on line 269 should look stylistically similar.
Created AppendExtensionItemsImpl so I could add AppendExtensionItemsWithoutIcons to keep icons off of the action context menus. https://codereview.chromium.org/359493005/diff/150008/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/150008/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:290: it != sorted_menu_titles.end(); ++it) { Oops. Done.
I'm happy after this. But again, please put an updated screenshot in the CL description (they get lost in the comments). https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.h:46: void AppendExtensionItemsWithoutIcons( I know we did this in the other patch for ExtensionService, but this method is only called four times in three files (and we're already touching some of them, anyway). Let's just add the bool param directly to AppendExtensionItems. https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:272: const Extension* extension = enabled_extensions.GetByID(iter->extension_id); nit: simpler yet: if (!enabled_extensions.GetByID(iter->extension_id)) continue; base::string16 menu_title... https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:290: iter != sorted_menu_titles.end(); ++iter) { nit: match line wrapping style in for loops, too.
Uploaded an updated screenshot and added it to the description. Went through and cleaned up a few minor things and removed AppendExtensionItemsImpl. Glad this is looking good! https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.h:46: void AppendExtensionItemsWithoutIcons( On 2014/07/15 17:37:54, Devlin wrote: > I know we did this in the other patch for ExtensionService, but this method is > only called four times in three files (and we're already touching some of them, > anyway). Let's just add the bool param directly to AppendExtensionItems. Done. https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:272: const Extension* extension = enabled_extensions.GetByID(iter->extension_id); On 2014/07/15 17:37:54, Devlin wrote: > nit: simpler yet: > if (!enabled_extensions.GetByID(iter->extension_id)) > continue; > base::string16 menu_title... Done. https://codereview.chromium.org/359493005/diff/170001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:290: iter != sorted_menu_titles.end(); ++iter) { On 2014/07/15 17:37:55, Devlin wrote: > nit: match line wrapping style in for loops, too. Done.
Great screenshot! Do the full create properties work? like checkbox, radio, separator, etc? https://developer.chrome.com/extensions/contextMenus#property-create-createPr...
I'd love to see some tests btw.
Nice! Now just need to test it :) https://codereview.chromium.org/359493005/diff/210001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/210001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:21: #include "base/time/time.h" nit: remove this.
On 2014/07/15 21:16:05, kalman wrote: > Great screenshot! Do the full create properties work? like checkbox, radio, > separator, etc? > > https://developer.chrome.com/extensions/contextMenus#property-create-createPr... Indeed they do! I updated the description with a new screenshot that has all the items in the browser action context menu. As far as testing goes, I'm assuming I'll need to add to the extension_context_menu_model_unittest? I haven't made or modified any unittests yet, so I'm unfamiliar with the process. Is there anything particularly unusual I need to know about testing? Or is it as straightforward as adding to the unittest?
https://codereview.chromium.org/359493005/diff/210001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/210001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:21: #include "base/time/time.h" On 2014/07/15 21:25:08, Devlin wrote: > nit: remove this. Nice catch! Taken care of.
On 2014/07/15 22:17:40, gpdavis wrote: > On 2014/07/15 21:16:05, kalman wrote: > > Great screenshot! Do the full create properties work? like checkbox, radio, > > separator, etc? > > > > > https://developer.chrome.com/extensions/contextMenus#property-create-createPr... > > Indeed they do! I updated the description with a new screenshot that has all > the items in the browser action context menu. > > As far as testing goes, I'm assuming I'll need to add to the > extension_context_menu_model_unittest? I haven't made or modified any unittests > yet, so I'm unfamiliar with the process. Is there anything particularly unusual > I need to know about testing? Or is it as straightforward as adding to the > unittest? Typically, you'll add new unittests (to the existing file), rather than adding to an existing unittest. The exception is if you are modifying current behavior, and there's a very similar test. Adding more tests is typically pretty straightforward (as much so as anything is "straightforward" in this codebase), and following the example of other tests is usually good practice. You'll definitely wanna add at least a test to the model. Yoyo would know better if there are other tests that could benefit from an update.
On 2014/07/15 22:17:40, gpdavis wrote: > On 2014/07/15 21:16:05, kalman wrote: > > Great screenshot! Do the full create properties work? like checkbox, radio, > > separator, etc? > > > > > https://developer.chrome.com/extensions/contextMenus#property-create-createPr... > > Indeed they do! I updated the description with a new screenshot that has all > the items in the browser action context menu. > Awesome! What about multiple items at the top level? It would be good to see that in the screenshot as well.
On 2014/07/15 22:33:51, kalman wrote: > On 2014/07/15 22:17:40, gpdavis wrote: > > On 2014/07/15 21:16:05, kalman wrote: > > > Great screenshot! Do the full create properties work? like checkbox, radio, > > > separator, etc? > > > > > > > > > https://developer.chrome.com/extensions/contextMenus#property-create-createPr... > > > > Indeed they do! I updated the description with a new screenshot that has all > > the items in the browser action context menu. > > > > Awesome! What about multiple items at the top level? It would be good to see > that in the screenshot as well. Updated the screenshot! I'll go ahead and look into testing now.
On 2014/07/15 22:59:30, gpdavis wrote: > On 2014/07/15 22:33:51, kalman wrote: > > On 2014/07/15 22:17:40, gpdavis wrote: > > > On 2014/07/15 21:16:05, kalman wrote: > > > > Great screenshot! Do the full create properties work? like checkbox, > radio, > > > > separator, etc? > > > > > > > > > > > > > > https://developer.chrome.com/extensions/contextMenus#property-create-createPr... > > > > > > Indeed they do! I updated the description with a new screenshot that has > all > > > the items in the browser action context menu. > > > > > > > Awesome! What about multiple items at the top level? It would be good to see > > that in the screenshot as well. > > Updated the screenshot! > > I'll go ahead and look into testing now. I don't see it. Still looks like there's only a single entry in the browser action. Should be able to do: My Extension ----- Some entry [ ] Some checkbox (o) Some radio button ( ) Other radio button ----- Options Remove From Chromium ----- Manage ----- Inspect Popup
I agree with what kalman is saying about multiple items: in the screenshot, you have Extension Title () Extension Title -> submenu (the multiple extension-provided items) which looks a little funny and is one layer deeper than you need. As for tests, you can add tests to extension_context_menu_model_unittest.cc. For example, beyond testing just simply that specifying a browser action context menu item causes there to be one, you can also test the multiple-item behavior above, and 'wrong' cases (e.g. specifying a page action menu item when you actually have a browser action). https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/e... File chrome/browser/extensions/context_menu_matcher.h (right): https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/e... chrome/browser/extensions/context_menu_matcher.h:37: // the extension's icon should be included with the items. "included with the items": does that mean "shown"? https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/e... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model.cc:46: // Returns true if the given |item| is for the given |extension| and |type|. nit: for -> of https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model.cc:269: for (std::set<MenuItem::ExtensionKey>::iterator iter = ids.begin(); I still don't understand why we're iterating over all extensions, instead of just using extension_. (Maybe they get filtered out by the filter above? But even so, why do all that extra work for other extensions?) https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/r... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/r... chrome/browser/renderer_context_menu/render_view_context_menu.cc:564: true); // include_icons nit: two spaces before // comments https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/u... File chrome/browser/ui/app_list/app_context_menu.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/u... chrome/browser/ui/app_list/app_context_menu.cc:159: true); // include_icons ditto https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/u... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/browser/u... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:167: true); // include_icons ditto https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/common/ex... File chrome/common/extensions/api/web_view_internal.json (right): https://chromiumcodereview.appspot.com/359493005/diff/210001/chrome/common/ex... chrome/common/extensions/api/web_view_internal.json:127: "enum": ["all", "page", "frame", "selection", "link", "editable", "image", "video", "audio", "launcher", "browser_action", "page_action"] These don't make sense for webviews because: Webviews are a means by which Chrome apps can embed a frame of web content. The webview context menus are so the app can specify custom menu elements in that frame. Since it's an app, it by necessity doesn't have a page or browser action (and neither does the webview itself).
On 2014/07/15 23:16:16, kalman wrote: > On 2014/07/15 22:59:30, gpdavis wrote: > > On 2014/07/15 22:33:51, kalman wrote: > > > On 2014/07/15 22:17:40, gpdavis wrote: > > > > On 2014/07/15 21:16:05, kalman wrote: > > > > > Great screenshot! Do the full create properties work? like checkbox, > > radio, > > > > > separator, etc? > > > > > > > > > > > > > > > > > > > > https://developer.chrome.com/extensions/contextMenus#property-create-createPr... > > > > > > > > Indeed they do! I updated the description with a new screenshot that has > > all > > > > the items in the browser action context menu. > > > > > > > > > > Awesome! What about multiple items at the top level? It would be good to see > > > that in the screenshot as well. > > > > Updated the screenshot! > > > > I'll go ahead and look into testing now. > > I don't see it. Still looks like there's only a single entry in the browser > action. Should be able to do: > > My Extension > ----- > Some entry > [ ] Some checkbox > (o) Some radio button > ( ) Other radio button > ----- > Options > Remove From Chromium > ----- > Manage > ----- > Inspect Popup Oh, I misunderstood; I thought you were talking about top level items that get nested like they do in other context menus. I see what you're saying though; considering we're only adding items for the extension being clicked on, it doesn't make sense to have all these items in a submenu. So, referring to the screenshot, all of those items (radio 1, radio 2, 'test parent item') that are nested should end up in the root context menu? Looks like that's what yoyo clarified. I'll go ahead and take care of that then.
Items are no longer nested in an unnecessary submenu (see new screenshot in description). This is what you two were suggesting, yes? I removed the unnecessary logic from AppendExtensionItems, since only the relevant extension's items get added. I also removed the extension id check in MenuItemMatchesExtension and renamed it to MenuItemMatchesAction, since the items have already been filtered by extension at this point. I updated the |include_icons| flag to |is_action_menu| so that it can also be used for the submenu logic. This way, action menus don't use nested submenus and they also don't include the extension's icon. https://codereview.chromium.org/359493005/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/359493005/diff/210001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.h:37: // the extension's icon should be included with the items. On 2014/07/15 23:35:00, Yoyo Zhou wrote: > "included with the items": does that mean "shown"? Done. https://codereview.chromium.org/359493005/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/359493005/diff/210001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:46: // Returns true if the given |item| is for the given |extension| and |type|. On 2014/07/15 23:35:00, Yoyo Zhou wrote: > nit: for -> of Done. https://codereview.chromium.org/359493005/diff/210001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.cc:269: for (std::set<MenuItem::ExtensionKey>::iterator iter = ids.begin(); On 2014/07/15 23:35:00, Yoyo Zhou wrote: > I still don't understand why we're iterating over all extensions, instead of > just using extension_. (Maybe they get filtered out by the filter above? But > even so, why do all that extra work for other extensions?) This is a really good point; I don't think I fully understood before. I simplified this function to only look for items of the relevant extension. https://codereview.chromium.org/359493005/diff/210001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/210001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:564: true); // include_icons On 2014/07/15 23:35:00, Yoyo Zhou wrote: > nit: two spaces before // comments Done. https://codereview.chromium.org/359493005/diff/210001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/210001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_context_menu.cc:159: true); // include_icons On 2014/07/15 23:35:00, Yoyo Zhou wrote: > ditto Done. https://codereview.chromium.org/359493005/diff/210001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/210001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:167: true); // include_icons On 2014/07/15 23:35:00, Yoyo Zhou wrote: > ditto Done. https://codereview.chromium.org/359493005/diff/210001/chrome/common/extension... File chrome/common/extensions/api/web_view_internal.json (right): https://codereview.chromium.org/359493005/diff/210001/chrome/common/extension... chrome/common/extensions/api/web_view_internal.json:127: "enum": ["all", "page", "frame", "selection", "link", "editable", "image", "video", "audio", "launcher", "browser_action", "page_action"] On 2014/07/15 23:35:01, Yoyo Zhou wrote: > These don't make sense for webviews because: > > Webviews are a means by which Chrome apps can embed a frame of web content. The > webview context menus are so the app can specify custom menu elements in that > frame. Since it's an app, it by necessity doesn't have a page or browser action > (and neither does the webview itself). Ah, I see. That makes sense. The only problem I run into with this is in context_menus_api_helpers.h; there's an issue regarding templates that arises when this array is different from the one in context_menus.json. Without adding these here, the logic would need to be set up to accommodate for these differences. Should I go ahead and make that change, or leave these here in this json file along with a comment?
Looks great! And now for a security opinion, I'm worried that an extension can add a lot of context menu items and hide the remove-from-chromium button and/or the manage button. Should also get a UI opinion on this actually. but anyhow, options would be to shuffle around the context menu items a bit to put the extension's at the bottom, and/or limit the number of items an extension can put at the top level (let's say, 10).
On 2014/07/16 01:14:50, kalman wrote: > Looks great! > > And now for a security opinion, I'm worried that an extension can add a lot of > context menu items and hide the remove-from-chromium button and/or the manage > button. > > Should also get a UI opinion on this actually. > > but anyhow, options would be to shuffle around the context menu items a bit to > put the extension's at the bottom, and/or limit the number of items an extension > can put at the top level (let's say, 10). Limiting the number is reasonable. Is it possible to distinguish extension controlled menu items from Chrome items (e.g. by adding an icon to Chrome controlled ones), or maybe put Chrome controlled items at the top?
On 2014/07/16 17:43:59, Mustafa Emre Acer wrote: > On 2014/07/16 01:14:50, kalman wrote: > > Looks great! > > > > And now for a security opinion, I'm worried that an extension can add a lot of > > context menu items and hide the remove-from-chromium button and/or the manage > > button. > > > > Should also get a UI opinion on this actually. > > > > but anyhow, options would be to shuffle around the context menu items a bit to > > put the extension's at the bottom, and/or limit the number of items an > extension > > can put at the top level (let's say, 10). > > Limiting the number is reasonable. Is it possible to distinguish extension > controlled menu items from Chrome items (e.g. by adding an icon to Chrome > controlled ones), or maybe put Chrome controlled items at the top? Having an icon next to every extension-controlled option would probably look pretty weird. Can we move this discussion to the bug? I put a UI person on that to comment on whether we can put Chrome controlled items at the top.
On 2014/07/16 18:39:01, kalman wrote: > On 2014/07/16 17:43:59, Mustafa Emre Acer wrote: > > On 2014/07/16 01:14:50, kalman wrote: > > > Looks great! > > > > > > And now for a security opinion, I'm worried that an extension can add a lot > of > > > context menu items and hide the remove-from-chromium button and/or the > manage > > > button. > > > > > > Should also get a UI opinion on this actually. > > > > > > but anyhow, options would be to shuffle around the context menu items a bit > to > > > put the extension's at the bottom, and/or limit the number of items an > > extension > > > can put at the top level (let's say, 10). > > > > Limiting the number is reasonable. Is it possible to distinguish extension > > controlled menu items from Chrome items (e.g. by adding an icon to Chrome > > controlled ones), or maybe put Chrome controlled items at the top? > > Having an icon next to every extension-controlled option would probably look > pretty weird. Can we move this discussion to the bug? I put a UI person on that > to comment on whether we can put Chrome controlled items at the top. I went ahead and limited the number of top level items to six. What's the protocol for dealing with magic numbers like this? Is it okay to just leave as is? I also wanted to ask if there ought to be some sort of warning logged when an attempt is made to add more than six top level items. I've taken a look at the unittest file, and I just wanted to clarify how I should be going about adding tests. Essentially, I just need to mimic all the behavior that would happen if an extension registered all these context menu items, and test the behavior via the methods available in the function? So for example, I could create an extension with a particular ID, create menu items in the menu manager that are attributed to browser or page actions for that particular extension, build the context menu, and start testing things like command ids, and ensure that everything behaves as expected?
Also, can I get some clarification about your comment on the bug regarding activeTab permissions? I don't think I saw anything relevant to this in this code, but I could be wrong.
> What's the protocol for dealing with magic numbers like this It's nice to put those constants in the schema file so that extensions can actually read them. For example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... give it a nice name. you can access it from the code like: https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/chro... > Also, can I get some clarification about your comment on the bug regarding > activeTab permissions? I don't think I saw anything relevant to this in this > code, but I could be wrong. We grant the active tab permission here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... just check that it isn't triggered with the code you're adding.
On 2014/07/16 22:17:57, kalman wrote: > > What's the protocol for dealing with magic numbers like this > > It's nice to put those constants in the schema file so that extensions can > actually read them. For example: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > give it a nice name. you can access it from the code like: > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/chro... > > > Also, can I get some clarification about your comment on the bug regarding > > activeTab permissions? I don't think I saw anything relevant to this in this > > code, but I could be wrong. > > We grant the active tab permission here: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > just check that it isn't triggered with the code you're adding. How's this look for testing and the limit constant? The test I added ensures that a browser_action context menu item does not show up in a page action context menu, and that the top level items are limited to six items. I figured I would probably need to test more, but I wanted to show you what I've got and ask for suggestions on further tests. I also just realized that I should use this new constant in the test as well. I'll go ahead and change that now. I looked into the active tab permissions, and it is indeed triggered with this code. I'm not sure I understand why that's triggered in the first place. What do you think?
PTAL :)
The gist of the test makes sense to me. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... File chrome/browser/extensions/context_menu_matcher.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/context_menu_matcher.cc:20: extern const int ACTION_MENU_TOP_LEVEL_LIMIT; Why not include the generated chrome/common/extensions/api/context_menus.h file? https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/context_menu_matcher.cc:66: // Action menus only include items from one extension, and do not include the Rewrite this: Action menus do not include the extension action, and they only include items from one extension, so they are not placed within a submenu. Otherwise, ... https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model.cc:47: bool MenuItemMatchesAction(ExtensionContextMenuModel::ActionType type, You probably don't need to match against the extension again here. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:62: void ResetMenu(ExtensionContextMenuModel* model) { "Reset" has kind of ambiguous connotations. Maybe call it Refresh? https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:67: int DetectExtensionItems(ExtensionContextMenuModel* model) { nit: CountExtensionItems? https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:125: (MenuManagerFactory::GetInstance()->SetTestingFactoryAndUse( I don't understand what's going on here. It looks like you're just using the standard factory, not a testing factory. Just call GetForProfile? https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:140: // not be detected. 'detected' is kind of odd. Say 'present'. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:147: // The page action item should be detected because |extension| has a page ditto https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:161: // items, and we limit the number of top level extension items to six. Ok. By the way, do we count separators as top-level items for this purpose? (Either way is possibly valid, I'm just curious.) And what happens to the 7th item?
https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... File chrome/browser/extensions/context_menu_matcher.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/context_menu_matcher.cc:20: extern const int ACTION_MENU_TOP_LEVEL_LIMIT; On 2014/07/22 00:44:32, Yoyo Zhou wrote: > Why not include the generated chrome/common/extensions/api/context_menus.h file? Ah, that is more convenient. I wasn't sure exactly how this worked under the hood, so I did it this way. Done. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/context_menu_matcher.cc:66: // Action menus only include items from one extension, and do not include the On 2014/07/22 00:44:32, Yoyo Zhou wrote: > Rewrite this: Action menus do not include the extension action, and they only > include items from one extension, so they are not placed within a submenu. > Otherwise, ... Done. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model.cc:47: bool MenuItemMatchesAction(ExtensionContextMenuModel::ActionType type, On 2014/07/22 00:44:32, Yoyo Zhou wrote: > You probably don't need to match against the extension again here. Ah, I removed the check but I left the extension parameter. Done. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:62: void ResetMenu(ExtensionContextMenuModel* model) { On 2014/07/22 00:44:32, Yoyo Zhou wrote: > "Reset" has kind of ambiguous connotations. Maybe call it Refresh? Done. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:67: int DetectExtensionItems(ExtensionContextMenuModel* model) { On 2014/07/22 00:44:32, Yoyo Zhou wrote: > nit: CountExtensionItems? Done. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:125: (MenuManagerFactory::GetInstance()->SetTestingFactoryAndUse( On 2014/07/22 00:44:32, Yoyo Zhou wrote: > I don't understand what's going on here. It looks like you're just using the > standard factory, not a testing factory. Just call GetForProfile? Calling GetForProfile returned NULL, so I needed to use BuildInstanceFor (see changes to menu_manager_factory). https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:140: // not be detected. On 2014/07/22 00:44:32, Yoyo Zhou wrote: > 'detected' is kind of odd. Say 'present'. Done. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:147: // The page action item should be detected because |extension| has a page On 2014/07/22 00:44:32, Yoyo Zhou wrote: > ditto Done. https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:161: // items, and we limit the number of top level extension items to six. On 2014/07/22 00:44:32, Yoyo Zhou wrote: > Ok. By the way, do we count separators as top-level items for this purpose? > (Either way is possibly valid, I'm just curious.) And what happens to the 7th > item? User added separators (via create calls in the extension JS) are counted as top level items, but separators that are automatically added (as in the case of radio items) do not count towards the count. Any extra items are simply not appended to the action context menu model when AppendExtensionItems is called in the ContextMenuMatcher. https://chromiumcodereview.appspot.com/359493005/diff/370001/chrome/browser/e... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://chromiumcodereview.appspot.com/359493005/diff/370001/chrome/browser/e... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:89: // Not available for <webview>. Should I add this comment in for the browser and page action types as well? It looks like the launcher type was added in for the same reason I added browser and page actions to the web view json file (to maintain the generic nature of the methods in this class).
LGTM https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:161: // items, and we limit the number of top level extension items to six. On 2014/07/22 18:52:52, gpdavis wrote: > On 2014/07/22 00:44:32, Yoyo Zhou wrote: > > Ok. By the way, do we count separators as top-level items for this purpose? > > (Either way is possibly valid, I'm just curious.) And what happens to the 7th > > item? > > User added separators (via create calls in the extension JS) are counted as top > level items, but separators that are automatically added (as in the case of > radio items) do not count towards the count. > > Any extra items are simply not appended to the action context menu model when > AppendExtensionItems is called in the ContextMenuMatcher. So additional top-level items beyond the 6th are just silently dropped? This should be documented. https://chromiumcodereview.appspot.com/359493005/diff/370001/chrome/common/ex... File chrome/common/extensions/api/context_menus.json (right): https://chromiumcodereview.appspot.com/359493005/diff/370001/chrome/common/ex... chrome/common/extensions/api/context_menus.json:12: "description": "The maximum number of top level extension items that can be added to an extension action context menu." document it here. "Any items beyond this limit will be ignored." Also, it would be nice if this was checked at extension load time as well, and resulted in a manifest warning if this was exceeded. Feel free to add that in a followup CL.
https://chromiumcodereview.appspot.com/359493005/diff/370001/chrome/common/ex... File chrome/common/extensions/api/context_menus.json (right): https://chromiumcodereview.appspot.com/359493005/diff/370001/chrome/common/ex... chrome/common/extensions/api/context_menus.json:12: "description": "The maximum number of top level extension items that can be added to an extension action context menu." On 2014/07/22 21:35:42, Yoyo Zhou wrote: > document it here. "Any items beyond this limit will be ignored." > > Also, it would be nice if this was checked at extension load time as well, and > resulted in a manifest warning if this was exceeded. Feel free to add that in a > followup CL. Sorry, what I meant is, this should be checked in the context menu API and the call to contextMenus.create should fail if this limit is exceeded.
I've got a few very minor changes in some other files here. Could you guys take a look at these real quick? Thanks! brettw@chromium.org: Please review changes in chrome/browser/renderer_context_menu msw@chromium.org: Please review changes in chrome/browser/ui
https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://chromiumcodereview.appspot.com/359493005/diff/330001/chrome/browser/e... chrome/browser/extensions/extension_context_menu_model_unittest.cc:161: // items, and we limit the number of top level extension items to six. On 2014/07/22 21:35:42, Yoyo Zhou wrote: > On 2014/07/22 18:52:52, gpdavis wrote: > > On 2014/07/22 00:44:32, Yoyo Zhou wrote: > > > Ok. By the way, do we count separators as top-level items for this purpose? > > > (Either way is possibly valid, I'm just curious.) And what happens to the > 7th > > > item? > > > > User added separators (via create calls in the extension JS) are counted as > top > > level items, but separators that are automatically added (as in the case of > > radio items) do not count towards the count. > > > > Any extra items are simply not appended to the action context menu model when > > AppendExtensionItems is called in the ContextMenuMatcher. > > So additional top-level items beyond the 6th are just silently dropped? This > should be documented. it would be better if it set an error, if possible (if that method can take a callback which communiates an error)
https://codereview.chromium.org/359493005/diff/370001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/370001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:152: if (contexts.Contains(MenuItem::LAUNCHER)) { @yoyo, Since you pointed out the bit about webviews, should this conditional also check for browser_action or page_action contexts being added for <webview>? Also, this is the function that I ought to be adding this top level limitation check to, correct? https://codereview.chromium.org/359493005/diff/370001/chrome/common/extension... File chrome/common/extensions/api/context_menus.json (right): https://codereview.chromium.org/359493005/diff/370001/chrome/common/extension... chrome/common/extensions/api/context_menus.json:12: "description": "The maximum number of top level extension items that can be added to an extension action context menu." On 2014/07/22 21:35:42, Yoyo Zhou wrote: > document it here. "Any items beyond this limit will be ignored." > > Also, it would be nice if this was checked at extension load time as well, and > resulted in a manifest warning if this was exceeded. Feel free to add that in a > followup CL. Done.
https://codereview.chromium.org/359493005/diff/370001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/370001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:89: // Not available for <webview>. On 2014/07/22 18:52:53, gpdavis wrote: > Should I add this comment in for the browser and page action types as well? It > looks like the launcher type was added in for the same reason I added browser > and page actions to the web view json file (to maintain the generic nature of > the methods in this class). Yes, these are also not available for webview. https://codereview.chromium.org/359493005/diff/370001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:152: if (contexts.Contains(MenuItem::LAUNCHER)) { On 2014/07/23 23:09:52, gpdavis wrote: > @yoyo, > > Since you pointed out the bit about webviews, should this conditional also check > for browser_action or page_action contexts being added for <webview>? > > Also, this is the function that I ought to be adding this top level limitation > check to, correct? Likewise, yes, this is where you should be returning an error for webviews. https://codereview.chromium.org/359493005/diff/370001/chrome/common/extension... File chrome/common/extensions/api/context_menus.json (right): https://codereview.chromium.org/359493005/diff/370001/chrome/common/extension... chrome/common/extensions/api/context_menus.json:12: "description": "The maximum number of top level extension items that can be added to an extension action context menu." On 2014/07/23 23:09:52, gpdavis wrote: > On 2014/07/22 21:35:42, Yoyo Zhou wrote: > > document it here. "Any items beyond this limit will be ignored." > > > > Also, it would be nice if this was checked at extension load time as well, and > > resulted in a manifest warning if this was exceeded. Feel free to add that in > a > > followup CL. > > Done. Done? I don't see this returning an error from contextMenus.create. As I mentioned, you can do it in a followup CL.
https://codereview.chromium.org/359493005/diff/370001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/370001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:89: // Not available for <webview>. On 2014/07/23 23:37:21, Yoyo Zhou wrote: > On 2014/07/22 18:52:53, gpdavis wrote: > > Should I add this comment in for the browser and page action types as well? > It > > looks like the launcher type was added in for the same reason I added browser > > and page actions to the web view json file (to maintain the generic nature of > > the methods in this class). > > Yes, these are also not available for webview. Done. https://codereview.chromium.org/359493005/diff/370001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:152: if (contexts.Contains(MenuItem::LAUNCHER)) { On 2014/07/23 23:37:21, Yoyo Zhou wrote: > On 2014/07/23 23:09:52, gpdavis wrote: > > @yoyo, > > > > Since you pointed out the bit about webviews, should this conditional also > check > > for browser_action or page_action contexts being added for <webview>? > > > > Also, this is the function that I ought to be adding this top level limitation > > check to, correct? > > Likewise, yes, this is where you should be returning an error for webviews. Done. https://codereview.chromium.org/359493005/diff/370001/chrome/common/extension... File chrome/common/extensions/api/context_menus.json (right): https://codereview.chromium.org/359493005/diff/370001/chrome/common/extension... chrome/common/extensions/api/context_menus.json:12: "description": "The maximum number of top level extension items that can be added to an extension action context menu." On 2014/07/23 23:37:21, Yoyo Zhou wrote: > On 2014/07/23 23:09:52, gpdavis wrote: > > On 2014/07/22 21:35:42, Yoyo Zhou wrote: > > > document it here. "Any items beyond this limit will be ignored." > > > > > > Also, it would be nice if this was checked at extension load time as well, > and > > > resulted in a manifest warning if this was exceeded. Feel free to add that > in > > a > > > followup CL. > > > > Done. > > Done? I don't see this returning an error from contextMenus.create. As I > mentioned, you can do it in a followup CL. Sorry-- should have clarified this. The done was for the part relevant to this CL (documenting that items will be ignored).
https://codereview.chromium.org/359493005/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc (right): https://codereview.chromium.org/359493005/diff/390001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc:21: "Only packaged apps are allowed to use action contexts"; Only extensions are allowed... https://codereview.chromium.org/359493005/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/390001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:166: if (!extension->is_platform_app() || is_webview) { Actually, the comment above is not really clear. Launchers are available only to apps. Browser and page actions are only available to extensions. So this should be if (!extension->is_extension() || is_webview)
https://codereview.chromium.org/359493005/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc (right): https://codereview.chromium.org/359493005/diff/390001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc:21: "Only packaged apps are allowed to use action contexts"; On 2014/07/24 00:17:57, Yoyo Zhou wrote: > Only extensions are allowed... Done. https://codereview.chromium.org/359493005/diff/390001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/390001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:166: if (!extension->is_platform_app() || is_webview) { On 2014/07/24 00:17:58, Yoyo Zhou wrote: > Actually, the comment above is not really clear. Launchers are available only to > apps. Browser and page actions are only available to extensions. So this should > be > > if (!extension->is_extension() || is_webview) Ah, I see. I should have paid more attention to both of these. Done.
Great, LGTM
https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc:20: const char kActionNotAllowedError[] = here too, A < L https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:46: extern const char kActionNotAllowedError[]; nit: A < L. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:100: false); nit: document anonymous bool https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:227: num_items++; nit: ++num_items https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:253: false); document anon bool https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:118: int extension_items_count_ = 0; Don't initialize values in the class definition like this... https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:26: extern const int ACTION_MENU_TOP_LEVEL_LIMIT; why not just include the .h? https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:35: scoped_refptr<Extension> BuildExtension(std::string action_type) { even in tests, we *should* have the practice of not inlining our functions. (You are certainly not the only one who does this, but let's set a good example for everyone else :)) https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:35: scoped_refptr<Extension> BuildExtension(std::string action_type) { If you take my suggestion on line 80, this should be a const char*. Otherwise, this should be a const std::string&. It should not take a copy. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:58: extension, new MenuItem(id, "test", false, true, type, contexts)); doc anon bools https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:71: int cur_id = 0; Similarly, we should not have public class variables. Protected might be okay, though I prefer making them private if possible. Also, this is initialized incorrectly. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:80: scoped_refptr<Extension> extension = BuildExtension("page_action"); Why don't we define these as const char[]s kPageAction and kBrowserAction. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:130: new ExtensionContextMenuModel(extension.get(), &browser, NULL)); doc NULL https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:133: ASSERT_TRUE(CountExtensionItems(menu) == 0); Some of these can be EXPECTs, and you should also be using ASSERT/EXPECT_EQ for equality operations. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:152: for (int i = 0; i < api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT; ++i) { nit: no brackets. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/menu_manager_factory.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/menu_manager_factory.cc:29: KeyedService* MenuManagerFactory::BuildInstanceFor( If you're gonna do this, why not just make BuildServiceInstanceFor public, or (better yet) expose a ForTest() method so people get errors if they try to use it in real code?
https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:124: MenuManager* manager = static_cast<MenuManager*>( What about creating a MenuManager without using the factory? MenuManager manager(profile, state_store);
https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc:20: const char kActionNotAllowedError[] = On 2014/07/24 21:51:26, Devlin wrote: > here too, A < L Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:46: extern const char kActionNotAllowedError[]; On 2014/07/24 21:51:27, Devlin wrote: > nit: A < L. Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:100: false); On 2014/07/24 21:51:27, Devlin wrote: > nit: document anonymous bool Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:227: num_items++; On 2014/07/24 21:51:27, Devlin wrote: > nit: ++num_items Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:253: false); On 2014/07/24 21:51:27, Devlin wrote: > document anon bool Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model.h (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model.h:118: int extension_items_count_ = 0; On 2014/07/24 21:51:27, Devlin wrote: > Don't initialize values in the class definition like this... My mistake. I did this because the Google C++ style guide says it's okay to do for simple initializations (especially when it is intialized to the same value in multiple constructors, as is the case). No good? http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia... https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:26: extern const int ACTION_MENU_TOP_LEVEL_LIMIT; On 2014/07/24 21:51:28, Devlin wrote: > why not just include the .h? Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:35: scoped_refptr<Extension> BuildExtension(std::string action_type) { On 2014/07/24 21:51:27, Devlin wrote: > even in tests, we *should* have the practice of not inlining our functions. > (You are certainly not the only one who does this, but let's set a good example > for everyone else :)) Sure thing! I wasn't sure if this was still protocol, since surrounding code is different and there's no header file for this. Should there be a header for the class declaration? https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:35: scoped_refptr<Extension> BuildExtension(std::string action_type) { On 2014/07/24 21:51:27, Devlin wrote: > If you take my suggestion on line 80, this should be a const char*. Otherwise, > this should be a const std::string&. It should not take a copy. Done (parameter change). https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:58: extension, new MenuItem(id, "test", false, true, type, contexts)); On 2014/07/24 21:51:28, Devlin wrote: > doc anon bools Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:71: int cur_id = 0; On 2014/07/24 21:51:28, Devlin wrote: > Similarly, we should not have public class variables. Protected might be okay, > though I prefer making them private if possible. Also, this is initialized > incorrectly. Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:80: scoped_refptr<Extension> extension = BuildExtension("page_action"); On 2014/07/24 21:51:28, Devlin wrote: > Why don't we define these as const char[]s kPageAction and kBrowserAction. Done. I left out kBrowserAction for now, since it's an unused variable. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:124: MenuManager* manager = static_cast<MenuManager*>( On 2014/07/24 22:21:03, Yoyo Zhou wrote: > What about creating a MenuManager without using the factory? > > MenuManager manager(profile, state_store); MenuManager::Get(profile_.get()) returns null if I do it this way. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:130: new ExtensionContextMenuModel(extension.get(), &browser, NULL)); On 2014/07/24 21:51:28, Devlin wrote: > doc NULL I can actually just use the other constructor, which doesn't take a delegate instead of passing in NULL. Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:133: ASSERT_TRUE(CountExtensionItems(menu) == 0); On 2014/07/24 21:51:28, Devlin wrote: > Some of these can be EXPECTs, and you should also be using ASSERT/EXPECT_EQ for > equality operations. Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:152: for (int i = 0; i < api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT; ++i) { On 2014/07/24 21:51:28, Devlin wrote: > nit: no brackets. Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/menu_manager_factory.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/menu_manager_factory.cc:29: KeyedService* MenuManagerFactory::BuildInstanceFor( On 2014/07/24 21:51:28, Devlin wrote: > If you're gonna do this, why not just make BuildServiceInstanceFor public, or > (better yet) expose a ForTest() method so people get errors if they try to use > it in real code? BuildServiceInstanceFor is an overridden non-static method, and BuildInstanceFor must be static, so there's some conflict (unless I'm misunderstanding something). In what way can we ensure that a ForTest() method is only run in a testing environment? I'm a bit confused about what this method would be. The only thing I could find was this: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... but Associate is called in the code I've already added (GetServiceForBrowserContext calls it). Maybe a little explanation about what's bad about the way I've implemented this would help steer me in the right direction.
https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:35: scoped_refptr<Extension> BuildExtension(std::string action_type) { On 2014/07/28 21:08:22, gpdavis wrote: > On 2014/07/24 21:51:27, Devlin wrote: > > even in tests, we *should* have the practice of not inlining our functions. > > (You are certainly not the only one who does this, but let's set a good > example > > for everyone else :)) > > Sure thing! I wasn't sure if this was still protocol, since surrounding code is > different and there's no header file for this. Should there be a header for the > class declaration? You don't need a separate header file unless you need to include the class (e.g. as a base class, like ExtensionServiceTestBase). https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/menu_manager_factory.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/menu_manager_factory.cc:29: KeyedService* MenuManagerFactory::BuildInstanceFor( On 2014/07/28 21:08:22, gpdavis wrote: > On 2014/07/24 21:51:28, Devlin wrote: > > If you're gonna do this, why not just make BuildServiceInstanceFor public, or > > (better yet) expose a ForTest() method so people get errors if they try to use > > it in real code? > > BuildServiceInstanceFor is an overridden non-static method, and BuildInstanceFor > must be static, so there's some conflict (unless I'm misunderstanding > something). > > In what way can we ensure that a ForTest() method is only run in a testing > environment? I'm a bit confused about what this method would be. The only > thing I could find was this: > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... > > but Associate is called in the code I've already added > (GetServiceForBrowserContext calls it). Maybe a little explanation about what's > bad about the way I've implemented this would help steer me in the right > direction. We have presubmits that warn about the use of methods named "...ForTesting" in non-test code. The goal here is not to have methods on the factory that can be accidentally misused. Basically, it's better to define the method that creates the MenuManager for tests in the test itself, even if your creating the MenuManager looks a bit redundant with what's in here. (Sorry, in my earlier comment, I hadn't realized we had ServiceIsNULLWhileTesting = true.) Here's a simpler example of how one can use SetTestingFactoryAndUse: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
I went ahead and moved the Build method into the unit test, and added the test as a friend class so it has access to BuildServiceInstanceFor without exposing it publicly. Does that seem reasonable?
Yes, LGTM. https://codereview.chromium.org/359493005/diff/450001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://codereview.chromium.org/359493005/diff/450001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:47: static KeyedService* BuildInstanceForTesting( nit: Add function documentation.
On 2014/07/22 21:49:00, gpdavis wrote: > I've got a few very minor changes in some other files here. Could you guys take > a look at these real quick? Thanks! > > mailto:brettw@chromium.org: Please review changes in > > chrome/browser/renderer_context_menu > > mailto:msw@chromium.org: Please review changes in > > chrome/browser/ui PTAL :)
https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc:20: const char kActionNotAllowedError[] = On 2014/07/28 21:08:21, gpdavis wrote: > On 2014/07/24 21:51:26, Devlin wrote: > > here too, A < L > > Done. A < G, too. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:46: extern const char kActionNotAllowedError[]; On 2014/07/28 21:08:21, gpdavis wrote: > On 2014/07/24 21:51:27, Devlin wrote: > > nit: A < L. > > Done. Also, A < G. https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:235: (*index)++; nit: ++(*index); https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.h:61: friend class ::ExtensionContextMenuModelTest; Do we need this? https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:7: #include "chrome/browser/extensions/context_menu_matcher.h" Do we need this? https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:33: scoped_refptr<Extension> BuildExtension(const char* action_type); It looks like |action_type| is always just kPageAction...? https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:100: return MenuManagerFactory::GetInstance()->BuildServiceInstanceFor(context); nit: indentation https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:192: RefreshMenu(menu); The fact we say "RefreshMenu" every time we AddContextItem() (with the semi-exception of the for-loop) makes me think that it should just be done in AddContextItem().
Rubber stamp lgtm for mechanical c/b/ui/app_list changes. Really, I'm deferring review to c/b/extensions/OWNERS.
https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.cc:20: const char kActionNotAllowedError[] = On 2014/07/29 20:49:45, Devlin wrote: > On 2014/07/28 21:08:21, gpdavis wrote: > > On 2014/07/24 21:51:26, Devlin wrote: > > > here too, A < L > > > > Done. > > A < G, too. Oops, that was a very negligent mistake. Sorry about that. Done. https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h (right): https://codereview.chromium.org/359493005/diff/410001/chrome/browser/extensio... chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:46: extern const char kActionNotAllowedError[]; On 2014/07/29 20:49:45, Devlin wrote: > On 2014/07/28 21:08:21, gpdavis wrote: > > On 2014/07/24 21:51:27, Devlin wrote: > > > nit: A < L. > > > > Done. > > Also, A < G. Done. https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:235: (*index)++; On 2014/07/29 20:49:45, Devlin wrote: > nit: ++(*index); Done. https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.h:61: friend class ::ExtensionContextMenuModelTest; On 2014/07/29 20:49:45, Devlin wrote: > Do we need this? Oops, no, we don't. I had an idea that would have required this, but ended up doing it differently and forgot to remove it. Done. https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:7: #include "chrome/browser/extensions/context_menu_matcher.h" On 2014/07/29 20:49:46, Devlin wrote: > Do we need this? Ditto last comment. Done. https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:33: scoped_refptr<Extension> BuildExtension(const char* action_type); On 2014/07/29 20:49:46, Devlin wrote: > It looks like |action_type| is always just kPageAction...? I added this in to make it simple to create a browser_action if necessary, but didn't end up using it in my test (and had to remove the associated constant since it was an unused variable). I figured I'd leave it in since it seems reasonable to pass in the type of action, but I'll revert it back since it's unnecessary. https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:100: return MenuManagerFactory::GetInstance()->BuildServiceInstanceFor(context); On 2014/07/29 20:49:45, Devlin wrote: > nit: indentation Done. https://codereview.chromium.org/359493005/diff/470001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:192: RefreshMenu(menu); On 2014/07/29 20:49:46, Devlin wrote: > The fact we say "RefreshMenu" every time we AddContextItem() (with the > semi-exception of the for-loop) makes me think that it should just be done in > AddContextItem(). That's probably a good idea. I was going to combine them, but I figured I'd leave RefreshMenu as its own method in case future tests want to refresh the model without adding an item. Done.
https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.h:17: class Profile; Don't need this. https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right): https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... chrome/browser/extensions/extension_context_menu_model_unittest.cc:46: static KeyedService* BuildMenuManagerForTesting( May as well just have this in an anon namespace rather than the test. https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... File chrome/browser/extensions/menu_manager_factory.h (right): https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... chrome/browser/extensions/menu_manager_factory.h:27: friend class ExtensionContextMenuModelTest; Why do we still need this? What about Yoyo's idea of just constructing the MenuManager explicitly in the test?
https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... File chrome/browser/extensions/menu_manager_factory.h (right): https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... chrome/browser/extensions/menu_manager_factory.h:27: friend class ExtensionContextMenuModelTest; On 2014/07/30 21:32:58, Devlin wrote: > Why do we still need this? What about Yoyo's idea of just constructing the > MenuManager explicitly in the test? This slightly reduces code duplication, so I think it's fine.
https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... File chrome/browser/extensions/menu_manager_factory.h (right): https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... chrome/browser/extensions/menu_manager_factory.h:27: friend class ExtensionContextMenuModelTest; On 2014/07/30 21:49:33, Yoyo Zhou wrote: > On 2014/07/30 21:32:58, Devlin wrote: > > Why do we still need this? What about Yoyo's idea of just constructing the > > MenuManager explicitly in the test? > > This slightly reduces code duplication, so I think it's fine. Could we instead wrap the call to BuildServiceInstanceFor() in a ForTest method then, so that future tests can use it instead of adding to the friend list?
https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.h:17: class Profile; On 2014/07/30 21:32:57, Devlin wrote: > Don't need this. Done. https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... File chrome/browser/extensions/menu_manager_factory.h (right): https://codereview.chromium.org/359493005/diff/530001/chrome/browser/extensio... chrome/browser/extensions/menu_manager_factory.h:27: friend class ExtensionContextMenuModelTest; On 2014/07/30 21:54:15, Devlin wrote: > On 2014/07/30 21:49:33, Yoyo Zhou wrote: > > On 2014/07/30 21:32:58, Devlin wrote: > > > Why do we still need this? What about Yoyo's idea of just constructing the > > > MenuManager explicitly in the test? > > > > This slightly reduces code duplication, so I think it's fine. > > Could we instead wrap the call to BuildServiceInstanceFor() in a ForTest method > then, so that future tests can use it instead of adding to the friend list? That seems reasonable to me. Done.
On 2014/07/22 21:49:00, gpdavis wrote: > I've got a few very minor changes in some other files here. Could you guys take > a look at these real quick? Thanks! > > mailto:brettw@chromium.org: Please review changes in > > chrome/browser/renderer_context_menu Ping :)
lgtm
https://codereview.chromium.org/359493005/diff/550001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/550001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:586: for (std::vector<base::string16>::iterator iter = sorted_menu_titles.begin(); It's not clear to me why you rewrote this loop. To me, the new code seems strictly worse than the old code, and even if it wasn't, this isn't the type of thing that's worth doing.
https://codereview.chromium.org/359493005/diff/550001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/550001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:586: for (std::vector<base::string16>::iterator iter = sorted_menu_titles.begin(); On 2014/08/04 20:17:47, brettw wrote: > It's not clear to me why you rewrote this loop. To me, the new code seems > strictly worse than the old code, and even if it wasn't, this isn't the type of > thing that's worth doing. Greg probably did this based off my comments on similar code in extensions/ (I prefer this way because it's marginally faster to loop through the vector with an iter than to dereference all the different indices). But we don't need to change old code to match, and OWNERS preference takes priority.
https://codereview.chromium.org/359493005/diff/550001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/359493005/diff/550001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:586: for (std::vector<base::string16>::iterator iter = sorted_menu_titles.begin(); On 2014/08/04 20:23:02, Devlin wrote: > On 2014/08/04 20:17:47, brettw wrote: > > It's not clear to me why you rewrote this loop. To me, the new code seems > > strictly worse than the old code, and even if it wasn't, this isn't the type > of > > thing that's worth doing. > > Greg probably did this based off my comments on similar code in extensions/ (I > prefer this way because it's marginally faster to loop through the vector with > an iter than to dereference all the different indices). But we don't need to > change old code to match, and OWNERS preference takes priority. Yep, that was it. Since Devlin suggested these changes to code I borrowed from here, I thought I'd change it here as well (made a note of it in an earlier patch set). I'll go ahead and put this back to the way it was.
lgtm
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/359493005/5...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
https://codereview.chromium.org/359493005/diff/570001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/570001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:11: #include "chrome/common/extensions/api/context_menus.h" This appears to be the cause of the failed trybots-- this file does not exist on an android build. How exactly do I approach this situation? Should I just wrap the relevant code and include in an #ifndef for OS_ANDROID?
https://codereview.chromium.org/359493005/diff/570001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/570001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:11: #include "chrome/common/extensions/api/context_menus.h" On 2014/08/05 01:02:13, gpdavis wrote: > This appears to be the cause of the failed trybots-- this file does not exist on > an android build. > > How exactly do I approach this situation? Should I just wrap the relevant code > and include in an #ifndef for OS_ANDROID? Yes, sort of. Move this to the bottom of the include list, and wrap it in #if defined(ENABLE_EXTENSIONS) ... #endif
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/359493005/6...
https://codereview.chromium.org/359493005/diff/570001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/570001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:11: #include "chrome/common/extensions/api/context_menus.h" On 2014/08/05 01:06:35, Yoyo Zhou wrote: > On 2014/08/05 01:02:13, gpdavis wrote: > > This appears to be the cause of the failed trybots-- this file does not exist > on > > an android build. > > > > How exactly do I approach this situation? Should I just wrap the relevant > code > > and include in an #ifndef for OS_ANDROID? > > Yes, sort of. Move this to the bottom of the include list, and wrap it in > #if defined(ENABLE_EXTENSIONS) > ... > #endif Okay, cool. Don't I also need to wrap the code that uses the constant from this file (line 258)? And in the unittest file, I also use this constant. Does this need to be done there as well? (Sorry if this is a dumb question-- just not entirely sure I understand the environment in which unit tests are run)
https://codereview.chromium.org/359493005/diff/570001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/570001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:11: #include "chrome/common/extensions/api/context_menus.h" On 2014/08/05 01:52:28, gpdavis wrote: > On 2014/08/05 01:06:35, Yoyo Zhou wrote: > > On 2014/08/05 01:02:13, gpdavis wrote: > > > This appears to be the cause of the failed trybots-- this file does not > exist > > on > > > an android build. > > > > > > How exactly do I approach this situation? Should I just wrap the relevant > > code > > > and include in an #ifndef for OS_ANDROID? > > > > Yes, sort of. Move this to the bottom of the include list, and wrap it in > > #if defined(ENABLE_EXTENSIONS) > > ... > > #endif > > Okay, cool. Don't I also need to wrap the code that uses the constant from this > file (line 258)? Ah, sorry. Yes, you need to put that in an ifdef also. I suggest assigning a name to that constant, similar to lines 24-25, with an #else clause that sets it to 0. > And in the unittest file, I also use this constant. Does this need to be done > there as well? (Sorry if this is a dumb question-- just not entirely sure I > understand the environment in which unit tests are run) It's better to not compile the unittest at all on Android. It's already excluded in the appropriate .gyp file, so this will be fine.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/359493005/6...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Is the android_clang_dbg trybot flaky? The stdio gave a real compiler error before I made the changes in patchset 24, but now it says that a command timed out.
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/359493005/6...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Message was sent while issue was closed.
Change committed as 288418
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/459493002/ by tommycli@chromium.org. The reason for reverting is: This adds two new static initializers, and breaks the perf bot. See http://build.chromium.org/p/chromium/builders/Linux%20x64 Good: http://build.chromium.org/p/chromium/builders/Linux%20x64/builds/68460 Bad: http://build.chromium.org/p/chromium/builders/Linux%20x64/builds/68461 # Static initializers in src/out/Release/chrome: ... # context_menu_matcher.cc extensions::(anonymous namespace)::action_menu_top_level_limit # context_menu_matcher.cc __init_array_end+0xd950 ... # Found 36 static initializers in 8 files..
Message was sent while issue was closed.
https://codereview.chromium.org/359493005/diff/630001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/630001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:27: api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT; Is this the problematic static initializer that's causing trouble? How might I rework this so that it's not an issue?
https://codereview.chromium.org/359493005/diff/650001/chrome/browser/extensio... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/359493005/diff/650001/chrome/browser/extensio... chrome/browser/extensions/context_menu_matcher.cc:25: int GetActionMenuTopLevelLimit() { @kalman, Is this what you had in mind when you suggested I make this a function instead of static initialized data?
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/359493005/6...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40857) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46084) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by gpdavis.chromium@gmail.com
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/359493005/6...
Message was sent while issue was closed.
Change committed as 289211 |
