[Media Router] Adds a contextual menu item to remove the MR component action.

[Media Router] Adds a contextual menu item to remove the MR component action.

This adds a menu item to remove the MR component action from the toolbar.  It
sets a pref so this preference is respected across sessions.

The user can restore the action by reinstalling the Cast extension from the Web
Store.  (They may have to reload the Web Store page immediately after
installation to do so.)

BUG=576362
TESTED=Unit test, manually tested

Committed: https://crrev.com/95cfa837a726c4400e025e826fcaddf1bfc751eb
Cr-Commit-Position: refs/heads/master@{#371629}

[mark a. foltz, 2016-01-21 23:16:32]

mfoltz@chromium.org changed reviewers:
+ apacible@chromium.org, cpu@chromium.org, mek@chromium.org

[mark a. foltz, 2016-01-21 23:16:32]

apacible@ for everything
mek@ for browser/extensions and browser/ui/toolbar
cpu@ for chrome/app

PTAL

[apacible, 2016-01-22 16:53:55]

https://codereview.chromium.org/1612203002/diff/1/chrome/app/chrome_command_i...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/1612203002/diff/1/chrome/app/chrome_command_i...
chrome/app/chrome_command_ids.h:351: #define IDC_MEDIA_ROUTER_REMOVE_FROM_CHROME
51204
nit: Since this command is usable in Chromium, perhaps something like
IDC_MEDIA_ROUTER_REMOVE_FROM_BROWSER?

https://codereview.chromium.org/1612203002/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/1612203002/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:34:
IDS_EXTENSIONS_UNINSTALL);
Does this fall back to Chromium's IDS_EXTENSIONS_UNINSTALL when in an unbranded
build?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chromiu...

[mark a. foltz, 2016-01-22 18:01:55]

https://codereview.chromium.org/1612203002/diff/1/chrome/app/chrome_command_i...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/1612203002/diff/1/chrome/app/chrome_command_i...
chrome/app/chrome_command_ids.h:351: #define IDC_MEDIA_ROUTER_REMOVE_FROM_CHROME
51204
On 2016/01/22 at 16:53:55, apacible wrote:
> nit: Since this command is usable in Chromium, perhaps something like
IDC_MEDIA_ROUTER_REMOVE_FROM_BROWSER?

Changed to IDC_MEDIA_ROUTER_REMOVE_TOOLBAR_ACTION

https://codereview.chromium.org/1612203002/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/1612203002/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:34:
IDS_EXTENSIONS_UNINSTALL);
On 2016/01/22 at 16:53:55, apacible wrote:
> Does this fall back to Chromium's IDS_EXTENSIONS_UNINSTALL when in an
unbranded build?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chromiu...

I changed the #include to chromium_strings.h which I believe will fix unbranded
builds; through some non-obvious magic, I think the string gets overridden in
branded builds, but going to manually check.

[apacible, 2016-01-22 18:30:46]

lgtm when strings checked.

https://codereview.chromium.org/1612203002/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/1612203002/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:34:
IDS_EXTENSIONS_UNINSTALL);
On 2016/01/22 18:01:55, mark a. foltz wrote:
> On 2016/01/22 at 16:53:55, apacible wrote:
> > Does this fall back to Chromium's IDS_EXTENSIONS_UNINSTALL when in an
> unbranded build?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chromiu...
> 
> I changed the #include to chromium_strings.h which I believe will fix
unbranded
> builds; through some non-obvious magic, I think the string gets overridden in
> branded builds, but going to manually check.

Acknowledged.

[Marijn Kruisselbrink, 2016-01-22 19:19:34]

On 2016/01/21 at 23:16:32, mfoltz wrote:
> mek@ for browser/extensions and browser/ui/toolbar

lgtm, although I'm not a browser/ui/toolbar OWNER

[mark a. foltz, 2016-01-22 19:54:34]

mfoltz@chromium.org changed reviewers:
+ finnur@chromium.org

[mark a. foltz, 2016-01-22 19:54:36]

+finnur for toolbar related changes

Marjin: Thanks for the LGTM and heads up.

Finnur: Devlin is most familiar with the component migration code, but he isn't
available until next week.  Do you mind taking a look?

[Finnur, 2016-01-25 10:10:17]

On 2016/01/22 19:54:36, mark a. foltz wrote:
> +finnur for toolbar related changes

I'm neither owner for these files in browser/ui/toolbar nor familiar with this
code... I therefore think my approval means little here.

[mark a. foltz, 2016-01-25 19:32:02]

On 2016/01/25 at 10:10:17, finnur wrote:
> On 2016/01/22 19:54:36, mark a. foltz wrote:
> > +finnur for toolbar related changes
> 
> I'm neither owner for these files in browser/ui/toolbar nor familiar with this
code... I therefore think my approval means little here.

