Refactor preference syncing.

Refactor preference syncing.

The following are the primary changes:
1. Whenever a profile preference is registered, it must specify whether that 
preference should be synced or not. Local state preferences do not require this.
However, we DCHECK that local state preferences are in fact registered to the
 local state.
2. We've created a new sync model associator interface, which all datatypes
will eventually implement. This new model associator is designed to be
decoupled from much of the sync internals, and to be owned by the chrome service
being synced. It also implements most of the functionality previous handled by
the change processor.
3. The PrefService now owns its sync model associator
(pref_model_associator), which partially implements the new model associator
functionality. Further work will be done to remove the use of sync
transactions from the model associator, as well as to migrate other datatypes.

BUG=76232
TEST=

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

[Nicolas Zea, 2011-04-27 05:46:20]

Feel free to get get started. Looks like I still have a race I need to resolve,
as well as make sure I'm not leaking anywhere, but this is a start.

The switch to having the change processor do all the transaction stuff still
needs to happen, but that's a big todo.

[dhollowa, 2011-04-27 14:01:16]

http://codereview.chromium.org/6905044/diff/2001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/6905044/diff/2001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_manager.cc:256:
prefs->RegisterBooleanPref(prefs::kAutofillAuxiliaryProfilesEnabled,
This should not be sync'd because it is not a cross-platform feature.  Setting
it to |true| on Windows or Linux will cause asserts to fire.  See
personal_data_manager.cc:618.

[tim (not reviewing), 2011-04-27 19:04:31]

just occurred to me: this is an OWNERS nightmare :\  The only alternative I can
think of is having to add both APIs for a period and swap prefs over
incrementally, but it looks like that'd be tricky, due to having multiple places
declaring syncability of a pref.  And this is working now.  But it's also going
to be a challenge to find the OWNER for each subdir and get an LGTM...

On 2011/04/27 14:01:16, dhollowa wrote:
>
http://codereview.chromium.org/6905044/diff/2001/chrome/browser/autofill/auto...
> File chrome/browser/autofill/autofill_manager.cc (right):
> 
>
http://codereview.chromium.org/6905044/diff/2001/chrome/browser/autofill/auto...
> chrome/browser/autofill/autofill_manager.cc:256:
> prefs->RegisterBooleanPref(prefs::kAutofillAuxiliaryProfilesEnabled,
> This should not be sync'd because it is not a cross-platform feature.  Setting
> it to |true| on Windows or Linux will cause asserts to fire.  See
> personal_data_manager.cc:618.

[Randy Smith (Not in Mondays), 2011-04-27 20:07:52]

chrome/browser/download/* LGTM.

[Nicolas Zea, 2011-04-27 20:07:57]

Addressed comment.

If you feel the pref shouldn't be syncable on any platform though, feel free to
let me know, and I'll change the OSX case as well. Note that in current versions
of Chrome it is in fact being synced, as seen by its inclusion in the list of
preferences in synchronized_preferences.h (which is part of the reason for this
refactor, to make it more obvious what is synced and what isn't).

Re: the owners issue, I believe the conclusion of the massive email thread about
owners was that refactors that affect call sites (like this one), don't need all
owners LGTM, just from those where the code functionality is changed.

http://codereview.chromium.org/6905044/diff/2001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/6905044/diff/2001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_manager.cc:256:
prefs->RegisterBooleanPref(prefs::kAutofillAuxiliaryProfilesEnabled,
On 2011/04/27 14:01:16, dhollowa wrote:
> This should not be sync'd because it is not a cross-platform feature.  Setting
> it to |true| on Windows or Linux will cause asserts to fire.  See
> personal_data_manager.cc:618.

Thanks for catching this. The model associator actually supports non-cross
platform preferences. They are simply dropped on the machine that doesn't
support them, as long as it doesn't register them as syncable.

Changed non-osx case to not register as syncable.

[dhollowa, 2011-04-27 20:32:29]

LGTM for Autofill.  Fine to have this syncing on Mac-only.  Thanks.

On 2011/04/27 20:07:57, nzea wrote:
> Addressed comment.
> 
> If you feel the pref shouldn't be syncable on any platform though, feel free
to
> let me know, and I'll change the OSX case as well. Note that in current
versions
> of Chrome it is in fact being synced, as seen by its inclusion in the list of
> preferences in synchronized_preferences.h (which is part of the reason for
this
> refactor, to make it more obvious what is synced and what isn't).
> 
> Re: the owners issue, I believe the conclusion of the massive email thread
about
> owners was that refactors that affect call sites (like this one), don't need
all
> owners LGTM, just from those where the code functionality is changed.
> 
>
http://codereview.chromium.org/6905044/diff/2001/chrome/browser/autofill/auto...
> File chrome/browser/autofill/autofill_manager.cc (right):
> 
>
http://codereview.chromium.org/6905044/diff/2001/chrome/browser/autofill/auto...
> chrome/browser/autofill/autofill_manager.cc:256:
> prefs->RegisterBooleanPref(prefs::kAutofillAuxiliaryProfilesEnabled,
> On 2011/04/27 14:01:16, dhollowa wrote:
> > This should not be sync'd because it is not a cross-platform feature. 
Setting
> > it to |true| on Windows or Linux will cause asserts to fire.  See
> > personal_data_manager.cc:618.
> 
> Thanks for catching this. The model associator actually supports non-cross
> platform preferences. They are simply dropped on the machine that doesn't
> support them, as long as it doesn't register them as syncable.
> 
> Changed non-osx case to not register as syncable.

[tim (not reviewing), 2011-04-28 21:51:43]

Comments on /prefs/.  Will tackle /sync/ and friends next.

http://codereview.chromium.org/6905044/diff/3154/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.h (right):

http://codereview.chromium.org/6905044/diff/3154/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.h:27: class WriteNode;
nit- no indent for namespaces

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:27:
processing_syncapi_changes_(false),
could we combine these into a single "processing_changes_" bool?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:42:
NotificationType::PROFILE_SYNC_SERVICE_LOADING,
Are we sure we always get this? That there is no race where the PSS could be
created first?  Should we assert?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:64:
sync_service_->RegisterModelAssociator(this);
Could we registrar_.RevokeAll at this point?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:82: if (pref->IsUserModifiable())
{
It might be a bit cleaner to do
if (!IsUserModifiable()) {
  Associate(...);
  return true;
}

// do the rest

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:130: }
maybe a comment for the implicit 'else' case here?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:171: models_associated_ = false;
does it make sense to clear / reset anything else here, like
processing_changes_?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:220:
SyncIdToPreferenceNameMap::iterator iter = id_map_inverse_.find(sync_id);
do we actually use id_map_inverse_ anywhere?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:241: if (name ==
prefs::kURLsToRestoreOnStartup ||
Instead of doing this, could we not do local_pref->GetType() ?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:277: const Value& to_value) {
nit - indent

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:305: DCHECK(from_value.GetType()
== Value::TYPE_DICTIONARY);
DCHECK_EQ

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:334: void
PrefModelAssociator::AfterUpdateOperations(
SendUpdateNotificationsIfNecessary?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:346: // Not implemented;
nit - .

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:382: // TODO(ncarter): Can't look
up the name for deletions: lookup of
is this still true?  we have the entire specifics now, don't we?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:514: processing_syncapi_changes_ =
false;
Check out base::AutoReset (auto_reset.h)

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.h (right):

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.h:70: std::set<std::string>
synced_preferences();
const?

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/sync/glue/mod...
File chrome/browser/sync/glue/model_associator.h (right):

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/sync/glue/mod...
chrome/browser/sync/glue/model_associator.h:90: public
base::RefCounted<NewAssociatorInterface> {
Why RefCounted?

[Nicolas Zea, 2011-05-03 18:59:36]

Please take another look. I've refactored to no longer use notifications and to
minimize the amount of changes I'm making to various places. There is a separate
SyncableService we can inherit from, although it itself inherits from
AssociatorInterface (I've todo'd separating those).

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:27:
processing_syncapi_changes_(false),
On 2011/04/28 21:51:44, timsteele wrote:
> could we combine these into a single "processing_changes_" bool?

I don't think so. They get reset in different places and its possible for one
place it gets set to call the other.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:42:
NotificationType::PROFILE_SYNC_SERVICE_LOADING,
On 2011/04/28 21:51:44, timsteele wrote:
> Are we sure we always get this? That there is no race where the PSS could be
> created first?  Should we assert?

Not used anymore.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:64:
sync_service_->RegisterModelAssociator(this);
On 2011/04/28 21:51:44, timsteele wrote:
> Could we registrar_.RevokeAll at this point?

n/a.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:82: if (pref->IsUserModifiable())
{
On 2011/04/28 21:51:44, timsteele wrote:
> It might be a bit cleaner to do
> if (!IsUserModifiable()) {
>   Associate(...);
>   return true;
> }
> 
> // do the rest

Done.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:130: }
On 2011/04/28 21:51:44, timsteele wrote:
> maybe a comment for the implicit 'else' case here?

Done.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:171: models_associated_ = false;
On 2011/04/28 21:51:44, timsteele wrote:
> does it make sense to clear / reset anything else here, like
> processing_changes_?

Rest should be remain, as they're set before AssociateModels().

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:220:
SyncIdToPreferenceNameMap::iterator iter = id_map_inverse_.find(sync_id);
On 2011/04/28 21:51:44, timsteele wrote:
> do we actually use id_map_inverse_ anywhere?

Aside from here, no. There's a higher level todo though to remove the id_map in
general, since the preference tags are the pref names themselves, and that
should be all we need.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:241: if (name ==
prefs::kURLsToRestoreOnStartup ||
On 2011/04/28 21:51:44, timsteele wrote:
> Instead of doing this, could we not do local_pref->GetType() ?

I'm not sure how that would solve this? GetType tells you the type of
preference, while this is concerned with the preference name.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:277: const Value& to_value) {
On 2011/04/28 21:51:44, timsteele wrote:
> nit - indent

Done.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:305: DCHECK(from_value.GetType()
== Value::TYPE_DICTIONARY);
On 2011/04/28 21:51:44, timsteele wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:334: void
PrefModelAssociator::AfterUpdateOperations(
On 2011/04/28 21:51:44, timsteele wrote:
> SendUpdateNotificationsIfNecessary?

Done.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:346: // Not implemented;
On 2011/04/28 21:51:44, timsteele wrote:
> nit - .

Done.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:382: // TODO(ncarter): Can't look
up the name for deletions: lookup of
On 2011/04/28 21:51:44, timsteele wrote:
> is this still true?  we have the entire specifics now, don't we?

True, although we never delete preferences (due to legacy versions). I've
updated the comments.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:514: processing_syncapi_changes_ =
false;
On 2011/04/28 21:51:44, timsteele wrote:
> Check out base::AutoReset (auto_reset.h)

Nice, was wondering if something like that existed.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.h (right):

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.h:70: std::set<std::string>
synced_preferences();
On 2011/04/28 21:51:44, timsteele wrote:
> const?

Done.

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/sync/glue/mod...
File chrome/browser/sync/glue/model_associator.h (right):

http://codereview.chromium.org/6905044/diff/2002/chrome/browser/sync/glue/mod...
chrome/browser/sync/glue/model_associator.h:90: public
base::RefCounted<NewAssociatorInterface> {
On 2011/04/28 21:51:44, timsteele wrote:
> Why RefCounted?

Done.

[tim (not reviewing), 2011-05-05 17:45:41]

After all the discussions we've had and looking this over a bunch of times I
think it's safe to say the sync stuff LGTM :)  with a few nits.  

Looking forward to watching the processor / associator / bridge / service /
controller / manager soap opera unfold over the next week!

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.h (right):

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:34: static const char
kPreferencesTag[] = "google_chrome_preferences";
probably shouldn't have this in the global ns, at least not with this name. also
chrome code typically would use extern or store this on the class, and do the
init in a cc file.

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:141: bool
processing_syncer_changes_;
think we decided these could be combined after all?

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/sync/glue/ge...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:21: local_service_ = NULL;
... just a bit freaked out that this comment implies |this| would be used after
destruction?

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1126: void
ProfileSyncService::DeactivateDataType(
what's the plan for PSS::Activate/Deactivate?  can we add TODOs to remove them?
Seems like whoever calls this should have a ptr to the backend instead,
especially given the multi-threadedness

[Nicolas Zea, 2011-05-06 00:26:43]

Done! Will attempt to land tonight.

http://codereview.chromium.org/6905044/diff/3154/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.h (right):

http://codereview.chromium.org/6905044/diff/3154/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.h:27: class WriteNode;
On 2011/04/28 21:51:44, timsteele wrote:
> nit- no indent for namespaces

Done.

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:34: }
On 2011/05/05 17:45:42, timsteele wrote:
> probably shouldn't have this in the global ns, at least not with this name.
also
> chrome code typically would use extern or store this on the class, and do the
> init in a cc file.

Done.

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:141: PrefService* pref_service_;
On 2011/05/05 17:45:42, timsteele wrote:
> think we decided these could be combined after all?

Done.

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/sync/glue/ge...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:21: local_service_ = NULL;
On 2011/05/05 17:45:42, timsteele wrote:
> ... just a bit freaked out that this comment implies |this| would be used
after
> destruction?

It obviously shouldn't, but if it were I'd like it to crash immediately. Might
not be necessary, but we've had too many issues with change processor
destruction for me to not want to be safe.

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6905044/diff/11001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1126: void
ProfileSyncService::DeactivateDataType(
On 2011/05/05 17:45:42, timsteele wrote:
> what's the plan for PSS::Activate/Deactivate?  can we add TODOs to remove
them?
> Seems like whoever calls this should have a ptr to the backend instead,
> especially given the multi-threadedness

Done.

[Nicolas Zea, 2011-05-06 03:57:36]

Adding you two as reviewers to avoid sending out too many emails to the full
list :-)

[Mattias Nissler (ping if slow), 2011-05-06 04:00:01]

Looking at the pref parts of the change, I wondered about the following:
Nowadays, there are actually three different types of PrefService instances
within a running chrome instance:
(a) Profile::GetPrefs()
(b) BrowserProcess::local_state()
(c) Profile::GetOffTheRecordProfile::GetPrefs()
It seems that we only want to sync to handle (a), but not (b) and (c). For (b),
it doesn't even make sense to have the syncable parameter upon pref
registration, since it doesn't apply at all. For (c), we share registered prefs
with (a), so the situation is different: The syncable parameter makes sense, but
that PrefService instance doesn't care about anything sync-related. Thinking
about these observations, it seems questionable to implement the sync bits in
the PrefService core, given that it's not something that all PrefService
customers need. I very much like the idea of forcing people to say upon
registration time whether they want a pref to be synced, but can we find a way
to keep the more generic PrefService that isn't tied to sync and still have
that?

On a side note: Shouldn't you be removing the synchronized_preferences.h file?

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

http://codereview.chromium.org/6905044/diff/16002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:374: scoped_ptr<PrefModelAssociator>
pref_sync_associator_;
What's the advantage of adding the PrefModelAssociator into the internals of the
already non-trivial pref system? It seems like PrefModelAssociator needs two
things:
(a) check whether a pref is to be synced
(b) respond to pref notifications
We could achieve (a) by adding a state bit that can be queried through the
PrefService::Preference and (b) is already observable through the public
interface, so there'd be no need to host the associator within the PrefService.

[Peter Kasting, 2011-05-06 04:26:44]

Drive-by: Please use an enum instead of a bool.  I've tried to add detail to the
"argument types" section of
https://sites.google.com/a/chromium.org/dev/developers/coding-style .

My apologies for imposing this at such a late stage.

[tim (not reviewing), 2011-05-06 04:27:54]

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

http://codereview.chromium.org/6905044/diff/16002/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service.h:374: scoped_ptr<PrefModelAssociator>
pref_sync_associator_;
On 2011/05/06 04:00:02, Mattias Nissler wrote:
> What's the advantage of adding the PrefModelAssociator into the internals of
the
> already non-trivial pref system? It seems like PrefModelAssociator needs two
> things:
> (a) check whether a pref is to be synced
> (b) respond to pref notifications
> We could achieve (a) by adding a state bit that can be queried through the
> PrefService::Preference and (b) is already observable through the public
> interface, so there'd be no need to host the associator within the
PrefService.

Part of this is we're getting rid of our "glue" layer in sync and organizing
per-data type glue code alongside the main services as part of the "think sync"
effort.

Perhaps we should discuss over VC / in person? Nicolas / myself would be happy
to discuss how we anticipate this making life easier for maintainers of prefs.

We were assuming that prefs ownership was as decentralized as use of
RegisterPref :)  It's a relief to know there's someone we can talk to.

[tim (not reviewing), 2011-05-06 04:35:15]

And a general PSA to anyone who happens to be reading this review due to the
chromium-dev effect: yes, there will be plenty of discussion over the next few
weeks on how sync is changing, and you will be involved if any of it affects
code that you work on well before such changes are made :)

[Mattias Nissler (ping if slow), 2011-05-06 16:00:28]

On 2011/05/06 04:27:54, timsteele wrote:
>
http://codereview.chromium.org/6905044/diff/16002/chrome/browser/prefs/pref_s...
> File chrome/browser/prefs/pref_service.h (right):
> 
>
http://codereview.chromium.org/6905044/diff/16002/chrome/browser/prefs/pref_s...
> chrome/browser/prefs/pref_service.h:374: scoped_ptr<PrefModelAssociator>
> pref_sync_associator_;
> On 2011/05/06 04:00:02, Mattias Nissler wrote:
> > What's the advantage of adding the PrefModelAssociator into the internals of
> the
> > already non-trivial pref system? It seems like PrefModelAssociator needs two
> > things:
> > (a) check whether a pref is to be synced
> > (b) respond to pref notifications
> > We could achieve (a) by adding a state bit that can be queried through the
> > PrefService::Preference and (b) is already observable through the public
> > interface, so there'd be no need to host the associator within the
> PrefService.
> 
> Part of this is we're getting rid of our "glue" layer in sync and organizing
> per-data type glue code alongside the main services as part of the "think
sync"
> effort.

That useful, I'm just saying that inside the core PrefService may not be the
best location, but something that lives alongside the PrefService in the profile
might be better.

> 
> Perhaps we should discuss over VC / in person? Nicolas / myself would be happy
> to discuss how we anticipate this making life easier for maintainers of prefs.

Sure, feel free to grab some time on my calendar next week (but note I'll be ooo
Monday).

> 
> We were assuming that prefs ownership was as decentralized as use of
> RegisterPref :)  It's a relief to know there's someone we can talk to.

Happy to help!

[Nicolas Zea, 2011-05-08 02:38:12]

As per our discussions, the following changes have been made.
1. All bool should_sync have been converted to a PrefService::PrefSyncStatus
enum.
2. All callsites registering local state prefs now use the old format, and don't
require the sync enum (hence about 30 less modified files in this version of the
patch), with the exception of:
- ssl_config_service_manager.cc
- content_settings_policy_provider.cc
- cloud_policy_subsystems.cc
- web_resource_service.cc
These still use the PrefSyncStatus enum because they actually register the same
pref as both a local state pref and a profile pref (from the same callsite). As
a result, I defaulted to using the enum. It may be useful to file bugs against
them to remedy this (I don't know why they would want it so).
3. Added DCHECK's that ensure that any preference registered as a local state
pref is in fact being registered to the local state. There is a complementary
dcheck for profile prefs, but due to the four cases above, it's commented out
with a TODO to bring it back once all prefs are exclusively local or profile
prefs. I verified with it on that all other profile prefs are in fact profile
prefs.
4. Added comments to try and make the distinction between the registration of
local state and profile prefs clear.

Please take a look, and let me know what you think!

[Mattias Nissler (ping if slow), 2011-05-08 15:09:19]

LGTM from the pref side of things.

On 2011/05/08 02:38:12, nzea wrote:
> As per our discussions, the following changes have been made.
> 1. All bool should_sync have been converted to a PrefService::PrefSyncStatus
> enum.
> 2. All callsites registering local state prefs now use the old format, and
don't
> require the sync enum (hence about 30 less modified files in this version of
the
> patch), with the exception of:
> - ssl_config_service_manager.cc
> - content_settings_policy_provider.cc
> - cloud_policy_subsystems.cc
> - web_resource_service.cc
> These still use the PrefSyncStatus enum because they actually register the
same
> pref as both a local state pref and a profile pref (from the same callsite).
As
> a result, I defaulted to using the enum. It may be useful to file bugs against
> them to remedy this (I don't know why they would want it so).

I can't speak for all of them, but for the prefs::kPolicyRefreshRate pref, it's
intentionally registered in both namespaces (the user pref might go away soon
though). The same is true for prefs::kProxy. So we'll have to look at these on a
case by case basis. If they intentionally live in both namespaces, we just need
separate registration code paths.

> 3. Added DCHECK's that ensure that any preference registered as a local state
> pref is in fact being registered to the local state. There is a complementary
> dcheck for profile prefs, but due to the four cases above, it's commented out
> with a TODO to bring it back once all prefs are exclusively local or profile
> prefs. I verified with it on that all other profile prefs are in fact profile
> prefs.

Seems some of the trybots fail due to hitting the local_state DCHECK.

> 4. Added comments to try and make the distinction between the registration of
> local state and profile prefs clear.

Thanks!

> 
> Please take a look, and let me know what you think!

Good work and thanks for your patience. Don't hesitate to ping me in case you
want to discuss aspects of the follow-up work.