sync: Implementation of Priority Preferences.

sync: Implementation of Priority Preferences.

Priority preferences are similar to normal preferences but are never encrypted and are synced ahead of other datatypes. Because they're never encrypted, on first sync, they can be synced immediately after login, before the user is prompted for a passphrase.

Expected uses of priority preferences include hardware setting (eg. trackpad speed, scroll direction, etc.) as well as settings that could be 2-way synced with existing Google Account settings (eg. language).

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

[albertb, 2013-02-12 22:50:06]

Since all the preferences that could benefit from this are chromeos-only, I
tested by making bookmark_bar.show_on_all_tabs a priority preference.

[albertb, 2013-02-21 22:31:18]

On 2013/02/12 22:50:06, albertb wrote:
> Since all the preferences that could benefit from this are chromeos-only, I
> tested by making bookmark_bar.show_on_all_tabs a priority preference.

Ping?

[Nicolas Zea, 2013-02-21 23:41:11]

FYI, you'll need a prefs/ owner to review as well

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/api/sync/p...
File chrome/browser/api/sync/profile_sync_service_base.h (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/api/sync/p...
chrome/browser/api/sync/profile_sync_service_base.h:40: virtual
syncer::ModelTypeSet GetPriorityDataTypes() const = 0;
Do you use this anywhere outside of ProfileSyncService? If not, I don't think
this doesn't need to be declared here.

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:343: sync_pb::PreferenceSpecifics
pref_specifics =
nit:prefer an if/else here based on type

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:446: LOG(ERROR) << "synced pref("
<< type_ << ") change: " << name;
did you mean to live this?

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry_syncable.h (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry_syncable.h:34: SyncableRegistrationCallback;
nit: third line should be indented another 4 spaces I believe

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1509:
data_type_manager_->Configure(priority_types,
hmm, I'm not sure this is the right place to do this. This means we'll
reconfigure the priority types anytime the user changes their preferred types.
Perhaps this should only be called as part of OnBackendInitialized?

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:448: if (type == syncer::PREFERENCES) {
Why is this necessary? Don't the pref groups handle this?

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs_unittest.cc (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs_unittest.cc:71: const syncer::ModelTypeSet
user_visible_types = GetUserVisibleTypes();
Why did this change?

https://codereview.chromium.org/12033093/diff/18001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/12033093/diff/18001/sync/syncable/model_type....
sync/syncable/model_type.cc:356:
encryptable_user_types.Remove(PRIORITY_PREFERENCES);
RemoveAll(PriorityUserTypes())?

[albertb, 2013-02-22 20:08:47]

I also fixed a couple of places in pref_model_associator where I wasn't reading
from the right specifics. Let me know if you think this "if (type_ == ...)"
business should live inside a separate function instead.

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/api/sync/p...
File chrome/browser/api/sync/profile_sync_service_base.h (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/api/sync/p...
chrome/browser/api/sync/profile_sync_service_base.h:40: virtual
syncer::ModelTypeSet GetPriorityDataTypes() const = 0;
On 2013/02/21 23:41:11, Nicolas Zea wrote:
> Do you use this anywhere outside of ProfileSyncService? If not, I don't think
> this doesn't need to be declared here.

Done.

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:343: sync_pb::PreferenceSpecifics
pref_specifics =
On 2013/02/21 23:41:11, Nicolas Zea wrote:
> nit:prefer an if/else here based on type

Done.

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:446: LOG(ERROR) << "synced pref("
<< type_ << ") change: " << name;
On 2013/02/21 23:41:11, Nicolas Zea wrote:
> did you mean to live this?

Done.

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry_syncable.h (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry_syncable.h:34: SyncableRegistrationCallback;
On 2013/02/21 23:41:11, Nicolas Zea wrote:
> nit: third line should be indented another 4 spaces I believe

Done.

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1509:
data_type_manager_->Configure(priority_types,
On 2013/02/21 23:41:11, Nicolas Zea wrote:
> hmm, I'm not sure this is the right place to do this. This means we'll
> reconfigure the priority types anytime the user changes their preferred types.
> Perhaps this should only be called as part of OnBackendInitialized?

Done.

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:448: if (type == syncer::PREFERENCES) {
On 2013/02/21 23:41:11, Nicolas Zea wrote:
> Why is this necessary? Don't the pref groups handle this?

Done.

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs_unittest.cc (right):

https://codereview.chromium.org/12033093/diff/18001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs_unittest.cc:71: const syncer::ModelTypeSet
user_visible_types = GetUserVisibleTypes();
On 2013/02/21 23:41:11, Nicolas Zea wrote:
> Why did this change?

Oops, probably a bad conflict resolve.

https://codereview.chromium.org/12033093/diff/18001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/12033093/diff/18001/sync/syncable/model_type....
sync/syncable/model_type.cc:356:
encryptable_user_types.Remove(PRIORITY_PREFERENCES);
On 2013/02/21 23:41:11, Nicolas Zea wrote:
> RemoveAll(PriorityUserTypes())?

Done.

[Nicolas Zea, 2013-02-25 23:47:47]

It would be nice to have some unit tests that exercise the model associator
logic, although really it's just the same unit tests with a different type. I
think there might be a way to do that with test fixtures?

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/api/sync/p...
File chrome/browser/api/sync/profile_sync_service_base.h (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/api/sync/p...
chrome/browser/api/sync/profile_sync_service_base.h:39: 
remove extra newline

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:51: if (type_ == PREFERENCES) {
Yeah, I think an anon namespace helper function would be useful for this logic,
e.g. GetSpecificsForPreference(&preference)

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:67: syncer::SyncableService*
GetSyncableService();
perhaps merge into one call that takes a ModelType as a parameter?

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:828: const syncer::ModelTypeSet
priority_types = GetPriorityDataTypes();
how about just intersecting the preferred datatypes with the set of priority
types instead of defining an extra function?

Also, now that I think about it, this call will only cover the case where a user
restarts after having set up sync already, or where the user is signing in with
an auto start device (cros, android). We should probably also handle the case
where setup is in progress, which means you have to trigger the priority
configuration after a SetSetupInProgress(false) call.

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs_unittest.cc (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs_unittest.cc:118: if (it.Get() ==
syncer::HISTORY_DELETE_DIRECTIVES) {
This should stay removed.

[albertb, 2013-03-01 22:01:56]

I made the suggested changes, but I'm a bit lost trying to make the PSS prefs
test fixture also work with priority prefs. Is testing::TestWithParam the way to
go? I can't find anybody else using this and having a hard time making it work.

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/api/sync/p...
File chrome/browser/api/sync/profile_sync_service_base.h (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/api/sync/p...
chrome/browser/api/sync/profile_sync_service_base.h:39: 
On 2013/02/25 23:47:47, Nicolas Zea wrote:
> remove extra newline

Done.

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:51: if (type_ == PREFERENCES) {
On 2013/02/25 23:47:47, Nicolas Zea wrote:
> Yeah, I think an anon namespace helper function would be useful for this
logic,
> e.g. GetSpecificsForPreference(&preference)

Done.

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:67: syncer::SyncableService*
GetSyncableService();
On 2013/02/25 23:47:47, Nicolas Zea wrote:
> perhaps merge into one call that takes a ModelType as a parameter? 

Done.

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:828: const syncer::ModelTypeSet
priority_types = GetPriorityDataTypes();
On 2013/02/25 23:47:47, Nicolas Zea wrote:
> how about just intersecting the preferred datatypes with the set of priority
> types instead of defining an extra function?
> 
> Also, now that I think about it, this call will only cover the case where a
user
> restarts after having set up sync already, or where the user is signing in
with
> an auto start device (cros, android). We should probably also handle the case
> where setup is in progress, which means you have to trigger the priority
> configuration after a SetSetupInProgress(false) call.

Done.

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs_unittest.cc (right):

https://codereview.chromium.org/12033093/diff/17002/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs_unittest.cc:118: if (it.Get() ==
syncer::HISTORY_DELETE_DIRECTIVES) {
On 2013/02/25 23:47:47, Nicolas Zea wrote:
> This should stay removed.

Done.

[Nicolas Zea, 2013-03-05 01:15:44]

See sync/notifier/invalidator_test_template.h for an example. That said, with
the GetSpecifics methods you have now, this isn't too important in my view, so
if it's hairy feel free to skip.

https://codereview.chromium.org/12033093/diff/24001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/24001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:28: const
sync_pb::PreferenceSpecifics& GetSpecifics(const syncer::ModelType& type,
|type| isn't needed in either of these methods (you can extrapolate it directly
from the specifics in the latter case, or the SyncData object in this case (it
has a GetDataType method)

https://codereview.chromium.org/12033093/diff/24001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12033093/diff/24001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:827: ConfigurePriorityDataTypes();
this should be before the normal types

[albertb, 2013-03-15 21:08:45]

I reverted the crazy tests for priority preferences.

https://codereview.chromium.org/12033093/diff/24001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/24001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:28: const
sync_pb::PreferenceSpecifics& GetSpecifics(const syncer::ModelType& type,
On 2013/03/05 01:15:44, Nicolas Zea wrote:
> |type| isn't needed in either of these methods (you can extrapolate it
directly
> from the specifics in the latter case, or the SyncData object in this case (it
> has a GetDataType method)

Done for the first one. But the second one needs to know the type because the
specifics that passed in is empty.

https://codereview.chromium.org/12033093/diff/24001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12033093/diff/24001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:827: ConfigurePriorityDataTypes();
On 2013/03/05 01:15:44, Nicolas Zea wrote:
> this should be before the normal types
Done.

[Nicolas Zea, 2013-03-18 19:18:27]

LGTM with nits

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:40: return
specifics->mutable_preference();
nit: DCHECK that the priority pref field doesn't exist (and vice versa below) to
ensure you don't accidentally set both datatypes.

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:75: void
AddRegisteredSyncablePreference(const char* path,
nit: pull down path argument onto new line, so they line up

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_components_factory_impl.cc:307:
syncer::PRIORITY_PREFERENCES)->AsWeakPtr();
nit: fix indent

[albertb, 2013-03-18 21:33:03]

+battre for chrome/browser/prefs and components/user_prefs

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:40: return
specifics->mutable_preference();
On 2013/03/18 19:18:27, Nicolas Zea wrote:
> nit: DCHECK that the priority pref field doesn't exist (and vice versa below)
to
> ensure you don't accidentally set both datatypes.

Done.

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:75: void
AddRegisteredSyncablePreference(const char* path,
On 2013/03/18 19:18:27, Nicolas Zea wrote:
> nit: pull down path argument onto new line, so they line up

Done.

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/12033093/diff/30001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_components_factory_impl.cc:307:
syncer::PRIORITY_PREFERENCES)->AsWeakPtr();
On 2013/03/18 19:18:27, Nicolas Zea wrote:
> nit: fix indent

Done.

[albertb, 2013-03-20 19:14:42]

-battre (vacation), +mnissler

Hi Mattias, can you please take a look a this?

Thanks!

[Mattias Nissler (ping if slow), 2013-03-21 06:50:15]

LGTM provided you address the nits and have a good answer to my question in the
comments. Note that I have now idea why we want priority prefs in general, I
assume Nicolas covers that part :)

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:32: return
pref.GetSpecifics().priority_preference().preference();
nit: Is assuming priority preference as the default if the data type being
passed is bogus the right thing? (I'm aware this cannot happen (TM))

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:44: return
specifics->mutable_priority_preference()->mutable_preference();
ditto

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:143: const char* path, const
PrefRegistrySyncable::PrefSyncStatus sync_status) {
nit: parameter decls on separate lines

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:66: // syncer::SyncableService
directly.
Is that TODO still relevant given that we can't implement both instances in the
same object? Or can we?

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:348: return prefs::kSyncPriorityPreferences;
Is there any migration needed here? I.e. for existing Chrome Profiles that have
Pref sync enabled but nothing else, what should happen if priority prefs become
supported after an update?

[Mattias Nissler (ping if slow), 2013-03-21 06:50:47]

Oh, the CL description should elaborate a bit on what this CL does and why.

[tim (not reviewing), 2013-03-21 16:36:00]

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:109: bool
PrefServiceSyncable::IsSyncing() {
This isn't going to work properly...  This is the mechanism (along with
PrefServiceObserver) that lets client code tell whether prefs have finished
model association yet and when that happens, which is important in certain
contexts (e.g https://codereview.chromium.org/11233049/), and we may need it for
priority prefs too (at the very least, it's not clear at all what IsSyncing
covers now).

https://codereview.chromium.org/12033093/diff/44001/components/user_prefs/pre...
File components/user_prefs/pref_registry_syncable.h (right):

https://codereview.chromium.org/12033093/diff/44001/components/user_prefs/pre...
components/user_prefs/pref_registry_syncable.h:38: SYNCABLE_PRIORITY_PREF,
We should explain these here.

[Mattias Nissler (ping if slow), 2013-03-21 16:59:40]

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:109: bool
PrefServiceSyncable::IsSyncing() {
On 2013/03/21 16:36:00, timsteele wrote:
> This isn't going to work properly...  This is the mechanism (along with
> PrefServiceObserver) that lets client code tell whether prefs have finished
> model association yet and when that happens, which is important in certain
> contexts (e.g <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/__extension_id__.png);">https://codereview.chromium.org/11233049/</crxref>),
and we may need it for
> priority prefs too (at the very least, it's not clear at all what IsSyncing
> covers now).    

FWIW, I consider this whole IsSyncing() call a hack. It seems like it was added
to support the case of pref consumers doing special logic on first sync. That's
something that could be implemented in better ways, e.g. allowing pref consumers
to register custom callbacks for merging incoming sync data with existing prefs.

[albertb, 2013-03-21 17:10:33]

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_model_associator.cc:32: return
pref.GetSpecifics().priority_preference().preference();
On 2013/03/21 06:50:15, Mattias Nissler wrote:
> nit: Is assuming priority preference as the default if the data type being
> passed is bogus the right thing? (I'm aware this cannot happen (TM))

I made normal preferences the default and added a DCHECK. Or were you suggesting
something else?

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:109: bool
PrefServiceSyncable::IsSyncing() {
On 2013/03/21 16:36:00, timsteele wrote:
> This isn't going to work properly...  This is the mechanism (along with
> PrefServiceObserver) that lets client code tell whether prefs have finished
> model association yet and when that happens, which is important in certain
> contexts (e.g https://codereview.chromium.org/11233049/), and we may need it
for
> priority prefs too (at the very least, it's not clear at all what IsSyncing
> covers now).    

Done.

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:109: bool
PrefServiceSyncable::IsSyncing() {
On 2013/03/21 16:36:00, timsteele wrote:
> This isn't going to work properly...  This is the mechanism (along with
> PrefServiceObserver) that lets client code tell whether prefs have finished
> model association yet and when that happens, which is important in certain
> contexts (e.g https://codereview.chromium.org/11233049/), and we may need it
for
> priority prefs too (at the very least, it's not clear at all what IsSyncing
> covers now).    

I updated the condition to also check priority prefs. Or are you suggesting that
there should be two IsSyncing methods instead?

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:143: const char* path, const
PrefRegistrySyncable::PrefSyncStatus sync_status) {
On 2013/03/21 06:50:15, Mattias Nissler wrote:
> nit: parameter decls on separate lines

Done.

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:348: return prefs::kSyncPriorityPreferences;
On 2013/03/21 06:50:15, Mattias Nissler wrote:
> Is there any migration needed here? I.e. for existing Chrome Profiles that
have
> Pref sync enabled but nothing else, what should happen if priority prefs
become
> supported after an update?

I'm not sure about this. Nicolas/Tim: what do you think?

https://codereview.chromium.org/12033093/diff/44001/components/user_prefs/pre...
File components/user_prefs/pref_registry_syncable.h (right):

https://codereview.chromium.org/12033093/diff/44001/components/user_prefs/pre...
components/user_prefs/pref_registry_syncable.h:38: SYNCABLE_PRIORITY_PREF,
On 2013/03/21 16:36:00, timsteele wrote:
> We should explain these here.

Done.

[tim (not reviewing), 2013-03-21 18:10:54]

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:109: bool
PrefServiceSyncable::IsSyncing() {
On 2013/03/21 17:10:33, albertb wrote:
> On 2013/03/21 16:36:00, timsteele wrote:
> > This isn't going to work properly...  This is the mechanism (along with
> > PrefServiceObserver) that lets client code tell whether prefs have finished
> > model association yet and when that happens, which is important in certain
> > contexts (e.g https://codereview.chromium.org/11233049/), and we may need it
> for
> > priority prefs too (at the very least, it's not clear at all what IsSyncing
> > covers now).    
> 
> I updated the condition to also check priority prefs. Or are you suggesting
that
> there should be two IsSyncing methods instead?

@albert - well, that won't work if you want to know when priority prefs have
synced, which is likely more useful than "normal prefs" -- in fact, I think we
might only want to care about priority prefs and change the "normal prefs" that
fall into this "must happen quickly", like the one in the linked CL, bucket to
be priority.  For now though, maybe just have two methods and a TODO.

@mnissler - one example is on Chrome OS, we'd like to avoid showing the
avatar-picker screen altogether if sync pulls down a choice the user made on
another device (which will be stored in a pref). The thinking is to use this
observer to signal. Can you explain what you were thinking in this context? 
(Happy to follow up offline / in email, we probably shouldn't change it with
this CL :P).

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:348: return prefs::kSyncPriorityPreferences;
On 2013/03/21 17:10:33, albertb wrote:
> On 2013/03/21 06:50:15, Mattias Nissler wrote:
> > Is there any migration needed here? I.e. for existing Chrome Profiles that
> have
> > Pref sync enabled but nothing else, what should happen if priority prefs
> become
> > supported after an update?
> 
> I'm not sure about this. Nicolas/Tim: what do you think?

We're not adding a checkbox in the UI for this, right?  So they should just get
enabled as part of prefs (I haven't checked the whole review too closely, but I
assume this is what you / Nicolas did?)

[albertb, 2013-03-21 18:46:29]

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:109: bool
PrefServiceSyncable::IsSyncing() {
On 2013/03/21 18:10:55, timsteele wrote:
> On 2013/03/21 17:10:33, albertb wrote:
> > On 2013/03/21 16:36:00, timsteele wrote:
> > > This isn't going to work properly...  This is the mechanism (along with
> > > PrefServiceObserver) that lets client code tell whether prefs have
finished
> > > model association yet and when that happens, which is important in certain
> > > contexts (e.g https://codereview.chromium.org/11233049/), and we may need
it
> > for
> > > priority prefs too (at the very least, it's not clear at all what
IsSyncing
> > > covers now).    
> > 
> > I updated the condition to also check priority prefs. Or are you suggesting
> that
> > there should be two IsSyncing methods instead?
> 
> @albert - well, that won't work if you want to know when priority prefs have
> synced, which is likely more useful than "normal prefs" -- in fact, I think we
> might only want to care about priority prefs and change the "normal prefs"
that
> fall into this "must happen quickly", like the one in the linked CL, bucket to
> be priority.  For now though, maybe just have two methods and a TODO.
> 
> @mnissler - one example is on Chrome OS, we'd like to avoid showing the
> avatar-picker screen altogether if sync pulls down a choice the user made on
> another device (which will be stored in a pref). The thinking is to use this
> observer to signal. Can you explain what you were thinking in this context? 
> (Happy to follow up offline / in email, we probably shouldn't change it with
> this CL :P).

Done.

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/12033093/diff/44001/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:348: return prefs::kSyncPriorityPreferences;
On 2013/03/21 18:10:55, timsteele wrote:
> On 2013/03/21 17:10:33, albertb wrote:
> > On 2013/03/21 06:50:15, Mattias Nissler wrote:
> > > Is there any migration needed here? I.e. for existing Chrome Profiles that
> > have
> > > Pref sync enabled but nothing else, what should happen if priority prefs
> > become
> > > supported after an update?
> > 
> > I'm not sure about this. Nicolas/Tim: what do you think?
> 
> We're not adding a checkbox in the UI for this, right?  So they should just
get
> enabled as part of prefs (I haven't checked the whole review too closely, but
I
> assume this is what you / Nicolas did?)

Yes, priority preferences are automatically enabled if (normal) preferences are.

[tim (not reviewing), 2013-03-21 20:58:15]

LGTM

[commit-bot: I haz the power, 2013-03-21 22:27:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/albertb@chromium.org/12033093/47002

[commit-bot: I haz the power, 2013-03-22 17:50:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/albertb@chromium.org/12033093/47002

[commit-bot: I haz the power, 2013-03-23 01:31:34]

Retried try job too often on mac_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-27 18:08:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/albertb@chromium.org/12033093/109001

[commit-bot: I haz the power, 2013-03-27 22:19:02]

Change committed as 191047