Implement new SyncAPI and convert Preferences to it.

Implement new SyncAPI and convert Preferences to it.

BUG=76232
TEST=Everything sync.

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

[Nicolas Zea, 2011-05-10 01:40:51]

Very much WIP.

[tim (not reviewing), 2011-05-10 02:19:56]

Couple meta comments. Let's put this new stuff under its own directory in sync,
such as /api/ or /includes/ or something. sync/ is pretty cluttered.

Could really be two patches.  We should at least add prefs folks to the review
for that portion.

'adapater'

Think you can remove GetSyncableHash from syncapi?

[Nicolas Zea, 2011-05-13 19:54:58]

The bulk of the changes are in pref_model_associator and
generic_change_processor. In addition, I added/moved a number of files to the
api folder. Rest of changes are to just make everything work and cleaning up
some headers.

Re: GenerateSyncableHash, I went ahead and reverted those changes and instead
rewrote the associator to not rely on it or sync id's. Instead, everything is
based on the pref name as the client tag.

+akalin for pref model associator and api changes
+tim for general sync changes
+mattias as fyi on how sync services are changing and for /prefs ownership. The
functionality of preference syncing is still the same.

[akalin, 2011-05-13 21:53:19]

Some initial comments (API only)

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_event.h (right):

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:13: #include
"chrome/browser/sync/api/sync_event_value.h"
you can forward-declare SyncEventValue

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:14: #include
"chrome/browser/sync/protocol/sync.pb.h"
Also forward-declare EntitySpecifics.  Hopefully that'll mean that including
this header won't pull in generated headers (unless model_type somehow does so)

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:21: // Note: it is safe to pass these by
value or make copies, as they do not
safe -> safe and cheap

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:25: // Default constructor creates an
immutable event with no action.
maybe say that it creates an 'invalid' event, and default constructor is only
needed for STL container inclusion

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:28: // Copy constructor and assignemnt
operator welcome.
assignemnt -> assignment

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:32: static SyncEvent CreateDelete(const
std::string& tag);
I'd prefer a single method Create(Type) -- perhaps the specifics of the Delete
could be for the just-deleted item?  Seems useful to have.

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:44: // Query methods for event action. One
of theses will always be set.
theses -> these

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:45: bool IsSet() const;
maybe IsValid()?

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:56: const sync_pb::EntitySpecifics&
Specifics() const;
maybe GetSpecifics, GetType, etc.

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:58: syncable::ModelType Type() const;
what about typedef-ing syncable::ModelType to SyncType?  Exposing the syncable
namespace seems kinda icky to me.

