Add policy for ash launcher auto-hide behavior

Add policy for ash launcher auto-hide behavior

This CL adds a policy that controls the ash launcher auto-hide behavior.
It also modifies the way that the launcher context menu reads and toggles
the current auto-hide behavior, ensuring that all changes go through the
kShelfAutoHideBehaviorLocal pref that the policy can override.

BUG=152929
TBR=davemoore@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170947

[bartfab (slow), 2012-11-21 14:01:48]

Hi Mitsuru,

Could you review these changes? As discussed offline, I am adding a policy that
allows the launcher auto-hide behavior to be managed.

Hi Scott,

Could you do an owner review of the ash/ changes?

Hi Dave,

Could you do an owner review of the chrome/browser/ui/ash/launcher/* changes?

Hi Mattias,

Could you review the policy changes?

[oshima, 2012-11-21 18:09:43]

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:570:
prefs::kShelfAutoHideBehavior));
There will be multiple launchers per displays, and a user will be
able to turn on/off auto hide per display (and alignment too).
And these settings really depends on which device you're using,
or what kind of display you're using, so it don't think it makes
sense to store remotely.

 Instead of saving the auto hide status in policy, how about
just storing the ability to change launcher behavior?

[bartfab (slow), 2012-11-21 22:17:42]

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:570:
prefs::kShelfAutoHideBehavior));
I know that support for multiple launchers is planned and I took it into account
in this CL:

Whenever the auto-hide behavior for a specific launcher is to be retrieved or
set, control passes through ChromeLauncherController's
GetShelfAutoHideBehavior() and ToggleShelfAutoHideBehavior() methods.

Right now, there is no support for multiple launchers and the methods simply
access the existing single-launcher prefs. Once support for multiple launchers
is added, I presume that you will still want to preserve the user's choices
locally across logouts and reboots. So you will still need some form of prefs.
You can simply modify these two methods then to retrieve and set different
prefs.

The fact that there are two prefs here and one of them is synced is not my
doing. As the code comments tell, this mechanism was introduced so that when a
user logs into a new device, the default behavior resembles their most recent
choice on the previously used device. When multiple-launcher support is added,
this will have to be rethought. If having this kind of persistent default
behavior is desired, *something* will have to be synced whenever the user makes
an explicit choice. How to condense the user's choices for X number of launchers
into a single syncable pref is an open question that will need to be addressed
then. But control can still continue to go through these two methods - they will
just need to handle whatever form of syncing is deemed appropriate.

Note that the way the new policy behaves, its effects will never be synced. The
policy overlays the kShelfAUtoHideBehaviorLocal pref. This pref's value is never
synced. It *does* get copied to the kShelfAutoHideBehavior pref which is synced
but *only* if the pref value has been explicitly set by the user. Thus, policy
never changes the synced value.

In summary, I think the way this code works is reasonable and easily adaptable
to the multiple-launcher future: Right now, policy simply overlays the single
kShelfAutoHideBehaviorLocal pref. When multiple launchers are added, this pref
will have to extended or adapted somehow. At that point, policy will have to
overlay the new pref(s). Independently of that consideration, there is a shadow
pref that gets synced. Its semantics are very simple right now (it mirrors the
local pref 1:1). In the future, this will also have to be changed - not because
of policy in any way but because of multiple launchers.

Please let me know if these explanations make sense. If you would like me to do
something differently, we can chat about it anytime. Right now, I still think
this is the correct way to implement things.

[oshima, 2012-11-26 18:57:33]

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:570:
prefs::kShelfAutoHideBehavior));
On 2012/11/21 22:17:42, bartfab wrote:
> I know that support for multiple launchers is planned and I took it into
account
> in this CL:
> 
> Whenever the auto-hide behavior for a specific launcher is to be retrieved or
> set, control passes through ChromeLauncherController's
> GetShelfAutoHideBehavior() and ToggleShelfAutoHideBehavior() methods.
> 
> Right now, there is no support for multiple launchers and the methods simply
> access the existing single-launcher prefs. Once support for multiple launchers
> is added, I presume that you will still want to preserve the user's choices
> locally across logouts and reboots. So you will still need some form of prefs.
> You can simply modify these two methods then to retrieve and set different
> prefs.

I'm working on this, and there are already code to support this. It's just not
completed.

> 
> The fact that there are two prefs here and one of them is synced is not my
> doing. As the code comments tell, this mechanism was introduced so that when a
> user logs into a new device, the default behavior resembles their most recent
> choice on the previously used device. When multiple-launcher support is added,
> this will have to be rethought. If having this kind of persistent default
> behavior is desired, *something* will have to be synced whenever the user
makes
> an explicit choice. How to condense the user's choices for X number of
launchers
> into a single syncable pref is an open question that will need to be addressed
> then. But control can still continue to go through these two methods - they
will
> just need to handle whatever form of syncing is deemed appropriate.
>
> Note that the way the new policy behaves, its effects will never be synced.
The
> policy overlays the kShelfAUtoHideBehaviorLocal pref. This pref's value is
never
> synced. It *does* get copied to the kShelfAutoHideBehavior pref which is
synced
> but *only* if the pref value has been explicitly set by the user. Thus, policy
> never changes the synced value.
> 
> In summary, I think the way this code works is reasonable and easily adaptable
> to the multiple-launcher future: Right now, policy simply overlays the single
> kShelfAutoHideBehaviorLocal pref. When multiple launchers are added, this pref
> will have to extended or adapted somehow. At that point, policy will have to
> overlay the new pref(s). Independently of that consideration, there is a
shadow
> pref that gets synced. Its semantics are very simple right now (it mirrors the
> local pref 1:1). In the future, this will also have to be changed - not
because
> of policy in any way but because of multiple launchers.
> 
> Please let me know if these explanations make sense. If you would like me to
do
> something differently, we can chat about it anytime. Right now, I still think
> this is the correct way to implement things.

My concern was whether or not launcher setting should be synced and you're
right that your CL doesn't change the behavior, so please disregard my comment
for now. I'll discuss this with PM/UX folks.

[oshima, 2012-11-26 19:13:00]

https://codereview.chromium.org/11418114/diff/1/ash/shell.cc
File ash/shell.cc (left):

https://codereview.chromium.org/11418114/diff/1/ash/shell.cc#oldcode698
ash/shell.cc:698: }
Given that this is used in two places, I prefer to keep this.

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:576: //
(http://code.google.com/p/chromium/issues/detail?id=146773)
nit: crbug.com/146773

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:236: void
ToggleShelfAutoHideBehavior(aura::RootWindow* root_window);
document these methods.

https://codereview.chromium.org/11418114/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:238: public:
do you need this?

[bartfab (slow), 2012-11-29 18:25:51]

https://chromiumcodereview.appspot.com/11418114/diff/1/ash/shell.cc
File ash/shell.cc (left):

https://chromiumcodereview.appspot.com/11418114/diff/1/ash/shell.cc#oldcode698
ash/shell.cc:698: }
On 2012/11/26 19:13:00, oshima wrote:
> Given that this is used in two places, I prefer to keep this.

This is actually not used in production code at all. The only legitimate user
was |ContextMenu|, a part of the ash_shell and thus developer code. You are
right that |LauncherContextMenu| also used to call this method but that was
wrong: Rather than asking the shell, the context menu should ask the
|ChromeLauncherController|. Only the controller knows about prefs and will
always return the current pref value (very important if the pref is being forced
to a certain value by policy).

If you want me to, I can restore this method - but there will be a single user,
in developer code. It would be wrong to have support code only needed for that
one use case in |Shell| IMHO.

https://chromiumcodereview.appspot.com/11418114/diff/1/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://chromiumcodereview.appspot.com/11418114/diff/1/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:576: //
(http://code.google.com/p/chromium/issues/detail?id=146773)
On 2012/11/26 19:13:00, oshima wrote:
> nit: crbug.com/146773

Done.

https://chromiumcodereview.appspot.com/11418114/diff/1/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://chromiumcodereview.appspot.com/11418114/diff/1/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:236: void
ToggleShelfAutoHideBehavior(aura::RootWindow* root_window);
On 2012/11/26 19:13:00, oshima wrote:
> document these methods.

Done.

https://chromiumcodereview.appspot.com/11418114/diff/1/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:238: public:
On 2012/11/26 19:13:00, oshima wrote:
> do you need this?

Sorry, this was a leftover from debugging.

[oshima, 2012-11-29 18:43:35]

https://chromiumcodereview.appspot.com/11418114/diff/1/ash/shell.cc
File ash/shell.cc (left):

https://chromiumcodereview.appspot.com/11418114/diff/1/ash/shell.cc#oldcode698
ash/shell.cc:698: }
On 2012/11/29 18:25:51, bartfab wrote:
> On 2012/11/26 19:13:00, oshima wrote:
> > Given that this is used in two places, I prefer to keep this.
> 
> This is actually not used in production code at all. The only legitimate user
> was |ContextMenu|, a part of the ash_shell and thus developer code. You are
> right that |LauncherContextMenu| also used to call this method but that was
> wrong: Rather than asking the shell, the context menu should ask the
> |ChromeLauncherController|. Only the controller knows about prefs and will
> always return the current pref value (very important if the pref is being
forced
> to a certain value by policy).
> 
> If you want me to, I can restore this method - but there will be a single
user,
> in developer code. It would be wrong to have support code only needed for that
> one use case in |Shell| IMHO.

This is used in launcher_context_menu.cc, and it's production code, isn't it?

[bartfab (slow), 2012-11-29 19:01:10]

https://chromiumcodereview.appspot.com/11418114/diff/1/ash/shell.cc
File ash/shell.cc (left):

https://chromiumcodereview.appspot.com/11418114/diff/1/ash/shell.cc#oldcode698
ash/shell.cc:698: }
On 2012/11/29 18:43:35, oshima wrote:
> On 2012/11/29 18:25:51, bartfab wrote:
> > On 2012/11/26 19:13:00, oshima wrote:
> > > Given that this is used in two places, I prefer to keep this.
> > 
> > This is actually not used in production code at all. The only legitimate
user
> > was |ContextMenu|, a part of the ash_shell and thus developer code. You are
> > right that |LauncherContextMenu| also used to call this method but that was
> > wrong: Rather than asking the shell, the context menu should ask the
> > |ChromeLauncherController|. Only the controller knows about prefs and will
> > always return the current pref value (very important if the pref is being
> forced
> > to a certain value by policy).
> > 
> > If you want me to, I can restore this method - but there will be a single
> user,
> > in developer code. It would be wrong to have support code only needed for
that
> > one use case in |Shell| IMHO.
> 
> This is used in launcher_context_menu.cc, and it's production code, isn't it?

As I mentioned above, yes, launcher_context_menu.cc was using this method but
that was wrong. By talking to the shell directly, the context menu was bypassing
the controller, taking away its chance to read the current value from prefs and
prevent any attempts to change it if the value is enforced by policy.

[oshima, 2012-11-29 19:06:19]

LGTM

On 2012/11/29 19:01:10, bartfab wrote:
> https://chromiumcodereview.appspot.com/11418114/diff/1/ash/shell.cc
> File ash/shell.cc (left):
> 
> https://chromiumcodereview.appspot.com/11418114/diff/1/ash/shell.cc#oldcode698
> ash/shell.cc:698: }
> On 2012/11/29 18:43:35, oshima wrote:
> > On 2012/11/29 18:25:51, bartfab wrote:
> > > On 2012/11/26 19:13:00, oshima wrote:
> > > > Given that this is used in two places, I prefer to keep this.
> > > 
> > > This is actually not used in production code at all. The only legitimate
> user
> > > was |ContextMenu|, a part of the ash_shell and thus developer code. You
are
> > > right that |LauncherContextMenu| also used to call this method but that
was
> > > wrong: Rather than asking the shell, the context menu should ask the
> > > |ChromeLauncherController|. Only the controller knows about prefs and will
> > > always return the current pref value (very important if the pref is being
> > forced
> > > to a certain value by policy).
> > > 
> > > If you want me to, I can restore this method - but there will be a single
> > user,
> > > in developer code. It would be wrong to have support code only needed for
> that
> > > one use case in |Shell| IMHO.
> > 
> > This is used in launcher_context_menu.cc, and it's production code, isn't
it?
> 
> As I mentioned above, yes, launcher_context_menu.cc was using this method but
> that was wrong. By talking to the shell directly, the context menu was
bypassing
> the controller, taking away its chance to read the current value from prefs
and
> prevent any attempts to change it if the value is enforced by policy.

Sorry I missed your message.That makes sense.

[commit-bot: I haz the power, 2012-11-29 19:17:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11418114/7001

[commit-bot: I haz the power, 2012-11-29 19:17:57]

Presubmit check for 11418114-7001 failed and returned exit status 1.


Running presubmit commit checks ...
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash
    chrome/browser/ui/ash/launcher

Presubmit checks took 1.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[bartfab (slow), 2012-11-29 19:19:06]

Hi Scott, Dave, Mattias,

Review ping.

[sky, 2012-11-29 20:32:48]

Can you wait for Harry to finish off his patch (
https://codereview.chromium.org/11377062/ ). He's changing a bunch of this code
and is nearly done. Also, Stefan recently copied of CLC into two files, you'll
need to update both for the time being.

[bartfab (slow), 2012-11-29 22:34:31]

On 2012/11/29 20:32:48, sky wrote:
> Can you wait for Harry to finish off his patch (
> https://codereview.chromium.org/11377062/ ). He's changing a bunch of this
code
> and is nearly done. Also, Stefan recently copied of CLC into two files, you'll
> need to update both for the time being.

That looks like a large refactor. Unfortunate timing as one of us will have to
redo his CL quite a bit.

[Harry McCleave, 2012-11-29 22:59:57]

On 2012/11/29 22:34:31, bartfab wrote:
> On 2012/11/29 20:32:48, sky wrote:
> > Can you wait for Harry to finish off his patch (
> > https://codereview.chromium.org/11377062/ ). He's changing a bunch of this
> code
> > and is nearly done. Also, Stefan recently copied of CLC into two files,
you'll
> > need to update both for the time being.
> 
> That looks like a large refactor. Unfortunate timing as one of us will have to
> redo his CL quite a bit.

I suspect it will be easier to rebase my change than wait and rebase this one.
So FWIW I say fire away.

[bartfab (slow), 2012-11-29 23:10:06]

On 2012/11/29 22:59:57, Harry McCleave wrote:
> On 2012/11/29 22:34:31, bartfab wrote:
> > On 2012/11/29 20:32:48, sky wrote:
> > > Can you wait for Harry to finish off his patch (
> > > https://codereview.chromium.org/11377062/ ). He's changing a bunch of this
> > code
> > > and is nearly done. Also, Stefan recently copied of CLC into two files,
> you'll
> > > need to update both for the time being.
> > 
> > That looks like a large refactor. Unfortunate timing as one of us will have
to
> > redo his CL quite a bit.
> 
> I suspect it will be easier to rebase my change than wait and rebase this one.
> So FWIW I say fire away.

Thanks Harry. Let the review continue... we will see whether this CL gets
approved first.

[sky, 2012-11-29 23:46:06]

https://codereview.chromium.org/11418114/diff/7001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/11418114/diff/7001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:134: if
(!pref_service->FindPreference(local_path)->HasUserSetting() &&
I'm not familiar with HasUserSetting. How does this differ than the old code?

https://codereview.chromium.org/11418114/diff/7001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:236: if
(prefs->GetString(prefs::kShelfAlignmentLocal).empty() ||
Does this need to be updated? Seems like at a minimum you can remove the
AddObserver because of 179/180 new.

[bartfab (slow), 2012-11-30 11:55:47]

https://chromiumcodereview.appspot.com/11418114/diff/7001/chrome/browser/ui/a...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://chromiumcodereview.appspot.com/11418114/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:134: if
(!pref_service->FindPreference(local_path)->HasUserSetting() &&
On 2012/11/29 23:46:06, sky wrote:
> I'm not familiar with HasUserSetting. How does this differ than the old code?

A quick bit of context: There are multiple sources of pref settings with
precedence increasing as: default, recommended, user, mandatory. Before policies
were added to the game here, the pref at |local_path| could either have a
default value (which is empty) or be user-set to some other value (which is
non-empty). So the current code worked as expected.

Now that policy can be used to control |local_path|, a non-empty pref value no
means that the user set a value. It could be coming from one of the other
sources, e.g. recommended or mandatory policy. The HasUserSetting() call does
what it says on the tin, it checks whether there actually is a user-set value or
not.

And a final bit of context: If there is no user-set value, line 138 will copy a
value from |synced_path| to |local_path|. If mandatory policy is in effect, it
will still continue to take precedence and the value just copied will be ignored
for now. But if policies goes away at any time (and it can, the admin can change
policy), the value will come into effect. So it is important to copy the value,
even if it should be currently overridden by policy.

https://chromiumcodereview.appspot.com/11418114/diff/7001/chrome/browser/ui/a...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:236: if
(prefs->GetString(prefs::kShelfAlignmentLocal).empty() ||
On 2012/11/29 23:46:06, sky wrote:
> Does this need to be updated? Seems like at a minimum you can remove the
> AddObserver because of 179/180 new.

Actually, prefs->AddObserver(this) is very misleading. The observer this adds
does not listen for pref changes but for sync status changes. And this is still
needed. What I should have updated is to switch from empty() to HasUserSetting()
- done now.

[Mattias Nissler (ping if slow), 2012-11-30 12:07:15]

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:3325: 'type': 'string-enum',
Is there a good reason not to make this a boolean?

[bartfab (slow), 2012-11-30 12:13:47]

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:3325: 'type': 'string-enum',
On 2012/11/30 12:07:15, Mattias Nissler wrote:
> Is there a good reason not to make this a boolean?

The reason is that it can be trivially added to
configuration_policy_handler_list and translates directly into the corresponding
pref. I can make it a bool and translate in a few lines of code instead of
course.

[Mattias Nissler (ping if slow), 2012-11-30 12:17:39]

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:3325: 'type': 'string-enum',
On 2012/11/30 12:13:47, bartfab wrote:
> On 2012/11/30 12:07:15, Mattias Nissler wrote:
> > Is there a good reason not to make this a boolean?
> 
> The reason is that it can be trivially added to
> configuration_policy_handler_list and translates directly into the
corresponding
> pref. I can make it a bool and translate in a few lines of code instead of
> course.

I would prefer the bool unless we have indication there's a chance the shelf
code adds additional enum constants. Do you know? If the authors are actually
keeping that option open, we should probably be defensive and keep the enum.

[bartfab (slow), 2012-11-30 12:22:38]

On 2012/11/30 12:17:39, Mattias Nissler wrote:
>
https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
> File chrome/app/policy/policy_templates.json (right):
> 
>
https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
> chrome/app/policy/policy_templates.json:3325: 'type': 'string-enum',
> On 2012/11/30 12:13:47, bartfab wrote:
> > On 2012/11/30 12:07:15, Mattias Nissler wrote:
> > > Is there a good reason not to make this a boolean?
> > 
> > The reason is that it can be trivially added to
> > configuration_policy_handler_list and translates directly into the
> corresponding
> > pref. I can make it a bool and translate in a few lines of code instead of
> > course.
> 
> I would prefer the bool unless we have indication there's a chance the shelf
> code adds additional enum constants. Do you know? If the authors are actually
> keeping that option open, we should probably be defensive and keep the enum.

Mitsuru, can you comment on the auto-hide enum? Is it planned to extend the enum
beyond "Always" and "Never" hide at some point? Or will this remain essentially
a boolean choice?

[sky, 2012-11-30 17:37:37]

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:3325: 'type': 'string-enum',
On 2012/11/30 12:17:40, Mattias Nissler wrote:
> On 2012/11/30 12:13:47, bartfab wrote:
> > On 2012/11/30 12:07:15, Mattias Nissler wrote:
> > > Is there a good reason not to make this a boolean?
> > 
> > The reason is that it can be trivially added to
> > configuration_policy_handler_list and translates directly into the
> corresponding
> > pref. I can make it a bool and translate in a few lines of code instead of
> > course.
> 
> I would prefer the bool unless we have indication there's a chance the shelf
> code adds additional enum constants. Do you know? If the authors are actually
> keeping that option open, we should probably be defensive and keep the enum.

Originally we had three values hence the enum, now we're at two. I prefer a
string value pref in the chrome side of things as it gives more flexibility and
is less error prone when migrating. Additionally the UX folks have frequently
gone back and forth in this area. That said, I'm not sure how this code maps to
the prefs or if compatibility matters here. If it's trivial to convert from
boolean->string or string->bool and you don't have to worry about backwards
compatibility then go for the bool.

[Mattias Nissler (ping if slow), 2012-11-30 17:39:04]

LGTM

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/11418114/diff/6003/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:3325: 'type': 'string-enum',
On 2012/11/30 17:37:37, sky wrote:
> On 2012/11/30 12:17:40, Mattias Nissler wrote:
> > On 2012/11/30 12:13:47, bartfab wrote:
> > > On 2012/11/30 12:07:15, Mattias Nissler wrote:
> > > > Is there a good reason not to make this a boolean?
> > > 
> > > The reason is that it can be trivially added to
> > > configuration_policy_handler_list and translates directly into the
> > corresponding
> > > pref. I can make it a bool and translate in a few lines of code instead of
> > > course.
> > 
> > I would prefer the bool unless we have indication there's a chance the shelf
> > code adds additional enum constants. Do you know? If the authors are
actually
> > keeping that option open, we should probably be defensive and keep the enum.
> 
> Originally we had three values hence the enum, now we're at two. I prefer a
> string value pref in the chrome side of things as it gives more flexibility
and
> is less error prone when migrating. Additionally the UX folks have frequently
> gone back and forth in this area. That said, I'm not sure how this code maps
to
> the prefs or if compatibility matters here. If it's trivial to convert from
> boolean->string or string->bool and you don't have to worry about backwards
> compatibility then go for the bool.

Thanks Scott for the background information. We have to worry about backwards
compatibility unfortunately. An enum it is then.

[bartfab (slow), 2012-12-03 09:26:40]

Hi Dave, Scott,

Review ping :).

[sky, 2012-12-03 16:02:58]

LGTM

[bartfab (slow), 2012-12-04 10:00:55]

Hi Dave,

Review ping.

[bartfab (slow), 2012-12-04 11:03:07]

Since I have been unable to reach Dave in almost two weeks and the CL has
approval from a chrome/browser/ui/ash owner, TBR'ing Dave for the launcher
changes. Dave, if there is any fallout from this CL, I am ready to jump in and
fix it of course.

[commit-bot: I haz the power, 2012-12-04 11:04:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11418114/14003

[commit-bot: I haz the power, 2012-12-04 13:00:37]

Change committed as 170947