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

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 
 #include <set>
 #include <vector>
 
 #include "ash/launcher/launcher_model.h"
@@ skipping 106 matching lines @@
 
   // Stores the optional refocus url pattern for this item.
   const GURL& refocus_url() const { return refocus_url_; }
   void set_refocus_url(const GURL& refocus_url) { refocus_url_ = refocus_url; }
 
  private:
   GURL refocus_url_;
   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 ----------------------------------------------------
 
 // statics
 ChromeLauncherController* ChromeLauncherController::instance_ = NULL;
 
 ChromeLauncherController::ChromeLauncherController(Profile* profile,
                                                    ash::LauncherModel* model)
     : model_(model),
@@ skipping 46 matching lines @@
     model_->RemoveItemAt(model_->ItemIndexByID(i->first));
   }
   if (instance_ == this)
     instance_ = NULL;
 
   if (ash::Shell::HasInstance())
     ash::Shell::GetInstance()->RemoveShellObserver(this);
 
   if (observed_sync_service_)
     observed_sync_service_->RemoveObserver(this);
+
+  profile_->GetPrefs()->RemoveObserver(this);
 }
 
 void ChromeLauncherController::Init() {
   // TODO(xiyuan): Remove migration code and kUseDefaultPinnedApp after M20.
   // Migration cases:
   // - Users that unpin all apps:
   //   - have default pinned apps
   //   - kUseDefaultPinnedApps set to false
   //   Migrate them by setting an empty list for kPinnedLauncherApps.
   //
@@ skipping 12 matching lines @@
     ListPrefUpdate updater(profile_->GetPrefs(), prefs::kPinnedLauncherApps);
     updater.Get()->Clear();
   }
 
   UpdateAppLaunchersFromPref();
 
   // TODO(sky): update unit test so that this test isn't necessary.
   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);
   }
 }
 
 ash::LauncherID ChromeLauncherController::CreateTabbedLauncherItem(
     LauncherItemController* controller,
     IncognitoState is_incognito,
     ash::LauncherItemStatus status) {
   ash::LauncherID id = model_->next_id();
   DCHECK(!HasItemController(id));
@@ skipping 311 matching lines @@
   ash::Shell::GetInstance()->SetShelfAutoHideBehavior(behavior);
   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::RemoveTabFromRunningApp(
     TabContents* tab,
     const std::string& app_id) {
   tab_contents_to_app_id_.erase(tab);
   AppIDToTabContentsListMap::iterator i_app_id =
       app_id_to_tab_contents_list_.find(app_id);
   if (i_app_id != app_id_to_tab_contents_list_.end()) {
@@ skipping 180 matching lines @@
           details);
       const Extension* extension = unload_info->extension;
       if (IsAppPinned(extension->id()))
         DoUnpinAppsWithID(extension->id());
       app_icon_loader_->ClearImage(extension->id());
       break;
     }
     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:
       NOTREACHED() << "Unexpected notification type=" << type;
   }
 }
 
 void ChromeLauncherController::OnShelfAlignmentChanged() {
   const char* pref_value = NULL;
   switch (ash::Shell::GetInstance()->GetShelfAlignment()) {
     case ash::SHELF_ALIGNMENT_BOTTOM:
       pref_value = ash::kShelfAlignmentBottom;
       break;
     case ash::SHELF_ALIGNMENT_LEFT:
       pref_value = ash::kShelfAlignmentLeft;
       break;
     case ash::SHELF_ALIGNMENT_RIGHT:
       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);
 }
 
 void ChromeLauncherController::OnStateChanged() {
   DCHECK(observed_sync_service_);
   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
   // modification actions that trigger persisting the state.
   if (!CanPin()) {
     NOTREACHED() << "Can't pin but pinned state being updated";
     return;
   }
 
   // Set kUseDefaultPinnedApps to false and use pinned apps list from prefs
@@ skipping 133 matching lines @@
     else
       ++index;
   }
 
   // Append unprocessed items from the pref to the end of the model.
   for (; pref_app_id != pinned_apps.end(); ++pref_app_id)
     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));
+
   // 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)
     behavior = ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS;
   ash::Shell::GetInstance()->SetShelfAutoHideBehavior(behavior);
 }
 
 void ChromeLauncherController::SetShelfAlignmentFromPrefs() {
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
           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);
 }
 
 TabContents* ChromeLauncherController::GetLastActiveTabContents(
     const std::string& app_id) {
   AppIDToTabContentsListMap::const_iterator i =
       app_id_to_tab_contents_list_.find(app_id);
   if (i == app_id_to_tab_contents_list_.end())
     return NULL;
   DCHECK_GT(i->second.size(), 0u);
@@ skipping 80 matching lines @@
 }
 
 void ChromeLauncherController::StopLoadingAnimation() {
   DCHECK(observed_sync_service_);
 
   model_->SetStatus(ash::LauncherModel::STATUS_NORMAL);
   loading_timer_.Stop();
   observed_sync_service_->RemoveObserver(this);
   observed_sync_service_ = NULL;
 }