|
|
Created:
5 years, 9 months ago by edwardjung Modified:
5 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestructure the wrench menu into a more coherant order taking into account metrics gather in M40.
See bug for new structure and screenshots.
Gist of the groupings
- Go somewhere new: new tabs, windows…
- Revisit a site: History, bookmarks, recent tabs…
- Take action: Zoom, edit, print, save…
- Customise and learn: Settings, sign in, help…
- Exit
BUG=432561
Committed: https://crrev.com/1d975a123822d69563d3a7c6244cf82d9d92d05a
Cr-Commit-Position: refs/heads/master@{#323704}
Patch Set 1 #Patch Set 2 : Fix duplicate sign in error messages and extra separators #Patch Set 3 : Update wrench menu unit tests #Patch Set 4 : Remove _new file #Patch Set 5 : Update unit tests #Patch Set 6 : Fix unit test for Mac #Patch Set 7 : #Patch Set 8 : Fix ChromeOS test #Patch Set 9 : Restructure the Other tools submenu #Patch Set 10 : Corrected bookmark test #
Total comments: 36
Patch Set 11 : Address review comments #
Total comments: 1
Patch Set 12 : Update menu action index in unittest. #Patch Set 13 : Update unit test. #
Total comments: 23
Patch Set 14 : Simplify top error block and address comments #Patch Set 15 : Fix mac compilation errors #
Total comments: 4
Patch Set 16 : Fix nits #
Messages
Total messages: 18 (5 generated)
edwardjung@chromium.org changed reviewers: + pkasting@chromium.org
Peter, please take a look at this menu restructure. Thanks.
https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc:39: AddSeparator(ui::NORMAL_SEPARATOR); Shouldn't this be inside the above conditional? Otherwise, if the submenu isn't visible, we'll just have a separator alone atop these other items. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc:47: AddSeparator(ui::NORMAL_SEPARATOR); This seems really weird. I assume there's a Mac-specific file somewhere else that appends more items here? Maybe whatever code does that should be adding this separator instead. I realize this was already like this, but while you're here anyway... https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:838: add_separator = true; This seems strange. Where is the UI for these enumerated modules going to live? If it's going to be the VIEW_INCOMPATIBILITIES icon, why is that controlling whether we'll add a separator two items lower down? https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:841: if (browser_defaults::kShowUpgradeMenuItem) { Nit: No {} https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:849: SetIcon(GetIndexOfCommandId(IDC_VIEW_INCOMPATIBILITIES), Why is this code here instead of being up with the other IDC_VIEW_INCOMPATIBILITIES code? https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:857: // Tabs and windows. If it really adds clarity to comment each section of the menu like this, then I would consider breaking this long function into a series of smaller helpers with descriptive names, which will accomplish the same thing more readably. Otherwise I would probably omit these comments. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:865: // Places previously been - History, bookmarks, recent tabs. This description doesn't seem to apply to SHOW_DOWNLOADS? It's also weird that the comment lists items in a different order than the code. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:890: switches::kEnableDomDistiller)) { Nit: No {} https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:901: // Customisation and learn. "Customization and learning"? Though I don't know what "learning" (or "learn") actually means here. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:913: browser_->profile()->GetOriginalProfile()); Nit: Since you don't actually care about the errors, just whether there are some, I'd inline this call below and omit the temp. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:915: if (signin_errors.size() == 0) { Nit: Use .empty() https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:919: IDS_SYNC_MENU_PRE_SYNCED_LABEL, short_product_name)); Nit: Existing wrapping isn't actually style-compliant. How about this: AddItem(IDC_SHOW_SYNC_SETUP, l10n_util::GetStringFUTF16( IDS_SYNC_MENU_PRE_SYNCED_LABEL, l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME))); https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:965: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { Nit: No {} https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.h (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.h:193: bool AddGlobalErrorMenuItems(); Nit: Please add a comment explaining what this function does, in particular the return value. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:4: #include <stdio.h> Why add this? https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:135: // Make sure to use the index that is not separator in all configurations. Is there an index that is a non-separator in all platforms, like there was in the old code, so you can avoid the next #ifdef? https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:136: #if defined(OS_MACOSX) or defined(CHROMEOS) "or" needs to be ||, this won't work as you have it https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:154: // Bookmarks is now under the tabs submenu item. What does "now" mean? This seems to be implicitly referring to some previous state, which readers in the future won't be aware of. Make sure your comments are contextually self-contained.
Addressed your comments. I'm working on testing a menu item index that works across all platforms in the test. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc:39: AddSeparator(ui::NORMAL_SEPARATOR); On 2015/03/30 19:37:02, Peter Kasting wrote: > Shouldn't this be inside the above conditional? Otherwise, if the submenu isn't > visible, we'll just have a separator alone atop these other items. Yes, good point. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc:47: AddSeparator(ui::NORMAL_SEPARATOR); On 2015/03/30 19:37:02, Peter Kasting wrote: > This seems really weird. I assume there's a Mac-specific file somewhere else > that appends more items here? Maybe whatever code does that should be adding > this separator instead. > > I realize this was already like this, but while you're here anyway... Removed. Curiously the OSX cocoa code does add a separator before adding the the extra menu items. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:838: add_separator = true; On 2015/03/30 19:37:02, Peter Kasting wrote: > This seems strange. Where is the UI for these enumerated modules going to live? > If it's going to be the VIEW_INCOMPATIBILITIES icon, why is that controlling > whether we'll add a separator two items lower down? I altered this from the previous code, which was using the enumerated models to determine whether there are any module errors to add a separator. I don't believe the enumerated modules are used in the UI at all. When AddItem(IDC_VIEW_INCOMPATIBILITIES…) is called, WrenchMenuModel::IsCommandIdVisible() is a called to do a check for the incompatible modules and that determines if a menuitem is displayed. current code: if (model->modules_to_notify_about() > 0 || model->confirmed_bad_modules_detected() > 0) AddSeparator(ui::NORMAL_SEPARATOR); So this code does the same check for module errors. What I want to achieve is a single separator after all the errors are displayed, rather than a separator after each error/warning which is how it is currently. So the add_separator flag aggregates all the current separators to one. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:841: if (browser_defaults::kShowUpgradeMenuItem) { On 2015/03/30 19:37:02, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:849: SetIcon(GetIndexOfCommandId(IDC_VIEW_INCOMPATIBILITIES), On 2015/03/30 19:37:02, Peter Kasting wrote: > Why is this code here instead of being up with the other > IDC_VIEW_INCOMPATIBILITIES code? Good point, didn't think since it was in a separate block before. Moved up. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:857: // Tabs and windows. On 2015/03/30 19:37:02, Peter Kasting wrote: > If it really adds clarity to comment each section of the menu like this, then I > would consider breaking this long function into a series of smaller helpers with > descriptive names, which will accomplish the same thing more readably. > > Otherwise I would probably omit these comments. Point taken. I've added a comment at the top of the function. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:865: // Places previously been - History, bookmarks, recent tabs. On 2015/03/30 19:37:02, Peter Kasting wrote: > This description doesn't seem to apply to SHOW_DOWNLOADS? It's also weird that > the comment lists items in a different order than the code. Removed. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:890: switches::kEnableDomDistiller)) { On 2015/03/30 19:37:02, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:901: // Customisation and learn. On 2015/03/30 19:37:02, Peter Kasting wrote: > "Customization and learning"? Though I don't know what "learning" (or "learn") > actually means here. Removed. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:913: browser_->profile()->GetOriginalProfile()); On 2015/03/30 19:37:02, Peter Kasting wrote: > Nit: Since you don't actually care about the errors, just whether there are > some, I'd inline this call below and omit the temp. Done. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:915: if (signin_errors.size() == 0) { On 2015/03/30 19:37:02, Peter Kasting wrote: > Nit: Use .empty() Done. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:919: IDS_SYNC_MENU_PRE_SYNCED_LABEL, short_product_name)); On 2015/03/30 19:37:02, Peter Kasting wrote: > Nit: Existing wrapping isn't actually style-compliant. How about this: > > AddItem(IDC_SHOW_SYNC_SETUP, > l10n_util::GetStringFUTF16( > IDS_SYNC_MENU_PRE_SYNCED_LABEL, > l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME))); Done. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:965: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2015/03/30 19:37:02, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.h (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.h:193: bool AddGlobalErrorMenuItems(); On 2015/03/30 19:37:02, Peter Kasting wrote: > Nit: Please add a comment explaining what this function does, in particular the > return value. Done. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:4: #include <stdio.h> On 2015/03/30 19:37:02, Peter Kasting wrote: > Why add this? Removed, debug. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:135: // Make sure to use the index that is not separator in all configurations. On 2015/03/30 19:37:02, Peter Kasting wrote: > Is there an index that is a non-separator in all platforms, like there was in > the old code, so you can avoid the next #ifdef? Let me find something that works for all. i tried a number of options which failed one platform or another. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:136: #if defined(OS_MACOSX) or defined(CHROMEOS) On 2015/03/30 19:37:02, Peter Kasting wrote: > "or" needs to be ||, this won't work as you have it Done. https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:154: // Bookmarks is now under the tabs submenu item. On 2015/03/30 19:37:02, Peter Kasting wrote: > What does "now" mean? This seems to be implicitly referring to some previous > state, which readers in the future won't be aware of. Make sure your comments > are contextually self-contained. Done. https://codereview.chromium.org/1007993004/diff/200001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:855: add_separator = true; Added a check for the upgrade warning too.
Peter, updated the unit test with an menu index that works across all platforms. Turns out only the last item in the menu is a guaranteed non-separator.
https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:826: // - Global browser errors and warnings. Nit: Should probably add "- Extension toolbar overflow items." atop this list. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:833: bool add_separator = false; Nit: This can be eliminated; see further notes below. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:849: add_separator = true; Instead of checking the EnumerateModulesModel directly, let's make the visibility determination using "IsCommandIdVisible(IDC_VIEW_INCOMPATIBILITIES);". This will be simpler, and more future-proof if we later change the conditions under which this item is shown. This also will enable you to remove the #if defined(OS_WIN) around this block, as on non-Windows, the item will always be invisible and thus harmless to add. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:852: if (browser_defaults::kShowUpgradeMenuItem) { Nit: Move the check of this variable into IsCommandIdVisible(). This eliminates the conditional here and is necessary for a simplification I suggest below. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:854: if (UpgradeDetector::GetInstance()->notify_upgrade()) Similarly to above, this should call IsCommandIdVisible() instead of duplicating that function's functionality. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:858: if (AddGlobalErrorMenuItems()) You can eliminate |add_separator| entirely and shorten this function by moving the IsCommandIdVisible() calls I suggest above down to here. (Note that this requires moving the kShowUpgradeMenuItem check into IsCommandIdVisible().) This simply becomes: if (AddGlobalErrorMenuItems() || IsCommandIdVisible(IDC_VIEW_INCOMPATIBILITIES) || IsCommandIdVisible(IDC_UPGRADE_DIALOG)) AddSeparator(ui::NORMAL_SEPARATOR); https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:868: AddItemWithStringId(IDC_NEW_INCOGNITO_WINDOW, IDS_NEW_INCOGNITO_WINDOW); Nit: I'd probably remove the blank line above this conditional and add one just below it. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:913: std::vector<GlobalError*> signin_errors; This temp is unused and can be removed. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:917: const base::string16 short_product_name = This temp is unused and can be removed. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:946: AddSeparator(ui::NORMAL_SEPARATOR); This line should be removed. * If we try to add the "restart in metro" items in the big #if OS_WIN block just below, they add their own separator, so this will result in a double separator. * If instead we try to show the exit menu, that also adds its own separator, so again, this will result in a double separator. * If we do neither of these, this becomes a trailing separator, and will be removed anyway by the RemoveTrailingSeparators() call. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:986: RemoveTrailingSeparators(); I think this call can be removed (and, since this is the only call to this function AFAIK, the whole function can be removed). As long as you remove the separator I comment about above, I believe all codepaths result in a non-separator item being last in the menu. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (left): https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:4: Nit: Don't remove this blank line
Please take another look. Refactored the code as suggested. Looks cleaner now. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:849: add_separator = true; On 2015/04/01 21:06:28, Peter Kasting wrote: > Instead of checking the EnumerateModulesModel directly, let's make the > visibility determination using > "IsCommandIdVisible(IDC_VIEW_INCOMPATIBILITIES);". > > This will be simpler, and more future-proof if we later change the conditions > under which this item is shown. > > This also will enable you to remove the #if defined(OS_WIN) around this block, > as on non-Windows, the item will always be invisible and thus harmless to add. Nice, I like simplicity. Done. As IDS_VIEW_INCOMPATIBILITIES was wrapped in a win only block I've made modifications to the generated_resources.grd too. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:852: if (browser_defaults::kShowUpgradeMenuItem) { On 2015/04/01 21:06:28, Peter Kasting wrote: > Nit: Move the check of this variable into IsCommandIdVisible(). This eliminates > the conditional here and is necessary for a simplification I suggest below. Done. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:854: if (UpgradeDetector::GetInstance()->notify_upgrade()) On 2015/04/01 21:06:28, Peter Kasting wrote: > Similarly to above, this should call IsCommandIdVisible() instead of duplicating > that function's functionality. Done. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:858: if (AddGlobalErrorMenuItems()) On 2015/04/01 21:06:28, Peter Kasting wrote: > You can eliminate |add_separator| entirely and shorten this function by moving > the IsCommandIdVisible() calls I suggest above down to here. (Note that this > requires moving the kShowUpgradeMenuItem check into IsCommandIdVisible().) > > This simply becomes: > > if (AddGlobalErrorMenuItems() || > IsCommandIdVisible(IDC_VIEW_INCOMPATIBILITIES) || > IsCommandIdVisible(IDC_UPGRADE_DIALOG)) > AddSeparator(ui::NORMAL_SEPARATOR); Done. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:868: AddItemWithStringId(IDC_NEW_INCOGNITO_WINDOW, IDS_NEW_INCOGNITO_WINDOW); On 2015/04/01 21:06:28, Peter Kasting wrote: > Nit: I'd probably remove the blank line above this conditional and add one just > below it. Done. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:913: std::vector<GlobalError*> signin_errors; On 2015/04/01 21:06:28, Peter Kasting wrote: > This temp is unused and can be removed. Done. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:917: const base::string16 short_product_name = On 2015/04/01 21:06:28, Peter Kasting wrote: > This temp is unused and can be removed. Done. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:946: AddSeparator(ui::NORMAL_SEPARATOR); On 2015/04/01 21:06:28, Peter Kasting wrote: > This line should be removed. > > * If we try to add the "restart in metro" items in the big #if OS_WIN block just > below, they add their own separator, so this will result in a double separator. > * If instead we try to show the exit menu, that also adds its own separator, so > again, this will result in a double separator. > * If we do neither of these, this becomes a trailing separator, and will be > removed anyway by the RemoveTrailingSeparators() call. Good point. Done. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:986: RemoveTrailingSeparators(); On 2015/04/01 21:06:28, Peter Kasting wrote: > I think this call can be removed (and, since this is the only call to this > function AFAIK, the whole function can be removed). As long as you remove the > separator I comment about above, I believe all codepaths result in a > non-separator item being last in the menu. Removed, but didn't remove the function, a search in the codebase shows that it's used by: chrome/browser/ui/ash/launcher/launcher_application_menu_item_model.cc https://cs.corp.google.com/#clankium/src/chrome/browser/ui/ash/launcher/launc... https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (left): https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:4: On 2015/04/01 21:06:29, Peter Kasting wrote: > Nit: Don't remove this blank line Done.
LGTM https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:986: RemoveTrailingSeparators(); On 2015/04/02 13:13:28, edwardjung wrote: > On 2015/04/01 21:06:28, Peter Kasting wrote: > > I think this call can be removed (and, since this is the only call to this > > function AFAIK, the whole function can be removed). As long as you remove the > > separator I comment about above, I believe all codepaths result in a > > non-separator item being last in the menu. > > Removed, but didn't remove the function, a search in the codebase shows that > it's used by: > chrome/browser/ui/ash/launcher/launcher_application_menu_item_model.cc > > https://cs.corp.google.com/#clankium/src/chrome/browser/ui/ash/launcher/launc... Can you remove that instance too so we can kill this? You're welcome to do it as a followon CL if you'd prefer. I believe changing that function as follows should work: void LauncherApplicationMenuItemModel::Build() { if (launcher_items_.empty()) return; AddSeparator(ui::SPACING_SEPARATOR); for (...) { ... } AddSeparator(ui::SPACING_SEPARATOR); } In other words, add an initial early-return, remove the RemoveTrailingSeparators() call, and make the final AddSeparator() call unconditional. https://codereview.chromium.org/1007993004/diff/280001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/280001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:826: // - Extensions toolbar overflow. Nit: Extension (singular matches the function name called below) https://codereview.chromium.org/1007993004/diff/280001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:898: if (signin && signin->IsSigninAllowed()) { Nit: Can combine this conditional with the one below
Thanks Peter. I'll create a separate CL to remove RemoveTrailingSeparators. https://codereview.chromium.org/1007993004/diff/280001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/280001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:826: // - Extensions toolbar overflow. On 2015/04/02 20:25:37, Peter Kasting wrote: > Nit: Extension > > (singular matches the function name called below) Done. https://codereview.chromium.org/1007993004/diff/280001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:898: if (signin && signin->IsSigninAllowed()) { On 2015/04/02 20:25:37, Peter Kasting wrote: > Nit: Can combine this conditional with the one below Done.
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1007993004/#ps300001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1007993004/300001
The CQ bit was unchecked by commit-bot@chromium.org
This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1007993004/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/1d975a123822d69563d3a7c6244cf82d9d92d05a Cr-Commit-Position: refs/heads/master@{#323704} |