Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1042003:
Initial version of an experimental Extensions Context Menu API.... (Closed)
|
Created:
10 years, 9 months ago by asargent_no_longer_on_chrome Modified:
9 years, 7 months ago CC:
chromium-reviews, jam+cc_chromium.org, brettw+cc_chromium.org, ben+cc_chromium.org, Erik does not do reviews, Aaron Boodman, pam+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionInitial version of an experimental Extensions Context Menu API.
The proposal for the API is documented at:
http://dev.chromium.org/developers/design-documents/extensions/context-menu-api
Notable limitations in this initial implementation:
-No reliable way to get at the unique, actual node clicked on in contexts like
IMAGE/LINK/etc. We'll need to fix this in the long run - see the API proposal
page for some notes on this.
-No update or deleteAll methods ; the only way you can change items is to delete
by id and re-add.
-We aren't yet matching the UI goal of having the extension items at the
top level include the extension icon on the left. This will require a
refactoring of RenderViewContextMenu to steal some of the code from the
bookmarks bar context menus, which can display favicons.
-The only kind of parent->child menu that currently works is if you have
a single top-level parent, and only one level of children. (This is because
of how RenderViewContextMenu currently works)
-No browser tests that the menu items actually get drawn (will wait on those
until the above mentioned refactor is complete), or API tests (the API may
change a bit based on feedback, at which point I'll write more tests).
-Unit tests need to cover some more cases.
BUG=32363
TEST=Should be able to create context menus with this API.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42321
Patch Set 1 #Patch Set 2 : '' #
Total comments: 219
Patch Set 3 : '' #
Total comments: 10
Patch Set 4 : '' #
Total comments: 5
Messages
Total messages: 15 (0 generated)
Drive-by with minor test comments. By the way, please watch for the API test flakiness after you land this. http://codereview.chromium.org/1042003/diff/13001/14008 File chrome/browser/extensions/extension_menu_manager_unittest.cc (right): http://codereview.chromium.org/1042003/diff/13001/14008#newcode34 chrome/browser/extensions/extension_menu_manager_unittest.cc:34: std::string extension_id = "0123456789"; Please comment what is the intention of this initialization (which is going to be overwritten if the properties contain the right key). Is it the default value? http://codereview.chromium.org/1042003/diff/13001/14008#newcode57 chrome/browser/extensions/extension_menu_manager_unittest.cc:57: ASSERT_GT(id1, 0) << "id1 was " << id1; nit: No need for <<, ASSERT_GT will print needed info automatically. Similarly in later cases. http://codereview.chromium.org/1042003/diff/13001/14008#newcode268 chrome/browser/extensions/extension_menu_manager_unittest.cc:268: ASSERT_TRUE(params.is_editable == bool_tmp); ASSERT_EQ?
Overall, looks promising. I have a few requests for clarification, a few things I'm concerned about and a ton of simple nits (sorry). :) You should get someone to look at extension_process_bindings.js because I feel like I don't know the js bindings code well enough to judge if it is OK. http://codereview.chromium.org/1042003/diff/13001/14001 File chrome/app/chrome_dll_resource.h (right): http://codereview.chromium.org/1042003/diff/13001/14001#newcode277 chrome/app/chrome_dll_resource.h:277: #define IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST 49000 nit: Document http://codereview.chromium.org/1042003/diff/13001/14005 File chrome/browser/extensions/extension_context_menu_api.cc (right): http://codereview.chromium.org/1042003/diff/13001/14005#newcode25 chrome/browser/extensions/extension_context_menu_api.cc:25: ExtensionMenuItem::ContextList tmp_result; Why create this on the stack? Why not work on |result| directly? http://codereview.chromium.org/1042003/diff/13001/14005#newcode50 chrome/browser/extensions/extension_context_menu_api.cc:50: } nit: There are lots of one line if/else clauses in this function that don't need braces. http://codereview.chromium.org/1042003/diff/13001/14005#newcode69 chrome/browser/extensions/extension_context_menu_api.cc:69: return false; don't you need to set bad_message_ = true, since you are not using EXTENSION_FUNCTION_VALIDATE here? http://codereview.chromium.org/1042003/diff/13001/14005#newcode75 chrome/browser/extensions/extension_context_menu_api.cc:75: return false; Same here (and other locations below). Also, what if |enabled_contexts| includes values that |contexts| doesn't? http://codereview.chromium.org/1042003/diff/13001/14005#newcode95 chrome/browser/extensions/extension_context_menu_api.cc:95: error_ = "All menu items except for separators must have a title"; Add a const for this error (and others in this file). http://codereview.chromium.org/1042003/diff/13001/14005#newcode115 chrome/browser/extensions/extension_context_menu_api.cc:115: if (properties->GetInteger(kParentIdKey, &parent_id)) { why not HasKey() combined with EXTENSION_FUNCTION_VALIDATE? http://codereview.chromium.org/1042003/diff/13001/14005#newcode124 chrome/browser/extensions/extension_context_menu_api.cc:124: } Looks like you have some good candidates for testing in this file... http://codereview.chromium.org/1042003/diff/13001/14005#newcode149 chrome/browser/extensions/extension_context_menu_api.cc:149: return false; bad_message_ = true? http://codereview.chromium.org/1042003/diff/13001/14006 File chrome/browser/extensions/extension_menu_manager.cc (right): http://codereview.chromium.org/1042003/diff/13001/14006#newcode23 chrome/browser/extensions/extension_menu_manager.cc:23: children_.erase(i); Can child menus not have child menus? http://codereview.chromium.org/1042003/diff/13001/14006#newcode49 chrome/browser/extensions/extension_menu_manager.cc:49: for (MenuItemMap::iterator i = context_items_.begin(); Have you tried const_iterator with these loops (here and elsewhere)? http://codereview.chromium.org/1042003/diff/13001/14006#newcode76 chrome/browser/extensions/extension_menu_manager.cc:76: if (extension_id.size() == 0) { if (extension_id.empty()) return 0; (also no braces) http://codereview.chromium.org/1042003/diff/13001/14006#newcode79 chrome/browser/extensions/extension_menu_manager.cc:79: item->set_id(next_item_id_++); nit: newline before this line. Wait, didn't some of your tests test for id > 0? It seems like id can be zero, no? http://codereview.chromium.org/1042003/diff/13001/14006#newcode85 chrome/browser/extensions/extension_menu_manager.cc:85: } nit: you don't need braces, your teeth look fine. http://codereview.chromium.org/1042003/diff/13001/14006#newcode101 chrome/browser/extensions/extension_menu_manager.cc:101: bool ExtensionMenuManager::RemoveContextMenuItem(int id) { Same question here with the extension_id. I haven't looked at the usage, just wondering if you can pass in the extension_id to speed up the search? http://codereview.chromium.org/1042003/diff/13001/14006#newcode111 chrome/browser/extensions/extension_menu_manager.cc:111: return true; Pardon my ignorance, but I don't understand this logic... If the id matches, we erase the item, otherwise we RemoveChild? Waa? This warrants at least a comment... http://codereview.chromium.org/1042003/diff/13001/14006#newcode131 chrome/browser/extensions/extension_menu_manager.cc:131: break; Do we have a guarantee that items cannot be inserted in-between RADIO items? http://codereview.chromium.org/1042003/diff/13001/14006#newcode146 chrome/browser/extensions/extension_menu_manager.cc:146: } Wait... Isn't this failing to clear the last radio button in the list? http://codereview.chromium.org/1042003/diff/13001/14006#newcode147 chrome/browser/extensions/extension_menu_manager.cc:147: } This function is another good candidate for testing... (test multiple items with first one selected, middle one selected and last one selected). http://codereview.chromium.org/1042003/diff/13001/14006#newcode156 chrome/browser/extensions/extension_menu_manager.cc:156: size_t* index) { This function essentially iterates over everything. Is it possible to pass in the extension_id also to at least reduce the load, or do the callers not always know the id? http://codereview.chromium.org/1042003/diff/13001/14006#newcode169 chrome/browser/extensions/extension_menu_manager.cc:169: } And if not found? Shouldn't you clear the index and item? http://codereview.chromium.org/1042003/diff/13001/14006#newcode190 chrome/browser/extensions/extension_menu_manager.cc:190: } nit: No braces. http://codereview.chromium.org/1042003/diff/13001/14006#newcode209 chrome/browser/extensions/extension_menu_manager.cc:209: default: {} // Do nothing. Should we list the ones we don't care about and DCHECK on default so we know when new ones are added? http://codereview.chromium.org/1042003/diff/13001/14006#newcode214 chrome/browser/extensions/extension_menu_manager.cc:214: AddURLProperty(properties, L"mainFrameUrl", params.page_url); why not call this pageUrl? http://codereview.chromium.org/1042003/diff/13001/14006#newcode233 chrome/browser/extensions/extension_menu_manager.cc:233: // true. I had to read this twice to figure out what you meant. Can you change this to something like: "Executing a RADIO item results in radio item being marked as checked, but executing a CHECKBOX item can mean check/uncheck, depending on its current state (in other words: CHECKBOX toggles when executed)". http://codereview.chromium.org/1042003/diff/13001/14006#newcode254 chrome/browser/extensions/extension_menu_manager.cc:254: return; NOTREACH it too? http://codereview.chromium.org/1042003/diff/13001/14007 File chrome/browser/extensions/extension_menu_manager.h (right): http://codereview.chromium.org/1042003/diff/13001/14007#newcode25 chrome/browser/extensions/extension_menu_manager.h:25: // Represents an item added by an extension in a menu. How about: Represents a menu item, added by an extension to the RenderView context menu. http://codereview.chromium.org/1042003/diff/13001/14007#newcode28 chrome/browser/extensions/extension_menu_manager.h:28: enum Context { nit: Document this (and other enum). http://codereview.chromium.org/1042003/diff/13001/14007#newcode46 chrome/browser/extensions/extension_menu_manager.h:46: class ContextList { nit: Document. http://codereview.chromium.org/1042003/diff/13001/14007#newcode50 chrome/browser/extensions/extension_menu_manager.h:50: Add(context); nit: Why initialize to 0 and call Add? Why not just initialize to |context|? http://codereview.chromium.org/1042003/diff/13001/14007#newcode61 chrome/browser/extensions/extension_menu_manager.h:61: void Add(Context context) { nit: Add newline above? http://codereview.chromium.org/1042003/diff/13001/14007#newcode66 chrome/browser/extensions/extension_menu_manager.h:66: uint32 value_; Document (especially that this is a bitmask). http://codereview.chromium.org/1042003/diff/13001/14007#newcode69 chrome/browser/extensions/extension_menu_manager.h:69: ExtensionMenuItem(std::string extension_id, std::string title, bool checked, const std::string& http://codereview.chromium.org/1042003/diff/13001/14007#newcode74 chrome/browser/extensions/extension_menu_manager.h:74: enabled_contexts_(enabled_contexts), parent_id_(0) {} These initializers need to be one on each line. Also, DCHECK on (!checked || type == NORMAL)? Read next comment first, though. http://codereview.chromium.org/1042003/diff/13001/14007#newcode76 chrome/browser/extensions/extension_menu_manager.h:76: virtual ~ExtensionMenuItem() {} Can you move the ctor and dtor to the .cc file? (see longer comment below) http://codereview.chromium.org/1042003/diff/13001/14007#newcode79 chrome/browser/extensions/extension_menu_manager.h:79: virtual const std::string& extension_id() { return extension_id_; } does this need to be virtual? http://codereview.chromium.org/1042003/diff/13001/14007#newcode81 chrome/browser/extensions/extension_menu_manager.h:81: virtual int id() const { return id_; } same here http://codereview.chromium.org/1042003/diff/13001/14007#newcode87 chrome/browser/extensions/extension_menu_manager.h:87: bool checked() { return checked_; } not const? http://codereview.chromium.org/1042003/diff/13001/14007#newcode89 chrome/browser/extensions/extension_menu_manager.h:89: ExtensionMenuItem* child_at(int index) const { isn't it easier to just make this take a size_t? http://codereview.chromium.org/1042003/diff/13001/14007#newcode112 chrome/browser/extensions/extension_menu_manager.h:112: return true; I forget... at what point do we stop using unix_hacker style function names? :) (same for child_at(...)) http://codereview.chromium.org/1042003/diff/13001/14007#newcode119 chrome/browser/extensions/extension_menu_manager.h:119: } I'm not a big fan of implementing so many functions in the .h file. If it is just returning a pointer to a member variable, then fine, but once you start having multi-line function bodies then I think they should be moved to the .cc file. There is a tendency for functions to grow, yet people have an aversion to moving functions between files so they just cause .h bloat. I also think that functions in .cc files are easier to find during debugging and they don't cause the world to be rebuilt when I need to change the implementation. http://codereview.chromium.org/1042003/diff/13001/14007#newcode122 chrome/browser/extensions/extension_menu_manager.h:122: ExtensionMenuItem() {} nit: This should be at the top of the protected clause (maybe below the friend statement). http://codereview.chromium.org/1042003/diff/13001/14007#newcode135 chrome/browser/extensions/extension_menu_manager.h:135: std::vector<linked_ptr<ExtensionMenuItem> > children_; How about documenting those member variables? :) http://codereview.chromium.org/1042003/diff/13001/14007#newcode136 chrome/browser/extensions/extension_menu_manager.h:136: DISALLOW_COPY_AND_ASSIGN(ExtensionMenuItem); nit: New line above DISALLOW... http://codereview.chromium.org/1042003/diff/13001/14007#newcode140 chrome/browser/extensions/extension_menu_manager.h:140: class ExtensionMenuManager : public NotificationObserver { nit: Remove extra line break above and document (the class, not the lack of line break). :) http://codereview.chromium.org/1042003/diff/13001/14007#newcode143 chrome/browser/extensions/extension_menu_manager.h:143: ~ExtensionMenuManager(); virtual, for readability. (base class defines it virtual) http://codereview.chromium.org/1042003/diff/13001/14007#newcode146 chrome/browser/extensions/extension_menu_manager.h:146: // particular order. "Returns (in no particular order) the ids..." (no apostrophe). http://codereview.chromium.org/1042003/diff/13001/14007#newcode150 chrome/browser/extensions/extension_menu_manager.h:150: // *not* including child items. If an extension defines more than one item, it gets shunted to a submenu, right? Will this return the top-level menu with that submenu or the top level items within the submenu? Worth documenting, I would think. http://codereview.chromium.org/1042003/diff/13001/14007#newcode154 chrome/browser/extensions/extension_menu_manager.h:154: // Returns the id assigned to the item. Document what it does also, and the next few ones? :) One thing to note is that it has the side effect of incrementing the next_item_id_ counter (or mention it assigns an id automatically). http://codereview.chromium.org/1042003/diff/13001/14007#newcode162 chrome/browser/extensions/extension_menu_manager.h:162: ExtensionMenuItem* GetItemById(int id); Have you checked which functions in this class can be const? http://codereview.chromium.org/1042003/diff/13001/14007#newcode174 chrome/browser/extensions/extension_menu_manager.h:174: // items in the same group (ie that are adjacent in the list). The |index| nit: period after ie. http://codereview.chromium.org/1042003/diff/13001/14007#newcode187 chrome/browser/extensions/extension_menu_manager.h:187: int next_item_id_; nit: Document http://codereview.chromium.org/1042003/diff/13001/14007#newcode189 chrome/browser/extensions/extension_menu_manager.h:189: DISALLOW_COPY_AND_ASSIGN(ExtensionMenuManager); nit: New line before DISALLOW. http://codereview.chromium.org/1042003/diff/13001/14008 File chrome/browser/extensions/extension_menu_manager_unittest.cc (right): http://codereview.chromium.org/1042003/diff/13001/14008#newcode33 chrome/browser/extensions/extension_menu_manager_unittest.cc:33: static ExtensionMenuItem* GetTestItem(DictionaryValue* properties) { This is not a GetTestItem but a CreateTestItem, no? You should document whether the caller is responsible for cleaning up the return value. http://codereview.chromium.org/1042003/diff/13001/14008#newcode48 chrome/browser/extensions/extension_menu_manager_unittest.cc:48: ExtensionMenuManager manager_; Wait? Where is the value for manager_ set? nit: For completeness, add DISALLOW_COPY_AND_ASSIGN also? http://codereview.chromium.org/1042003/diff/13001/14008#newcode54 chrome/browser/extensions/extension_menu_manager_unittest.cc:54: ExtensionMenuItem* item1 = GetTestItem(NULL); You never clean up item1. I presume AddContextItem takes ownership of it? http://codereview.chromium.org/1042003/diff/13001/14008#newcode62 chrome/browser/extensions/extension_menu_manager_unittest.cc:62: ASSERT_EQ(item1, items[0]); You never check if the content of what you inserted has changed. I guess you are just transferring ownership of pointers, so maybe there is no need to. What do you think? http://codereview.chromium.org/1042003/diff/13001/14008#newcode81 chrome/browser/extensions/extension_menu_manager_unittest.cc:81: ASSERT_EQ(NULL, manager_.GetItemById(id3)); nit: why not check the items.size() == 2 here also? (what if 'Remove one' accidentally removes all?) http://codereview.chromium.org/1042003/diff/13001/14008#newcode108 chrome/browser/extensions/extension_menu_manager_unittest.cc:108: item3 = NULL; use a scoped_ptr instead. http://codereview.chromium.org/1042003/diff/13001/14008#newcode113 chrome/browser/extensions/extension_menu_manager_unittest.cc:113: nit: don't need the extra newline http://codereview.chromium.org/1042003/diff/13001/14008#newcode120 chrome/browser/extensions/extension_menu_manager_unittest.cc:120: ASSERT_EQ(item2, manager_.MenuItems(item2->extension_id()).at(0)); Why not check item2->child_count() == 1? (and item1->child_count() == 0) http://codereview.chromium.org/1042003/diff/13001/14008#newcode130 chrome/browser/extensions/extension_menu_manager_unittest.cc:130: // unloaded. This got me thinking... Does everything work as expected if the extension gets auto-updated while the menu is showing? (not necessarily a showstopper for this CL in particular, but good to file a bug if it doesn't work). http://codereview.chromium.org/1042003/diff/13001/14008#newcode144 chrome/browser/extensions/extension_menu_manager_unittest.cc:144: ASSERT_TRUE(extension.InitFromValue(extension_properties, false, &errors)) << nit: false is... what again? Maybe comment? http://codereview.chromium.org/1042003/diff/13001/14008#newcode147 chrome/browser/extensions/extension_menu_manager_unittest.cc:147: // Create a ExtensionMenuItem and put it into the manager. nit: a->an http://codereview.chromium.org/1042003/diff/13001/14008#newcode185 chrome/browser/extensions/extension_menu_manager_unittest.cc:185: virtual ~MockExtensionMessageService() {} ExtensionMessageService already has a virtual private dtor. DISABLE_COPY_AND_ASSIGN? http://codereview.chromium.org/1042003/diff/13001/14008#newcode190 chrome/browser/extensions/extension_menu_manager_unittest.cc:190: virtual ~MockTestingProfile() {} TestingProfile already has a virtual destructor. http://codereview.chromium.org/1042003/diff/13001/14008#newcode192 chrome/browser/extensions/extension_menu_manager_unittest.cc:192: MOCK_METHOD0(IsOffTheRecord, bool()); nit: DISABLE_COPY_AND_ASSIGN? http://codereview.chromium.org/1042003/diff/13001/14008#newcode204 chrome/browser/extensions/extension_menu_manager_unittest.cc:204: TestTabContents tab_contents(&profile, NULL); nit: NULL is for... what again? :) http://codereview.chromium.org/1042003/diff/13001/14011 File chrome/browser/extensions/extensions_service.h (right): http://codereview.chromium.org/1042003/diff/13001/14011#newcode286 chrome/browser/extensions/extensions_service.h:286: ExtensionMenuManager* MenuManager() { nit: document http://codereview.chromium.org/1042003/diff/13001/14011#newcode362 chrome/browser/extensions/extensions_service.h:362: scoped_ptr<ExtensionMenuManager> menu_manager_; nit: document. http://codereview.chromium.org/1042003/diff/13001/14013 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/1042003/diff/13001/14013#newcode5 chrome/browser/tab_contents/render_view_context_menu.cc:5: #include <functional> Ah... finally. It was totally non-functional up to this point. ;) http://codereview.chromium.org/1042003/diff/13001/14013#newcode82 chrome/browser/tab_contents/render_view_context_menu.cc:82: return true; You had the concept of "disabled/enabled context", do you not need to consult that here? also nit: Here you actually need braces (Multi-line 'if' condition needs braces for if/else) http://codereview.chromium.org/1042003/diff/13001/14013#newcode95 chrome/browser/tab_contents/render_view_context_menu.cc:95: break; Same question here, should we list all the items and NOTREACH on default, to know when a new item is added? http://codereview.chromium.org/1042003/diff/13001/14013#newcode100 chrome/browser/tab_contents/render_view_context_menu.cc:100: contexts.Contains(ExtensionMenuItem::PAGE)) I think it warrants a comment why has_link and all that must be false for PAGE to be considered. http://codereview.chromium.org/1042003/diff/13001/14013#newcode107 chrome/browser/tab_contents/render_view_context_menu.cc:107: class SortByExtensionName This class seems like general purpose class that should not be part of this file (moved to another shared location). http://codereview.chromium.org/1042003/diff/13001/14013#newcode120 chrome/browser/tab_contents/render_view_context_menu.cc:120: return extension1->name() < extension2->name(); I worry this is not appropriate for i18n extensions... I think sorting order would be incorrect. http://codereview.chromium.org/1042003/diff/13001/14013#newcode124 chrome/browser/tab_contents/render_view_context_menu.cc:124: ExtensionsService* service_; DISALLOW_COPY_... ? http://codereview.chromium.org/1042003/diff/13001/14013#newcode127 chrome/browser/tab_contents/render_view_context_menu.cc:127: void RenderViewContextMenu::GetItemsForExtension( Does this belong as a static function in ExtensionMenuManager.cc or something? http://codereview.chromium.org/1042003/diff/13001/14013#newcode135 chrome/browser/tab_contents/render_view_context_menu.cc:135: service->MenuManager()->MenuItems(extension_id); nit: 4 col indentation, not 2. http://codereview.chromium.org/1042003/diff/13001/14013#newcode155 chrome/browser/tab_contents/render_view_context_menu.cc:155: if (first_item->child_count() > 0) { I had to reread this a few times to figure this out. I think you are trying to respect the top level menu if the extension creates one, but use extension name if not. This warrants a comment (I think). http://codereview.chromium.org/1042003/diff/13001/14013#newcode163 chrome/browser/tab_contents/render_view_context_menu.cc:163: nit: extra newline. http://codereview.chromium.org/1042003/diff/13001/14013#newcode174 chrome/browser/tab_contents/render_view_context_menu.cc:174: } This whole 'if' clause part I don't understand. Expand comment? http://codereview.chromium.org/1042003/diff/13001/14013#newcode184 chrome/browser/tab_contents/render_view_context_menu.cc:184: if (!extension || *index >= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST) what if index <= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST? Or < 0, I guess, since we seem to be passing 0. In any case, it seems like we should DCHECK on the range. http://codereview.chromium.org/1042003/diff/13001/14013#newcode206 chrome/browser/tab_contents/render_view_context_menu.cc:206: if (submenu_started) { Why is this dependent on a submenu being started? Maybe document that? http://codereview.chromium.org/1042003/diff/13001/14013#newcode226 chrome/browser/tab_contents/render_view_context_menu.cc:226: last_type = ExtensionMenuItem::NORMAL; Why not delete these last_type lines and put last_type = item->type() at the bottom. Seems less error prone... ? http://codereview.chromium.org/1042003/diff/13001/14013#newcode231 chrome/browser/tab_contents/render_view_context_menu.cc:231: // Auto-append a separator if needed to group radio items together. You mean auto-prepend, not append. Also, comma before 'if' and after 'needed'. http://codereview.chromium.org/1042003/diff/13001/14013#newcode234 chrome/browser/tab_contents/render_view_context_menu.cc:234: AppendSeparator(); nit: need braces. http://codereview.chromium.org/1042003/diff/13001/14013#newcode245 chrome/browser/tab_contents/render_view_context_menu.cc:245: } nit: Don't need braces. http://codereview.chromium.org/1042003/diff/13001/14013#newcode251 chrome/browser/tab_contents/render_view_context_menu.cc:251: ExtensionMenuManager* manager = service ? service->MenuManager() : NULL; manager is a bit general, can you call it menu_manager? http://codereview.chromium.org/1042003/diff/13001/14013#newcode259 chrome/browser/tab_contents/render_view_context_menu.cc:259: SortByExtensionName(service)); Hmm... Name sort. Hope we don't see extension with weird names to fight over top position. :) Wonder if extension install date (if we had that) would be more appropriate - or sorting on a one-way hash of the name or something. Just wondering out loud... :) Feel free to ignore. http://codereview.chromium.org/1042003/diff/13001/14015 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/1042003/diff/13001/14015#newcode2330 chrome/common/extensions/api/extension_api.json:2330: "description": "An experimental API to add context menu items.", nit: I'm not a big fan of annotating the description with "experimental". I think the namespace says it all and when you change the namespace you don't have to go hunting for more references to experimental to take out. http://codereview.chromium.org/1042003/diff/13001/14015#newcode2345 chrome/common/extensions/api/extension_api.json:2345: }, nit: I would put this parameter first, since you refer to it in the description for other items. http://codereview.chromium.org/1042003/diff/13001/14015#newcode2356 chrome/common/extensions/api/extension_api.json:2356: "description": "List of contexts this menu item will appear in. Legal values are: ALL,PAGE,SELECTION,LINK,EDITABLE,IMAGE,VIDEO,AUDIO. Defaults to ['PAGE']." nit: Should these values have quotes around them, signifying that they are not const values? http://codereview.chromium.org/1042003/diff/13001/14015#newcode2364 chrome/common/extensions/api/extension_api.json:2364: }, From reading this, I don't quite get what this does. It seems that this makes some of the contexts enabled (and the excluded ones will get disabled), but I don't see a function to enable contexts. http://codereview.chromium.org/1042003/diff/13001/14015#newcode2374 chrome/common/extensions/api/extension_api.json:2374: } Looks like all of these parameters are optional. Makes me wonder what happens if you specify nothing (just make a note to add it to test maybe, if not there already). :) Wait... why is onclick optional? Does it make sense to add a menu item with no handler? I guess it does, but only if you are adding a separator. It would be good to test, though. (this is the first file I'm looking at, so I don't yet know what the rest of the CL contains). :) http://codereview.chromium.org/1042003/diff/13001/14016 File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/1042003/diff/13001/14016#newcode176 chrome/renderer/resources/extension_process_bindings.js:176: } nit: no braces http://codereview.chromium.org/1042003/diff/13001/14016#newcode304 chrome/renderer/resources/extension_process_bindings.js:304: var menuItemId = arguments[0].menuItemId; where does arguments come from? http://codereview.chromium.org/1042003/diff/13001/14016#newcode308 chrome/renderer/resources/extension_process_bindings.js:308: } nit: no braces http://codereview.chromium.org/1042003/diff/13001/14016#newcode313 chrome/renderer/resources/extension_process_bindings.js:313: } nit: no braces http://codereview.chromium.org/1042003/diff/13001/14016#newcode582 chrome/renderer/resources/extension_process_bindings.js:582: } nit: no braces http://codereview.chromium.org/1042003/diff/13001/14016#newcode597 chrome/renderer/resources/extension_process_bindings.js:597: } nit: no braces
And there are some compile errors in the unit test, looks like...
Ok, I believe I've addressed all of the comments (phew!). Finnur - I'll get Rafael to take a look at the extension_process_bindings.js parts. http://codereview.chromium.org/1042003/diff/13001/14001 File chrome/app/chrome_dll_resource.h (right): http://codereview.chromium.org/1042003/diff/13001/14001#newcode277 chrome/app/chrome_dll_resource.h:277: #define IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST 49000 On 2010/03/17 21:01:04, Finnur wrote: > nit: Document Done. http://codereview.chromium.org/1042003/diff/13001/14005 File chrome/browser/extensions/extension_context_menu_api.cc (right): http://codereview.chromium.org/1042003/diff/13001/14005#newcode25 chrome/browser/extensions/extension_context_menu_api.cc:25: ExtensionMenuItem::ContextList tmp_result; On 2010/03/17 21:01:04, Finnur wrote: > Why create this on the stack? Why not work on |result| directly? In the case of errors (where I return false) I wanted to keep |result| unchanged. http://codereview.chromium.org/1042003/diff/13001/14005#newcode50 chrome/browser/extensions/extension_context_menu_api.cc:50: } On 2010/03/17 21:01:04, Finnur wrote: > nit: There are lots of one line if/else clauses in this function that don't need > braces. Done. http://codereview.chromium.org/1042003/diff/13001/14005#newcode69 chrome/browser/extensions/extension_context_menu_api.cc:69: return false; On 2010/03/17 21:01:04, Finnur wrote: > don't you need to set bad_message_ = true, since you are not using > EXTENSION_FUNCTION_VALIDATE here? Good call. I created a new macro called EXTENSION_FUNCTION_ERROR which sets bad_message_ to true, sets the error message, and returns false. http://codereview.chromium.org/1042003/diff/13001/14005#newcode75 chrome/browser/extensions/extension_context_menu_api.cc:75: return false; On 2010/03/17 21:01:04, Finnur wrote: > Same here (and other locations below). Done. > Also, what if |enabled_contexts| includes values that |contexts| doesn't? Essentially they will be ignored. I suppose I could generate an error, but I wasn't sure if it was worth it since nothing will break; they just won't be displayed. http://codereview.chromium.org/1042003/diff/13001/14005#newcode95 chrome/browser/extensions/extension_context_menu_api.cc:95: error_ = "All menu items except for separators must have a title"; On 2010/03/17 21:01:04, Finnur wrote: > Add a const for this error (and others in this file). Done. http://codereview.chromium.org/1042003/diff/13001/14005#newcode115 chrome/browser/extensions/extension_context_menu_api.cc:115: if (properties->GetInteger(kParentIdKey, &parent_id)) { On 2010/03/17 21:01:04, Finnur wrote: > why not HasKey() combined with EXTENSION_FUNCTION_VALIDATE? Done. http://codereview.chromium.org/1042003/diff/13001/14005#newcode124 chrome/browser/extensions/extension_context_menu_api.cc:124: } On 2010/03/17 21:01:04, Finnur wrote: > Looks like you have some good candidates for testing in this file... Yep. I'd like to hold off on some of that for now because the API may be undergoing changes based on developer feedback. http://codereview.chromium.org/1042003/diff/13001/14005#newcode149 chrome/browser/extensions/extension_context_menu_api.cc:149: return false; On 2010/03/17 21:01:04, Finnur wrote: > bad_message_ = true? Done. http://codereview.chromium.org/1042003/diff/13001/14006 File chrome/browser/extensions/extension_menu_manager.cc (right): http://codereview.chromium.org/1042003/diff/13001/14006#newcode23 chrome/browser/extensions/extension_menu_manager.cc:23: children_.erase(i); On 2010/03/17 21:01:04, Finnur wrote: > Can child menus not have child menus? Good catch - the UI doesn't support it yet, but AddChild doesn't prevent it. I've also added a unit test for this. http://codereview.chromium.org/1042003/diff/13001/14006#newcode49 chrome/browser/extensions/extension_menu_manager.cc:49: for (MenuItemMap::iterator i = context_items_.begin(); On 2010/03/17 21:01:04, Finnur wrote: > Have you tried const_iterator with these loops (here and elsewhere)? It added it here and in one or two other cases. http://codereview.chromium.org/1042003/diff/13001/14006#newcode76 chrome/browser/extensions/extension_menu_manager.cc:76: if (extension_id.size() == 0) { On 2010/03/17 21:01:04, Finnur wrote: > if (extension_id.empty()) > return 0; > > (also no braces) Done. http://codereview.chromium.org/1042003/diff/13001/14006#newcode79 chrome/browser/extensions/extension_menu_manager.cc:79: item->set_id(next_item_id_++); On 2010/03/17 21:01:04, Finnur wrote: > nit: newline before this line. > > Wait, didn't some of your tests test for id > 0? It seems like id can be zero, > no? The incoming item should have id == 0 meaning it is currently not added anythere, so I've added a DCHECK for that. http://codereview.chromium.org/1042003/diff/13001/14006#newcode85 chrome/browser/extensions/extension_menu_manager.cc:85: } On 2010/03/17 21:01:04, Finnur wrote: > nit: you don't need braces, your teeth look fine. Done. http://codereview.chromium.org/1042003/diff/13001/14006#newcode101 chrome/browser/extensions/extension_menu_manager.cc:101: bool ExtensionMenuManager::RemoveContextMenuItem(int id) { On 2010/03/17 21:01:04, Finnur wrote: > Same question here with the extension_id. I haven't looked at the usage, just > wondering if you can pass in the extension_id to speed up the search? I'm not as worried about remove, but I did end up putting in a cache mapping id to item so that GetItemById is fast. http://codereview.chromium.org/1042003/diff/13001/14006#newcode111 chrome/browser/extensions/extension_menu_manager.cc:111: return true; On 2010/03/17 21:01:04, Finnur wrote: > Pardon my ignorance, but I don't understand this logic... If the id matches, we > erase the item, otherwise we RemoveChild? Waa? This warrants at least a > comment... Added a comment that we call RemoveChild here because the item might be a child of one of the top-level items. http://codereview.chromium.org/1042003/diff/13001/14006#newcode131 chrome/browser/extensions/extension_menu_manager.cc:131: break; On 2010/03/17 21:01:04, Finnur wrote: > Do we have a guarantee that items cannot be inserted in-between RADIO items? I consider a radio group to be a set of contiguous radio items - adding a separator/normal/checkbox node implicitly creates a new radio group. http://codereview.chromium.org/1042003/diff/13001/14006#newcode146 chrome/browser/extensions/extension_menu_manager.cc:146: } On 2010/03/17 21:01:04, Finnur wrote: > Wait... Isn't this failing to clear the last radio button in the list? Nope. http://codereview.chromium.org/1042003/diff/13001/14006#newcode147 chrome/browser/extensions/extension_menu_manager.cc:147: } On 2010/03/17 21:01:04, Finnur wrote: > This function is another good candidate for testing... (test multiple items with > first one selected, middle one selected and last one selected). Will do that in a subsequent pass if that's ok. http://codereview.chromium.org/1042003/diff/13001/14006#newcode156 chrome/browser/extensions/extension_menu_manager.cc:156: size_t* index) { On 2010/03/17 21:01:04, Finnur wrote: > This function essentially iterates over everything. Is it possible to pass in > the extension_id also to at least reduce the load, or do the callers not always > know the id? See response above ; I think leaving as-is for now is ok. http://codereview.chromium.org/1042003/diff/13001/14006#newcode169 chrome/browser/extensions/extension_menu_manager.cc:169: } On 2010/03/17 21:01:04, Finnur wrote: > And if not found? Shouldn't you clear the index and item? That's probably good hygiene. Done. http://codereview.chromium.org/1042003/diff/13001/14006#newcode190 chrome/browser/extensions/extension_menu_manager.cc:190: } On 2010/03/17 21:01:04, Finnur wrote: > nit: No braces. Done. http://codereview.chromium.org/1042003/diff/13001/14006#newcode209 chrome/browser/extensions/extension_menu_manager.cc:209: default: {} // Do nothing. On 2010/03/17 21:01:04, Finnur wrote: > Should we list the ones we don't care about and DCHECK on default so we know > when new ones are added? As I noted before, I don't think so. http://codereview.chromium.org/1042003/diff/13001/14006#newcode214 chrome/browser/extensions/extension_menu_manager.cc:214: AddURLProperty(properties, L"mainFrameUrl", params.page_url); On 2010/03/17 21:01:04, Finnur wrote: > why not call this pageUrl? I originally did, but then on a chromium-dev thread Adam Barth suggested "mainFrameUrl" as being a little more precise. http://codereview.chromium.org/1042003/diff/13001/14006#newcode233 chrome/browser/extensions/extension_menu_manager.cc:233: // true. On 2010/03/17 21:01:04, Finnur wrote: > I had to read this twice to figure out what you meant. Can you change this to > something like: "Executing a RADIO item results in radio item being marked as > checked, but executing a CHECKBOX item can mean check/uncheck, depending on its > current state (in other words: CHECKBOX toggles when executed)". Done. http://codereview.chromium.org/1042003/diff/13001/14006#newcode254 chrome/browser/extensions/extension_menu_manager.cc:254: return; On 2010/03/17 21:01:04, Finnur wrote: > NOTREACH it too? Done. http://codereview.chromium.org/1042003/diff/13001/14007 File chrome/browser/extensions/extension_menu_manager.h (right): http://codereview.chromium.org/1042003/diff/13001/14007#newcode25 chrome/browser/extensions/extension_menu_manager.h:25: // Represents an item added by an extension in a menu. On 2010/03/17 21:01:04, Finnur wrote: > How about: Represents a menu item, added by an extension to the RenderView > context menu. Hmm.. I'm sort of thinking these two classes can be used for other kinds of menu items like the page/wrench menu in the future if we choose to support that. But I did reword the comment slightly. http://codereview.chromium.org/1042003/diff/13001/14007#newcode28 chrome/browser/extensions/extension_menu_manager.h:28: enum Context { On 2010/03/17 21:01:04, Finnur wrote: > nit: Document this (and other enum). Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode46 chrome/browser/extensions/extension_menu_manager.h:46: class ContextList { On 2010/03/17 21:01:04, Finnur wrote: > nit: Document. Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode50 chrome/browser/extensions/extension_menu_manager.h:50: Add(context); On 2010/03/17 21:01:04, Finnur wrote: > nit: Why initialize to 0 and call Add? Why not just initialize to |context|? Good point - that's simpler. Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode61 chrome/browser/extensions/extension_menu_manager.h:61: void Add(Context context) { On 2010/03/17 21:01:04, Finnur wrote: > nit: Add newline above? Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode66 chrome/browser/extensions/extension_menu_manager.h:66: uint32 value_; On 2010/03/17 21:01:04, Finnur wrote: > Document (especially that this is a bitmask). Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode69 chrome/browser/extensions/extension_menu_manager.h:69: ExtensionMenuItem(std::string extension_id, std::string title, bool checked, On 2010/03/17 21:01:04, Finnur wrote: > const std::string& Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode74 chrome/browser/extensions/extension_menu_manager.h:74: enabled_contexts_(enabled_contexts), parent_id_(0) {} On 2010/03/17 21:01:04, Finnur wrote: > These initializers need to be one on each line. Huh, the style guide seems slightly ambiguous about this, but I guess you're right. > Also, DCHECK on (!checked || type == NORMAL)? > > Read next comment first, though. Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode76 chrome/browser/extensions/extension_menu_manager.h:76: virtual ~ExtensionMenuItem() {} On 2010/03/17 21:01:04, Finnur wrote: > Can you move the ctor and dtor to the .cc file? (see longer comment below) Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode79 chrome/browser/extensions/extension_menu_manager.h:79: virtual const std::string& extension_id() { return extension_id_; } On 2010/03/17 21:01:04, Finnur wrote: > does this need to be virtual? Nope - I think I had started writing a unit test where I mocked this out but waved off of that and forgot to clean this up. http://codereview.chromium.org/1042003/diff/13001/14007#newcode81 chrome/browser/extensions/extension_menu_manager.h:81: virtual int id() const { return id_; } On 2010/03/17 21:01:04, Finnur wrote: > same here Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode87 chrome/browser/extensions/extension_menu_manager.h:87: bool checked() { return checked_; } On 2010/03/17 21:01:04, Finnur wrote: > not const? Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode89 chrome/browser/extensions/extension_menu_manager.h:89: ExtensionMenuItem* child_at(int index) const { On 2010/03/17 21:01:04, Finnur wrote: > isn't it easier to just make this take a size_t? http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types "Don't use an unsigned type." http://codereview.chromium.org/1042003/diff/13001/14007#newcode112 chrome/browser/extensions/extension_menu_manager.h:112: return true; On 2010/03/17 21:01:04, Finnur wrote: > I forget... at what point do we stop using unix_hacker style function names? :) > (same for child_at(...)) Good point - fixed. http://codereview.chromium.org/1042003/diff/13001/14007#newcode119 chrome/browser/extensions/extension_menu_manager.h:119: } On 2010/03/17 21:01:04, Finnur wrote: > I'm not a big fan of implementing so many functions in the .h file. If it is > just returning a pointer to a member variable, then fine, but once you start > having multi-line function bodies then I think they should be moved to the .cc > file. There is a tendency for functions to grow, yet people have an aversion to > moving functions between files so they just cause .h bloat. I also think that > functions in .cc files are easier to find during debugging and they don't cause > the world to be rebuilt when I need to change the implementation. Good point - I've moved these. http://codereview.chromium.org/1042003/diff/13001/14007#newcode122 chrome/browser/extensions/extension_menu_manager.h:122: ExtensionMenuItem() {} On 2010/03/17 21:01:04, Finnur wrote: > nit: This should be at the top of the protected clause (maybe below the friend > statement). Removed it since it turned out it wasn't needed. http://codereview.chromium.org/1042003/diff/13001/14007#newcode135 chrome/browser/extensions/extension_menu_manager.h:135: std::vector<linked_ptr<ExtensionMenuItem> > children_; On 2010/03/17 21:01:04, Finnur wrote: > How about documenting those member variables? :) Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode136 chrome/browser/extensions/extension_menu_manager.h:136: DISALLOW_COPY_AND_ASSIGN(ExtensionMenuItem); On 2010/03/17 21:01:04, Finnur wrote: > nit: New line above DISALLOW... Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode140 chrome/browser/extensions/extension_menu_manager.h:140: class ExtensionMenuManager : public NotificationObserver { On 2010/03/17 21:01:04, Finnur wrote: > nit: Remove extra line break above and document (the class, not the lack of line > break). :) Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode143 chrome/browser/extensions/extension_menu_manager.h:143: ~ExtensionMenuManager(); On 2010/03/17 21:01:04, Finnur wrote: > virtual, for readability. (base class defines it virtual) Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode146 chrome/browser/extensions/extension_menu_manager.h:146: // particular order. On 2010/03/17 21:01:04, Finnur wrote: > "Returns (in no particular order) the ids..." (no apostrophe). I switched this to returning a std::set - that is more self-documenting that there's no ordering. :) http://codereview.chromium.org/1042003/diff/13001/14007#newcode150 chrome/browser/extensions/extension_menu_manager.h:150: // *not* including child items. On 2010/03/17 21:01:04, Finnur wrote: > If an extension defines more than one item, it gets shunted to a submenu, right? > Will this return the top-level menu with that submenu or the top level items > within the submenu? Worth documenting, I would think. Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode154 chrome/browser/extensions/extension_menu_manager.h:154: // Returns the id assigned to the item. On 2010/03/17 21:01:04, Finnur wrote: > Document what it does also, and the next few ones? :) > > One thing to note is that it has the side effect of incrementing the > next_item_id_ counter (or mention it assigns an id automatically). Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode162 chrome/browser/extensions/extension_menu_manager.h:162: ExtensionMenuItem* GetItemById(int id); On 2010/03/17 21:01:04, Finnur wrote: > Have you checked which functions in this class can be const? Hmm.. I guess that's sort of an existential question - for GetItemById for instance, while calling that does not in and of itself change the ExtensionMenuManager, it purposefully doesn't return a const pointer so you could change the ExtensionMenuItem return value. Does that count as a change to the menu manager itself? http://codereview.chromium.org/1042003/diff/13001/14007#newcode174 chrome/browser/extensions/extension_menu_manager.h:174: // items in the same group (ie that are adjacent in the list). The |index| On 2010/03/17 21:01:04, Finnur wrote: > nit: period after ie. Ah, I looked this up, and it's actually two periods! "i.e." http://codereview.chromium.org/1042003/diff/13001/14007#newcode187 chrome/browser/extensions/extension_menu_manager.h:187: int next_item_id_; On 2010/03/17 21:01:04, Finnur wrote: > nit: Document Done. http://codereview.chromium.org/1042003/diff/13001/14007#newcode189 chrome/browser/extensions/extension_menu_manager.h:189: DISALLOW_COPY_AND_ASSIGN(ExtensionMenuManager); On 2010/03/17 21:01:04, Finnur wrote: > nit: New line before DISALLOW. Done. http://codereview.chromium.org/1042003/diff/13001/14008 File chrome/browser/extensions/extension_menu_manager_unittest.cc (right): http://codereview.chromium.org/1042003/diff/13001/14008#newcode33 chrome/browser/extensions/extension_menu_manager_unittest.cc:33: static ExtensionMenuItem* GetTestItem(DictionaryValue* properties) { On 2010/03/17 21:01:04, Finnur wrote: > This is not a GetTestItem but a CreateTestItem, no? You should document whether > the caller is responsible for cleaning up the return value. Good point. Renamed. http://codereview.chromium.org/1042003/diff/13001/14008#newcode34 chrome/browser/extensions/extension_menu_manager_unittest.cc:34: std::string extension_id = "0123456789"; On 2010/03/17 06:44:44, Paweł Hajdan Jr. wrote: > Please comment what is the intention of this initialization (which is going to > be overwritten if the properties contain the right key). Is it the default > value? I added a comment that this is just a default dummy value. http://codereview.chromium.org/1042003/diff/13001/14008#newcode48 chrome/browser/extensions/extension_menu_manager_unittest.cc:48: ExtensionMenuManager manager_; > Wait? Where is the value for manager_ set? Right here - the contructor has no arguments. :) > nit: For completeness, add DISALLOW_COPY_AND_ASSIGN also? Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode54 chrome/browser/extensions/extension_menu_manager_unittest.cc:54: ExtensionMenuItem* item1 = GetTestItem(NULL); On 2010/03/17 21:01:04, Finnur wrote: > You never clean up item1. I presume AddContextItem takes ownership of it? Yes, I've added a comment to the header file for AddContextItem that ownership is transferred, and added a note here too. http://codereview.chromium.org/1042003/diff/13001/14008#newcode57 chrome/browser/extensions/extension_menu_manager_unittest.cc:57: ASSERT_GT(id1, 0) << "id1 was " << id1; On 2010/03/17 06:44:44, Paweł Hajdan Jr. wrote: > nit: No need for <<, ASSERT_GT will print needed info automatically. Similarly > in later cases. Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode62 chrome/browser/extensions/extension_menu_manager_unittest.cc:62: ASSERT_EQ(item1, items[0]); On 2010/03/17 21:01:04, Finnur wrote: > You never check if the content of what you inserted has changed. I guess you are > just transferring ownership of pointers, so maybe there is no need to. What do > you think? I think it's ok - the primary job of the Menu Manager is holding the items, assigning id's, and removing them on-demand or when extensions get unloaded. http://codereview.chromium.org/1042003/diff/13001/14008#newcode81 chrome/browser/extensions/extension_menu_manager_unittest.cc:81: ASSERT_EQ(NULL, manager_.GetItemById(id3)); On 2010/03/17 21:01:04, Finnur wrote: > nit: why not check the items.size() == 2 here also? (what if 'Remove one' > accidentally removes all?) Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode108 chrome/browser/extensions/extension_menu_manager_unittest.cc:108: item3 = NULL; On 2010/03/17 21:01:04, Finnur wrote: > use a scoped_ptr instead. Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode113 chrome/browser/extensions/extension_menu_manager_unittest.cc:113: On 2010/03/17 21:01:04, Finnur wrote: > nit: don't need the extra newline Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode120 chrome/browser/extensions/extension_menu_manager_unittest.cc:120: ASSERT_EQ(item2, manager_.MenuItems(item2->extension_id()).at(0)); On 2010/03/17 21:01:04, Finnur wrote: > Why not check item2->child_count() == 1? (and item1->child_count() == 0) Done (added the check a few lines up near the call to AddChildItem). http://codereview.chromium.org/1042003/diff/13001/14008#newcode130 chrome/browser/extensions/extension_menu_manager_unittest.cc:130: // unloaded. On 2010/03/17 21:01:04, Finnur wrote: > This got me thinking... Does everything work as expected if the extension gets > auto-updated while the menu is showing? (not necessarily a showstopper for this > CL in particular, but good to file a bug if it doesn't work). Yes it should - anyone calling GetItemById() on the ExtensionMenuManager needs to be prepared to get back NULL either because the extension was unloaded or the extension removed that item, and I was careful to do that check everywhere in render_view_context_menu.cc. http://codereview.chromium.org/1042003/diff/13001/14008#newcode144 chrome/browser/extensions/extension_menu_manager_unittest.cc:144: ASSERT_TRUE(extension.InitFromValue(extension_properties, false, &errors)) << On 2010/03/17 21:01:04, Finnur wrote: > nit: false is... what again? Maybe comment? Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode147 chrome/browser/extensions/extension_menu_manager_unittest.cc:147: // Create a ExtensionMenuItem and put it into the manager. On 2010/03/17 21:01:04, Finnur wrote: > nit: a->an Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode185 chrome/browser/extensions/extension_menu_manager_unittest.cc:185: virtual ~MockExtensionMessageService() {} On 2010/03/17 21:01:04, Finnur wrote: > ExtensionMessageService already has a virtual private dtor. The MOCK_METHOD macros introduce virtual methods, so according to the style guide there should be a virtual destructor. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml "Make your destructor virtual if necessary. If your class has virtual methods, its destructor should be virtual." > DISABLE_COPY_AND_ASSIGN? Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode190 chrome/browser/extensions/extension_menu_manager_unittest.cc:190: virtual ~MockTestingProfile() {} On 2010/03/17 21:01:04, Finnur wrote: > TestingProfile already has a virtual destructor. Same as above - the MOCK_METHOD macros introduce virtual methods. http://codereview.chromium.org/1042003/diff/13001/14008#newcode192 chrome/browser/extensions/extension_menu_manager_unittest.cc:192: MOCK_METHOD0(IsOffTheRecord, bool()); On 2010/03/17 21:01:04, Finnur wrote: > nit: DISABLE_COPY_AND_ASSIGN? Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode204 chrome/browser/extensions/extension_menu_manager_unittest.cc:204: TestTabContents tab_contents(&profile, NULL); On 2010/03/17 21:01:04, Finnur wrote: > nit: NULL is for... what again? :) Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode268 chrome/browser/extensions/extension_menu_manager_unittest.cc:268: ASSERT_TRUE(params.is_editable == bool_tmp); On 2010/03/17 06:44:44, Paweł Hajdan Jr. wrote: > ASSERT_EQ? Done. http://codereview.chromium.org/1042003/diff/13001/14011 File chrome/browser/extensions/extensions_service.h (right): http://codereview.chromium.org/1042003/diff/13001/14011#newcode286 chrome/browser/extensions/extensions_service.h:286: ExtensionMenuManager* MenuManager() { On 2010/03/17 21:01:04, Finnur wrote: > nit: document Done. I also renamed this to menu_manager() and moved it next to some of the other simple getter methods just above. http://codereview.chromium.org/1042003/diff/13001/14011#newcode362 chrome/browser/extensions/extensions_service.h:362: scoped_ptr<ExtensionMenuManager> menu_manager_; On 2010/03/17 21:01:04, Finnur wrote: > nit: document. Done. http://codereview.chromium.org/1042003/diff/13001/14013 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/1042003/diff/13001/14013#newcode82 chrome/browser/tab_contents/render_view_context_menu.cc:82: return true; On 2010/03/17 21:01:04, Finnur wrote: > You had the concept of "disabled/enabled context", do you not need to consult > that here? That's covered separately for showing (does it get added in the first place) and enabling (RenderViewContextMenu::IsItemCommandEnabled). > also nit: Here you actually need braces (Multi-line 'if' condition needs braces > for if/else) Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode95 chrome/browser/tab_contents/render_view_context_menu.cc:95: break; On 2010/03/17 21:01:04, Finnur wrote: > Same question here, should we list all the items and NOTREACH on default, to > know when a new item is added? I think it's better to leave new things out by default, and explicitly add in new ones as it makes sense (developers ask for them, etc.). The person doing a webkit merge isn't necessarily in the best place to make the call about whether a new type of media deserves explicit support in the extensions API or not. http://codereview.chromium.org/1042003/diff/13001/14013#newcode100 chrome/browser/tab_contents/render_view_context_menu.cc:100: contexts.Contains(ExtensionMenuItem::PAGE)) On 2010/03/17 21:01:04, Finnur wrote: > I think it warrants a comment why has_link and all that must be false for PAGE > to be considered. Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode107 chrome/browser/tab_contents/render_view_context_menu.cc:107: class SortByExtensionName On 2010/03/17 21:01:04, Finnur wrote: > This class seems like general purpose class that should not be part of this file > (moved to another shared location). I simplified the sorting code a bit and din't need this anymore, so it's removed. http://codereview.chromium.org/1042003/diff/13001/14013#newcode120 chrome/browser/tab_contents/render_view_context_menu.cc:120: return extension1->name() < extension2->name(); On 2010/03/17 21:01:04, Finnur wrote: > I worry this is not appropriate for i18n extensions... I think sorting order > would be incorrect. It looks like we do a similar sort (in javascript) for the extensions management page. I've added a TODO to look into this more. http://codereview.chromium.org/1042003/diff/13001/14013#newcode124 chrome/browser/tab_contents/render_view_context_menu.cc:124: ExtensionsService* service_; On 2010/03/17 21:01:04, Finnur wrote: > DISALLOW_COPY_... ? Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode127 chrome/browser/tab_contents/render_view_context_menu.cc:127: void RenderViewContextMenu::GetItemsForExtension( On 2010/03/17 21:01:04, Finnur wrote: > Does this belong as a static function in ExtensionMenuManager.cc or something? It's sort of a judgement call, but this class (RenderViewContextMenu) has all the logic for deciding which of the regular context items to show, so it kind of seems like it should decide which extension items to show too. http://codereview.chromium.org/1042003/diff/13001/14013#newcode135 chrome/browser/tab_contents/render_view_context_menu.cc:135: service->MenuManager()->MenuItems(extension_id); On 2010/03/17 21:01:04, Finnur wrote: > nit: 4 col indentation, not 2. Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode155 chrome/browser/tab_contents/render_view_context_menu.cc:155: if (first_item->child_count() > 0) { On 2010/03/17 21:01:04, Finnur wrote: > I had to reread this a few times to figure this out. I think you are trying to > respect the top level menu if the extension creates one, but use extension name > if not. This warrants a comment (I think). There's a comment to this effect in the .h file where the function is defined - do you think I should move that here instead? http://codereview.chromium.org/1042003/diff/13001/14013#newcode163 chrome/browser/tab_contents/render_view_context_menu.cc:163: On 2010/03/17 21:01:04, Finnur wrote: > nit: extra newline. Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode174 chrome/browser/tab_contents/render_view_context_menu.cc:174: } On 2010/03/17 21:01:04, Finnur wrote: > This whole 'if' clause part I don't understand. Expand comment? Like the question above, perhaps this makes more sense after reading the comment in the header file about the goal of the function? http://codereview.chromium.org/1042003/diff/13001/14013#newcode184 chrome/browser/tab_contents/render_view_context_menu.cc:184: if (!extension || *index >= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST) On 2010/03/17 21:01:04, Finnur wrote: > what if index <= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST? Or < 0, I guess, since we > seem to be passing 0. > In any case, it seems like we should DCHECK on the range. Oh, this got me to look at this more carefully and fix a bug - since index is an offset relative to IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST I need to compare it to the difference between _LAST and _FIRST. http://codereview.chromium.org/1042003/diff/13001/14013#newcode206 chrome/browser/tab_contents/render_view_context_menu.cc:206: if (submenu_started) { On 2010/03/17 21:01:04, Finnur wrote: > Why is this dependent on a submenu being started? Maybe document that? Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode226 chrome/browser/tab_contents/render_view_context_menu.cc:226: last_type = ExtensionMenuItem::NORMAL; On 2010/03/17 21:01:04, Finnur wrote: > Why not delete these last_type lines and put last_type = item->type() at the > bottom. Seems less error prone... ? Good suggestion, done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode231 chrome/browser/tab_contents/render_view_context_menu.cc:231: // Auto-append a separator if needed to group radio items together. On 2010/03/17 21:01:04, Finnur wrote: > You mean auto-prepend, not append. Also, comma before 'if' and after 'needed'. Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode234 chrome/browser/tab_contents/render_view_context_menu.cc:234: AppendSeparator(); On 2010/03/17 21:01:04, Finnur wrote: > nit: need braces. Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode245 chrome/browser/tab_contents/render_view_context_menu.cc:245: } On 2010/03/17 21:01:04, Finnur wrote: > nit: Don't need braces. Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode251 chrome/browser/tab_contents/render_view_context_menu.cc:251: ExtensionMenuManager* manager = service ? service->MenuManager() : NULL; On 2010/03/17 21:01:04, Finnur wrote: > manager is a bit general, can you call it menu_manager? Done. http://codereview.chromium.org/1042003/diff/13001/14013#newcode259 chrome/browser/tab_contents/render_view_context_menu.cc:259: SortByExtensionName(service)); On 2010/03/17 21:01:04, Finnur wrote: > Hmm... Name sort. Hope we don't see extension with weird names to fight over top > position. :) Wonder if extension install date (if we had that) would be more > appropriate - or sorting on a one-way hash of the name or something. Just > wondering out loud... :) Feel free to ignore. I talked this over with Glen, and his take was sort by name was the least bad option. "AAAA+ RSS Subscription Extension" here we come! http://codereview.chromium.org/1042003/diff/13001/14015 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/1042003/diff/13001/14015#newcode2330 chrome/common/extensions/api/extension_api.json:2330: "description": "An experimental API to add context menu items.", On 2010/03/17 21:01:04, Finnur wrote: > nit: I'm not a big fan of annotating the description with "experimental". I > think the namespace says it all and when you change the namespace you don't have > to go hunting for more references to experimental to take out. Done. http://codereview.chromium.org/1042003/diff/13001/14015#newcode2345 chrome/common/extensions/api/extension_api.json:2345: }, On 2010/03/17 21:01:04, Finnur wrote: > nit: I would put this parameter first, since you refer to it in the description > for other items. Done. http://codereview.chromium.org/1042003/diff/13001/14015#newcode2356 chrome/common/extensions/api/extension_api.json:2356: "description": "List of contexts this menu item will appear in. Legal values are: ALL,PAGE,SELECTION,LINK,EDITABLE,IMAGE,VIDEO,AUDIO. Defaults to ['PAGE']." On 2010/03/17 21:01:04, Finnur wrote: > nit: Should these values have quotes around them, signifying that they are not > const values? Done. http://codereview.chromium.org/1042003/diff/13001/14015#newcode2364 chrome/common/extensions/api/extension_api.json:2364: }, On 2010/03/17 21:01:04, Finnur wrote: > From reading this, I don't quite get what this does. It seems that this makes > some of the contexts enabled (and the excluded ones will get disabled), but I > don't see a function to enable contexts. I reworded the comment to try and make this more clear. http://codereview.chromium.org/1042003/diff/13001/14015#newcode2374 chrome/common/extensions/api/extension_api.json:2374: } On 2010/03/17 21:01:04, Finnur wrote: > Looks like all of these parameters are optional. Makes me wonder what happens if > you specify nothing (just make a note to add it to test maybe, if not there > already). :) > > Wait... why is onclick optional? Does it make sense to add a menu item with no > handler? I guess it does, but only if you are adding a separator. It would be > good to test, though. (this is the first file I'm looking at, so I don't yet > know what the rest of the CL contains). :) I doubt this will cause problems for developers, and we allow page/browser actions with no onclick listeners and no default popup. http://codereview.chromium.org/1042003/diff/13001/14016 File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/1042003/diff/13001/14016#newcode176 chrome/renderer/resources/extension_process_bindings.js:176: } On 2010/03/17 21:01:04, Finnur wrote: > nit: no braces Done. http://codereview.chromium.org/1042003/diff/13001/14016#newcode304 chrome/renderer/resources/extension_process_bindings.js:304: var menuItemId = arguments[0].menuItemId; On 2010/03/17 21:01:04, Finnur wrote: > where does arguments come from? arguments is a magic javascript feature that gives you a handle to an array containing the arguments to a function. https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Functions_and_... http://codereview.chromium.org/1042003/diff/13001/14016#newcode308 chrome/renderer/resources/extension_process_bindings.js:308: } On 2010/03/17 21:01:04, Finnur wrote: > nit: no braces Done. http://codereview.chromium.org/1042003/diff/13001/14016#newcode313 chrome/renderer/resources/extension_process_bindings.js:313: } On 2010/03/17 21:01:04, Finnur wrote: > nit: no braces Done. http://codereview.chromium.org/1042003/diff/13001/14016#newcode582 chrome/renderer/resources/extension_process_bindings.js:582: } On 2010/03/17 21:01:04, Finnur wrote: > nit: no braces Done. http://codereview.chromium.org/1042003/diff/13001/14016#newcode597 chrome/renderer/resources/extension_process_bindings.js:597: } On 2010/03/17 21:01:04, Finnur wrote: > nit: no braces Done.
The bits I commented in the drive-by LGTM.
Minor issues... http://codereview.chromium.org/1042003/diff/13001/14008 File chrome/browser/extensions/extension_menu_manager_unittest.cc (right): http://codereview.chromium.org/1042003/diff/13001/14008#newcode33 chrome/browser/extensions/extension_menu_manager_unittest.cc:33: static ExtensionMenuItem* GetTestItem(DictionaryValue* properties) { nit: And the document for who cleans this up? On 2010/03/21 19:50:16, Antony Sargent wrote: > On 2010/03/17 21:01:04, Finnur wrote: > > This is not a GetTestItem but a CreateTestItem, no? You should document > whether > > the caller is responsible for cleaning up the return value. > Good point. Renamed. > http://codereview.chromium.org/1042003/diff/13001/14008#newcode185 chrome/browser/extensions/extension_menu_manager_unittest.cc:185: virtual ~MockExtensionMessageService() {} > The MOCK_METHOD macros introduce virtual methods, so according to the style > guide there should be a virtual destructor. No argument there. I'm just pointing out that the base class already has a virtual private dtor, so you don't need to add an empty one here... http://codereview.chromium.org/1042003/diff/13001/14008#newcode190 chrome/browser/extensions/extension_menu_manager_unittest.cc:190: virtual ~MockTestingProfile() {} Same reply here as above. http://codereview.chromium.org/1042003/diff/13001/14008#newcode204 chrome/browser/extensions/extension_menu_manager_unittest.cc:204: TestTabContents tab_contents(&profile, NULL); > Done. What is done exactly? :) http://codereview.chromium.org/1042003/diff/13001/14013 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/1042003/diff/13001/14013#newcode155 chrome/browser/tab_contents/render_view_context_menu.cc:155: if (first_item->child_count() > 0) { The comment doesn't mention the extension name being used. I guess that should be added to the .h file... On 2010/03/21 19:50:16, Antony Sargent wrote: > On 2010/03/17 21:01:04, Finnur wrote: > > I had to reread this a few times to figure this out. I think you are trying to > > respect the top level menu if the extension creates one, but use extension > name > > if not. This warrants a comment (I think). > There's a comment to this effect in the .h file where the function is defined - > do you think I should move that here instead? http://codereview.chromium.org/1042003/diff/13001/14013#newcode234 chrome/browser/tab_contents/render_view_context_menu.cc:234: AppendSeparator(); Done? :) http://codereview.chromium.org/1042003/diff/13001/14016 File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/1042003/diff/13001/14016#newcode304 chrome/renderer/resources/extension_process_bindings.js:304: var menuItemId = arguments[0].menuItemId; Oh, JavaScript. You so crazy. :) On 2010/03/21 19:50:16, Antony Sargent wrote: > On 2010/03/17 21:01:04, Finnur wrote: > > where does arguments come from? > > arguments is a magic javascript feature that gives you a handle to an array > containing the arguments to a function. > > https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Functions_and_... http://codereview.chromium.org/1042003/diff/29001/30008 File chrome/browser/extensions/extension_menu_manager.h (right): http://codereview.chromium.org/1042003/diff/29001/30008#newcode32 chrome/browser/extensions/extension_menu_manager.h:32: // For context menus, these are the contexts an item can appear/be enabled in. nit: in -> for? http://codereview.chromium.org/1042003/diff/29001/30008#newcode207 chrome/browser/extensions/extension_menu_manager.h:207: // This lets us make lookup by id fast. Can you document this a little further? Basically, this maps every single MenuItem id to an actual MenuItem*, across all extensions (including child items). http://codereview.chromium.org/1042003/diff/29001/30013 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/1042003/diff/29001/30013#newcode164 chrome/browser/tab_contents/render_view_context_menu.cc:164: DCHECK(index && *index >= 0); DCHECK(pointer); pointer->foo(); ... is not worth DCHECKing on. It just adds bloat to the debug build. I would therefore just DCHECK(*index >= 0); http://codereview.chromium.org/1042003/diff/29001/30013#newcode236 chrome/browser/tab_contents/render_view_context_menu.cc:236: return; Maybe document when these can be null? http://codereview.chromium.org/1042003/diff/29001/30015 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/1042003/diff/29001/30015#newcode2396 chrome/common/extensions/api/extension_api.json:2396: "description": "By default the values you pass for the contexts parameter make an item both shown and selectable in those contexts. If you want to limit the contexts where an item is selectable (i.e. not greyed out), you put the ones you want selectable in enabledContexts and any not listed will be shown but disabled. So for example if I wanted an item to appear for links and images but only be enabled for links, I would set 'contexts' : ['LINK', 'IMAGE'] and 'enabledContexts' : ['LINK']." nit: So for example if ... -> So, for example, if ... if I ... -> if you ... I would ... -> you would ...
Finnur - all comments addressed, I believe. Rafael - could you take a look at the changes to extension_process_bindings.js? This is pretty much what I had showed you last week with perhaps one or two slight differences. http://codereview.chromium.org/1042003/diff/13001/14008 File chrome/browser/extensions/extension_menu_manager_unittest.cc (right): http://codereview.chromium.org/1042003/diff/13001/14008#newcode33 chrome/browser/extensions/extension_menu_manager_unittest.cc:33: static ExtensionMenuItem* GetTestItem(DictionaryValue* properties) { On 2010/03/22 17:18:12, Finnur wrote: > nit: And the document for who cleans this up? > > On 2010/03/21 19:50:16, Antony Sargent wrote: > > On 2010/03/17 21:01:04, Finnur wrote: > > > This is not a GetTestItem but a CreateTestItem, no? You should document > > whether > > > the caller is responsible for cleaning up the return value. > > Good point. Renamed. > > > > Done. http://codereview.chromium.org/1042003/diff/13001/14008#newcode185 chrome/browser/extensions/extension_menu_manager_unittest.cc:185: virtual ~MockExtensionMessageService() {} On 2010/03/22 17:18:12, Finnur wrote: > > The MOCK_METHOD macros introduce virtual methods, so according to the style > > guide there should be a virtual destructor. > > No argument there. I'm just pointing out that the base class already has a > virtual private dtor, so you don't need to add an empty one here... Ok, removed it. http://codereview.chromium.org/1042003/diff/13001/14008#newcode190 chrome/browser/extensions/extension_menu_manager_unittest.cc:190: virtual ~MockTestingProfile() {} On 2010/03/22 17:18:12, Finnur wrote: > Same reply here as above. Ok, removed it. http://codereview.chromium.org/1042003/diff/13001/14008#newcode204 chrome/browser/extensions/extension_menu_manager_unittest.cc:204: TestTabContents tab_contents(&profile, NULL); On 2010/03/22 17:18:12, Finnur wrote: > > Done. > > What is done exactly? :) > Oops, I had put in a comment for this and then accidentally removed it at some point. It's back now. http://codereview.chromium.org/1042003/diff/13001/14013 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/1042003/diff/13001/14013#newcode155 chrome/browser/tab_contents/render_view_context_menu.cc:155: if (first_item->child_count() > 0) { On 2010/03/22 17:18:12, Finnur wrote: > The comment doesn't mention the extension name being used. I guess that should > be added to the .h file... Done. http://codereview.chromium.org/1042003/diff/29001/30008 File chrome/browser/extensions/extension_menu_manager.h (right): http://codereview.chromium.org/1042003/diff/29001/30008#newcode32 chrome/browser/extensions/extension_menu_manager.h:32: // For context menus, these are the contexts an item can appear/be enabled in. On 2010/03/22 17:18:13, Finnur wrote: > nit: in -> for? Reworded it slightly. http://codereview.chromium.org/1042003/diff/29001/30008#newcode207 chrome/browser/extensions/extension_menu_manager.h:207: // This lets us make lookup by id fast. On 2010/03/22 17:18:13, Finnur wrote: > Can you document this a little further? Basically, this maps every single > MenuItem id to an actual MenuItem*, across all extensions (including child > items). Done. http://codereview.chromium.org/1042003/diff/29001/30013 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/1042003/diff/29001/30013#newcode164 chrome/browser/tab_contents/render_view_context_menu.cc:164: DCHECK(index && *index >= 0); On 2010/03/22 17:18:13, Finnur wrote: > DCHECK(pointer); > pointer->foo(); > > ... is not worth DCHECKing on. It just adds bloat to the debug build. I would > therefore just DCHECK(*index >= 0); Done. http://codereview.chromium.org/1042003/diff/29001/30013#newcode236 chrome/browser/tab_contents/render_view_context_menu.cc:236: return; On 2010/03/22 17:18:13, Finnur wrote: > Maybe document when these can be null? It turns out in the early extensions days when they were turned off by default behind a flag, the ExtensionsService could be null, but that's no longer the case. So I just removed these checks. http://codereview.chromium.org/1042003/diff/29001/30015 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/1042003/diff/29001/30015#newcode2396 chrome/common/extensions/api/extension_api.json:2396: "description": "By default the values you pass for the contexts parameter make an item both shown and selectable in those contexts. If you want to limit the contexts where an item is selectable (i.e. not greyed out), you put the ones you want selectable in enabledContexts and any not listed will be shown but disabled. So for example if I wanted an item to appear for links and images but only be enabled for links, I would set 'contexts' : ['LINK', 'IMAGE'] and 'enabledContexts' : ['LINK']." On 2010/03/22 17:18:13, Finnur wrote: > nit: > So for example if ... -> So, for example, if ... > if I ... -> if you ... > I would ... -> you would ... Done.
LGTM
lgtm w/ nits on extension_process_bindings.js http://codereview.chromium.org/1042003/diff/51001/52016 File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/1042003/diff/51001/52016#newcode83 chrome/renderer/resources/extension_process_bindings.js:83: if (request.customCallback) need curly braces http://codereview.chromium.org/1042003/diff/51001/52016#newcode305 chrome/renderer/resources/extension_process_bindings.js:305: if (onclick) need curly braces http://codereview.chromium.org/1042003/diff/51001/52016#newcode310 chrome/renderer/resources/extension_process_bindings.js:310: if (parentOnclick) need curly braces http://codereview.chromium.org/1042003/diff/51001/52016#newcode578 chrome/renderer/resources/extension_process_bindings.js:578: if (chrome.extension.lastError || !response) need curly braces http://codereview.chromium.org/1042003/diff/51001/52016#newcode593 chrome/renderer/resources/extension_process_bindings.js:593: if (chromeHidden.contextMenuHandlers[menuItemId]) don't need the test here. you can just delete. (otherwise would need curly braces =-)
p.s. http://www.corp.google.com/eng/doc/javascriptguide.html#curlybraces On Mon, Mar 22, 2010 at 3:27 PM, <[email protected]> wrote: > lgtm w/ nits on extension_process_bindings.js > > > http://codereview.chromium.org/1042003/diff/51001/52016 > File chrome/renderer/resources/extension_process_bindings.js (right): > > http://codereview.chromium.org/1042003/diff/51001/52016#newcode83 > chrome/renderer/resources/extension_process_bindings.js:83: if > (request.customCallback) > need curly braces > > http://codereview.chromium.org/1042003/diff/51001/52016#newcode305 > chrome/renderer/resources/extension_process_bindings.js:305: if > (onclick) > need curly braces > > http://codereview.chromium.org/1042003/diff/51001/52016#newcode310 > chrome/renderer/resources/extension_process_bindings.js:310: if > (parentOnclick) > need curly braces > > http://codereview.chromium.org/1042003/diff/51001/52016#newcode578 > chrome/renderer/resources/extension_process_bindings.js:578: if > (chrome.extension.lastError || !response) > need curly braces > > http://codereview.chromium.org/1042003/diff/51001/52016#newcode593 > chrome/renderer/resources/extension_process_bindings.js:593: if > (chromeHidden.contextMenuHandlers[menuItemId]) > don't need the test here. you can just delete. (otherwise would need > curly braces =-) > > http://codereview.chromium.org/1042003 >
Is it possible to update the displayed 'string' in a menu item while it is showing? For instance, a user may right-click on selected text, which initiates a lookup of that text in a dictionary. If an entry is found, the menu item text would change to "See dictionary entry for %s". But if it isn't found, then it would change to "No dictionary entry found for %s." In addition, the update of the menu item may depend on other externally obtained information that is retrieved while the menu is showing. This is done asynchronously to not diminish the UI responsiveness. Will Chrome allow that? We use this functionality in our Firefox and IE versions to great effect.
It's possible we'll eventually support something like this, but not initially (the existing context menu code is mostly setup to display static contents, so there would need to be a larger refactoring to make this possible). On Tue, Mar 23, 2010 at 1:23 PM, <[email protected]> wrote: > > Is it possible to update the displayed 'string' in a menu item while it is > showing? For instance, a user may right-click on selected text, which > initiates > a lookup of that text in a dictionary. If an entry is found, the menu item > text > would change to "See dictionary entry for %s". But if it isn't found, then > it > would change to "No dictionary entry found for %s." In addition, the update > of > the menu item may depend on other externally obtained information that is > retrieved while the menu is showing. > > This is done asynchronously to not diminish the UI responsiveness. Will > Chrome > allow that? > > We use this functionality in our Firefox and IE versions to great effect. > > > http://codereview.chromium.org/1042003 >
Thanks for the reply. Just to clarify: the initial version also will not support the displaying/hiding of entire menu entries based on what the user selected on the page (because supporting that would require a 'beforeShowing' callback?) In our extension, we allow users to configure what they wish to see in the context menu based on what is selected. For instance, if a user selects a DOI (10.xxxx/yyyy), an entry appears to handle DOIs. If a user selects an ISBN or ISSN, we recognize the checksum and offer an appropriate option. It would be unfortunate to lose this functionality in our Chrome version. On 2010/03/23 17:40:04, Antony Sargent wrote: > It's possible we'll eventually support something like this, but not > initially (the existing context menu code is mostly setup to display static > contents, so there would need to be a larger refactoring to make this > possible). > > On Tue, Mar 23, 2010 at 1:23 PM, <mailto:[email protected]> wrote: > > > > > Is it possible to update the displayed 'string' in a menu item while it is > > showing?
Right, that is not currently supported by the initial version. Thanks for the feedback! Hearing about more use cases like this will help us weigh how we might modify the API. FYI, I'm about to send out a request for feedback to the [email protected] mailing list, so let's move further conversation there. On Tue, Mar 23, 2010 at 5:48 PM, <[email protected]> wrote: > > Thanks for the reply. > > Just to clarify: the initial version also will not support the > displaying/hiding > of entire menu entries based on what the user selected on the page (because > supporting that would require a 'beforeShowing' callback?) > > In our extension, we allow users to configure what they wish to see in the > context menu based on what is selected. For instance, if a user selects a > DOI > (10.xxxx/yyyy), an entry appears to handle DOIs. If a user selects an ISBN > or > ISSN, we recognize the checksum and offer an appropriate option. It would > be > unfortunate to lose this functionality in our Chrome version. > > > On 2010/03/23 17:40:04, Antony Sargent wrote: > >> It's possible we'll eventually support something like this, but not >> initially (the existing context menu code is mostly setup to display >> static >> contents, so there would need to be a larger refactoring to make this >> possible). >> > > On Tue, Mar 23, 2010 at 1:23 PM, <mailto:[email protected]> wrote: >> > > > >> > Is it possible to update the displayed 'string' in a menu item while it >> is >> > showing? >> > > > http://codereview.chromium.org/1042003 > |
