sync: Introduce control data types

sync: Introduce control data types

The Nigori node has always been treated specially.  It is downloaded
during early sync initialization.  It has custom encryption, update
application, and conflict resolution logic.  It is not user-selectable,
but is instead always implicitly enabled.

This patch replaces a lot of if (NIGORI)' code branches and creates the
infrastructure we will need to support other data types that have these
properties.  These types will be used to store metadata required for the
normal operation of the syncer.

Notable changes include:
- Introduce mutually exclusive lists of control types and user types in
  model_type.cc.
- Create an ApplyControlDataUpdates() function, which will be used to
  apply changes and resolve conflicts for control data types.
- Add some if statements to skip processing of control data updates in
  the regular conflict resolution code path.  
- Move around some code to accomodate the above changes.  Introduce
  apply_control_data_updates_unittest.cc and conflict_util.cc
- Have SyncBackendHost download all control types during init, not just
  the NIGORI.
- Introduce a PurgeForMigration() function on the DataTypeManagerImpl
  class to support migration of control data types.  This replaces
  ConfigureWithoutNigori(), which would not have scaled well as
  additional control data types were addded.
- Modify the encryption logic so that only user types may be set to be
  encrypted.  The Nigori is no longer expected to be in the set of
  encrypted types, though it is no less encrypted than it was before
  this change.
- Use the list of user types to help calculate the list of registered
  sync prefs.

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

[rlarocque, 2012-08-14 22:28:49]

tim: Please review everything.
zea: You might be interested in the changes related to the ApplyControlData()
function.  I think this is compatible with your plan to create a nigori change
processor.  You should take a look to make sure that's the case.  

This is part 2 of the nigori splitting patches.

- Part 1: http://codereview.chromium.org/10854050/
- Squashed diff: http://codereview.chromium.org/10825137/

We've reviewed this patch before, but I never committed it.  I was waiting for
Nicolas' configuration patches to land so I could make additional changes to the
early download code.

The original review is here: http://codereview.chromium.org/10559104/

Since then, I've added the 'EarlyDownloadTypes()' set and related code, and
replaced DataTypeManagerImpl::ConfigureWithoutNigori with
DataTypeManagerImpl::Purge.  I had tried to avoid the latter change, but
eventually realized that it (or something similar) is necessary to support
migration once I add additional 'control' types.

[rlarocque, 2012-08-14 22:33:06]

http://codereview.chromium.org/10832286/diff/7046/chrome/browser/sync/backend...
File chrome/browser/sync/backend_migrator.cc (right):

http://codereview.chromium.org/10832286/diff/7046/chrome/browser/sync/backend...
chrome/browser/sync/backend_migrator.cc:107: manager_->Purge(to_migrate_,
syncer::CONFIGURE_REASON_MIGRATION);
This is a scary change.  

I'm pretty sure it's a good idea to not use the preferred types here.  The Purge
function will take the difference against last_requested_types_, which should be
more reliable.  

I can't guarantee there won't be any unintended side effects, though.

[Nicolas Zea, 2012-08-16 20:55:02]

You should probably rebase this, as it is going to conflict mightily with my
recent SyncEncryptionHandler change.

http://codereview.chromium.org/10832286/diff/7046/chrome/browser/sync/sync_pr...
File chrome/browser/sync/sync_prefs.cc (right):

