Add policy for ash launcher auto-hide behavior

chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
index 6a0cfd173b2c2ef47e5d794172b04aff1990213f..801c7caa1d71a651093960f7113348c0ffc72374 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
@@ -126,12 +126,12 @@ std::string GetLocalOrRemotePref(PrefService* pref_service,
   return value.empty() ? pref_service->GetString(synced_path) : value;
 }
 
-// If prefs have synced and the pref value at |local_path| is empty the value
+// If prefs have synced and no user-set value exists at |local_path|, the value
 // from |synced_path| is copied to |local_path|.
 void MaybePropagatePrefToLocal(PrefService* pref_service,
                                const char* local_path,
                                const char* synced_path) {
-  if (pref_service->GetString(local_path).empty() &&
+  if (!pref_service->FindPreference(local_path)->HasUserSetting() &&
       pref_service->IsSyncing()) {
     // First time the user is using this machine, propagate from remote to
     // local.
@@ -176,6 +176,8 @@ ChromeLauncherController::ChromeLauncherController(Profile* profile,
                               content::Source<Profile>(profile_));
   pref_change_registrar_.Init(profile_->GetPrefs());
   pref_change_registrar_.Add(prefs::kPinnedLauncherApps, this);
+  pref_change_registrar_.Add(prefs::kShelfAlignmentLocal, this);
+  pref_change_registrar_.Add(prefs::kShelfAutoHideBehaviorLocal, this);
 }
 
 ChromeLauncherController::~ChromeLauncherController() {
@@ -557,28 +559,40 @@ bool ChromeLauncherController::CanPin() const {
   return pref && pref->IsUserModifiable();
 }
 
-void ChromeLauncherController::SetAutoHideBehavior(
-    ash::ShelfAutoHideBehavior behavior,
-    aura::RootWindow* root_window) {
-  ash::Shell::GetInstance()->SetShelfAutoHideBehavior(
-      behavior,
-      root_window);
-  // TODO(oshima): Support multiple launcher.
-  if (root_window != ash::Shell::GetPrimaryRootWindow())
-    return;
+ash::ShelfAutoHideBehavior ChromeLauncherController::GetShelfAutoHideBehavior(
+    aura::RootWindow* root_window) const {
+  // TODO(oshima): Support multiple launchers.
 
-  const char* value = NULL;
-  switch (behavior) {
-    case ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS:
-      value = ash::kShelfAutoHideBehaviorAlways;
-      break;
-    case ash::SHELF_AUTO_HIDE_BEHAVIOR_NEVER:
-      value = ash::kShelfAutoHideBehaviorNever;
-      break;
-  }
-  // See comment in |kShelfAlignment| about why we have two prefs here.
-  profile_->GetPrefs()->SetString(prefs::kShelfAutoHideBehaviorLocal, value);
-  profile_->GetPrefs()->SetString(prefs::kShelfAutoHideBehavior, value);
+  // See comment in |kShelfAlignment| as to why we consider two prefs.
+  const std::string behavior_value(
+      GetLocalOrRemotePref(profile_->GetPrefs(),
+                           prefs::kShelfAutoHideBehaviorLocal,
+                           prefs::kShelfAutoHideBehavior));

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

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]

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]

I'm working on this, and there are already code to support this. It's just not
completed.
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.

+
+  // Note: To maintain sync compatibility with old images of chrome/chromeos
+  // the set of values that may be encountered includes the now-extinct
+  // "Default" as well as "Never" and "Always", "Default" should now
+  // be treated as "Never".
+  // (http://code.google.com/p/chromium/issues/detail?id=146773)

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

nit: crbug.com/146773

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

Done.

+  if (behavior_value == ash::kShelfAutoHideBehaviorAlways)
+    return ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS;
+  return ash::SHELF_AUTO_HIDE_BEHAVIOR_NEVER;
+}
+
+bool ChromeLauncherController::CanUserModifyShelfAutoHideBehavior(
+    aura::RootWindow* root_window) const {
+  // TODO(oshima): Support multiple launchers.
+  return profile_->GetPrefs()->
+      FindPreference(prefs::kShelfAutoHideBehaviorLocal)->IsUserModifiable();
+}
+
+void ChromeLauncherController::ToggleShelfAutoHideBehavior(
+    aura::RootWindow* root_window) {
+  ash::ShelfAutoHideBehavior behavior = GetShelfAutoHideBehavior(root_window) ==
+      ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS ?
+          ash::SHELF_AUTO_HIDE_BEHAVIOR_NEVER :
+          ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS;
+  SetShelfAutoHideBehaviorPrefs(behavior, root_window);
 }
 
 void ChromeLauncherController::RemoveTabFromRunningApp(
@@ -979,25 +993,32 @@ void ChromeLauncherController::UpdateAppLaunchersFromPref() {
     DoPinAppWithID(*pref_app_id);
 }
 
-void ChromeLauncherController::SetShelfAutoHideBehaviorFromPrefs() {
-  // See comment in |kShelfAlignment| as to why we consider two prefs.
-  const std::string behavior_value(
-      GetLocalOrRemotePref(profile_->GetPrefs(),
-                           prefs::kShelfAutoHideBehaviorLocal,
-                           prefs::kShelfAutoHideBehavior));
+void ChromeLauncherController::SetShelfAutoHideBehaviorPrefs(
+    ash::ShelfAutoHideBehavior behavior,
+    aura::RootWindow* root_window) {
+  // TODO(oshima): Support multiple launchers.
+  if (root_window != ash::Shell::GetPrimaryRootWindow())
+    return;
 
-  // Note: To maintain sync compatibility with old images of chrome/chromeos
-  // the set of values that may be encountered includes the now-extinct
-  // "Default" as well as "Never" and "Always", "Default" should now
-  // be treated as "Never".
-  // (http://code.google.com/p/chromium/issues/detail?id=146773)
-  ash::ShelfAutoHideBehavior behavior =
-      ash::SHELF_AUTO_HIDE_BEHAVIOR_NEVER;
-  if (behavior_value == ash::kShelfAutoHideBehaviorAlways)
-    behavior = ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS;
+  const char* value = NULL;
+  switch (behavior) {
+    case ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS:
+      value = ash::kShelfAutoHideBehaviorAlways;
+      break;
+    case ash::SHELF_AUTO_HIDE_BEHAVIOR_NEVER:
+      value = ash::kShelfAutoHideBehaviorNever;
+      break;
+  }
+  // See comment in |kShelfAlignment| about why we have two prefs here.
+  profile_->GetPrefs()->SetString(prefs::kShelfAutoHideBehaviorLocal, value);
+  profile_->GetPrefs()->SetString(prefs::kShelfAutoHideBehavior, value);
+}
+
+void ChromeLauncherController::SetShelfAutoHideBehaviorFromPrefs() {
   // TODO(oshima): Support multiple displays.
+  aura::RootWindow* root_window = ash::Shell::GetPrimaryRootWindow();
   ash::Shell::GetInstance()->SetShelfAutoHideBehavior(
-      behavior, ash::Shell::GetPrimaryRootWindow());
+      GetShelfAutoHideBehavior(root_window), root_window);
 }
 
 void ChromeLauncherController::SetShelfAlignmentFromPrefs() {