Add a policy to always show the Cast toolbar icon

Add a policy to always show the Cast toolbar icon

This CL adds a policy "ShowCastIconInToolbar," which will force the media router
toolbar action to be always shown in the toolbar or the overflow menu when
enabled.

When the policy is enabled, the action's context menu item "Always show icon"
will be replaced by "Added by your administrator," similar to the item
"Installed by your administrator" for policy-enabled extensions. This menu item
will always be disabled.

This CL adds a new pref with ID prefs::kShowCastIconInToolbar for the policy.
When we later remove ComponentMigrationHelper and the prefs associated with it
that we currently use to pin the media router action to the toolbar, we can use
prefs::kShowCastIconInToolbar to record user-initiated pinning of the action to
the toolbar.

BUG=666003
Review-Url: https://codereview.chromium.org/2559323002
Cr-Commit-Position: refs/heads/master@{#447289}
Committed: https://chromium.googlesource.com/chromium/src/+/1a5f2851a8a4cf5222a676dc8bcc7d52fc29a7f1

[takumif, 2016-12-08 22:20:23]

Patchset #1 (id:1) has been deleted

[takumif, 2016-12-13 00:28:05]

Patchset #1 (id:20001) has been deleted

[takumif, 2016-12-13 19:04:37]

Description was changed from

==========
Add a policy to always show the Cast toolbar icon

BUG=
==========

to

==========
Add a policy to always show the Cast toolbar icon

This CL adds a policy "ShowCastIconInToolbar," which will force the media router
toolbar action to be always shown in the toolbar or the overflow menu when
enabled.

When the policy is enabled, the action's context menu item "Always show icon"
will be replaced by "Added by your administrator," similar to the item
"Installed by your administrator" for policy-enabled extensions. This menu item
will always be disabled.

This CL adds a new pref with ID prefs::kShowCastIconInToolbar for the policy.
When we later remove ComponentMigrationHelper and the prefs associated with it
that we currently use to pin the media router action to the toolbar, we can use
prefs::kShowCastIconInToolbar to record user-initiated pinning of the action to
the toolbar.

BUG=666003
==========

[takumif, 2016-12-13 19:04:49]

Description was changed from

==========
Add a policy to always show the Cast toolbar icon

This CL adds a policy "ShowCastIconInToolbar," which will force the media router
toolbar action to be always shown in the toolbar or the overflow menu when
enabled.

When the policy is enabled, the action's context menu item "Always show icon"
will be replaced by "Added by your administrator," similar to the item
"Installed by your administrator" for policy-enabled extensions. This menu item
will always be disabled.

This CL adds a new pref with ID prefs::kShowCastIconInToolbar for the policy.
When we later remove ComponentMigrationHelper and the prefs associated with it
that we currently use to pin the media router action to the toolbar, we can use
prefs::kShowCastIconInToolbar to record user-initiated pinning of the action to
the toolbar.

BUG=666003
==========

to

==========
Add a policy to always show the Cast toolbar icon

This CL adds a policy "ShowCastIconInToolbar," which will force the media router
toolbar action to be always shown in the toolbar or the overflow menu when
enabled.

When the policy is enabled, the action's context menu item "Always show icon"
will be replaced by "Added by your administrator," similar to the item
"Installed by your administrator" for policy-enabled extensions. This menu item
will always be disabled.

This CL adds a new pref with ID prefs::kShowCastIconInToolbar for the policy.
When we later remove ComponentMigrationHelper and the prefs associated with it
that we currently use to pin the media router action to the toolbar, we can use
prefs::kShowCastIconInToolbar to record user-initiated pinning of the action to
the toolbar.

BUG=666003
==========

[takumif, 2016-12-13 19:08:31]

takumif@chromium.org changed reviewers:
+ imcheng@chromium.org

[takumif, 2016-12-13 19:08:32]

Please take a look. I'll send this out to all the owners once it looks good to
you. Thanks!

[imcheng, 2016-12-15 19:57:39]

Overall looks good. 2 questions:
- what happens if an admin enables the MediaRouterAction policy, but disabled
the MediaRouter policy?
- do we need any migration of sorts for the pref that was set by
ComponentMigrationHelper?

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:232: #if !defined(OS_ANDROID)
#if defined(ENABLE_MEDIA_ROUTER)

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:48: if
(MediaRouterActionController::IsActionShownByPolicy(profile))
Seems like we also need to check media_router::MediaRouterEnabled()?

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_controller.h (right):

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller.h:36: static bool
IsActionShownByPolicy(content::BrowserContext* context);
might as well use Profile here? Then you can do profile->GetPrefs() in the .cc.

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right):

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu.h:22: // Constructor for
injecting values in tests.
nit: empty line above

