Enables storing shelf's auto_hide_behavior and alignment per display.

Enables storing shelf's auto_hide_behavior and alignment per display.


BUG=165419
TEST=on a dev-mode device, change the launcher's auto-hide behavior and see how it's stored in Preferences

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

[Jun Mukai, 2012-12-14 02:15:55]

bartfab -- I found you did the policy thing in the chrome_browser_launcher_*
thing. Could you review and make sure I'm not doing wrong things?

I think my code is fine because it checks 'IsRecommended' before looking up the
per-display settings.

Thanks!

[bartfab (slow), 2012-12-14 09:46:10]

A few fixes are needed but in general, your approach for integrating with policy
is correct.

You can test policy very easily by the way. Build Chromium with chromeos=1. Then
create a JSON file with the following content:

{
  "ShelfAutoHideBehavior": "Never"
}

Or:

{
  "ShelfAutoHideBehavior": "Always"
}

You can now place this file in one of these two directories:

/etc/chromium/policies/recommended
/etc/chromium/policies/managed

Putting the file in the first directory makes it a recommended policy (a default
that can be overridden by the user). Putting it in the second directory makes it
a managed policy (this should always be enforced and should gray out any menu
items allowing the user to change settings).

When you place the policy file in one of these directories, Chromium should pick
it up in a few seconds. You should see the policy in chrome://policy and the
shelf should behave accordingly.

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

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:153: // If
the value of the pref at |local_path is not empty, it is returned
Nit (not your fault, but nice to fix): |local_path is missing the closing |.

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:160: if
(pref_service->FindPreference(local_path)->IsRecommended())
This should be IsManaged(). The logic should be, in decreasing order of
priority:

* If the local_path pref is managed, its value overrides everything else.
* If a user-set value specific to root_window exists, return that.
* If the local_path pref is recommended, return that.
* Finally, if none of the above applies, return synced_path.

The idea is that the most basic default is the synced value. This can be
overridden by a recommendation. This can be overridden by a manual setting. This
can be overridden by managed setting.

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:170:
display_pref->GetString(synced_path, &per_display_value)) {
You should renamed |synced_path| here and elsewhere. It is not clear at all from
its name that it is both the name of the synced pref and the per-root-window
dictionary key used to look up the right pref.

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

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
See chrome_launcher_controller_per_app.cc for comments. They apply to this file
as well.

https://codereview.chromium.org/11570017/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11570017/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:2095: const char kShelfAlignment[] =
"shelf_alignment";
Document that these prefs are overridden by per-display settings, if available.

https://codereview.chromium.org/11570017/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:2105: // its value is a dictionary of 'alingment'
for alignment and
"Alignment" is misspelled.

[oshima, 2012-12-14 11:03:54]

https://codereview.chromium.org/11570017/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/11570017/diff/1/ash/shell.cc#newcode721
ash/shell.cc:721: if
(GetRootWindowController(root_window)->SetShelfAlignment(alignment))
nit: you need {}

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

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:124: if
(!root_window)
can this be NULL?

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:130: return
std::string();
This shouldn't really happen. Can you add DCHECK()? You may leave if-check for
safety-ness.

[Jun Mukai, 2012-12-14 18:47:16]

https://codereview.chromium.org/11570017/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/11570017/diff/1/ash/shell.cc#newcode721
ash/shell.cc:721: if
(GetRootWindowController(root_window)->SetShelfAlignment(alignment))
On 2012/12/14 11:03:55, oshima wrote:
> nit: you need {} 

Done.

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

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:124: if
(!root_window)
On 2012/12/14 11:03:55, oshima wrote:
> can this be NULL?

no... removed.

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:130: return
std::string();
On 2012/12/14 11:03:55, oshima wrote:
> This shouldn't really happen. Can you add DCHECK()? You may leave if-check for
> safety-ness.

Done.

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:153: // If
the value of the pref at |local_path is not empty, it is returned
On 2012/12/14 09:46:10, bartfab wrote:
> Nit (not your fault, but nice to fix): |local_path is missing the closing |.

Done.

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:160: if
(pref_service->FindPreference(local_path)->IsRecommended())
On 2012/12/14 09:46:10, bartfab wrote:
> This should be IsManaged(). The logic should be, in decreasing order of
> priority:
> 
> * If the local_path pref is managed, its value overrides everything else.
> * If a user-set value specific to root_window exists, return that.
> * If the local_path pref is recommended, return that.
> * Finally, if none of the above applies, return synced_path.
> 
> The idea is that the most basic default is the synced value. This can be
> overridden by a recommendation. This can be overridden by a manual setting.
This
> can be overridden by managed setting.

Done.

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:170:
display_pref->GetString(synced_path, &per_display_value)) {
On 2012/12/14 09:46:10, bartfab wrote:
> You should renamed |synced_path| here and elsewhere. It is not clear at all
from
> its name that it is both the name of the synced pref and the per-root-window
> dictionary key used to look up the right pref.

renamed to just 'path'

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

https://codereview.chromium.org/11570017/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2012/12/14 09:46:10, bartfab wrote:
> See chrome_launcher_controller_per_app.cc for comments. They apply to this
file
> as well.

I think I've done manually...

https://codereview.chromium.org/11570017/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11570017/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:2095: const char kShelfAlignment[] =
"shelf_alignment";
On 2012/12/14 09:46:10, bartfab wrote:
> Document that these prefs are overridden by per-display settings, if
available.

Done.

https://codereview.chromium.org/11570017/diff/1/chrome/common/pref_names.cc#n...
chrome/common/pref_names.cc:2105: // its value is a dictionary of 'alingment'
for alignment and
On 2012/12/14 09:46:10, bartfab wrote:
> "Alignment" is misspelled.

thanks... updated, my code is rather using the same value of kShelfAlignment and
kShelfAutoHideBehavior.

[oshima, 2012-12-14 18:59:46]

lgtm my bits. please makes sure you get lgtm from bartfab.

[bartfab (slow), 2012-12-14 21:34:02]

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:149: //
Returns the pref in |pref_service| for |root_window|. The pref value
Nit: "Returns a pref value from |pref_service| for |root_window|." would be
clearer IMHO.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:151: // the
priority:
Nit: Turn the bullet points into faux sentences (capital letter at the start,
period at the end).

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:152: //  *
the value managed by the admin
1/ "Managed by the admin" is not the wording we usually use. "Managed by policy"
is the canonical expression.

2/ Please also state that this value is window-independent (i.e. the same value
for all windows).

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:154: //  *
the value recommended by the admin
1/ "Recommended by the admin" is not the wording we usually use. "Recommended by
policy" is the canonical expression.

2/ Please also state that this value is window-independent (i.e. the same value
for all windows).

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:155: //  *
otherwise, lookup the default value. |local_path| is preferred.
Nit: "look up" (two words).

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:155: //  *
otherwise, lookup the default value. |local_path| is preferred.
"|local_path| is preferred" actually hides a lot of complexity behind three
simple words. I think this comment should be folded into the bullet points,
i.e.:

* A value managed by policy. This is a single value that applies to all windows.
It is stored at |local_path|.
* A user-set value. This value can be set per root window and is stored in a
per-window dict.
* A value recommended by policy. This is a single value that applies to all
windows. It is stored at |local_path|.
* A user-set value found at |local_path|. (maybe add an explanation here that
this comes from sync)
* A user-set or default value found at |path|.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:178: if
(local_pref->IsRecommended())
This will work but I want to double-check the logic is the one you want:

If there is both a value at |local_path| or |path| and a recommended value,
which one should take precedence? The case where this happens is the first login
into a device. One pref value can be pulled in by sync, another pref value could
be recommended. Would it not be better to use the value pulled in from sync?
Arguably, it was a value the user chose. It should override the admin's
recommendation.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc
(right):

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
Again, all the same comments as in the other file :).

