Splits shelf alignment and auto-hide behavior into two values, one

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 cbb76b9a1d88d313a126929eeeb59fe15f8f0537..95969cd7c6a390811c653e9cf1f9199b6285e468 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
@@ -124,6 +124,28 @@ class AppShortcutLauncherItemController : public LauncherItemController {
   DISALLOW_COPY_AND_ASSIGN(AppShortcutLauncherItemController);
 };
 
+// If the value of the pref at |local_path is not empty, it is returned
+// otherwise the value of the pref at |synced_path| is returned.
+std::string GetLocalOrRemotePref(PrefService* pref_service,
+                                 const char* local_path,
+                                 const char* synced_path) {
+  const std::string value(pref_service->GetString(local_path));
+  return value.empty() ? pref_service->GetString(synced_path) : value;
+}
+
+// If prefs have synced and the pref value at |local_path| is empty the value
+// from |synced_path| is copied to |local_path|.

[Bernhard Bauer, 2012/10/23 11:21:25]

Why do we need this propagation? If we always go through GetLocalOrRemotePref()
when reading prefs, we will consider the local value first, then the synced
value. The local value will only be written to when the user modifies the
setting, from which point on it will become the effective value (because it
takes precedence over the synced one). I think then we also don't need to find
out whether the pref service has synced, because we simply listen for pref
changes to the synced pref, and instead of propagating the value from synced to
local we then just read from prefs again via GetLocalOrRemotePref().

[sky, 2012/10/23 14:54:33]

Here's a better example of what the UX folks want. Lets say you have only one
machine A and set the pref to value to 1. If you then log into machine B the
pref should be that of the last set value, 1 in this case. If you go back to
machine A and change the pref to 2, only machine A should change to pref 2.
Machine B should remain at 1.

With what you are proposing machine B would get 2 in this case since the user
hasn't modified locally modified yet.

Ideally I would set the local pref from the synced pref at startup.
Unfortunately this doesn't work though since at startup sync hasn't yet
completed.

[Bernhard Bauer, 2012/10/23 15:31:50]

I see. In that case, ...

+void MaybePropagatePrefToLocal(PrefService* pref_service,
+                               const char* local_path,
+                               const char* synced_path) {
+  if (pref_service->GetString(local_path).empty() &&
+      pref_service->HasSynced()) {
+    // First time the user is using this machine, propagate from remote to
+    // local.
+    pref_service->SetString(local_path, pref_service->GetString(synced_path));
+  }
+}
+
 }  // namespace
 
 // ChromeLauncherController ----------------------------------------------------
@@ -190,6 +212,8 @@ ChromeLauncherController::~ChromeLauncherController() {
 
   if (observed_sync_service_)
     observed_sync_service_->RemoveObserver(this);
+
+  profile_->GetPrefs()->RemoveObserver(this);
 }
 
 void ChromeLauncherController::Init() {
@@ -222,6 +246,11 @@ void ChromeLauncherController::Init() {
   if (ash::Shell::HasInstance()) {
     SetShelfAutoHideBehaviorFromPrefs();
     SetShelfAlignmentFromPrefs();
+    PrefService* prefs = profile_->GetPrefs();
+    if (prefs->GetString(prefs::kShelfAlignmentLocal).empty() ||
+        prefs->GetString(prefs::kShelfAutoHideBehaviorLocal).empty()) {
+      prefs->AddObserver(this);
+    }
     ash::Shell::GetInstance()->AddShellObserver(this);
   }
 }
@@ -553,6 +582,8 @@ void ChromeLauncherController::SetAutoHideBehavior(
       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);
 }
 
