Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(377)

Issue 1007993004: Restructure the wrench menu into an action based order (Closed)

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.

Description

Restructure 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -110 lines) Patch
M chrome/app/generated_resources.grd View 2 chunks +3 lines, -5 lines 0 comments Download
M chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +6 lines, -9 lines 0 comments Download
M chrome/browser/ui/toolbar/wrench_menu_model.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/ui/toolbar/wrench_menu_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 8 chunks +85 lines, -90 lines 0 comments Download
M chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +9 lines, -5 lines 0 comments Download

Messages

Total messages: 18 (5 generated)
edwardjung
Peter, please take a look at this menu restructure. Thanks.
5 years, 8 months ago (2015-03-30 18:51:53 UTC) #2
Peter Kasting
https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc File chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/180001/chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc#newcode39 chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc:39: AddSeparator(ui::NORMAL_SEPARATOR); Shouldn't this be inside the above conditional? Otherwise, ...
5 years, 8 months ago (2015-03-30 19:37:03 UTC) #3
edwardjung
Addressed your comments. I'm working on testing a menu item index that works across all ...
5 years, 8 months ago (2015-03-31 11:44:06 UTC) #4
edwardjung
Peter, updated the unit test with an menu index that works across all platforms. Turns ...
5 years, 8 months ago (2015-04-01 13:55:52 UTC) #5
Peter Kasting
https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/toolbar/wrench_menu_model.cc File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/toolbar/wrench_menu_model.cc#newcode826 chrome/browser/ui/toolbar/wrench_menu_model.cc:826: // - Global browser errors and warnings. Nit: Should ...
5 years, 8 months ago (2015-04-01 21:06:29 UTC) #6
edwardjung
Please take another look. Refactored the code as suggested. Looks cleaner now. https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/toolbar/wrench_menu_model.cc File chrome/browser/ui/toolbar/wrench_menu_model.cc ...
5 years, 8 months ago (2015-04-02 13:13:28 UTC) #7
Peter Kasting
LGTM https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/toolbar/wrench_menu_model.cc File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/240001/chrome/browser/ui/toolbar/wrench_menu_model.cc#newcode986 chrome/browser/ui/toolbar/wrench_menu_model.cc:986: RemoveTrailingSeparators(); On 2015/04/02 13:13:28, edwardjung wrote: > On ...
5 years, 8 months ago (2015-04-02 20:25:37 UTC) #8
edwardjung
Thanks Peter. I'll create a separate CL to remove RemoveTrailingSeparators. https://codereview.chromium.org/1007993004/diff/280001/chrome/browser/ui/toolbar/wrench_menu_model.cc File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1007993004/diff/280001/chrome/browser/ui/toolbar/wrench_menu_model.cc#newcode826 ...
5 years, 8 months ago (2015-04-02 21:42:17 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1007993004/300001
5 years, 8 months ago (2015-04-02 21:43:24 UTC) #12
commit-bot: I haz the power
This issue passed the CQ dry run.
5 years, 8 months ago (2015-04-03 04:53:07 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1007993004/300001
5 years, 8 months ago (2015-04-03 08:29:59 UTC) #16
commit-bot: I haz the power
Committed patchset #16 (id:300001)
5 years, 8 months ago (2015-04-03 08:31:59 UTC) #17
commit-bot: I haz the power
5 years, 8 months ago (2015-04-03 20:36:02 UTC) #18
Message was sent while issue was closed.
Patchset 16 (id:??) landed as
https://crrev.com/1d975a123822d69563d3a7c6244cf82d9d92d05a
Cr-Commit-Position: refs/heads/master@{#323704}

Powered by Google App Engine
This is Rietveld 408576698