[takumif, 2016-12-15 21:26:57]

Patchset #2 (id:60001) has been deleted

[takumif, 2016-12-15 21:54:22]

Patchset #3 (id:100001) has been deleted

[takumif, 2016-12-15 22:02:51]

On 2016/12/15 19:57:39, imcheng wrote:
> Overall looks good. 2 questions:
> - what happens if an admin enables the MediaRouterAction policy, but disabled
> the MediaRouter policy?

The MediaRouter policy would take precedence. The MediaRouterAction policy is
used only in ComponentToolbarActionsFactory (where we check for
media_router::MediaRouterEnabled() first), and media router classes. I've added
a comment to policy_templates.json about this.

> - do we need any migration of sorts for the pref that was set by
> ComponentMigrationHelper?

Yes, we will, and it'll need to happen after we remove the Cast extension from
the Web Store. Another way to do this would be to keep on using the migration
pref (which is a dictionary mapping from action IDs to bool), but I thought that
wouldn't make much sense if we're removing the ComponentMigrationHelper.

[takumif, 2016-12-15 22:04:14]

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:232: #if !defined(OS_ANDROID)
On 2016/12/15 19:57:38, imcheng wrote:
> #if defined(ENABLE_MEDIA_ROUTER)

Done.

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:48: if
(MediaRouterActionController::IsActionShownByPolicy(profile))
On 2016/12/15 19:57:38, imcheng wrote:
> Seems like we also need to check media_router::MediaRouterEnabled()?

Yes, we should. Done.

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_controller.h (right):

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller.h:36: static bool
IsActionShownByPolicy(content::BrowserContext* context);
On 2016/12/15 19:57:38, imcheng wrote:
> might as well use Profile here? Then you can do profile->GetPrefs() in the
.cc.

Done.

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right):

https://codereview.chromium.org/2559323002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu.h:22: // Constructor for
injecting values in tests.
On 2016/12/15 19:57:38, imcheng wrote:
> nit: empty line above

Done.

[imcheng, 2016-12-20 19:34:35]

lgtm

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:50:
component_ids.insert(kMediaRouterActionId);
nit: blank line before return makes the code more readable IMO.

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.cc (right):

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.cc:84:
DCHECK(profile_);
This DCHECK won't work if profile is nullptr since it would segfault when
calling IsActionShownByPolicy. Consider adding a CHECK there and also changing
this to a CHECK.

[takumif, 2016-12-28 19:55:02]

takumif@chromium.org changed reviewers:
+ anthonyvd@chromium.org, grt@chromium.org, jwd@chromium.org, msw@chromium.org,
tnagel@chromium.org

[takumif, 2016-12-28 19:55:02]

takumif@chromium.org changed reviewers:
+ anthonyvd@chromium.org, grt@chromium.org, jwd@chromium.org, msw@chromium.org,
tnagel@chromium.org

[takumif, 2016-12-28 19:55:03]

anthonyvd@chromium.org: Please review changes in profile_impl.cc

grt@chromium.org: Please review changes in chrome_command_ids.h