https://codereview.chromium.org/11570017/diff/8001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11570017/diff/8001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:2095: // values in kShelfPreferences.
... unless overridden by managed policy, in which case these values take
precedence again :). I think that should be noted here.

Sorry for being so nit-picky but the behavior is non-obvious and it is very
useful to document it properly.

[Jun Mukai, 2012-12-14 22:44:56]

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:149: //
Returns the pref in |pref_service| for |root_window|. The pref value
On 2012/12/14 21:34:02, bartfab wrote:
> Nit: "Returns a pref value from |pref_service| for |root_window|." would be
> clearer IMHO.

Done.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:151: // the
priority:
On 2012/12/14 21:34:02, bartfab wrote:
> Nit: Turn the bullet points into faux sentences (capital letter at the start,
> period at the end).

Done.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:152: //  *
the value managed by the admin
On 2012/12/14 21:34:02, bartfab wrote:
> 1/ "Managed by the admin" is not the wording we usually use. "Managed by
policy"
> is the canonical expression.
> 
> 2/ Please also state that this value is window-independent (i.e. the same
value
> for all windows).

Done.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:154: //  *
the value recommended by the admin
On 2012/12/14 21:34:02, bartfab wrote:
> 1/ "Recommended by the admin" is not the wording we usually use. "Recommended
by
> policy" is the canonical expression.
> 
> 2/ Please also state that this value is window-independent (i.e. the same
value
> for all windows).

Done.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:155: //  *
otherwise, lookup the default value. |local_path| is preferred.
On 2012/12/14 21:34:02, bartfab wrote:
> Nit: "look up" (two words).

Done.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:155: //  *
otherwise, lookup the default value. |local_path| is preferred.
On 2012/12/14 21:34:02, bartfab wrote:
> "|local_path| is preferred" actually hides a lot of complexity behind three
> simple words. I think this comment should be folded into the bullet points,
> i.e.:
> 
> * A value managed by policy. This is a single value that applies to all
windows.
> It is stored at |local_path|.
> * A user-set value. This value can be set per root window and is stored in a
> per-window dict.
> * A value recommended by policy. This is a single value that applies to all
> windows. It is stored at |local_path|.
> * A user-set value found at |local_path|. (maybe add an explanation here that
> this comes from sync)
> * A user-set or default value found at |path|.

