sync: Introduce control data types

sync/engine/apply_updates_command.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/engine/apply_updates_command.h"
 
 #include "base/location.h"
 #include "sync/engine/update_applicator.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/directory.h"
@@ skipping 39 matching lines @@
       dir->GetServerTypesWithUnappliedUpdates(&trans);
   FullModelTypeSet server_type_restriction;
   for (FullModelTypeSet::Iterator it =
            server_types_with_unapplied_updates.First(); it.Good(); it.Inc()) {
     if (GetGroupForModelType(it.Get(), session->routing_info()) ==
         session->status_controller().group_restriction()) {
       server_type_restriction.Put(it.Get());
     }
   }
 
+  // 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);
 
   UpdateApplicator applicator(
       session->context()->resolver(),
       dir->GetCryptographer(&trans),
       handles.begin(), handles.end(), session->routing_info(),
       session->status_controller().group_restriction());
   while (applicator.AttemptOneApplication(&trans)) {}
   applicator.SaveProgressIntoSessionState(
       session->mutable_status_controller()->mutable_conflict_progress(),
       session->mutable_status_controller()->mutable_update_progress());
 
   // This might be the first time we've fully completed a sync cycle, for
   // some subset of the currently synced datatypes.
   const sessions::StatusController& status(session->status_controller());
   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);
+      }
     }
   }
 
   return SYNCER_OK;
 }
 
 }  // namespace syncer