tnagel@chromium.org: Please review changes in c/b/policy/* and
components/policy/*

msw@chromium.org: Please review changes in c/b/ui/toolbar/*

Thank you!

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:50:
component_ids.insert(kMediaRouterActionId);
On 2016/12/20 19:34:35, imcheng wrote:
> nit: blank line before return makes the code more readable IMO.

Done.

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.cc (right):

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.cc:84:
DCHECK(profile_);
On 2016/12/20 19:34:35, imcheng wrote:
> This DCHECK won't work if profile is nullptr since it would segfault when
> calling IsActionShownByPolicy. Consider adding a CHECK there and also changing
> this to a CHECK.

Done.

[takumif, 2016-12-28 19:55:03]

anthonyvd@chromium.org: Please review changes in profile_impl.cc

grt@chromium.org: Please review changes in chrome_command_ids.h

tnagel@chromium.org: Please review changes in c/b/policy/* and
components/policy/*

msw@chromium.org: Please review changes in c/b/ui/toolbar/*

Thank you!

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:50:
component_ids.insert(kMediaRouterActionId);
On 2016/12/20 19:34:35, imcheng wrote:
> nit: blank line before return makes the code more readable IMO.

Done.

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.cc (right):

https://codereview.chromium.org/2559323002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.cc:84:
DCHECK(profile_);
On 2016/12/20 19:34:35, imcheng wrote:
> This DCHECK won't work if profile is nullptr since it would segfault when
> calling IsActionShownByPolicy. Consider adding a CHECK there and also changing
> this to a CHECK.

Done.

[takumif, 2016-12-28 19:55:06]

anthonyvd@chromium.org: Please review changes in profile_impl.cc

grt@chromium.org: Please review changes in chrome_command_ids.h

tnagel@chromium.org: Please review changes in c/b/policy/* and
components/policy/*

msw@chromium.org: Please review changes in c/b/ui/toolbar/*

Thank you!

[takumif, 2016-12-28 19:55:06]

anthonyvd@chromium.org: Please review changes in profile_impl.cc

grt@chromium.org: Please review changes in chrome_command_ids.h

tnagel@chromium.org: Please review changes in c/b/policy/* and
components/policy/*

msw@chromium.org: Please review changes in c/b/ui/toolbar/*

Thank you!

[takumif, 2016-12-28 19:56:39]

jwd@chromium.org: Please review changes in histograms.xml. Thanks!

[grt (UTC plus 2), 2017-01-02 08:19:00]

chrome/app/chrome_command_ids.h lgtm

[anthonyvd, 2017-01-02 14:54:24]

c/b/profiles/ lgtm

[jwd, 2017-01-03 20:58:36]

histograms LGTM

[msw, 2017-01-05 01:14:58]

Mostly minor comments; thanks for your patience on my review.

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:49:
MediaRouterActionController::IsActionShownByPolicy(profile))
nit: curlies

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.cc (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.cc:33: if
(pref->IsManaged())
q: should this check for a non-null pref?

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.h (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.h:15: #include
"components/user_prefs/user_prefs.h"
nit: not needed here

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:44:
shown_by_policy_(shown_by_policy) {
This member isn't needed if it's only accessed in the ctor.

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_contextual_menu.h:24:
MediaRouterContextualMenu(Browser* browser, bool shown_by_policy);
Can the test mark the preference as managed (policy-defined) instead?

[takumif, 2017-01-06 21:54:50]

Patchset #6 (id:180001) has been deleted

[takumif, 2017-01-06 22:06:57]

Thanks for reviewing!

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:49:
MediaRouterActionController::IsActionShownByPolicy(profile))
On 2017/01/05 01:14:57, msw (vacation jan 4 maybe 5) wrote:
> nit: curlies

Done.

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.cc (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.cc:33: if
(pref->IsManaged())
On 2017/01/05 01:14:57, msw wrote:
> q: should this check for a non-null pref?

The pref gets registered in profile_impl.cc, so it shouldn't be null.

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.h (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.h:15: #include
"components/user_prefs/user_prefs.h"
On 2017/01/05 01:14:57, msw (vacation jan 4 maybe 5) wrote:
> nit: not needed here

Removed.

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:44:
shown_by_policy_(shown_by_policy) {
On 2017/01/05 01:14:57, msw (vacation jan 4 maybe 5) wrote:
> This member isn't needed if it's only accessed in the ctor.

Right, removed.

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_contextual_menu.h:24:
MediaRouterContextualMenu(Browser* browser, bool shown_by_policy);
On 2017/01/05 01:14:57, msw wrote:
> Can the test mark the preference as managed (policy-defined) instead?

That'd require moving the test to MediaRouterActionEnabledPolicyTest in
policy_browsertest.cc, and setting up there would be more difficult.

[msw, 2017-01-06 23:01:23]

lgtm

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right):

https://codereview.chromium.org/2559323002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_contextual_menu.h:24:
MediaRouterContextualMenu(Browser* browser, bool shown_by_policy);
On 2017/01/06 22:06:57, takumif wrote:
> On 2017/01/05 01:14:57, msw wrote:
> > Can the test mark the preference as managed (policy-defined) instead?
> 
> That'd require moving the test to MediaRouterActionEnabledPolicyTest in
> policy_browsertest.cc, and setting up there would be more difficult.

Hmm, okay; this ctor flag seems fine to use a unit test over a browser test.

[takumif, 2017-01-24 21:39:42]

takumif@chromium.org changed reviewers:
+ pastarmovj@chromium.org

[takumif, 2017-01-24 21:39:43]

pastarmovj@, please take a look at chrome/browser/policy/* and
components/policy/resources/policy_templates.json.
Thanks!

[pastarmovj, 2017-01-24 23:03:47]

https://codereview.chromium.org/2559323002/diff/240001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2559323002/diff/240001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:230: #if
defined(ENABLE_MEDIA_ROUTER)
nit: you can combine the two if-defs in one.

https://codereview.chromium.org/2559323002/diff/240001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2559323002/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:9116: 'desc': '''If this is
set to true or is not set, users will be able to cast tabs, sites or the desktop
from the browser. If set to false, this option will be disabled.'''
Please rework the description of this policy. This text is shown in the Group
policy Editor and therefore is considered public Chrome documentation. See the
policy below for inspiration.

Don't forget to include the section what happens if this policy is not set at
all. This is MS recommendation about the format.

Also what is "cast" in the label? Is this the "Chrome Cast" thing or is this
some generic name for a standard/protocol?

PS. Sorry to hi-jack the new policy to ask for these changes but I feel this
documentation has not been thoroughly reviewed when it was added in the first
place.

https://codereview.chromium.org/2559323002/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:9453: 'name':
'ShowCastIconInToolbar',
Please move this policy right after the EnableMediaRouter policy and consider
grouping them in a group called "Chrome Cast" or some such.

[takumif, 2017-01-26 23:12:34]

takumif@chromium.org changed reviewers:
- tnagel@chromium.org

[takumif, 2017-01-26 23:12:35]

Thanks for reviewing!

https://codereview.chromium.org/2559323002/diff/240001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2559323002/diff/240001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:230: #if
defined(ENABLE_MEDIA_ROUTER)
On 2017/01/24 23:03:46, pastarmovj wrote:
> nit: you can combine the two if-defs in one.

Done.

https://codereview.chromium.org/2559323002/diff/240001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2559323002/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:9116: 'desc': '''If this is
set to true or is not set, users will be able to cast tabs, sites or the desktop
from the browser. If set to false, this option will be disabled.'''
On 2017/01/24 23:03:46, pastarmovj wrote:
> Please rework the description of this policy. This text is shown in the Group
> policy Editor and therefore is considered public Chrome documentation. See the
> policy below for inspiration.
> 
> Don't forget to include the section what happens if this policy is not set at
> all. This is MS recommendation about the format.
> 
> Also what is "cast" in the label? Is this the "Chrome Cast" thing or is this
> some generic name for a standard/protocol?
> 
> PS. Sorry to hi-jack the new policy to ask for these changes but I feel this
> documentation has not been thoroughly reviewed when it was added in the first
> place.

Replacing 'cast' with 'Google Cast' and putting a description for it. The case
for the policy not being set is covered in the first sentence.

Sorry, what do you mean by MS recommendation?

https://codereview.chromium.org/2559323002/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:9453: 'name':
'ShowCastIconInToolbar',
On 2017/01/24 23:03:46, pastarmovj wrote:
> Please move this policy right after the EnableMediaRouter policy and consider
> grouping them in a group called "Chrome Cast" or some such.

Putting them in a group 'Google Cast'.

[pastarmovj, 2017-01-26 23:18:13]

lgtm

https://codereview.chromium.org/2559323002/diff/240001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2559323002/diff/240001/components/policy/reso...
components/policy/resources/policy_templates.json:9116: 'desc': '''If this is
set to true or is not set, users will be able to cast tabs, sites or the desktop
from the browser. If set to false, this option will be disabled.'''
On 2017/01/26 23:12:35, takumif wrote:
> On 2017/01/24 23:03:46, pastarmovj wrote:
> > Please rework the description of this policy. This text is shown in the
Group
> > policy Editor and therefore is considered public Chrome documentation. See
the
> > policy below for inspiration.
> > 
> > Don't forget to include the section what happens if this policy is not set
at
> > all. This is MS recommendation about the format.
> > 
> > Also what is "cast" in the label? Is this the "Chrome Cast" thing or is this
> > some generic name for a standard/protocol?
> > 
> > PS. Sorry to hi-jack the new policy to ask for these changes but I feel this
> > documentation has not been thoroughly reviewed when it was added in the
first
> > place.
> 
> Replacing 'cast' with 'Google Cast' and putting a description for it. The case
> for the policy not being set is covered in the first sentence.
> 
> Sorry, what do you mean by MS recommendation?

I meant the Microsoft guidelines for documenting policies. I can't dig the link
now but basically it says one should document the default behavior when the
policy is not set. 

I am sorry I missed the "...or is not set". It's good like it is.

[takumif, 2017-01-26 23:36:22]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 23:37:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 00:59:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 00:59:44]

Dry run: 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_...)

[takumif, 2017-01-27 20:29:58]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 20:30:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 21:41:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 21:41:57]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-01-31 17:53:53]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 17:54:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:01:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:01:59]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-01-31 19:02:29]

The CQ bit was checked by takumif@chromium.org

[takumif, 2017-01-31 19:02:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
imcheng@chromium.org, anthonyvd@chromium.org, jwd@chromium.org,
grt@chromium.org, msw@chromium.org, pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2559323002/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-31 19:03:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:10:54]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1485889349960790,
"parent_rev": "cde3926af285172a962c6befc4871af15d9d2adb", "commit_rev":
"1a5f2851a8a4cf5222a676dc8bcc7d52fc29a7f1"}

[commit-bot: I haz the power, 2017-01-31 19:11:32]

Description was changed from

==========
Add a policy to always show the Cast toolbar icon

This CL adds a policy "ShowCastIconInToolbar," which will force the media router
toolbar action to be always shown in the toolbar or the overflow menu when
enabled.

When the policy is enabled, the action's context menu item "Always show icon"
will be replaced by "Added by your administrator," similar to the item
"Installed by your administrator" for policy-enabled extensions. This menu item
will always be disabled.

This CL adds a new pref with ID prefs::kShowCastIconInToolbar for the policy.
When we later remove ComponentMigrationHelper and the prefs associated with it
that we currently use to pin the media router action to the toolbar, we can use
prefs::kShowCastIconInToolbar to record user-initiated pinning of the action to
the toolbar.

BUG=666003
==========

to

==========
Add a policy to always show the Cast toolbar icon

This CL adds a policy "ShowCastIconInToolbar," which will force the media router
toolbar action to be always shown in the toolbar or the overflow menu when
enabled.

When the policy is enabled, the action's context menu item "Always show icon"
will be replaced by "Added by your administrator," similar to the item
"Installed by your administrator" for policy-enabled extensions. This menu item
will always be disabled.

This CL adds a new pref with ID prefs::kShowCastIconInToolbar for the policy.
When we later remove ComponentMigrationHelper and the prefs associated with it
that we currently use to pin the media router action to the toolbar, we can use
prefs::kShowCastIconInToolbar to record user-initiated pinning of the action to
the toolbar.

BUG=666003

Review-Url: https://codereview.chromium.org/2559323002
Cr-Commit-Position: refs/heads/master@{#447289}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a5f2851a8a4cf5222a676dc8bcc...
==========

[commit-bot: I haz the power, 2017-01-31 19:11:34]

Committed patchset #10 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/1a5f2851a8a4cf5222a676dc8bcc...