[Sync] Refactor data type configuration/activation/deactivation

[Sync] Refactor data type configuration/activation/deactivation

Changed ConfigureDataTypes to take a set of types to add and a set of types to remove.

Changed Activate/DeactivateDataType to take ModelTypes instead of
the DataTypeControllers directly.

BUG=
TEST=


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

[akalin, 2011-08-04 03:37:50]

+tim for review

[tim (not reviewing), 2011-08-04 03:55:02]

I think I like the direction here.  Although, I have to say, the fact that there
are multiple patches per month (week?) including the words "simplify nigori"
"simplify encryption" and "refactor data type configuration" from every engineer
and their extended family is really starting make my head spin :\ 
But nothing personal :)

http://codereview.chromium.org/7511004/diff/1/chrome/browser/sync/glue/data_t...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/7511004/diff/1/chrome/browser/sync/glue/data_t...
chrome/browser/sync/glue/data_type_manager_impl.cc:219:
all_types.insert(syncable::NIGORI);
It just feels like "nigori" doesn't need to leak any higher than sync backend
host because it's not a user configurable type, which is what the DTM is all
about.  I know the previous impl had a bool to enable_nigori, not saying that
was better (in fact your approach might look better), but that was all for a
single edge case of data migration because we handle migration at this layer
rather than SBH.

Can you think of a way we can avoid dealing with nigori here (and as such at
every layer of the stack)?

[akalin, 2011-08-10 23:32:33]

PTAL

http://codereview.chromium.org/7511004/diff/1/chrome/browser/sync/glue/data_t...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/7511004/diff/1/chrome/browser/sync/glue/data_t...
chrome/browser/sync/glue/data_type_manager_impl.cc:219:
all_types.insert(syncable::NIGORI);
On 2011/08/04 03:55:02, timsteele wrote:
> It just feels like "nigori" doesn't need to leak any higher than sync backend
> host because it's not a user configurable type, which is what the DTM is all
> about.  I know the previous impl had a bool to enable_nigori, not saying that
> was better (in fact your approach might look better), but that was all for a
> single edge case of data migration because we handle migration at this layer
> rather than SBH.
> 
> Can you think of a way we can avoid dealing with nigori here (and as such at
> every layer of the stack)?

I see.  The whole enable_nigori thing is only really needed for backend
migration; otherwise, only SBH should know the details of when to add/remove the
nigori type.

Fixed.

[tim (not reviewing), 2011-08-10 23:39:08]

LGTM

http://codereview.chromium.org/7511004/diff/8001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/7511004/diff/8001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/sync_backend_host.cc:334: }
^^
Wow, what a mindffffffffffffffff

http://codereview.chromium.org/7511004/diff/8001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/7511004/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:397: virtual void
DeactivateDataType(syncable::ModelType type);
See that todo up there? ;)

[akalin, 2011-08-10 23:41:49]

http://codereview.chromium.org/7511004/diff/8001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/7511004/diff/8001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/sync_backend_host.cc:334: }
On 2011/08/10 23:39:08, timsteele wrote:
> ^^
> Wow, what a mindffffffffffffffff
> 

What do you suggest?  It's better than the special-casing in
GetPendingConfigModeState.

http://codereview.chromium.org/7511004/diff/8001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/7511004/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:397: virtual void
DeactivateDataType(syncable::ModelType type);
On 2011/08/10 23:39:08, timsteele wrote:
> See that todo up there? ;)

Future CL.  If I remove these, then I'd have to somehow get the backend to the
DTCs *and* fix tests.  This is already a yak. :(

[commit-bot: I haz the power, 2011-08-11 04:58:26]

Change committed as 96326