Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12299013:
Fix top-level context menus sorting by name (Closed)
|
Created:
7 years, 10 months ago by François Beaufort Modified:
7 years, 6 months ago CC:
chromium-reviews, Aaron Boodman, ajwong+watch_chromium.org, creis+watch_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFix top-level context menus sorting by name
BUG=174878
TEST=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205793
Patch Set 1 #
Total comments: 22
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : Added GetRelevantExtensionTopLevelItems() #
Total comments: 13
Patch Set 6 : Addressed style guide issues #
Total comments: 5
Patch Set 7 : Added localization to context menu titles sorting #Patch Set 8 : Used const #
Total comments: 9
Patch Set 9 : Fixed last nits #Patch Set 10 : Removed hard CHECK #
Total comments: 3
Patch Set 11 : I have no excuse here... #Messages
Total messages: 41 (0 generated)
As agreed at crbug.com/174878, here's a patch to fix top-level context menus sorting by name. You'll notice a TODO(fbeaufort) since I'm not sure about a UMA I'm updating.
Thanks for the patch! If you haven't already, please review the Chromium style guide: http://dev.chromium.org/developers/coding-style https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.cc:101: std::string ContextMenuMatcher::GetTopLevelContextMenuTitle( This duplicates code in AppendExtensionItems. Can you make that function use the logic here, so the code isn't duplicated? https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.cc:112: // Return extension name if there are no items Shouldn't this function never be called for extensions that have no items? You ought to be able to change this to CHECK(all_items && !all_items->empty()); https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.cc:125: kMaxExtensionItemTitleLength)); nit: if you continue function arguments onto the next line like this, indentation should be to the open paren. Since it won't fit, start the first argument on a new line (at +4 spaces). https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.h:44: // based on a printable selection text nit: Comments should end with a period. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:369: for (size_t position = printable_selection_text.find('&'); This is more complex than necessary. Use ReplaceChars - see https://chromiumcodereview.appspot.com/12294022/ (but that one is *also* more complex than necessary, see my comment). https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:374: // Get a list of extension id's that have context menu items, and sort it by delete 'it' https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:381: // AppendPlatformAppItems. Does AppendPlatformAppItems have the same sorting bug as in the case of extensions? You don't need to fix it in this CL, but it may be worthwhile to file a bug for it. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:383: std::string menu_title = extension_items_.GetTopLevelContextMenuTitle(*i, nit: put *i on the next line. You can put it on the same line as printable_selection_text. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:396: // TODO(fbeaufort): Where to move this UMA now? I think it's okay to leave it here. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:400: extension_items_.AppendExtensionItems(i->second, printable_selection_text, I don't see how this can behave equivalently to the old version. printable_selection_text never changes so it seems that you would append many copies of the same text.
Thank you for reviewing this code. You'll notice a new clause in an "if" to check if the number of items is not zero after checking incognito mode. See line 118 of chrome/browser/extensions/context_menu_matcher.cc file. I'm wondering actually if it's needed or not. Let me know what you think of it https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.cc:101: std::string ContextMenuMatcher::GetTopLevelContextMenuTitle( I'm not sure about how to implement the private function and would like your input on this. Do you think of a function that would take 3 parameters "extension_id", "extension*" and "items*" and sets "extension*" and "items*" OR something else? On 2013/02/19 19:00:50, Yoyo Zhou wrote: > This duplicates code in AppendExtensionItems. Can you make that function use the > logic here, so the code isn't duplicated? https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.cc:112: // Return extension name if there are no items On 2013/02/19 19:00:50, Yoyo Zhou wrote: > Shouldn't this function never be called for extensions that have no items? You > ought to be able to change this to > CHECK(all_items && !all_items->empty()); Done. https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.cc:125: kMaxExtensionItemTitleLength)); On 2013/02/19 19:00:50, Yoyo Zhou wrote: > nit: if you continue function arguments onto the next line like this, > indentation should be to the open paren. Since it won't fit, start the first > argument on a new line (at +4 spaces). Done. https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... File chrome/browser/extensions/context_menu_matcher.h (right): https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.h:44: // based on a printable selection text On 2013/02/19 19:00:50, Yoyo Zhou wrote: > nit: Comments should end with a period. Done. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:369: for (size_t position = printable_selection_text.find('&'); I added a TODO to commit this when the other code review is committed to avoid conflict. On 2013/02/19 19:00:50, Yoyo Zhou wrote: > This is more complex than necessary. Use ReplaceChars - see > https://chromiumcodereview.appspot.com/12294022/ > (but that one is *also* more complex than necessary, see my comment). https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:374: // Get a list of extension id's that have context menu items, and sort it by On 2013/02/19 19:00:50, Yoyo Zhou wrote: > delete 'it' Done. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:381: // AppendPlatformAppItems. From what I can read, there is no sorting involved in AppendPlatformAppItems since Context Menus are created in order by the owner of the App. See https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... On 2013/02/19 19:00:50, Yoyo Zhou wrote: > Does AppendPlatformAppItems have the same sorting bug as in the case of > extensions? You don't need to fix it in this CL, but it may be worthwhile to > file a bug for it. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:383: std::string menu_title = extension_items_.GetTopLevelContextMenuTitle(*i, On 2013/02/19 19:00:50, Yoyo Zhou wrote: > nit: put *i on the next line. You can put it on the same line as > printable_selection_text. Done. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:396: // TODO(fbeaufort): Where to move this UMA now? On 2013/02/19 19:00:50, Yoyo Zhou wrote: > I think it's okay to leave it here. Done. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:400: extension_items_.AppendExtensionItems(i->second, printable_selection_text, I'm not sure about what you mean here. Before, printable_selection_text was defined for each Extension in the loop. But now, we define it at the very top of this function and call AppendExtensionItems with it. Does that make sense? On 2013/02/19 19:00:50, Yoyo Zhou wrote: > I don't see how this can behave equivalently to the old version. > printable_selection_text never changes so it seems that you would append many > copies of the same text.
Sorry about taking this long to reply; it got pushed out of my working set. Can you add a unit test for this that fails without your change and succeeds with it? The TEST= line is for manual testing instructions to QA, so you can omit it from the CL description. https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... chrome/browser/extensions/context_menu_matcher.cc:101: std::string ContextMenuMatcher::GetTopLevelContextMenuTitle( On 2013/02/20 11:32:06, François Beaufort wrote: > I'm not sure about how to implement the private function and would like your > input on this. > > Do you think of a function that would take 3 parameters "extension_id", > "extension*" and "items*" and sets "extension*" and "items*" > > OR something else? > > On 2013/02/19 19:00:50, Yoyo Zhou wrote: > > This duplicates code in AppendExtensionItems. Can you make that function use > the > > logic here, so the code isn't duplicated? > I was thinking it should take an extension_id as input and return the MenuItem::List items. There will need to be some slight logic refactoring (around the early returns if manager->MenuItems(extension_id) is empty). You could call this GetRelevantExtensionTopLevelItems. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:400: extension_items_.AppendExtensionItems(i->second, printable_selection_text, On 2013/02/20 11:32:06, François Beaufort wrote: > I'm not sure about what you mean here. Before, printable_selection_text was > defined for each Extension in the loop. > But now, we define it at the very top of this function and call > AppendExtensionItems with it. > Does that make sense? > > On 2013/02/19 19:00:50, Yoyo Zhou wrote: > > I don't see how this can behave equivalently to the old version. > > printable_selection_text never changes so it seems that you would append many > > copies of the same text. > No. I'm not sure of this, but I believe PrintableSelectionText() returns a different value each time it's called. https://codereview.chromium.org/12299013/diff/6001/chrome/browser/tab_content... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/12299013/diff/6001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:367: // TODO(fbeaufort) Use EscapeAmpersands when nit: TODOs should have a colon: TODO(fbeaufort): Do this. Anyhow, EscapeAmpersands is now available.
I missed your email Yoyo. I'm so sorry! I've added a test like you said. You can find a before/now image at http://ge.tt/321KOeb/v/0 I would have liked to group all extensions folders into one "top_level" but I'm not sure how to do that. PS: Ignore Patch Set #3 On 2013/03/06 20:18:58, Yoyo Zhou wrote: > Sorry about taking this long to reply; it got pushed out of my working set. > > Can you add a unit test for this that fails without your change and succeeds > with it? > > The TEST= line is for manual testing instructions to QA, so you can omit it from > the CL description. > > https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... > File chrome/browser/extensions/context_menu_matcher.cc (right): > > https://codereview.chromium.org/12299013/diff/1/chrome/browser/extensions/con... > chrome/browser/extensions/context_menu_matcher.cc:101: std::string > ContextMenuMatcher::GetTopLevelContextMenuTitle( > On 2013/02/20 11:32:06, François Beaufort wrote: > > I'm not sure about how to implement the private function and would like your > > input on this. > > > > Do you think of a function that would take 3 parameters "extension_id", > > "extension*" and "items*" and sets "extension*" and "items*" > > > > OR something else? > > > > On 2013/02/19 19:00:50, Yoyo Zhou wrote: > > > This duplicates code in AppendExtensionItems. Can you make that function use > > the > > > logic here, so the code isn't duplicated? > > > > I was thinking it should take an extension_id as input and return the > MenuItem::List items. There will need to be some slight logic refactoring > (around the early returns if manager->MenuItems(extension_id) is empty). > > You could call this GetRelevantExtensionTopLevelItems. > > I have to define an extension variable in the main function since I need it for the default title name. Therefore, and I might be wrong, I don't see the point of creating a function to merge them. https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... > File chrome/browser/tab_contents/render_view_context_menu.cc (right): > > https://codereview.chromium.org/12299013/diff/1/chrome/browser/tab_contents/r... > chrome/browser/tab_contents/render_view_context_menu.cc:400: > extension_items_.AppendExtensionItems(i->second, printable_selection_text, > On 2013/02/20 11:32:06, François Beaufort wrote: > > I'm not sure about what you mean here. Before, printable_selection_text was > > defined for each Extension in the loop. > > But now, we define it at the very top of this function and call > > AppendExtensionItems with it. > > Does that make sense? > > > > On 2013/02/19 19:00:50, Yoyo Zhou wrote: > > > I don't see how this can behave equivalently to the old version. > > > printable_selection_text never changes so it seems that you would append > many > > > copies of the same text. > > > > No. I'm not sure of this, but I believe PrintableSelectionText() returns a > different value each time it's called. > > I ran some tests and PrintableSelectionText always returns the same string. As usual, I may be wrong ;) https://codereview.chromium.org/12299013/diff/6001/chrome/browser/tab_content... > File chrome/browser/tab_contents/render_view_context_menu.cc (right): > > https://codereview.chromium.org/12299013/diff/6001/chrome/browser/tab_content... > chrome/browser/tab_contents/render_view_context_menu.cc:367: // TODO(fbeaufort) > Use EscapeAmpersands when > nit: TODOs should have a colon: > TODO(fbeaufort): Do this. > Anyhow, EscapeAmpersands is now available.
Thank you for working on the patch and sorry for the drive-by :-) https://codereview.chromium.org/12299013/diff/6001/chrome/browser/tab_content... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/12299013/diff/6001/chrome/browser/tab_content... chrome/browser/tab_contents/render_view_context_menu.cc:367: // TODO(fbeaufort) Use EscapeAmpersands when On 2013/03/06 20:18:58, Yoyo Zhou wrote: > nit: TODOs should have a colon: > TODO(fbeaufort): Do this. > Anyhow, EscapeAmpersands is now available. https://chromiumcodereview.appspot.com/12294022/ has been committed. You can use EscapeAmpersands now.
> I have to define an extension variable in the main function since I need it for the default title name. > Therefore, and I might be wrong, I don't see the point of creating a function to merge them. The point is to avoid having duplicated code, which is a maintenance burden. If the same or very similar code is in several locations, it's all too easy to make changes to only one of them and forget the others. Defining a pointer is comparatively much less burdensome. By the way, it looks like you are correct about PrintableSelectionText. Sorry about that! https://codereview.chromium.org/12299013/diff/15001/chrome/browser/extensions... File chrome/browser/extensions/extension_context_menu_browsertest.cc (right): https://codereview.chromium.org/12299013/diff/15001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:338: // We expect to see the following items in the menu: nit: This would be clearer with 2-space indenting. https://codereview.chromium.org/12299013/diff/15001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:351: ASSERT_TRUE(LoadContextMenuExtension("top_level_single1")); You can write another helper function like LoadContextMenuExtension for this test which does test_data_dir_.AppendASCII("context_menus").AppendASCII("top_level").AppendASCII(subdirectory) https://codereview.chromium.org/12299013/diff/15001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:381: model->GetLabelAt(index)); You should just be able to use model = menu->menu_model(); model->GetLabelAt(0); etc. Also, please align indentation to the open paren. https://codereview.chromium.org/12299013/diff/15001/chrome/test/data/extensio... File chrome/test/data/extensions/context_menus/top_level_multi4/manifest.json (right): https://codereview.chromium.org/12299013/diff/15001/chrome/test/data/extensio... chrome/test/data/extensions/context_menus/top_level_multi4/manifest.json:2: "name" : "An Extension with multiple Context Menus", nit: no spaces before colon
I've finally took some time for this issue. Biggest change is the fact I've created the GetRelevantExtensionTopLevelItems() function which is called by GetTopLevelContextMenuTitle() and AppendExtensionItems() https://codereview.chromium.org/12299013/diff/15001/chrome/browser/extensions... File chrome/browser/extensions/extension_context_menu_browsertest.cc (right): https://codereview.chromium.org/12299013/diff/15001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:338: // We expect to see the following items in the menu: On 2013/03/20 21:54:13, Yoyo Zhou wrote: > nit: This would be clearer with 2-space indenting. Done. https://codereview.chromium.org/12299013/diff/15001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:351: ASSERT_TRUE(LoadContextMenuExtension("top_level_single1")); On 2013/03/20 21:54:13, Yoyo Zhou wrote: > You can write another helper function like LoadContextMenuExtension for this > test which does > > test_data_dir_.AppendASCII("context_menus").AppendASCII("top_level").AppendASCII(subdirectory) Done. https://codereview.chromium.org/12299013/diff/15001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:381: model->GetLabelAt(index)); model->GetLabelAt(0) actually returns &Back and so on ;) Hope you'll like this better. Indentation is fixed. On 2013/03/20 21:54:13, Yoyo Zhou wrote: > You should just be able to use > model = menu->menu_model(); > model->GetLabelAt(0); > etc. > > Also, please align indentation to the open paren. https://codereview.chromium.org/12299013/diff/15001/chrome/test/data/extensio... File chrome/test/data/extensions/context_menus/top_level_multi4/manifest.json (right): https://codereview.chromium.org/12299013/diff/15001/chrome/test/data/extensio... chrome/test/data/extensions/context_menus/top_level_multi4/manifest.json:2: "name" : "An Extension with multiple Context Menus", Done for "name" and "version" in all manifest files. On 2013/03/20 21:54:13, Yoyo Zhou wrote: > nit: no spaces before colon
Looks better. Just some style issues to address. You might want to look over http://www.chromium.org/developers/coding-style (and the Google C++ style guide) if you haven't already. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... File chrome/browser/extensions/context_menu_matcher.cc (left): https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:28: looks like this was accidentally deleted https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:41: if (!GetRelevantExtensionTopLevelItems(extension_id, &extension, See https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls on indentation. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:107: if (items.empty() || items.size() > 1 || items[0]->type() != MenuItem::NORMAL) { indent +1 https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:146: bool& can_cross_incognito, As per the style guide, if you are modifying these, pass them as pointers rather than non-const references. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:153: if (!extension) Seems this should be *extension. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... File chrome/browser/extensions/extension_context_menu_browsertest.cc (right): https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:389: IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST, &model, &index)); Line continuations like this are 4 space indents. See https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:391: model->GetLabelAt(index++)); Indentation is still not to the open paren.
Addresses all the style guide issues and learnt a lot of stuff. Thanks! https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:41: if (!GetRelevantExtensionTopLevelItems(extension_id, &extension, On 2013/05/08 22:54:30, Yoyo Zhou wrote: > See > https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls > on indentation. Done. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:107: if (items.empty() || items.size() > 1 || items[0]->type() != MenuItem::NORMAL) { On 2013/05/08 22:54:30, Yoyo Zhou wrote: > indent +1 Done. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:146: bool& can_cross_incognito, On 2013/05/08 22:54:30, Yoyo Zhou wrote: > As per the style guide, if you are modifying these, pass them as pointers rather > than non-const references. Done. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:153: if (!extension) On 2013/05/08 22:54:30, Yoyo Zhou wrote: > Seems this should be *extension. Done. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... File chrome/browser/extensions/extension_context_menu_browsertest.cc (right): https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:389: IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST, &model, &index)); On 2013/05/08 22:54:30, Yoyo Zhou wrote: > Line continuations like this are 4 space indents. See > https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls Done. https://codereview.chromium.org/12299013/diff/34001/chrome/browser/extensions... chrome/browser/extensions/extension_context_menu_browsertest.cc:391: model->GetLabelAt(index++)); On 2013/05/08 22:54:30, Yoyo Zhou wrote: > Indentation is still not to the open paren. Done.
LGTM. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/12299013/4...
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2013/06/05 08:22:33, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... Hello Avi, I also need your approval on this CL since you're an owner of chrome/browser/tab_contents/render_view_context_menu.cc Thank you in advance, Francois.
UI strings should be string16. https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/ext... File chrome/browser/extensions/context_menu_matcher.cc (right): https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/ext... chrome/browser/extensions/context_menu_matcher.cc:113: title = UTF16ToUTF8(item->TitleWithReplacement( MenuItem properly follows the rule of UI strings being string16s. Don't pass utf8 around. https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/ext... File chrome/browser/extensions/context_menu_matcher.h (right): https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/ext... chrome/browser/extensions/context_menu_matcher.h:46: const string16& selection_text); If this returns a string visible in the UI, then it should return a string16. (Oh, and while you're here, use |base::string16|.)
Hope my remarks do make sense https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/ext... File chrome/browser/extensions/context_menu_matcher.cc (right): https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/ext... chrome/browser/extensions/context_menu_matcher.cc:113: title = UTF16ToUTF8(item->TitleWithReplacement( Title is a string and not tied to the MenuItem here. Is that ok to let it UTF* since it won't be used in the UI? On 2013/06/05 14:34:10, Avi wrote: > MenuItem properly follows the rule of UI strings being string16s. Don't pass > utf8 around. https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/ext... File chrome/browser/extensions/context_menu_matcher.h (right): https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/ext... chrome/browser/extensions/context_menu_matcher.h:46: const string16& selection_text); This function doesn't return a string visible in the UI. It is there to help sort the Context Menu items. See https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/tab... On 2013/06/05 14:34:10, Avi wrote: > If this returns a string visible in the UI, then it should return a string16. > > (Oh, and while you're here, use |base::string16|.)
OK, so I'm even less happy. This sorts only vaguely correctly for English, and completely incorrectly for other languages. I'm looking into base/i18n and finding no wrapper for ICU sort. We need one if we're sorting here. https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/tab... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/tab... chrome/browser/tab_contents/render_view_context_menu.cc:527: // TODO(asargent) - See if this works properly for i18n names (bug 32363). No, std::sort will do completely the wrong thing.
On 2013/06/06 14:42:27, Avi wrote: > OK, so I'm even less happy. This sorts only vaguely correctly for English, and > completely incorrectly for other languages. I'm looking into base/i18n and > finding no wrapper for ICU sort. We need one if we're sorting here. > > https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/tab... > File chrome/browser/tab_contents/render_view_context_menu.cc (right): > > https://chromiumcodereview.appspot.com/12299013/diff/41001/chrome/browser/tab... > chrome/browser/tab_contents/render_view_context_menu.cc:527: // TODO(asargent) - > See if this works properly for i18n names (bug 32363). > No, std::sort will do completely the wrong thing. And where else in Chrome are we using std::sort for UI strings? :(
OK, so: - Switch to base::string16 - Use l10n_util::SortStrings16 (found in ui/base/l10n/l10n_util.h) (Examples at https://code.google.com/p/chromium/codesearch#search/&q=SortStrings16 )
Thank you! François Beaufort | beaufort.francois@gmail.com On Thu, Jun 6, 2013 at 4:57 PM, <avi@chromium.org> wrote: > OK, so: > > - Switch to base::string16 > - Use l10n_util::SortStrings16 (found in ui/base/l10n/l10n_util.h) > > (Examples at > https://code.google.com/p/**chromium/codesearch#search/&q=**SortStrings16<htt...) > > https://chromiumcodereview.**appspot.com/12299013/<https://chromiumcodereview... >
I added localization to context menu titles sorting based on your feedback.
LGTM with formatting nits fixed. https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/ext... File chrome/browser/extensions/context_menu_matcher.cc (left): https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/ext... chrome/browser/extensions/context_menu_matcher.cc:28: Please don't drop this blank line that separates functions. https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/tab... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/tab... chrome/browser/tab_contents/render_view_context_menu.cc:532: l10n_util::SortStrings16(app_locale, &sorted_menu_titles); Whoo! https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/tab... chrome/browser/tab_contents/render_view_context_menu.cc:537: const std::string & id = map_ids[sorted_menu_titles[i]]; no space before & https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/test/data/e... File chrome/test/data/extensions/context_menus/top_level/multi4/background.js (right): https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/test/data/e... chrome/test/data/extensions/context_menus/top_level/multi4/background.js:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. No "(c)" in new files.
Thank you again Yoyo and Avi for your insightful comments. https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/ext... File chrome/browser/extensions/context_menu_matcher.cc (left): https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/ext... chrome/browser/extensions/context_menu_matcher.cc:28: On 2013/06/07 21:05:49, Avi wrote: > Please don't drop this blank line that separates functions. Done. https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/tab... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/tab... chrome/browser/tab_contents/render_view_context_menu.cc:532: l10n_util::SortStrings16(app_locale, &sorted_menu_titles); On 2013/06/07 21:05:49, Avi wrote: > Whoo! Done. https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/tab... chrome/browser/tab_contents/render_view_context_menu.cc:537: const std::string & id = map_ids[sorted_menu_titles[i]]; On 2013/06/07 21:05:49, Avi wrote: > no space before & Done. https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/test/data/e... File chrome/test/data/extensions/context_menus/top_level/multi4/background.js (right): https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/test/data/e... chrome/test/data/extensions/context_menus/top_level/multi4/background.js:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/06/07 21:05:49, Avi wrote: > No "(c)" in new files. Done.
SLGTM https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/tab... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://chromiumcodereview.appspot.com/12299013/diff/65001/chrome/browser/tab... chrome/browser/tab_contents/render_view_context_menu.cc:532: l10n_util::SortStrings16(app_locale, &sorted_menu_titles); What does "done" mean here? :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/12299013/6...
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
On 2013/06/10 15:41:27, I haz the power (commit-bot) wrote: > Retried try job too often on linux_rel for step(s) browser_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Turns out a CHECK was too strong in the case of a Chrome App with no context menus.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/12299013/8...
Sorry for I got bad news for ya. Compile failed with a clobber build on win7_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/06/11 10:47:25, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on win7_aura. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. I think I will need your help here. I'm not sure what I'm doing wrong. Error from http://build.chromium.org/p/tryserver.chromium/builders/win7_aura/builds/4858... is: e:\b\build\slave\win7_aura\build\src\chrome\browser\extensions\context_menu_matcher.cc(166) : error C2220: warning treated as error - no 'object' file generated e:\b\build\slave\win7_aura\build\src\chrome\browser\extensions\context_menu_matcher.cc(166) : warning C4800: 'bool *' : forcing value to bool 'true' or 'false' (performance warning)
https://codereview.chromium.org/12299013/diff/87002/chrome/browser/extensions... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/12299013/diff/87002/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:166: can_cross_incognito); Oh, wow. I think that caught a bug. GetRelevantExtensionItems takes a bool as the second parameter. Are you passing a bool* here?
On 2013/06/11 12:03:43, François Beaufort wrote: > : warning C4800: 'bool *' : forcing value to bool 'true' or 'false' (performance > warning) BTW, this is a pretty easy error to read: it's forcing the value of a bool* to be true or false, so it's complaining about an implicit cast from bool* to bool.
https://codereview.chromium.org/12299013/diff/87002/chrome/browser/extensions... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/12299013/diff/87002/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:166: can_cross_incognito); I have no excuse here. I'm going to whip myself for that. On 2013/06/11 14:14:17, Avi wrote: > Oh, wow. I think that caught a bug. > > GetRelevantExtensionItems takes a bool as the second parameter. Are you passing > a bool* here?
https://codereview.chromium.org/12299013/diff/87002/chrome/browser/extensions... File chrome/browser/extensions/context_menu_matcher.cc (right): https://codereview.chromium.org/12299013/diff/87002/chrome/browser/extensions... chrome/browser/extensions/context_menu_matcher.cc:166: can_cross_incognito); On 2013/06/11 14:41:22, François Beaufort wrote: > I'm going to whip myself for that. I will accept a new patch in lieu of hara kiri.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/12299013/1...
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
On 2013/06/11 17:07:01, I haz the power (commit-bot) wrote: > Step "update" is always a major failure. > Look at the try server FAQ for more details. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... I'm not sure I can do anything here. Should I just try to commit again and hope there won't be any network issue? Message is: Syncing projects: 73% (111/151) src/third_party/mesa/src 112> No output for 30 seconds from command: 112> git clone --progress https://chromium.googlesource.com/chromiumos/platform/mtpd.git /mnt/scratch0/b_used/build/slave/linux_chromeos/build/src/third_party/mtpd/source; cwd=/mnt/scratch0/b_used/build/slave/linux_chromeos/build 112> No output for 60 seconds from command: 112> git clone --progress https://chromium.googlesource.com/chromiumos/platform/mtpd.git /mnt/scratch0/b_used/build/slave/linux_chromeos/build/src/third_party/mtpd/source; cwd=/mnt/scratch0/b_used/build/slave/linux_chromeos/build 112>error: Failed to connect to 2607:f8b0:400e:c04::52: Network is unreachable while accessing https://chromium.googlesource.com/chromiumos/platform/mtpd.git/info/refs?serv... 112>fatal: HTTP request failed Syncing projects: 74% (112/151) src/third_party/mtpd/source Error: Command git clone --progress https://chromium.googlesource.com/external/accessibility-developer-tools.git /mnt/scratch0/b_used/build/slave/linux_chromeos/build/src/third_party/accessibility-developer-tools returned non-zero exit status 128 in /mnt/scratch0/b_used/build/slave/linux_chromeos/build
Yeah, just keep mashing the commit button if the build fails and it's clearly not your patch's fault.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/12299013/1...
Message was sent while issue was closed.
Change committed as 205793 |
