sync: move invalidation version prefs out of SyncPrefs into InvalidatorStorage.

sync: move invalidation version prefs out of SyncPrefs into InvalidatorStorage.

De-coupling so we don't add more sync < > invalidations deps while adding support for storing opaque notification state in InvalidationStateTracker. 

Added get/setters to InvalidationStateTracker and impl (InvalidatorStorage), but it's not wired up for use yet.

Next step is to add migration code to move state from syncable::Directory to InvalidationStateTracker.

Comment in invalidator_storage.h explains motivations for not moving out of sync. This patch creates the c/b/s/invalidations directory,
which we can move non-profile-sync-specific chrome invalidations code to (such as BridgedSyncNotifier, and the android
related part of ChromeSyncNotificationsBridge).

BUG=124140
TEST=InvalidatorStorageTest, should be no functional changes


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

[tim (not reviewing), 2012-05-29 16:44:18]

Nilesh, please review sync/notifier.
Richard, please review everything else.

[tim (not reviewing), 2012-05-29 16:58:31]

+akalin FYI / TBR

[nilesh, 2012-05-29 18:24:40]

On 2012/05/29 16:44:18, timsteele wrote:
> Nilesh, please review sync/notifier.
LGTM

> Richard, please review everything else.

[rlarocque, 2012-05-29 18:28:24]

A few suggestions.  I'm most concerned about the 'if (!pref_service_)' cases in
InvalidatorStorage, the rest are mostly style issues.

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:18: if (pref_service_)
{
It looks like there are if branches in several member functions to deal with the
case where this object is constructed with a NULL pref_service.  Consider
creating a separate class for this (ie. MockInvalidatorStorage) so you can keep
both cases separate.

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:31: const {
This indent looks odd, though I think it's technically correct.  

Would it be possible to add 'using sync_notifier::InavlidationVersionMap' to the
top of this file?  That would make this declaration more readable.

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:36: // Complicated
gross code to convert from a string -> string
Consider moving this into a 'deserialize()' helper function?

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:86: // Gross code to
convert from a ModelType -> int64 map to a string
Consider moving this to a serialize() helper function?

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
File chrome/browser/sync/invalidations/invalidator_storage_unittest.cc (right):

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:16: class
InvalidatorStorageTest : public testing::Test {
I'd like to see some tests for the parsing code.  There are a few scenarios that
probably should be covered, ie. an empty dictionary or invalid model types.  

It doesn't have to be implemented in this patch, but it would be nice to have
eventually.

[tim (not reviewing), 2012-05-29 23:17:31]

Thanks for the reviews! Addressed first round of comments.

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:18: if (pref_service_)
{
On 2012/05/29 18:28:24, rlarocque wrote:
> It looks like there are if branches in several member functions to deal with
the
> case where this object is constructed with a NULL pref_service.  Consider
> creating a separate class for this (ie. MockInvalidatorStorage) so you can
keep
> both cases separate.  

As we discussed offline, there are a few gotchas to this. Added todos and filed
bug 130176.

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:31: const {
On 2012/05/29 18:28:24, rlarocque wrote:
> This indent looks odd, though I think it's technically correct.  
> 
> Would it be possible to add 'using sync_notifier::InavlidationVersionMap' to
the
> top of this file?  That would make this declaration more readable.  

Done.

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:36: // Complicated
gross code to convert from a string -> string
On 2012/05/29 18:28:24, rlarocque wrote:
> Consider moving this into a 'deserialize()' helper function?  

Done.

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage.cc:86: // Gross code to
convert from a ModelType -> int64 map to a string
On 2012/05/29 18:28:24, rlarocque wrote:
> Consider moving this to a serialize() helper function?

Done.

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
File chrome/browser/sync/invalidations/invalidator_storage_unittest.cc (right):

http://codereview.chromium.org/10451058/diff/6002/chrome/browser/sync/invalid...
chrome/browser/sync/invalidations/invalidator_storage_unittest.cc:16: class
InvalidatorStorageTest : public testing::Test {
On 2012/05/29 18:28:24, rlarocque wrote:
> I'd like to see some tests for the parsing code.  There are a few scenarios
that
> probably should be covered, ie. an empty dictionary or invalid model types.  
> 
> It doesn't have to be implemented in this patch, but it would be nice to have
> eventually.  

Get and Set actually deserialize and serialize from/to the pref format, so
MaxInvalidationVersions does actually hit the parsing code for the happy case,
but I've added more targeted tests for that and edge cases.

[rlarocque, 2012-05-29 23:29:34]

LGTM.

[commit-bot: I haz the power, 2012-05-30 00:01:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10451058/14003

[commit-bot: I haz the power, 2012-05-30 02:34:55]

Change committed as 139464