@@ -753,14 +784,23 @@ void ChromeLauncherController::Observe(
     case chrome::NOTIFICATION_PREF_CHANGED: {
       const std::string& pref_name(
           *content::Details<std::string>(details).ptr());
-      if (pref_name == prefs::kPinnedLauncherApps)
+      if (pref_name == prefs::kPinnedLauncherApps) {
         UpdateAppLaunchersFromPref();
-      else if (pref_name == prefs::kShelfAlignment)
+      } else if (pref_name == prefs::kShelfAlignment) {
+        MaybePropagatePrefToLocal(profile_->GetPrefs(),

[Bernhard Bauer, 2012/10/23 15:31:50]

...do we need this now? I think we're only interested in pref changes to the
synced pref for the initial sync, in which case we would be notified via
OnHasSyncedChanged()?

[sky, 2012/10/23 15:53:27]

Good point. Removed.

+                                  prefs::kShelfAlignmentLocal,
+                                  prefs::kShelfAlignment);
+      } else if (pref_name == prefs::kShelfAlignmentLocal) {
         SetShelfAlignmentFromPrefs();
-      else if (pref_name == prefs::kShelfAutoHideBehavior)
+      } else if (pref_name == prefs::kShelfAutoHideBehavior) {
+        MaybePropagatePrefToLocal(profile_->GetPrefs(),
+                                  prefs::kShelfAutoHideBehaviorLocal,
+                                  prefs::kShelfAutoHideBehavior);
+      } else if (pref_name == prefs::kShelfAutoHideBehaviorLocal) {
         SetShelfAutoHideBehaviorFromPrefs();
-      else
+      } else {
         NOTREACHED() << "Unexpected pref change for " << pref_name;
+      }
       break;
     }
     default:
@@ -781,6 +821,8 @@ void ChromeLauncherController::OnShelfAlignmentChanged() {
       pref_value = ash::kShelfAlignmentRight;
       break;
   }
+  // See comment in |kShelfAlignment| about why we have two prefs here.
+  profile_->GetPrefs()->SetString(prefs::kShelfAlignmentLocal, pref_value);
   profile_->GetPrefs()->SetString(prefs::kShelfAlignment, pref_value);
 }
 
@@ -789,6 +831,15 @@ void ChromeLauncherController::OnStateChanged() {
   CheckAppSync();
 }
 
+void ChromeLauncherController::OnHasSyncedChanged() {
+  MaybePropagatePrefToLocal(profile_->GetPrefs(),
+                            prefs::kShelfAlignmentLocal,
+                            prefs::kShelfAlignment);
+  MaybePropagatePrefToLocal(profile_->GetPrefs(),
+                            prefs::kShelfAutoHideBehaviorLocal,
+                            prefs::kShelfAutoHideBehavior);
+}
+
 void ChromeLauncherController::PersistPinnedState() {
   // It is a coding error to call PersistPinnedState() if the pinned apps are
   // not user-editable. The code should check earlier and not perform any
@@ -942,13 +993,17 @@ void ChromeLauncherController::UpdateAppLaunchersFromPref() {
 }
 
 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));
+
   // 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)
-  const std::string behavior_value(
-      profile_->GetPrefs()->GetString(prefs::kShelfAutoHideBehavior));
   ash::ShelfAutoHideBehavior behavior =
       ash::SHELF_AUTO_HIDE_BEHAVIOR_NEVER;
   if (behavior_value == ash::kShelfAutoHideBehaviorAlways)
@@ -961,13 +1016,17 @@ void ChromeLauncherController::SetShelfAlignmentFromPrefs() {
           switches::kShowLauncherAlignmentMenu))
     return;
 
+  // See comment in |kShelfAlignment| as to why we consider two prefs.
   const std::string alignment_value(
-      profile_->GetPrefs()->GetString(prefs::kShelfAlignment));
+      GetLocalOrRemotePref(profile_->GetPrefs(),
+                           prefs::kShelfAlignmentLocal,
+                           prefs::kShelfAlignment));
   ash::ShelfAlignment alignment = ash::SHELF_ALIGNMENT_BOTTOM;
   if (alignment_value == ash::kShelfAlignmentLeft)
     alignment = ash::SHELF_ALIGNMENT_LEFT;
   else if (alignment_value == ash::kShelfAlignmentRight)
     alignment = ash::SHELF_ALIGNMENT_RIGHT;
+
   ash::Shell::GetInstance()->SetShelfAlignment(alignment);
 }