|
|
Created:
3 years, 10 months ago by Evan Stade Modified:
3 years, 10 months ago Reviewers:
msw CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't use bold text for the "no recently closed tabs" menu item.
BUG=691986
Review-Url: https://codereview.chromium.org/2698613003
Cr-Commit-Position: refs/heads/master@{#451146}
Committed: https://chromium.googlesource.com/chromium/src/+/0c16383b5973aa220cb24ae2b14f589f5e994baa
Patch Set 1 #
Total comments: 4
Messages
Total messages: 14 (6 generated)
estade@chromium.org changed reviewers: + msw@chromium.org
https://codereview.chromium.org/2698613003/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/2698613003/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu.cc:862: return IsRecentTabsCommand(command_id) && GetLabelFontList(command_id); this may seem a little odd but it's what the code was doing before I touched it and I don't see a more elegant solution.
https://codereview.chromium.org/2698613003/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/2698613003/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu.cc:862: return IsRecentTabsCommand(command_id) && GetLabelFontList(command_id); On 2017/02/16 01:30:04, Evan Stade wrote: > this may seem a little odd but it's what the code was doing before I touched it > and I don't see a more elegant solution. Can we omit the recently closed item when there are no sub-items?
https://codereview.chromium.org/2698613003/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/2698613003/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu.cc:862: return IsRecentTabsCommand(command_id) && GetLabelFontList(command_id); On 2017/02/16 01:54:00, msw wrote: > On 2017/02/16 01:30:04, Evan Stade wrote: > > this may seem a little odd but it's what the code was doing before I touched > it > > and I don't see a more elegant solution. > > Can we omit the recently closed item when there are no sub-items? I think it's better to show it but have it disabled. If you're actually looking for it and it's just not there, you might think you're looking in the wrong place.
lgtm https://codereview.chromium.org/2698613003/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/2698613003/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu.cc:862: return IsRecentTabsCommand(command_id) && GetLabelFontList(command_id); On 2017/02/16 02:36:25, Evan Stade wrote: > On 2017/02/16 01:54:00, msw wrote: > > On 2017/02/16 01:30:04, Evan Stade wrote: > > > this may seem a little odd but it's what the code was doing before I touched > > it > > > and I don't see a more elegant solution. > > > > Can we omit the recently closed item when there are no sub-items? > > I think it's better to show it but have it disabled. If you're actually looking > for it and it's just not there, you might think you're looking in the wrong > place. Acknowledged.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 1, "attempt_start_ts": 1487285338364730, "parent_rev": "da4e99db2db55d566dfbcbf4dfe89fb4c3202ec7", "commit_rev": "0c16383b5973aa220cb24ae2b14f589f5e994baa"}
Message was sent while issue was closed.
Description was changed from ========== Don't use bold text for the "no recently closed tabs" menu item. BUG=691986 ========== to ========== Don't use bold text for the "no recently closed tabs" menu item. BUG=691986 Review-Url: https://codereview.chromium.org/2698613003 Cr-Commit-Position: refs/heads/master@{#451146} Committed: https://chromium.googlesource.com/chromium/src/+/0c16383b5973aa220cb24ae2b14f... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/0c16383b5973aa220cb24ae2b14f... |