This is slightly wrong actually.  User-set value should be prior to the
recommended one right? Following mostly but tweaking a bit.
I think the reason of |local_path| is preferred is too long and it's delegated
to the comment in pref_names.cc in this files, so I just noted here too.

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:178: if
(local_pref->IsRecommended())
On 2012/12/14 21:34:02, bartfab wrote:
> This will work but I want to double-check the logic is the one you want:
> 
> If there is both a value at |local_path| or |path| and a recommended value,
> which one should take precedence? The case where this happens is the first
login
> into a device. One pref value can be pulled in by sync, another pref value
could
> be recommended. Would it not be better to use the value pulled in from sync?
> Arguably, it was a value the user chose. It should override the admin's
> recommendation.

You make the point. Sure, the user pref from sync should override the
recommended value.  Fixed...

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc
(right):

https://codereview.chromium.org/11570017/diff/8001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2012/12/14 21:34:02, bartfab wrote:
> Again, all the same comments as in the other file :).

Done.

https://codereview.chromium.org/11570017/diff/8001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11570017/diff/8001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:2095: // values in kShelfPreferences.
On 2012/12/14 21:34:02, bartfab wrote:
> ... unless overridden by managed policy, in which case these values take
> precedence again :). I think that should be noted here.
> 
> Sorry for being so nit-picky but the behavior is non-obvious and it is very
> useful to document it properly.

Done.

[bartfab (slow), 2012-12-14 23:07:18]

I think the pref getter needs one more tweak - see comments inline. As it is
late at night here, I will not be able to look at the CL tonight.

LGTM if you make sure that the getter really does return values in the correct
priority order.

Thanks for all the fix-ups. And thanks for taking policy into account.

https://codereview.chromium.org/11570017/diff/8003/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/11570017/diff/8003/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:183: if
(!local_pref->HasUserSetting() &&
This condition will actually never be true. |pref| is the value that came from
sync. If will either be unset (in which the condition is obviously false) or the
value will have been copied to |local_pref| by MaybePropagatePrefToLocal(), in
which case the condition will also be false.

I think the right logic here is as simple as:

return value;

This yields the following priority order:

* If |local_pref| was set by the user, that user-set value gets returned.
* If |local_pref| is recommended by policy, the recommended value is returned.
* If neither of the above apply, by my reading of this file, |pref| will be
equally unset. Hence, no point in checking it. Just return |value|.

https://codereview.chromium.org/11570017/diff/8003/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc
(right):

https://codereview.chromium.org/11570017/diff/8003/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
As usual, same comment as in the other file.

[Jun Mukai, 2012-12-14 23:40:56]

https://codereview.chromium.org/11570017/diff/8003/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/11570017/diff/8003/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:183: if
(!local_pref->HasUserSetting() &&
On 2012/12/14 23:07:18, bartfab wrote:
> This condition will actually never be true. |pref| is the value that came from
> sync. If will either be unset (in which the condition is obviously false) or
the
> value will have been copied to |local_pref| by MaybePropagatePrefToLocal(), in
> which case the condition will also be false.
> 
> I think the right logic here is as simple as:
> 
> return value;
> 
> This yields the following priority order:
> 
> * If |local_pref| was set by the user, that user-set value gets returned.
> * If |local_pref| is recommended by policy, the recommended value is returned.
> * If neither of the above apply, by my reading of this file, |pref| will be
> equally unset. Hence, no point in checking it. Just return |value|.

Agreed. Done.

https://codereview.chromium.org/11570017/diff/8003/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc
(right):

https://codereview.chromium.org/11570017/diff/8003/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2012/12/14 23:07:18, bartfab wrote:
> As usual, same comment as in the other file.

Done.

[commit-bot: I haz the power, 2012-12-14 23:44:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/11570017/3006

[commit-bot: I haz the power, 2012-12-14 23:44:52]

Presubmit check for 11570017-3006 failed and returned exit status 1.


Running presubmit commit checks ...

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

Presubmit checks took 1.4s to calculate.

[Jun Mukai, 2012-12-14 23:49:53]

sky, please give me the OWNER approval for ash & c/b/ui/ash

[sky, 2012-12-17 17:00:03]

LGTM - is it possible to add test coverage?

[Jun Mukai, 2012-12-17 17:32:40]

On 2012/12/17 17:00:03, sky wrote:
> LGTM - is it possible to add test coverage?

Can I do that in another CL? I'd like to put this into M25 branch if possible.

[sky, 2012-12-17 17:43:18]

Ok

On Mon, Dec 17, 2012 at 9:32 AM,  <mukai@chromium.org> wrote:
> On 2012/12/17 17:00:03, sky wrote:
>>
>> LGTM - is it possible to add test coverage?
>
>
> Can I do that in another CL? I'd like to put this into M25 branch if
> possible.
>
> https://codereview.chromium.org/11570017/