sync: Merge apply updates and resolve conflicts

sync/engine/syncer.cc

Index: sync/engine/syncer.cc
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index ffb2fcaf421d9c452ac9e7d8a146bfa27999562c..bdc5f3cd5c0e3e036d685b8d2c808c5051c64e46 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -19,7 +19,6 @@
 #include "sync/engine/net/server_connection_manager.h"
 #include "sync/engine/process_commit_response_command.h"
 #include "sync/engine/process_updates_command.h"
-#include "sync/engine/resolve_conflicts_command.h"
 #include "sync/engine/store_timestamps_command.h"
 #include "sync/engine/syncer_types.h"
 #include "sync/engine/throttled_data_type_tracker.h"
@@ -61,8 +60,6 @@ const char* SyncerStepToString(const SyncerStep step)
     ENUM_CASE(STORE_TIMESTAMPS);
     ENUM_CASE(APPLY_UPDATES);
     ENUM_CASE(COMMIT);
-    ENUM_CASE(RESOLVE_CONFLICTS);

[tim (not reviewing), 2012/10/24 20:29:08]

It's great to move this to after applying updates, although as usual I think
hiding primary parts of what makes up a "sync cycle" from this layer makes it
harder to see what's going on (versus having APPLY_UPDATES followed by
RESOLVE_CONFLICTS). I'd prefer we wait to "merge" until we're ready to lay
everything out as "one big method" again (without SyncerStep) so you can clearly
see the major steps involved, because doing that "flattening" on its own is not
a priority and in the meantime I think the code is less readable. There's also
the minor side effect that we can't take advantage of early_exit_requested
between successive commands, (though not a big problem here today as resolve
should be fast in theory).  

But we've discussed this before (with commit), so... I guess you can ignore the
above :) 

That said, I feel most strongly about not redefining what "Processing updates"
and "Applying Updates" mean, (pushing protos into SERVER_ fields, and pushing
SERVER_ fields into local fields (in the happy case)). Renaming
ApplyUpdatesCommand to ApplyUpdatesAndResolveConflictsCommand would mostly
address my concern. 

[rlarocque, 2012/10/24 21:21:22]

Although I do think this looks nicer, that's not my only motivation.  I wouldn't
have done this if there weren't real benefits to be had.  The flattening was
necessary to remove PerModelSafeGroupState.  It also makes it easier to perform
the { apply updates; resolve conflicts; re-apply updates } steps all under the
same lock, without which the new logic would be incorrect.  
That's reasonable.  I'll do the rename.  

Looking towards the future, I would like to eventually make simple conflict
resolution so simple that it can consist of one or two if branch in the update
applicator (or the syncer_util.cc function it defers to).  At that point,
someone unfamiliar with the history of this project might look at the name
ApplyUpdatesAndResolveConflictsCommand and wonder why we're making such a big
deal about conflict resolution.

For now, I agree that ApplyUpdatesAndResolveConflictsCommand is a good name.  

-    ENUM_CASE(APPLY_UPDATES_TO_RESOLVE_CONFLICTS);
     ENUM_CASE(SYNCER_END);
   }
   NOTREACHED();
@@ -172,43 +169,6 @@ void Syncer::SyncShare(sessions::SyncSession* session,
       case COMMIT: {
         session->mutable_status_controller()->set_commit_result(
             BuildAndPostCommits(this, session));
-        next_step = RESOLVE_CONFLICTS;
-        break;
-      }
-      case RESOLVE_CONFLICTS: {
-        StatusController* status = session->mutable_status_controller();
-        status->reset_conflicts_resolved();
-        ResolveConflictsCommand resolve_conflicts_command;
-        resolve_conflicts_command.Execute(session);
-
-        // Has ConflictingUpdates includes both resolvable and unresolvable
-        // conflicts. If we have either, we want to attempt to reapply.
-        if (status->HasConflictingUpdates())
-          next_step = APPLY_UPDATES_TO_RESOLVE_CONFLICTS;
-        else
-          next_step = SYNCER_END;
-        break;
-      }
-      case APPLY_UPDATES_TO_RESOLVE_CONFLICTS: {
-        StatusController* status = session->mutable_status_controller();
-        DVLOG(1) << "Applying updates to resolve conflicts";
-        ApplyUpdatesCommand apply_updates;
-
-        // We only care to resolve conflicts again if we made progress on the
-        // simple conflicts.
-        int before_blocking_conflicting_updates =
-            status->num_simple_conflicts();
-        apply_updates.Execute(session);
-        int after_blocking_conflicting_updates =
-            status->num_simple_conflicts();
-        // If the following call sets the conflicts_resolved value to true,
-        // SyncSession::HasMoreToSync() will send us into another sync cycle
-        // after this one completes.
-        //
-        // TODO(rlarocque, 109072): Make conflict resolution not require
-        // extra sync cycles/GetUpdates.
-        status->update_conflicts_resolved(before_blocking_conflicting_updates >
-                                          after_blocking_conflicting_updates);
         next_step = SYNCER_END;
         break;
       }