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

Splits shelf alignment and auto-hide behavior into two values, one
synced and one not synced. On first login to a machine the local value
is initialized to the synced value. Changes made on the local machine
set both places. This gives the effect that new machines get an
initial value from the last set value, but changes made don't effect
any existing machines the user is using.

BUG=139256
TEST=see bug
R=bauerb@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163758

[sky, 2012-10-23 00:24:21]

Bernhard, could you review this one?

Tim, I added you since I believe this is what we discussed.

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:140: bool HasSynced();
I'm not the biggest fan of this name. If you have a better suggestion say it.

I'm also not the biggest fan of exposing HasSyncedChanged. If you prefer an
interface that PrefsModelAssociator uses (or a friend) say it.

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

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:137: // from
|synced_path| is copied to |local_path|.
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]

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:137: // from
|synced_path| is copied to |local_path|.
On 2012/10/23 11:21:25, Bernhard Bauer wrote:
> 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).

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.

> 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().

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

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:137: // from
|synced_path| is copied to |local_path|.
On 2012/10/23 14:54:33, sky wrote:
> On 2012/10/23 11:21:25, Bernhard Bauer wrote:
> > 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).
> 
> 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.

I see. In that case, ...

> > 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().
>

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:790:
MaybePropagatePrefToLocal(profile_->GetPrefs(),
...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]

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

http://codereview.chromium.org/11233049/diff/2001/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:790:
MaybePropagatePrefToLocal(profile_->GetPrefs(),
On 2012/10/23 15:31:50, Bernhard Bauer wrote:
> ...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()?

Good point. Removed.

[sky, 2012-10-23 15:53:33]

New snapshot uploaded.

[Bernhard Bauer, 2012-10-23 16:05:56]

LGTM

[commit-bot: I haz the power, 2012-10-23 17:02:23]

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

[commit-bot: I haz the power, 2012-10-23 17:02:32]

Failed to apply patch for chrome/browser/prefs/pref_service.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/prefs/pref_service.h
  Hunk #1 FAILED at 16.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/prefs/pref_service.h.rej

Patch:       chrome/browser/prefs/pref_service.h
Index: chrome/browser/prefs/pref_service.h
diff --git a/chrome/browser/prefs/pref_service.h
b/chrome/browser/prefs/pref_service.h
index
a083cc92ed9f7a38b91b7d86f5a8dd099aa5ab92..6f4fba29b2cd412504fb2e2584faf018c5a553c6
100644
--- a/chrome/browser/prefs/pref_service.h
+++ b/chrome/browser/prefs/pref_service.h
@@ -16,6 +16,7 @@
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/observer_list.h"
 #include "base/threading/non_thread_safe.h"
 #include "chrome/browser/api/prefs/pref_service_base.h"
 
@@ -25,6 +26,7 @@ class PersistentPrefStore;
 class PrefModelAssociator;
 class PrefNotifier;
 class PrefNotifierImpl;
+class PrefServiceObserver;
 class PrefStore;
 class PrefValueStore;
 
@@ -128,6 +130,18 @@ class PrefService : public PrefServiceBase, public
base::NonThreadSafe {
   // immediately (basically, during shutdown).
   void CommitPendingWrite();
 
+  void AddObserver(PrefServiceObserver* observer);
+  void RemoveObserver(PrefServiceObserver* observer);
+
+  // Returns true if preferences state has synchronized with the remote
+  // preferences. If true is returned it can be assumed the local preferences
+  // has applied changes from the remote preferences. The two may not be
+  // identical if a change is in flight (from either side).
+  bool HasSynced();
+
+  // Invoked internally when the HasSynced() state changes.
+  void HasSyncedChanged();
+
   // PrefServiceBase implementation.
   virtual bool IsManagedPreference(const char* pref_name) const OVERRIDE;
   virtual bool IsUserModifiablePreference(const char* pref_name) const
OVERRIDE;
@@ -343,6 +357,8 @@ class PrefService : public PrefServiceBase, public
base::NonThreadSafe {
   // "forked" PrefService.
   bool pref_service_forked_;
 
+  ObserverList<PrefServiceObserver> observer_list_;
+
   DISALLOW_COPY_AND_ASSIGN(PrefService);
 };

[tim (not reviewing), 2012-10-23 17:24:12]

(I won't hold up the review, but I do have some comments)

http://codereview.chromium.org/11233049/diff/7001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/11233049/diff/7001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:143: void HasSyncedChanged();
This naming is confusing to me, it sounds like we're asking if the PrefService
"has synced a change", but we're actually *telling* it something.  

Maybe OnSyncModelAssociatedChanged() and HasSyncModelAssociated()? 
Or IsSyncing() and OnIsSyncingChanged() (which follows the
"SyncableService::MergeDataAndStartSyncing" naming.

http://codereview.chromium.org/11233049/diff/7001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_observer.h (right):

http://codereview.chromium.org/11233049/diff/7001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_observer.h:10: // Invoked when
PrefService::HasSycned() changes.
HasSynced

[sky, 2012-10-23 17:46:37]

http://codereview.chromium.org/11233049/diff/7001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/11233049/diff/7001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:143: void HasSyncedChanged();
On 2012/10/23 17:24:12, timsteele wrote:
> This naming is confusing to me, it sounds like we're asking if the PrefService
> "has synced a change", but we're actually *telling* it something.  
> 
> Maybe OnSyncModelAssociatedChanged() and HasSyncModelAssociated()? 
> Or IsSyncing() and OnIsSyncingChanged() (which follows the
> "SyncableService::MergeDataAndStartSyncing" naming.

Like I said, I wasn't particularly enamored with the names I went with. I
originally had ModelsAssociated(), but that is meaningless outside of sync. How
about leaving HasSynced() and renaming HasSyncedCHanged() to
OnSyncModelAssociatedChanged() . I realize they aren't consistant, but the later
is really meant to be internal and HasSyncModelAssociated() is extremely obscure
outside of those steeped in sync;)

[sky, 2012-10-24 00:18:44]

I'm going to land this.

Tim, I'm happy to change the name if you decide on something.

[commit-bot: I haz the power, 2012-10-24 00:19:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/11233049/11004

[tim (not reviewing), 2012-10-24 01:50:56]

On 2012/10/24 00:19:43, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/sky%40chromium.org/11233049/11004

Sorry about the delay.  Hm, yeah.  Well, I think Syncing is a better choice than
"Synced", because 1) Syncing is the terminology used by the public sync api (the
thing that the prefs code implements) and 2) Synced makes it sound like
something is "up to date / synced".  So I'd vote for IsSyncing() and
OnIsSyncingChanged()/OnSyncModelAssociatedChanged.

[commit-bot: I haz the power, 2012-10-24 03:15:46]

Change committed as 163758