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

Issue 2721953002: [Media Router] Add "Hide in menu/Show in toolbar" option to Cast toolbar icon (Closed)

Created:
3 years, 9 months ago by takumif
Modified:
3 years, 9 months ago
CC:
chromium-reviews, tfarina
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Media Router] Add "Hide in menu/Show in toolbar" option to Cast toolbar icon This CL adds a context menu option of "Hide in menu/Show in toolbar" to the Cast/Media Router toolbar action to match the functionality of the context menus for extension actions. This menu option will be disabled when the action is in the ephemeral state (i.e. the "Always show icon" option is unchecked, and the icon only shows when Cast is in use), since the position of the icon would not persist in those cases. BUG=671399 Review-Url: https://codereview.chromium.org/2721953002 Cr-Commit-Position: refs/heads/master@{#454634} Committed: https://chromium.googlesource.com/chromium/src/+/1e061d2691156d61913a34463e2dfdec172e324b

Patch Set 1 #

Total comments: 12

Patch Set 2 : Address Jennifer's and Mike's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+152 lines, -40 lines) Patch
M chrome/app/chrome_command_ids.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/toolbar/media_router_action.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/toolbar/media_router_action.cc View 3 chunks +8 lines, -3 lines 0 comments Download
M chrome/browser/ui/toolbar/media_router_contextual_menu.h View 1 2 chunks +21 lines, -4 lines 0 comments Download
M chrome/browser/ui/toolbar/media_router_contextual_menu.cc View 1 5 chunks +37 lines, -6 lines 0 comments Download
M chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc View 1 9 chunks +57 lines, -26 lines 0 comments Download
M chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc View 1 2 chunks +27 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (16 generated)
takumif
Please take a look at: apacible: overall msw: c/b/ui/toolbar/ grt: chrome_command_ids.h Thank you!
3 years, 9 months ago (2017-02-28 19:24:14 UTC) #5
grt (UTC plus 2)
chrome/app lgtm w/ a question https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_action.cc File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_action.cc#newcode137 chrome/browser/ui/toolbar/media_router_action.cc:137: if (toolbar_actions_bar_->IsActionVisibleOnMainBar(this)) { should ...
3 years, 9 months ago (2017-02-28 19:56:13 UTC) #6
takumif
https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_action.cc File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_action.cc#newcode137 chrome/browser/ui/toolbar/media_router_action.cc:137: if (toolbar_actions_bar_->IsActionVisibleOnMainBar(this)) { On 2017/02/28 19:56:13, grt (UTC plus ...
3 years, 9 months ago (2017-02-28 22:02:04 UTC) #7
apacible
lgtm https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right): https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc#newcode187 chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:187: // in Chrome menu." nit here and below: ...
3 years, 9 months ago (2017-03-02 02:02:57 UTC) #8
msw
lgtm with nits https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_contextual_menu.h File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right): https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_contextual_menu.h#newcode17 chrome/browser/ui/toolbar/media_router_contextual_menu.h:17: static std::unique_ptr<MediaRouterContextualMenu> CreateForToolbar( nit: add function ...
3 years, 9 months ago (2017-03-02 03:12:58 UTC) #9
takumif
https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_contextual_menu.h File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right): https://codereview.chromium.org/2721953002/diff/1/chrome/browser/ui/toolbar/media_router_contextual_menu.h#newcode17 chrome/browser/ui/toolbar/media_router_contextual_menu.h:17: static std::unique_ptr<MediaRouterContextualMenu> CreateForToolbar( On 2017/03/02 03:12:58, msw wrote: > ...
3 years, 9 months ago (2017-03-03 00:10:48 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2721953002/20001
3 years, 9 months ago (2017-03-03 18:42:37 UTC) #21
commit-bot: I haz the power
3 years, 9 months ago (2017-03-03 18:47:49 UTC) #24
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/1e061d2691156d61913a34463e2d...

Powered by Google App Engine
This is Rietveld 408576698