Ideally, the type enum would exist separately from syncable, but that may be
something we can fix in the future.

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:61: const std::string& Tag() const;
did you mean to keep this exposed?

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:73: // Ordered list of SyncEvent's. The
list is filled in the order the events
I think this belongs in sync_event_bridge.h.

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event.h:78: typedef std::map<std::string,
SyncEvent> SyncEventMap;
Do you think we should expose a typedef for this?  Some data types may want to
have a non-string key.

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_event_bridge.h (right):

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event_bridge.h:13: public:
have an empty virtual protected destructor

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_event_value.h (right):

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event_value.h:26: // The action that triggered the
event.
Since these members are public, no need for trailing underscores

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event_value.h:42: class SyncEventValue : public
base::RefCountedThreadSafe<SyncEventValue> {
what about naming this SharedSyncEventData?  Naming it *Value may be confusing
since there are existing Value classese.

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_event_value.h:54: virtual ~SyncEventValue();
no need for virtual (inheritance is only really used to add methods)

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/syncable_service.h (right):

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/syncable_service.h:15: virtual ~SyncableService();
destructor should be protected

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/syncable_service.h:17: // SyncEventBridge
implementation.
not an implementation -- maybe say something like: repeating SyncEventBridge
methods for clarity.

OVERRIDE keyword?

http://codereview.chromium.org/6995008/diff/7002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/syncable_service.h:35:
DISALLOW_COPY_AND_ASSIGN(SyncableService);
shouldn't have this on interface classes

[Nicolas Zea, 2011-05-18 01:40:47]

Okay! Please take another look. Made the changes we discussed in splitting up
SyncEvent from SyncData and the resulting change of
SyncEventBridge->SyncEventProcessor. More forward declare action happening as
well.

[akalin, 2011-05-19 00:58:45]

Looked through everything.  Looks good overall, but a bunch of comments.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:141: // We're not syncing this
preference locally, ignore the sync data.
Hmm.  If we want to maintain the invariant that GetAllSyncData() called after
MergeDataAndStartSyncing() should return the same state as the sync state, then
wouldn't ignoring the sync data here break that invariant?

I guess this would only happen if, say, M15 adds a new preference to sync that
M14 didn't have.  I think that happens rarely enough that we can get away with
maintaining another map to keep track of these prefs.  Although I'm okay with
making that a TODO.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:194: JSONStringValueSerializer
json(&serialized);
You can use JSONWriter::Write from base/json/json_writer.h directly, which
always succeeds.  (json.Serialize only returns false if you passed in NULL in
its constructor or if you initialized it with a const string ref, which are
cases we shouldn't run into).  Then you can make this function not have to
return a bool.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:283: bool
PrefModelAssociator::GetAllSyncData(syncable::ModelType type,
You can make this function return SyncDataList directly if you fix
CreatePrefSyncData not to return a bool

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:321: NOTREACHED();
you can do:

NOTREACHED() << "Attempted ..."

since NOTREACHED() is a LOG(ERROR) in release mode.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:327: // Skip values we can't
deserialize.
I wonder if we should delete the sync data in this case?  Maybe add a TODO.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:335: const char* pref_name =
name.c_str();
I guess this is another one of those corner cases... :/

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:8: sync_entity_.reset(sync_entity);
Is there a swap method we could use instead?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.h (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:14: #include
"chrome/browser/sync/protocol/sync.pb.h"
replace with fwd declare

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:20: class RefCountedSyncEntity : public
consider making this a private inner class of SyncData.

Also, SharedSyncEntity may be a shorter name.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:27: const sync_pb::SyncEntity&
sync_entity();
function should be const

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:50: static SyncData CreateLocalData(const
std::string& sync_tag);
What do you think about making these (possibly-friended) non-member functions?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:55: // Helper method for creating SyncData
objects originating from the syncer.
I think we should find some way to not expose this in the public header.  Maybe
make them (possibly-friended) non-member functions that are declared in another
header?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:69: // Returns the value of the unique
client tag. This is only set for data going
Did we end up needing this?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_event.h (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_event.h:19: class SyncEvent {
As we discussed, SyncEvent -> SyncChange, etc.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_event.h:57: typedef std::vector<SyncEvent>
SyncEventList;
Remove this (already in sync_event_processor.h)

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service.h (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:30: syncable::ModelType type,
I'm not a fan of needing to reference the syncable namespace here.  I notice we
have a SyncDataType typedef in sync_data.h.  Perhaps have a sync_data_type.h
that only has 'typedef syncable::ModelType SyncDataType' and use that
everywhere?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:32: const SyncDataList&
initial_sync_data) = 0;
maybe move initial-sync_data to before the sync_processor, since that follows
the order of the name (merge, then start syncing)

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

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:97:
SyncEventList::const_iterator event_iterator;
usual style is to just use 'it' for iterators, right?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:110: NOTREACHED();
I feel like this is a recoverable error -- maybe we should just 'continue' in
this case?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:136: if
(event.sync_data().GetTag() == "" ||
i think the empty tag case is a recoverable error

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:149:
error_handler()->OnUnrecoverableError(FROM_HERE,
I think we can be resilient to bad clients and just drop invalid changes (with
NOTREACHED())

http://codereview.chromium.org/6995008/diff/17001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1987: 'browser/sync/api/sync_data.cc',
can you move the api stuff to its own target?  (if it's not too hairy)

[Nicolas Zea, 2011-05-19 21:17:45]

Changes made and/or explanation given. Please take another look.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:141: // We're not syncing this
preference locally, ignore the sync data.
On 2011/05/19 00:58:45, akalin wrote:
> Hmm.  If we want to maintain the invariant that GetAllSyncData() called after
> MergeDataAndStartSyncing() should return the same state as the sync state,
then
> wouldn't ignoring the sync data here break that invariant?
> 
> I guess this would only happen if, say, M15 adds a new preference to sync that
> M14 didn't have.  I think that happens rarely enough that we can get away with
> maintaining another map to keep track of these prefs.  Although I'm okay with
> making that a TODO.

In the case of preferences, it also happens for platform-specific prefs. For
example there are preferences on windows that mac and linux don't care about
(and in some cases would cause crashes if they did sync). Added TODO anyways
though.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:194: JSONStringValueSerializer
json(&serialized);
On 2011/05/19 00:58:45, akalin wrote:
> You can use JSONWriter::Write from base/json/json_writer.h directly, which
> always succeeds.  (json.Serialize only returns false if you passed in NULL in
> its constructor or if you initialized it with a const string ref, which are
> cases we shouldn't run into).  Then you can make this function not have to
> return a bool.

Added TODO for this, since it's kind of outside the scope of this patch.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:283: bool
PrefModelAssociator::GetAllSyncData(syncable::ModelType type,
On 2011/05/19 00:58:45, akalin wrote:
> You can make this function return SyncDataList directly if you fix
> CreatePrefSyncData not to return a bool

This would introduce a dependency on SyncData in the header though. In addition,
there may be some reason GetAllSyncData would fail, so I'd like to leave that
option open.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:321: NOTREACHED();
On 2011/05/19 00:58:45, akalin wrote:
> you can do:
> 
> NOTREACHED() << "Attempted ..."
> 
> since NOTREACHED() is a LOG(ERROR) in release mode.

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:327: // Skip values we can't
deserialize.
On 2011/05/19 00:58:45, akalin wrote:
> I wonder if we should delete the sync data in this case?  Maybe add a TODO.
 Well, there's no real concept of deleting preferences, so what would we delete?
And I don't really see any point of disassociating this preference, since I
think we would still want to support overwriting the value if it changes on any
machine.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:335: const char* pref_name =
name.c_str();
On 2011/05/19 00:58:45, akalin wrote:
> I guess this is another one of those corner cases... :/

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:8: sync_entity_.reset(sync_entity);
On 2011/05/19 00:58:45, akalin wrote:
> Is there a swap method we could use instead?

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.h (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:14: #include
"chrome/browser/sync/protocol/sync.pb.h"
On 2011/05/19 00:58:45, akalin wrote:
> replace with fwd declare

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:20: class RefCountedSyncEntity : public
On 2011/05/19 00:58:45, akalin wrote:
> consider making this a private inner class of SyncData.
> 
> Also, SharedSyncEntity may be a shorter name.

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:27: const sync_pb::SyncEntity&
sync_entity();
On 2011/05/19 00:58:45, akalin wrote:
> function should be const

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:50: static SyncData CreateLocalData(const
std::string& sync_tag);
On 2011/05/19 00:58:45, akalin wrote:
> What do you think about making these (possibly-friended) non-member functions?

Because we are in the global namespace, I think it's probably better to prefer
static member methods over global methods.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:55: // Helper method for creating SyncData
objects originating from the syncer.
On 2011/05/19 00:58:45, akalin wrote:
> I think we should find some way to not expose this in the public header. 
Maybe
> make them (possibly-friended) non-member functions that are declared in
another
> header?

Now that we have explicit validity rules/checking and SyncData's origin (local
vs remote) is exposed, I think it makes sense to have this here too, as opposed
to special casing the syncer usage. 

SyncData is a generic sync construct now, and I think it's apparent both from
the name and the comment that this is not for usage by a local syncableservice.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:69: // Returns the value of the unique
client tag. This is only set for data going
On 2011/05/19 00:58:45, akalin wrote:
> Did we end up needing this?

Yes, all changes coming from a local syncableservice must specify the tag.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_event.h (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_event.h:19: class SyncEvent {
On 2011/05/19 00:58:45, akalin wrote:
> As we discussed, SyncEvent -> SyncChange, etc.

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_event.h:57: typedef std::vector<SyncEvent>
SyncEventList;
On 2011/05/19 00:58:45, akalin wrote:
> Remove this (already in sync_event_processor.h)

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service.h (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:30: syncable::ModelType type,
On 2011/05/19 00:58:45, akalin wrote:
> I'm not a fan of needing to reference the syncable namespace here.  I notice
we
> have a SyncDataType typedef in sync_data.h.  Perhaps have a sync_data_type.h
> that only has 'typedef syncable::ModelType SyncDataType' and use that
> everywhere?

I don't think it's anything we can really avoid. The syncableservice's should be
using model_type's util methods anyways. And it reinforces that this is
something coming from the syncer.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:32: const SyncDataList&
initial_sync_data) = 0;
On 2011/05/19 00:58:45, akalin wrote:
> maybe move initial-sync_data to before the sync_processor, since that follows
> the order of the name (merge, then start syncing)

Done.

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

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:97:
SyncEventList::const_iterator event_iterator;
On 2011/05/19 00:58:45, akalin wrote:
> usual style is to just use 'it' for iterators, right?

I think "iter" is more common, but there's no real guideline. Went ahead and
changed to that though.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:110: NOTREACHED();
On 2011/05/19 00:58:45, akalin wrote:
> I feel like this is a recoverable error -- maybe we should just 'continue' in
> this case?

This is how change processors normally handle failures, and for now it's better
to be consistent I think. If we're not managing to lookup tags, then something
is very wrong. And if we're receiving the wrong tags, things are likely wrong
too, and we don't want to risk propagating that to other clients.

In addition, if the client is misbehaving, we should disable. Currently this
will disable sync, but eventually I think we want to be able to support datatype
specific unrecoverable errors, in which case this would be the correct approach.
Considering this isn't an expected code path and that NOTREACHED is in there, I
prefer this route.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:136: if
(event.sync_data().GetTag() == "" ||
On 2011/05/19 00:58:45, akalin wrote:
> i think the empty tag case is a recoverable error

See previous comment.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor.cc:149:
error_handler()->OnUnrecoverableError(FROM_HERE,
On 2011/05/19 00:58:45, akalin wrote:
> I think we can be resilient to bad clients and just drop invalid changes (with
> NOTREACHED())

See previous comment.

http://codereview.chromium.org/6995008/diff/17001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1987: 'browser/sync/api/sync_data.cc',
On 2011/05/19 00:58:45, akalin wrote:
> can you move the api stuff to its own target?  (if it's not too hairy)

I suppose I could, but what would be the benefit? This is definitely a part of
the browser in my view.

[akalin, 2011-05-20 00:08:05]

Replies.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:283: bool
PrefModelAssociator::GetAllSyncData(syncable::ModelType type,
On 2011/05/19 21:17:45, nzea wrote:
> On 2011/05/19 00:58:45, akalin wrote:
> > You can make this function return SyncDataList directly if you fix
> > CreatePrefSyncData not to return a bool
> 
> This would introduce a dependency on SyncData in the header though. In
addition,
> there may be some reason GetAllSyncData would fail, so I'd like to leave that
> option open.

I think it's okay if there's a SyncData dependency in the header.  Can you think
of any other data types where GetAllSyncData may fail?  What's wrong with
returning, say, a partially-filled vector or an empty vector in that case?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:327: // Skip values we can't
deserialize.
On 2011/05/19 21:17:45, nzea wrote:
> On 2011/05/19 00:58:45, akalin wrote:
> > I wonder if we should delete the sync data in this case?  Maybe add a TODO.
>  Well, there's no real concept of deleting preferences, so what would we
delete?
> And I don't really see any point of disassociating this preference, since I
> think we would still want to support overwriting the value if it changes on
any
> machine.

I meant something like set the synced pref to a null value, i.e. wipe out the
invalid value.  Maybe add a TODO to consider this edge case?

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.h (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:69: // Returns the value of the unique
client tag. This is only set for data going
On 2011/05/19 21:17:45, nzea wrote:
> On 2011/05/19 00:58:45, akalin wrote:
> > Did we end up needing this?
> 
> Yes, all changes coming from a local syncableservice must specify the tag.

I mean is this accessor specifically needed?  Doesn't the sync code work with
the hash, and why would the local data need to read the tag back out?

http://codereview.chromium.org/6995008/diff/17001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1987: 'browser/sync/api/sync_data.cc',
On 2011/05/19 21:17:45, nzea wrote:
> On 2011/05/19 00:58:45, akalin wrote:
> > can you move the api stuff to its own target?  (if it's not too hairy)
> 
> I suppose I could, but what would be the benefit? This is definitely a part of
> the browser in my view.

Well by that logic, we should just have everything in one 'browser' target,
since we're writing a browser. :-)

Currently the sync code is a hairball of tangled dependencies that is nearly
impossible to modularize.  Given that this is mostly new code, I'd like to not
add to the hairball, although this might end up being infeasible (if the api
stuff depends back on browser).

[akalin, 2011-05-20 00:19:39]

Few more comments.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:8: sync_entity_.reset(sync_entity);
On 2011/05/19 21:17:45, nzea wrote:
> On 2011/05/19 00:58:45, akalin wrote:
> > Is there a swap method we could use instead?
> 
> Done.

I meant a swap for the protocol buffer itself.  So you'd do:

sync_entity_->Swap(sync_entity);

which would effectively reset the argument.

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

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:18: #include
"chrome/browser/sync/protocol/preference_specifics.pb.h"
forward-declare

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:23: typedef std::map<std::string,
SyncData> SyncDataMap;
is this only used by the MA?  If so, make it a private typedef in the class.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_change.cc (right):

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change.cc:7: #include
"chrome/browser/sync/api/sync_data.h"
no need for this, since you include it in the header already

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change.cc:26: if (sync_data_.GetDataType() ==
syncable::UNSPECIFIED)
return sync_data_.GetDataType() != syncable::UNSPECIFIED;

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change.cc:38: if (sync_data_.GetDataType() !=
syncable::UNSPECIFIED)
here, too.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_change_processor.h (right):

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change_processor.h:18: virtual
~SyncChangeProcessor() {}
make destructor protected (nothing should delete via the SyncChangeProcessor
pointer).

Also, I notice you inline the destructor here, but not for SyncableService. 
Maybe be consistent?

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.h (right):

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:14: #include
"chrome/browser/sync/protocol/sync.pb.h"
remove this header since you forward-declare now

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:64: protected:
why not private?

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service.h (right):

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:20: SyncableService();
use default constructor

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:21: virtual ~SyncableService();
make destructor protected

[Nicolas Zea, 2011-05-20 02:00:27]

Comments addressed, PTAL.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:283: bool
PrefModelAssociator::GetAllSyncData(syncable::ModelType type,
On 2011/05/20 00:08:05, akalin wrote:
> On 2011/05/19 21:17:45, nzea wrote:
> > On 2011/05/19 00:58:45, akalin wrote:
> > > You can make this function return SyncDataList directly if you fix
> > > CreatePrefSyncData not to return a bool
> > 
> > This would introduce a dependency on SyncData in the header though. In
> addition,
> > there may be some reason GetAllSyncData would fail, so I'd like to leave
that
> > option open.
> 
> I think it's okay if there's a SyncData dependency in the header.  Can you
think
> of any other data types where GetAllSyncData may fail?  What's wrong with
> returning, say, a partially-filled vector or an empty vector in that case?

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:327: // Skip values we can't
deserialize.
On 2011/05/20 00:08:05, akalin wrote:
> On 2011/05/19 21:17:45, nzea wrote:
> > On 2011/05/19 00:58:45, akalin wrote:
> > > I wonder if we should delete the sync data in this case?  Maybe add a
TODO.
> >  Well, there's no real concept of deleting preferences, so what would we
> delete?
> > And I don't really see any point of disassociating this preference, since I
> > think we would still want to support overwriting the value if it changes on
> any
> > machine.
> 
> I meant something like set the synced pref to a null value, i.e. wipe out the
> invalid value.  Maybe add a TODO to consider this edge case?

Done.

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.h (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:69: // Returns the value of the unique
client tag. This is only set for data going
On 2011/05/20 00:08:05, akalin wrote:
> On 2011/05/19 21:17:45, nzea wrote:
> > On 2011/05/19 00:58:45, akalin wrote:
> > > Did we end up needing this?
> > 
> > Yes, all changes coming from a local syncableservice must specify the tag.
> 
> I mean is this accessor specifically needed?  Doesn't the sync code work with
> the hash, and why would the local data need to read the tag back out?

The sync code creates the hash from this field, so uses this accessor to get the
tag.

The local data might use it if it holds on to a syncdata object for whatever
reason, for example if it must buffer local changes, and possibly overwriting
with the newest version (for example for the purpose of doing datatype specific
commit throttling, which I've thought about).

Plus it helps to have it around for testing/sanity checks in the code. The way I
see it, if the local service can set it, it should be able to verify it.

http://codereview.chromium.org/6995008/diff/17001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6995008/diff/17001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1987: 'browser/sync/api/sync_data.cc',
On 2011/05/20 00:08:05, akalin wrote:
> On 2011/05/19 21:17:45, nzea wrote:
> > On 2011/05/19 00:58:45, akalin wrote:
> > > can you move the api stuff to its own target?  (if it's not too hairy)
> > 
> > I suppose I could, but what would be the benefit? This is definitely a part
of
> > the browser in my view.
> 
> Well by that logic, we should just have everything in one 'browser' target,
> since we're writing a browser. :-) 
>
> Currently the sync code is a hairball of tangled dependencies that is nearly
> impossible to modularize.  Given that this is mostly new code, I'd like to not
> add to the hairball, although this might end up being infeasible (if the api
> stuff depends back on browser).

Discussed offline

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

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:18: #include
"chrome/browser/sync/protocol/preference_specifics.pb.h"
On 2011/05/20 00:19:39, akalin wrote:
> forward-declare

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:23: typedef std::map<std::string,
SyncData> SyncDataMap;
On 2011/05/20 00:19:39, akalin wrote:
> is this only used by the MA?  If so, make it a private typedef in the class.

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_change.cc (right):

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change.cc:7: #include
"chrome/browser/sync/api/sync_data.h"
On 2011/05/20 00:19:39, akalin wrote:
> no need for this, since you include it in the header already

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change.cc:26: if (sync_data_.GetDataType() ==
syncable::UNSPECIFIED)
On 2011/05/20 00:19:39, akalin wrote:
> return sync_data_.GetDataType() != syncable::UNSPECIFIED;

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change.cc:38: if (sync_data_.GetDataType() !=
syncable::UNSPECIFIED)
On 2011/05/20 00:19:39, akalin wrote:
> here, too.

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_change_processor.h (right):

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change_processor.h:18: virtual
~SyncChangeProcessor() {}
On 2011/05/20 00:19:39, akalin wrote:
> make destructor protected (nothing should delete via the SyncChangeProcessor
> pointer).
> 
> Also, I notice you inline the destructor here, but not for SyncableService. 
> Maybe be consistent?

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.h (right):

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:14: #include
"chrome/browser/sync/protocol/sync.pb.h"
On 2011/05/20 00:19:39, akalin wrote:
> remove this header since you forward-declare now

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:64: protected:
On 2011/05/20 00:19:39, akalin wrote:
> why not private?

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service.h (right):

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:20: SyncableService();
On 2011/05/20 00:19:39, akalin wrote:
> use default constructor

Done.

http://codereview.chromium.org/6995008/diff/26001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:21: virtual ~SyncableService();
On 2011/05/20 00:19:39, akalin wrote:
> make destructor protected

Done.

[akalin, 2011-05-21 00:57:58]

Few more things.  (Getting close!)

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:10: scoped_ptr<sync_pb::SyncEntity>*
sync_entity) {
Change to:

SyncData::SharedSyncEntity::SharedSyncEntity(
    sync_pb::SyncEntity* sync_entity) : sync_entity_(new sync_pb::SyncEntity())
{
  sync_entity_->Swap(sync_entity);
}

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:41: const sync_pb::EntitySpecifics&
specifics) {
Change to:

SyncData SyncData::CreateLocalData(
  ...,
  const sync_pb::EntitySpecifics* specifics) {
  sync_pb::SyncEntity entity;
  entity->set ...
  entity->mutable_specifics()->Swap(specifics);
  a.shared_entity_ = new SharedSyncEntity(&entity);
  ..
}

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:61: const sync_pb::EntitySpecifics&
specifics) {
change similar to CreateLocalData

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.h (right):

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:35: static SyncData CreateLocalData(const
std::string& sync_tag);
Is it really a valid use case to create a SyncData object with only a tag?

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service.h (right):

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:38: virtual SyncDataList
GetAllSyncData(syncable::ModelType type) = 0;
What do you think about making this function const?

[akalin, 2011-05-21 00:58:55]

On 2011/05/21 00:57:58, akalin wrote:
> Few more things.  (Getting close!)

Actually, I think these comments are pretty minor.  LGTM after you address them.

[Mattias Nissler (ping if slow), 2011-05-21 18:17:52]

chrome/browser/pref changes LGTM if you answer/resolve my question below.

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

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:57: // begins).
You may run into trouble regarding this. Design-wise, all prefs should be
registered at initialization time, but unfortunately some code violates that
rule. What happens if we call this after initialization time?

[Nicolas Zea, 2011-05-23 18:13:26]

Comments addressed. Checking in once bots are green.

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

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:57: // begins).
On 2011/05/21 18:17:52, Mattias Nissler wrote:
> You may run into trouble regarding this. Design-wise, all prefs should be
> registered at initialization time, but unfortunately some code violates that
> rule. What happens if we call this after initialization time?

It will DCHECK, so should be caught quickly. That said, as long as all syncable
preferences have registered before the sync engine starts up, which is late in
the startup process, everything should be fine.

In release mode it will simply register the preference as syncable but the
preference won't get updated unless a new sync change is received, so it should
be handled (relatively) gracefully.

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:10: scoped_ptr<sync_pb::SyncEntity>*
sync_entity) {
On 2011/05/21 00:57:58, akalin wrote:
> Change to:
> 
> SyncData::SharedSyncEntity::SharedSyncEntity(
>     sync_pb::SyncEntity* sync_entity) : sync_entity_(new
sync_pb::SyncEntity())
> {
>   sync_entity_->Swap(sync_entity);
> }

Done.

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:41: const sync_pb::EntitySpecifics&
specifics) {
On 2011/05/21 00:57:58, akalin wrote:
> Change to:
> 
> SyncData SyncData::CreateLocalData(
>   ...,
>   const sync_pb::EntitySpecifics* specifics) {
>   sync_pb::SyncEntity entity;
>   entity->set ...
>   entity->mutable_specifics()->Swap(specifics);
>   a.shared_entity_ = new SharedSyncEntity(&entity);
>   ..
> }

Done.

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:61: const sync_pb::EntitySpecifics&
specifics) {
On 2011/05/21 00:57:58, akalin wrote:
> change similar to CreateLocalData

Done.

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.h (right):

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.h:35: static SyncData CreateLocalData(const
std::string& sync_tag);
On 2011/05/21 00:57:58, akalin wrote:
> Is it really a valid use case to create a SyncData object with only a tag?

Deletes only need to identify which node to delete.

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service.h (right):

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service.h:38: virtual SyncDataList
GetAllSyncData(syncable::ModelType type) = 0;
On 2011/05/21 00:57:58, akalin wrote:
> What do you think about making this function const?

Done.

[Mattias Nissler (ping if slow), 2011-05-23 20:43:08]

Still LGTM

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

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:57: // begins).
On 2011/05/23 18:13:27, nzea wrote:
> On 2011/05/21 18:17:52, Mattias Nissler wrote:
> > You may run into trouble regarding this. Design-wise, all prefs should be
> > registered at initialization time, but unfortunately some code violates that
> > rule. What happens if we call this after initialization time?
> 
> It will DCHECK, so should be caught quickly. That said, as long as all
syncable
> preferences have registered before the sync engine starts up, which is late in
> the startup process, everything should be fine.

I have learned to not rely on this property of prefs, so don't complain later
you haven't been warned :)

> 
> In release mode it will simply register the preference as syncable but the
> preference won't get updated unless a new sync change is received, so it
should
> be handled (relatively) gracefully.

That's good.

[tim (not reviewing), 2011-05-23 20:51:02]

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

http://codereview.chromium.org/6995008/diff/32002/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:57: // begins).
On 2011/05/23 20:43:08, Mattias Nissler wrote:
> On 2011/05/23 18:13:27, nzea wrote:
> > On 2011/05/21 18:17:52, Mattias Nissler wrote:
> > > You may run into trouble regarding this. Design-wise, all prefs should be
> > > registered at initialization time, but unfortunately some code violates
that
> > > rule. What happens if we call this after initialization time?
> > 
> > It will DCHECK, so should be caught quickly. That said, as long as all
> syncable
> > preferences have registered before the sync engine starts up, which is late
in
> > the startup process, everything should be fine.
> 
> I have learned to not rely on this property of prefs, so don't complain later
> you haven't been warned :)
> 
> > 
> > In release mode it will simply register the preference as syncable but the
> > preference won't get updated unless a new sync change is received, so it
> should
> > be handled (relatively) gracefully.
> 
> That's good.

AFAIK, the behavior was pretty similar in the old world - we'd associate prefs
at one point and expect that prefs were registered by then.  It's just that it
was happening in some dark corner of sync rather than here in prefs code so no
one who knew better realized it :|

Hope to catch more of these with the new api...

[commit-bot: I haz the power, 2011-05-23 22:41:18]

Can't process patch for file chrome/browser/sync/api/syncable_service_mock.cc.
A +