http://codereview.chromium.org/10832286/diff/7046/chrome/browser/sync/sync_pr...
chrome/browser/sync/sync_prefs.cc:344: if (IsControlType(type)) {
nit: remove curly brace around single line if

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:59: if
(nigori_node.Get(IS_UNAPPLIED_UPDATE)) {
seems simpler to return early above if IS_UNAPPLIED_UPDATE isn't true.

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:122: // We deliberately leave the
server's device information. This client will
remove this comment

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:126: << nigori_node;
line up <<

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates_unittest.cc (right):

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:124:
EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(ModelTypeSet::All()));
check nigori's initial sync ended for each of these tests?

[rlarocque, 2012-08-17 19:47:18]

Applied recommendations from comments then rebased.

I had to resolve a few rebase conflicts by hand because some code had migrated
across files.  Be on the lookout for rebase bugs as you're reviewing.

http://codereview.chromium.org/10832286/diff/7046/chrome/browser/sync/sync_pr...
File chrome/browser/sync/sync_prefs.cc (right):

http://codereview.chromium.org/10832286/diff/7046/chrome/browser/sync/sync_pr...
chrome/browser/sync/sync_prefs.cc:344: if (IsControlType(type)) {
On 2012/08/16 20:55:02, nzea wrote:
> nit: remove curly brace around single line if

OK... normally I'd just follow the file's existing format, but that's not an
option here.  There are instances of one-line if statements with and without
braces in this file.

Is there a preference against using braces when they're not necessary?

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:59: if
(nigori_node.Get(IS_UNAPPLIED_UPDATE)) {
On 2012/08/16 20:55:02, nzea wrote:
> seems simpler to return early above if IS_UNAPPLIED_UPDATE isn't true.

Good point.  I'll try that.

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:122: // We deliberately leave the
server's device information. This client will
On 2012/08/16 20:55:02, nzea wrote:
> remove this comment

Done.

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:126: << nigori_node;
On 2012/08/16 20:55:02, nzea wrote:
> line up <<

Done.

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates_unittest.cc (right):

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:124:
EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(ModelTypeSet::All()));
On 2012/08/16 20:55:02, nzea wrote:
> check nigori's initial sync ended for each of these tests?

These are supposed to test changes that occur in steady-state, after we've
configured and started syncing.

I guess we could assert that the nigori is synced before each of these tests,
but I don't see why we should.

[Nicolas Zea, 2012-08-20 20:44:28]

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates_unittest.cc (right):

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:124:
EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(ModelTypeSet::All()));
On 2012/08/17 19:47:18, rlarocque wrote:
> On 2012/08/16 20:55:02, nzea wrote:
> > check nigori's initial sync ended for each of these tests?
> 
> These are supposed to test changes that occur in steady-state, after we've
> configured and started syncing.
> 
> I guess we could assert that the nigori is synced before each of these tests,
> but I don't see why we should.

NigoriUpdate above seems to just test the application of a nigori node update,
right? Part of that application sets initial sync ended, so it seems like we
should verify that happens (at least in that test case).

http://codereview.chromium.org/10832286/diff/12004/sync/engine/apply_updates_...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10832286/diff/12004/sync/engine/apply_updates_...
sync/engine/apply_updates_command.cc:39: // The INTERNAL types are special. 
They have their own update routines,
INTERNAL -> CONTROL?

http://codereview.chromium.org/10832286/diff/10002/sync/engine/apply_control_...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10832286/diff/10002/sync/engine/apply_control_...
sync/engine/apply_control_data_updates.cc:43: // We always update the
cryptographer with the server's nigori node,
I think this comment should be right above the ApplyNigoriUpdate call, not this
conditional.

[tim (not reviewing), 2012-08-22 22:42:28]

http://codereview.chromium.org/10832286/diff/10002/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/10832286/diff/10002/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/data_type_manager_impl.h:37: virtual void Purge(
Lets call this PurgeForMigration to back up that comment and to make it so that
anyone would feel guilty even thinking about using this method in a
non-migration scenario.

http://codereview.chromium.org/10832286/diff/10002/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/data_type_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10832286/diff/10002/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/data_type_manager_impl_unittest.cc:71: class
SyncDataTypeManagerImplTest : public testing::Test {
I think this removes all coverage for the Purge flow... it'd be useful to have
some tests that call ConfigureImpl without nigori in the set of datatypes,
possibly calls the real Purge (not the mock, which is what BackendMigratorTest
uses).

http://codereview.chromium.org/10832286/diff/10002/sync/engine/syncer_util.h
File sync/engine/syncer_util.h (right):

http://codereview.chromium.org/10832286/diff/10002/sync/engine/syncer_util.h#...
sync/engine/syncer_util.h:68: void IgnoreLocalChanges(syncable::MutableEntry*
entry);
Is this actually needed outside of ConflictResolver?  It's unfortunate to add
more to this disorganized util dumping ground.  The concepts of ignoring /
overwriting changes from one side or another seem very tied to conflict
resolution. If they're needed externally to that file now, static class methods
would be slightly better from an organizational point of view, as would a
separate conflict_resolver_util file / namespace.

http://codereview.chromium.org/10832286/diff/10002/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type.h (right):

http://codereview.chromium.org/10832286/diff/10002/sync/internal_api/public/b...
sync/internal_api/public/base/model_type.h:136: // initialization has finished.
The concept of "early download types" should go in SyncBackendHost, which is the
thing responsible for making the request to download them early, and knows about
the fact that that needs to happen before it reports "backend initialized". 
Note that it can't request a config until the SyncManager reports
OnInitializationComplete, and does this work before reporting
OnBackendInitialized to higher layers.

[rlarocque, 2012-08-22 23:46:24]

The responses to Nicolas' comments are in patch set 9.

I'm still working on the response to Tim's comments.

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates_unittest.cc (right):

http://codereview.chromium.org/10832286/diff/7046/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:124:
EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(ModelTypeSet::All()));
On 2012/08/20 20:44:28, nzea wrote:
> On 2012/08/17 19:47:18, rlarocque wrote:
> > On 2012/08/16 20:55:02, nzea wrote:
> > > check nigori's initial sync ended for each of these tests?
> > 
> > These are supposed to test changes that occur in steady-state, after we've
> > configured and started syncing.
> > 
> > I guess we could assert that the nigori is synced before each of these
tests,
> > but I don't see why we should.
> 
> NigoriUpdate above seems to just test the application of a nigori node update,
> right? Part of that application sets initial sync ended, so it seems like we
> should verify that happens (at least in that test case).

Good point.  I forgot that ApplyControlDataUpdates() is expected to handle the
initial sync case as well as steady state.

In fact, the initial case is exactly what this test is testing because this test
starts with an empty database.

I'll add some assertions in the next patch.

http://codereview.chromium.org/10832286/diff/12004/sync/engine/apply_updates_...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10832286/diff/12004/sync/engine/apply_updates_...
sync/engine/apply_updates_command.cc:39: // The INTERNAL types are special. 
They have their own update routines,
On 2012/08/20 20:44:28, nzea wrote:
> INTERNAL -> CONTROL?

Done.

http://codereview.chromium.org/10832286/diff/10002/sync/engine/apply_control_...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10832286/diff/10002/sync/engine/apply_control_...
sync/engine/apply_control_data_updates.cc:43: // We always update the
cryptographer with the server's nigori node,
On 2012/08/20 20:44:28, nzea wrote:
> I think this comment should be right above the ApplyNigoriUpdate call, not
this
> conditional.

Agreed.  I think it can be rephrased a bit, too.  For example, the reference to
conflict resolver is now irrelevant.

[rlarocque, 2012-08-23 20:11:23]

Patch updated with responses to Tim's comments.

I've also changed some of the logic in DataTypeManagerImpl() so it will
implicitly add all ControlDataTypes() on every configure, rather than adding
just the NIGORI specifically.  Currently NIGORI is the only control data type,
but that should change soon.

PTAL.

http://codereview.chromium.org/10832286/diff/10002/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/10832286/diff/10002/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/data_type_manager_impl.h:37: virtual void Purge(
On 2012/08/22 22:42:28, timsteele wrote:
> Lets call this PurgeForMigration to back up that comment and to make it so
that
> anyone would feel guilty even thinking about using this method in a
> non-migration scenario.

Good idea.  Done.

http://codereview.chromium.org/10832286/diff/10002/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/data_type_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10832286/diff/10002/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/data_type_manager_impl_unittest.cc:71: class
SyncDataTypeManagerImplTest : public testing::Test {
On 2012/08/22 22:42:28, timsteele wrote:
> I think this removes all coverage for the Purge flow... it'd be useful to have
> some tests that call ConfigureImpl without nigori in the set of datatypes,
> possibly calls the real Purge (not the mock, which is what BackendMigratorTest
> uses).

I've added a few tests.

You're right that function should at least be invoked at least once during the
tests.

However, from a whitebox perspective, there's not a lot there to test.  The
function is three lines long.  I wrote only a few tests, rather than trying to
revive all the WITHOUT_NIGORI test variants.

http://codereview.chromium.org/10832286/diff/10002/sync/engine/syncer_util.h
File sync/engine/syncer_util.h (right):

http://codereview.chromium.org/10832286/diff/10002/sync/engine/syncer_util.h#...
sync/engine/syncer_util.h:68: void IgnoreLocalChanges(syncable::MutableEntry*
entry);
On 2012/08/22 22:42:28, timsteele wrote:
> Is this actually needed outside of ConflictResolver?  It's unfortunate to add
> more to this disorganized util dumping ground.  The concepts of ignoring /
> overwriting changes from one side or another seem very tied to conflict
> resolution. If they're needed externally to that file now, static class
methods
> would be slightly better from an organizational point of view, as would a
> separate conflict_resolver_util file / namespace.

Moved to conflict_util.cc/h.

http://codereview.chromium.org/10832286/diff/10002/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type.h (right):

http://codereview.chromium.org/10832286/diff/10002/sync/internal_api/public/b...
sync/internal_api/public/base/model_type.h:136: // initialization has finished.
On 2012/08/22 22:42:28, timsteele wrote:
> The concept of "early download types" should go in SyncBackendHost, which is
the
> thing responsible for making the request to download them early, and knows
about
> the fact that that needs to happen before it reports "backend initialized". 
> Note that it can't request a config until the SyncManager reports
> OnInitializationComplete, and does this work before reporting
> OnBackendInitialized to higher layers.

Done.

[rlarocque, 2012-08-28 19:58:42]

Here's the big refactor we discussed on Friday.

Because we now have a solid definition of "Control type", and the definition
includes rules about how these types are encrypted, I decided to squash the
encryption changes (which would have been patch 3/5) into this commit.

If you want, you could review the diffs between patch set 11 and the current
patch set.  That would show you the changes that have happened since we decided
to remove the GROUP_CONTROL.  However, that was such a significant change that I
think it's easier to just review the diffs against trunk.

Nicolas: Please review the encryption-related changes.
Tim: Please review everything else.

[tim (not reviewing), 2012-08-30 20:18:33]

Looking good, just a couple nits remaining on my end.

http://codereview.chromium.org/10832286/diff/27001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

http://codereview.chromium.org/10832286/diff/27001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_registrar.cc:89: void
SyncBackendRegistrar::SetInitialTypes(syncer::ModelTypeSet initial_types) {
nit - shouldn't this be a const ModelTypeSet&?

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
sync/engine/apply_updates_command.cc:63:
server_type_restriction.Remove(i.Get());
This would be nicer to read as a single line 'RemoveAll' (or Difference), but
EnumSet can't support it :(  

It'd be useful (especially as we add more type-subgroups) to add a util in
model_type to Difference a FullModelTypeSet with a ModelTypeSet and return a
FullModelTypeSet(and union, etc, down the road).

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
sync/engine/apply_updates_command.cc:87: if (!IsControlType(it.Get())) {
style nit - It's a bit less unintuitive treat the case we'll be handling en
masse here as "if (!something)..." would you be opposed to writing this as

// Don't set the flag for control types.
if (IsControlType(it.Get()))
  continue;

dir->set_initial_sync_ended_for_type(...)

[Nicolas Zea, 2012-08-30 20:37:02]

encryption LGTM with some nits

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_control_...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_control_...
sync/engine/apply_control_data_updates.cc:35: // Update the cryptographer with
the server's nigori node.
cryptographer -> sync's encryption handler

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_control_...
sync/engine/apply_control_data_updates.cc:83: if (!nigori_node.Get(IS_UNSYNCED))
{ // Update only.
nit: two spaces after curly brace (here and below)

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_control_...
sync/engine/apply_control_data_updates.cc:90: // Store the merged set of
encrypted types (cryptographer->Update(..) will
cryptographer->Update(..) -> NigoriHandler::ApplyNigoriUpdate(..)

here and elsewhere

[tim (not reviewing), 2012-08-30 20:40:07]

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
sync/engine/apply_updates_command.cc:87: if (!IsControlType(it.Get())) {
On 2012/08/30 20:18:33, timsteele wrote:
> style nit - It's a bit less unintuitive treat the case we'll be handling en
> masse here as "if (!something)..." would you be opposed to writing this as
> 
> // Don't set the flag for control types.
> if (IsControlType(it.Get()))
>   continue;
> 
> dir->set_initial_sync_ended_for_type(...)

Heh, speaking of double negatives ... "it's a bit less *intuitive* to...".

[rlarocque, 2012-08-30 21:01:48]

Patch updated.  

Tim, we should discuss the ModelTypeSet::RemoveAll() issue offline.

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_control_...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_control_...
sync/engine/apply_control_data_updates.cc:35: // Update the cryptographer with
the server's nigori node.
On 2012/08/30 20:37:02, nzea wrote:
> cryptographer -> sync's encryption handler

Done.

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_control_...
sync/engine/apply_control_data_updates.cc:83: if (!nigori_node.Get(IS_UNSYNCED))
{ // Update only.
On 2012/08/30 20:37:02, nzea wrote:
> nit: two spaces after curly brace (here and below)

Done.

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_control_...
sync/engine/apply_control_data_updates.cc:90: // Store the merged set of
encrypted types (cryptographer->Update(..) will
On 2012/08/30 20:37:02, nzea wrote:
> cryptographer->Update(..) -> NigoriHandler::ApplyNigoriUpdate(..)
> 
> here and elsewhere

Done.

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
sync/engine/apply_updates_command.cc:63:
server_type_restriction.Remove(i.Get());
On 2012/08/30 20:18:33, timsteele wrote:
> This would be nicer to read as a single line 'RemoveAll' (or Difference), but
> EnumSet can't support it :(  
> 
> It'd be useful (especially as we add more type-subgroups) to add a util in
> model_type to Difference a FullModelTypeSet with a ModelTypeSet and return a
> FullModelTypeSet(and union, etc, down the road).

I feel the same way.  I'd like to enforce that we use the right enums in the
right places, but C++'s type system really isn't helping.

If we're going to have to check every conversion manually at run-time through
conversion functions, I'd prefer to have just one type (FullModelTypeSet) and
DCHECK its contents where we think it's important to do so.

That's not something I'd like to do in this patch, though.

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
sync/engine/apply_updates_command.cc:87: if (!IsControlType(it.Get())) {
On 2012/08/30 20:18:33, timsteele wrote:
> style nit - It's a bit less unintuitive treat the case we'll be handling en
> masse here as "if (!something)..." would you be opposed to writing this as
> 
> // Don't set the flag for control types.
> if (IsControlType(it.Get()))
>   continue;
> 
> dir->set_initial_sync_ended_for_type(...)

That's how I did it originally, but then I figured that 'continue' might be
considered bad C++ style.  I agree that this is much clearer.  

Will fix.

[tim (not reviewing), 2012-08-30 21:41:04]

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
sync/engine/apply_updates_command.cc:63:
server_type_restriction.Remove(i.Get());
On 2012/08/30 21:01:48, rlarocque wrote:
> On 2012/08/30 20:18:33, timsteele wrote:
> > This would be nicer to read as a single line 'RemoveAll' (or Difference),
but
> > EnumSet can't support it :(  
> > 
> > It'd be useful (especially as we add more type-subgroups) to add a util in
> > model_type to Difference a FullModelTypeSet with a ModelTypeSet and return a
> > FullModelTypeSet(and union, etc, down the road).
> 
> I feel the same way.  I'd like to enforce that we use the right enums in the
> right places, but C++'s type system really isn't helping.
> 
> If we're going to have to check every conversion manually at run-time through
> conversion functions, I'd prefer to have just one type (FullModelTypeSet) and
> DCHECK its contents where we think it's important to do so.
> 
> That's not something I'd like to do in this patch, though.

I was really thinking low key, like

full_set.Difference(ToFullModelTypeSet(ControlTypes()));

or an explicit one-off:
UnionToFullModelTypeSet(full_set, ControlTypes());

would work fine?

Alternatively Fred mentioned we could likely have an EnumSet constructor that
adds items from a passed in EnumSet, possibly of different range, but
compile-asserts the min/max values.

[akalin, 2012-08-30 21:45:59]

On 2012/08/30 21:41:04, timsteele wrote:
> Alternatively Fred mentioned we could likely have an EnumSet constructor that
> adds items from a passed in EnumSet, possibly of different range, but
> compile-asserts the min/max values.

Yeah.  I like the conversion function better, like
ModelTypeToFullModelTypeSet().  Maybe do the one-off for now, and then we can
generalize if something else actually uses EnumSet and needs it?

[rlarocque, 2012-08-30 21:53:59]

On 2012/08/30 21:41:04, timsteele wrote:
>
http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
> File sync/engine/apply_updates_command.cc (right):
> 
>
http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
> sync/engine/apply_updates_command.cc:63:
> server_type_restriction.Remove(i.Get());
> On 2012/08/30 21:01:48, rlarocque wrote:
> > On 2012/08/30 20:18:33, timsteele wrote:
> > > This would be nicer to read as a single line 'RemoveAll' (or Difference),
> but
> > > EnumSet can't support it :(  
> > > 
> > > It'd be useful (especially as we add more type-subgroups) to add a util in
> > > model_type to Difference a FullModelTypeSet with a ModelTypeSet and return
a
> > > FullModelTypeSet(and union, etc, down the road).
> > 
> > I feel the same way.  I'd like to enforce that we use the right enums in the
> > right places, but C++'s type system really isn't helping.
> > 
> > If we're going to have to check every conversion manually at run-time
through
> > conversion functions, I'd prefer to have just one type (FullModelTypeSet)
and
> > DCHECK its contents where we think it's important to do so.
> > 
> > That's not something I'd like to do in this patch, though.
> 
> I was really thinking low key, like
> 
> full_set.Difference(ToFullModelTypeSet(ControlTypes()));
> 
> or an explicit one-off:
> UnionToFullModelTypeSet(full_set, ControlTypes());
> 
> would work fine?
> 
> Alternatively Fred mentioned we could likely have an EnumSet constructor that
> adds items from a passed in EnumSet, possibly of different range, but
> compile-asserts the min/max values.

I prefer Fred's solution, but I think it leads us towards something even simpler
in the end.

Your solution would require us to re-implement the set functions, or to use
generic functions with wrappers that convert the input types.  The second
alternative would be preferable.  The natural next step after that would be to
handle the conversion at the place where the generic function is called, because
that way we won't have to write wrappers for all the different instances of the
conversion.  That sounds like Fred's solution.

If we do it that way, then we end up having to explicitly convert from one type
of EnumSet to another all over the place.  The conversion would be a no-op
except for the DCHECKs.  Once we're at that point, why not just get rid of the
conversions, use a single enum type that holds all values, and do the DCHECK
ourselves?

[tim (not reviewing), 2012-08-30 22:22:12]

On 2012/08/30 21:53:59, rlarocque wrote:
> On 2012/08/30 21:41:04, timsteele wrote:
> >
>
http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
> > File sync/engine/apply_updates_command.cc (right):
> > 
> >
>
http://codereview.chromium.org/10832286/diff/34002/sync/engine/apply_updates_...
> > sync/engine/apply_updates_command.cc:63:
> > server_type_restriction.Remove(i.Get());
> > On 2012/08/30 21:01:48, rlarocque wrote:
> > > On 2012/08/30 20:18:33, timsteele wrote:
> > > > This would be nicer to read as a single line 'RemoveAll' (or
Difference),
> > but
> > > > EnumSet can't support it :(  
> > > > 
> > > > It'd be useful (especially as we add more type-subgroups) to add a util
in
> > > > model_type to Difference a FullModelTypeSet with a ModelTypeSet and
return
> a
> > > > FullModelTypeSet(and union, etc, down the road).
> > > 
> > > I feel the same way.  I'd like to enforce that we use the right enums in
the
> > > right places, but C++'s type system really isn't helping.
> > > 
> > > If we're going to have to check every conversion manually at run-time
> through
> > > conversion functions, I'd prefer to have just one type (FullModelTypeSet)
> and
> > > DCHECK its contents where we think it's important to do so.
> > > 
> > > That's not something I'd like to do in this patch, though.
> > 
> > I was really thinking low key, like
> > 
> > full_set.Difference(ToFullModelTypeSet(ControlTypes()));
> > 
> > or an explicit one-off:
> > UnionToFullModelTypeSet(full_set, ControlTypes());
> > 
> > would work fine?
> > 
> > Alternatively Fred mentioned we could likely have an EnumSet constructor
that
> > adds items from a passed in EnumSet, possibly of different range, but
> > compile-asserts the min/max values.
> 
> I prefer Fred's solution, but I think it leads us towards something even
simpler
> in the end.
> 
> Your solution would require us to re-implement the set functions, or to use
> generic functions with wrappers that convert the input types.  The second
> alternative would be preferable.  The natural next step after that would be to
> handle the conversion at the place where the generic function is called,
because
> that way we won't have to write wrappers for all the different instances of
the
> conversion.  That sounds like Fred's solution.
> 
> If we do it that way, then we end up having to explicitly convert from one
type
> of EnumSet to another all over the place.  The conversion would be a no-op
> except for the DCHECKs.  Once we're at that point, why not just get rid of the
> conversions, use a single enum type that holds all values, and do the DCHECK
> ourselves?

Because that requires every single change to code to fret about the possibility
of having unwanted types, doesn't it?  In most places, passing a ModelTypeSet
protects you and you have to explicitly break out of that protection or make a
conscious decision to change the type passed in to your function / class,
whatever.

[akalin, 2012-08-30 22:24:51]

> If we do it that way, then we end up having to explicitly convert from one
type
> of EnumSet to another all over the place.  The conversion would be a no-op
> except for the DCHECKs.  Once we're at that point, why not just get rid of the
> conversions, use a single enum type that holds all values, and do the DCHECK
> ourselves?

FullModelTypeSet is used pretty rarely -- I remember only having to convert two
spots.  Did that change?  Given that using ModelTypeSet almost everywhere else
gets us the DCHECKs for free at compile time, I think it's worth it to convert
to FullModelTypeSet only when needed; otherwise we'd have a lot of places to do
the DCHECKs.  What do you think?

[rlarocque, 2012-08-30 22:35:20]

On 2012/08/30 22:24:51, akalin wrote:
> > If we do it that way, then we end up having to explicitly convert from one
> type
> > of EnumSet to another all over the place.  The conversion would be a no-op
> > except for the DCHECKs.  Once we're at that point, why not just get rid of
the
> > conversions, use a single enum type that holds all values, and do the DCHECK
> > ourselves?
> 
> FullModelTypeSet is used pretty rarely -- I remember only having to convert
two
> spots.  Did that change?  Given that using ModelTypeSet almost everywhere else
> gets us the DCHECKs for free at compile time, I think it's worth it to convert
> to FullModelTypeSet only when needed; otherwise we'd have a lot of places to
do
> the DCHECKs.  What do you think?

I agree it works reasonably well when the only types are FullModelTypeSet and
ModelTypeSet.  I don't think that distinction is really all that helpful, but it
doesn't cause too many problems, either (except for the specific issue that
kicked off this series of comments).  

I'm more concerned about what would happen if we were to make EnumSet
instantiations for ControlTypes and UserTypes.  That's the main reason why I'm
arguing against this.  I'd prefer a few DCHECKs to using different kinds of
EnumSets all over the place and having to convert back and forth all the time.

[akalin, 2012-08-30 22:53:30]

> I'm more concerned about what would happen if we were to make EnumSet
> instantiations for ControlTypes and UserTypes.  That's the main reason why I'm
> arguing against this.  I'd prefer a few DCHECKs to using different kinds of
> EnumSets all over the place and having to convert back and forth all the time.

Sure, it's certainly possible to have just one EnumSet type for ControlTypes and
UserTypes if that works out better.

[tim (not reviewing), 2012-08-30 23:22:03]

On 2012/08/30 22:35:20, rlarocque wrote:
> On 2012/08/30 22:24:51, akalin wrote:
> > > If we do it that way, then we end up having to explicitly convert from one
> > type
> > > of EnumSet to another all over the place.  The conversion would be a no-op
> > > except for the DCHECKs.  Once we're at that point, why not just get rid of
> the
> > > conversions, use a single enum type that holds all values, and do the
DCHECK
> > > ourselves?
> > 
> > FullModelTypeSet is used pretty rarely -- I remember only having to convert
> two
> > spots.  Did that change?  Given that using ModelTypeSet almost everywhere
else
> > gets us the DCHECKs for free at compile time, I think it's worth it to
convert
> > to FullModelTypeSet only when needed; otherwise we'd have a lot of places to
> do
> > the DCHECKs.  What do you think?
> 
> I agree it works reasonably well when the only types are FullModelTypeSet and
> ModelTypeSet.  I don't think that distinction is really all that helpful, but
it
> doesn't cause too many problems, either (except for the specific issue that
> kicked off this series of comments).  
> 
> I'm more concerned about what would happen if we were to make EnumSet
> instantiations for ControlTypes and UserTypes.  That's the main reason why I'm
> arguing against this.  I'd prefer a few DCHECKs to using different kinds of
> EnumSets all over the place and having to convert back and forth all the time.

I don't see the issue that kicked this off as that big of a problem, though. It
has a pretty straightforward solution?

The general principle that's guiding me here is that I think trusting that no
one will accidentally mess up in the future is not the best strategy. You might
be able to sprinkle them all perfectly today, based on the code at a moment in
time, but you then rely on people adding DCHECKs in all the places you would, as
the code evolves. I'd rather force a thought to break out of the safety net.

[rlarocque, 2012-08-30 23:56:37]

Patch updated.  I ended up doing it the easy way, as Tim suggested.

[tim (not reviewing), 2012-08-31 00:06:10]

LGTM with nit

http://codereview.chromium.org/10832286/diff/52003/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type.h (right):

http://codereview.chromium.org/10832286/diff/52003/sync/internal_api/public/b...
sync/internal_api/public/base/model_type.h:165: FullModelTypeSet
ToFullModelTypeSet(ModelTypeSet in);
const ModelTypeSet& in

[rlarocque, 2012-08-31 00:12:43]

http://codereview.chromium.org/10832286/diff/52003/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type.h (right):

http://codereview.chromium.org/10832286/diff/52003/sync/internal_api/public/b...
sync/internal_api/public/base/model_type.h:165: FullModelTypeSet
ToFullModelTypeSet(ModelTypeSet in);
On 2012/08/31 00:06:10, timsteele wrote:
> const ModelTypeSet& in

That's what I thought, too, but Fred reminded me that this is just a wrapper
around an std::bitset.  It's small enough that it makes sense to copy it.

[commit-bot: I haz the power, 2012-08-31 17:54:41]

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

[commit-bot: I haz the power, 2012-08-31 22:32:51]

Change committed as 154522