sync: Remove WITH_NIGORI and WITHOUT_NIGORI flags

sync: Remove WITH_NIGORI and WITHOUT_NIGORI flags

This is part of a series of patches intended to add more types which
behave similarly to Nigori.

These new types will be downloaded early, much like the Nigori node
currently is.  Removing these flags will make it easier to calculate the
appropriate parameters for configure calls during early initialization.

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

[rlarocque, 2012-08-09 20:03:08]

While rebasing the 'control data' patch, I found that it made sense to separate
this change and put it at the bottom of the stack.

Please review.

[tim (not reviewing), 2012-08-13 20:19:52]

https://chromiumcodereview.appspot.com/10854050/diff/6001/chrome/browser/sync...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10854050/diff/6001/chrome/browser/sync...
chrome/browser/sync/glue/data_type_manager_impl.cc:49:
desired_types.Put(syncer::NIGORI);
Can we make it so that the configurer always assumes "WITH_NIGORI", unless
NIGORI is explicitly removed (via the types_to_remove parameter to Configure)? 

I think if we change ConfigureImpl to take types_to_add and types_to_remove
versus |desired_types|, then the *only* place where we need to even mention
syncer::NIGORI will be below, in ConfigureWithoutNigori, because we'll have to
add it to types_to_remove.  You won't need to add it to all_types below either
(though you might want to make that a simple method on this class so you can
compute the Difference() on line 124 inline.

https://chromiumcodereview.appspot.com/10854050/diff/6001/chrome/browser/sync...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10854050/diff/6001/chrome/browser/sync...
chrome/browser/sync/glue/data_type_manager_impl.h:81: syncer::ConfigureReason
reason);
these params may fit on same line now. although see comment in .cc file before
changing.

(either way it'd be better to line up as
void ConfigureImpl(Param param1,
                   OtherParam param2, ) etc)

[rlarocque, 2012-08-13 20:45:58]

https://chromiumcodereview.appspot.com/10854050/diff/6001/chrome/browser/sync...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10854050/diff/6001/chrome/browser/sync...
chrome/browser/sync/glue/data_type_manager_impl.cc:49:
desired_types.Put(syncer::NIGORI);
On 2012/08/13 20:19:52, timsteele wrote:
> Can we make it so that the configurer always assumes "WITH_NIGORI", unless
> NIGORI is explicitly removed (via the types_to_remove parameter to Configure)?

> 
> I think if we change ConfigureImpl to take types_to_add and types_to_remove
> versus |desired_types|, then the *only* place where we need to even mention
> syncer::NIGORI will be below, in ConfigureWithoutNigori, because we'll have to
> add it to types_to_remove.  You won't need to add it to all_types below either
> (though you might want to make that a simple method on this class so you can
> compute the Difference() on line 124 inline.

The types_to_add/types_to_remove concept isn't really used at the level of this
function's callers (ProfileSyncService and BackendMigrator).  I think it's too
complicated to be used correctly from those contexts.

If we were going to clean this up, I think the right direction to take it would
be to give the BackendMigrator and access to a 'ClearDataTypes()' API that
explicitly purges types.  That better describes what it's trying to do, and
would remove the need for a ConfigureWithoutNigori() function.

However, that's starting to stray from the original intent of this commit, and
probably should be tackled separately.  Would it be OK if I open a bug and add a
TODO to look at this later?

[rlarocque, 2012-08-13 22:04:38]

Uploaded new patch.

http://codereview.chromium.org/10854050/diff/6001/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/10854050/diff/6001/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.cc:49:
desired_types.Put(syncer::NIGORI);
On 2012/08/13 20:45:58, rlarocque wrote:
> On 2012/08/13 20:19:52, timsteele wrote:
> > Can we make it so that the configurer always assumes "WITH_NIGORI", unless
> > NIGORI is explicitly removed (via the types_to_remove parameter to
Configure)?
> 
> > 
> > I think if we change ConfigureImpl to take types_to_add and types_to_remove
> > versus |desired_types|, then the *only* place where we need to even mention
> > syncer::NIGORI will be below, in ConfigureWithoutNigori, because we'll have
to
> > add it to types_to_remove.  You won't need to add it to all_types below
either
> > (though you might want to make that a simple method on this class so you can
> > compute the Difference() on line 124 inline.
> 
> The types_to_add/types_to_remove concept isn't really used at the level of
this
> function's callers (ProfileSyncService and BackendMigrator).  I think it's too
> complicated to be used correctly from those contexts.
> 
> If we were going to clean this up, I think the right direction to take it
would
> be to give the BackendMigrator and access to a 'ClearDataTypes()' API that
> explicitly purges types.  That better describes what it's trying to do, and
> would remove the need for a ConfigureWithoutNigori() function.
> 
> However, that's starting to stray from the original intent of this commit, and
> probably should be tackled separately.  Would it be OK if I open a bug and add
a
> TODO to look at this later?  

Discussed offline.

http://codereview.chromium.org/10854050/diff/6001/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/10854050/diff/6001/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.h:81: syncer::ConfigureReason
reason);
On 2012/08/13 20:19:52, timsteele wrote:
> these params may fit on same line now. although see comment in .cc file before
> changing.
> 
> (either way it'd be better to line up as
> void ConfigureImpl(Param param1,
>                    OtherParam param2, ) etc)
>                    

Done.

http://codereview.chromium.org/10854050/diff/7009/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/10854050/diff/7009/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.cc:56:
DCHECK(!desired_types.Has(syncer::NIGORI));
Although I couldn't reduce line 49 to a DCHECK, I was able to convert this one.

[tim (not reviewing), 2012-08-13 22:08:38]

LGTM with one nit

https://chromiumcodereview.appspot.com/10854050/diff/7009/chrome/browser/sync...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10854050/diff/7009/chrome/browser/sync...
chrome/browser/sync/glue/data_type_manager_impl.cc:96: syncer::ConfigureReason
reason) {
nit - previous line

[rlarocque, 2012-08-13 22:14:23]

https://chromiumcodereview.appspot.com/10854050/diff/7009/chrome/browser/sync...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10854050/diff/7009/chrome/browser/sync...
chrome/browser/sync/glue/data_type_manager_impl.cc:96: syncer::ConfigureReason
reason) {
On 2012/08/13 22:08:39, timsteele wrote:
> nit - previous line

Done.

[commit-bot: I haz the power, 2012-08-13 22:14:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10854050/9010

[commit-bot: I haz the power, 2012-08-13 23:30:37]

Change committed as 151389