Hm, you're listed as an OWNER of toolbar_actions_model.h?

per-file *toolbar_action*=finnur@chromium.org

I'll find a different reviewer for the other files.

[mark a. foltz, 2016-01-25 21:41:44]

mfoltz@chromium.org changed reviewers:
+ pkasting@chromium.org

[mark a. foltz, 2016-01-25 21:41:44]

Now adding +pkasting@ to review c/b/ui/toolbar changes, at the suggestion of
apacible@ :)

PTAL

[cpu_(ooo_6.6-7.5), 2016-01-25 23:30:34]

rubberstamp lgtm for command_ids header.

[Peter Kasting, 2016-01-26 02:12:35]

LGTM with one major question.

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:57: base::string16
MediaRouterContextualMenu::GetLabelForCommandId(
Why do you need this function at all?  It seems like all these items are
non-dynamic and thus SimpleMenuModel should never call this function.  Am I
mistaken?

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:136:
component_migration_helper->OnActionRemoved(
Nit: I'd probably just inline:

  ToolbarActionsModel::Get(browser_->profile())->component_migration_helper()
      ->OnActionRemoved(ComponentToolbarActionsFactory::kMediaRouterActionId);

This seems more readable to me.

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.h:183:
extensions::ComponentMigrationHelper* component_migration_helper() {
Nit: Seems like there's a section above for cheap accessors, I'd imagine this
should be with them.

I doubt the function comment is needed then, since what it says is already
reasonably obvious.

[mark a. foltz, 2016-01-26 21:28:39]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2016-01-26 21:28:40]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
mek@chromium.org, apacible@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1612203002/#ps40001
(title: "Respond to pkasting@ comments")

[commit-bot: I haz the power, 2016-01-26 21:29:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1612203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1612203002/40001

[commit-bot: I haz the power, 2016-01-26 23:10:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-26 23:12:43]

Description was changed from

==========
[Media Router] Adds a contextual menu item to remove the MR component action.

This adds a menu item to remove the MR component action from the toolbar.  It
sets a pref so this preference is respected across sessions.

The user can restore the action by reinstalling the Cast extension from the Web
Store.  (They may have to reload the Web Store page immediately after
installation to do so.)

BUG=576362
TESTED=Unit test, manually tested
==========

to

==========
[Media Router] Adds a contextual menu item to remove the MR component action.

This adds a menu item to remove the MR component action from the toolbar.  It
sets a pref so this preference is respected across sessions.

The user can restore the action by reinstalling the Cast extension from the Web
Store.  (They may have to reload the Web Store page immediately after
installation to do so.)

BUG=576362
TESTED=Unit test, manually tested

Committed: https://crrev.com/95cfa837a726c4400e025e826fcaddf1bfc751eb
Cr-Commit-Position: refs/heads/master@{#371629}
==========

[commit-bot: I haz the power, 2016-01-26 23:12:44]

Patchset 3 (id:??) landed as
https://crrev.com/95cfa837a726c4400e025e826fcaddf1bfc751eb
Cr-Commit-Position: refs/heads/master@{#371629}

[Finnur, 2016-01-27 17:18:39]

> Hm, you're listed as an OWNER of toolbar_actions_model.h?

Ah, yes. I missed that part. The other files in b/ui/toolbar I'm not owner for,
though.

toolbar_actions_model.h LGTM, FWIW.

[mark a. foltz, 2016-01-29 00:37:44]

Forgot to publish drafts after my last PS - sorry!

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:57: base::string16
MediaRouterContextualMenu::GetLabelForCommandId(
On 2016/01/26 at 02:12:35, Peter Kasting wrote:
> Why do you need this function at all?  It seems like all these items are
non-dynamic and thus SimpleMenuModel should never call this function.  Am I
mistaken?

Nope, this is a static menu so it looks like it's never called.  
Good catch - removed.

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:136:
component_migration_helper->OnActionRemoved(
On 2016/01/26 at 02:12:35, Peter Kasting wrote:
> Nit: I'd probably just inline:
> 
>   ToolbarActionsModel::Get(browser_->profile())->component_migration_helper()
>       ->OnActionRemoved(ComponentToolbarActionsFactory::kMediaRouterActionId);
> 
> This seems more readable to me.

Done

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1612203002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.h:183:
extensions::ComponentMigrationHelper* component_migration_helper() {
On 2016/01/26 at 02:12:35, Peter Kasting wrote:
> Nit: Seems like there's a section above for cheap accessors, I'd imagine this
should be with them.
> 
> I doubt the function comment is needed then, since what it says is already
reasonably obvious.

Both done.