sync: Introduce control data types

sync/engine/apply_updates_command.cc

Index: sync/engine/apply_updates_command.cc
diff --git a/sync/engine/apply_updates_command.cc b/sync/engine/apply_updates_command.cc
index da8a1cc4c2e987d464445cfbef8bd63cd0be4513..2379cbd70f01cc8506788b604c31151d4416f0e7 100644
--- a/sync/engine/apply_updates_command.cc
+++ b/sync/engine/apply_updates_command.cc
@@ -57,6 +57,12 @@ SyncerError ApplyUpdatesCommand::ModelChangingExecuteImpl(
     }
   }
 
+  // Don't process control type updates here.  They will be handled elsewhere.
+  ModelTypeSet control_types = ControlTypes();
+  for (ModelTypeSet::Iterator i = control_types.First(); i.Good(); i.Inc()) {
+    server_type_restriction.Remove(i.Get());

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

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).

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

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.

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

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.

+  }
+
   std::vector<int64> handles;
   dir->GetUnappliedUpdateMetaHandles(
       &trans, server_type_restriction, &handles);
@@ -77,8 +83,11 @@ SyncerError ApplyUpdatesCommand::ModelChangingExecuteImpl(
   if (status.ServerSaysNothingMoreToDownload()) {
     for (ModelTypeSet::Iterator it =
              status.updates_request_types().First(); it.Good(); it.Inc()) {
-      // This gets persisted to the directory's backing store.
-      dir->set_initial_sync_ended_for_type(it.Get(), true);
+      // Don't set the flag for control types.  We didn't process them here.
+      if (!IsControlType(it.Get())) {

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

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(...)

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

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

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

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.

+        // This gets persisted to the directory's backing store.
+        dir->set_initial_sync_ended_for_type(it.Get(), true);
+      }
     }
   }