sync: Remove the remaining conflict sets code

chrome/browser/sync/engine/conflict_resolver.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 "chrome/browser/sync/engine/conflict_resolver.h"
 
 #include <algorithm>
 #include <list>
 #include <map>
 #include <set>
@@ skipping 343 matching lines @@
             entry.Get(syncable::META_HANDLE))
           << server_update << entry;
       UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                                 UNDELETE,
                                 CONFLICT_RESOLUTION_SIZE);
     }
     return SYNC_PROGRESS;
   }
 }
 
-bool ConflictResolver::ResolveSimpleConflicts(
-    syncable::WriteTransaction* trans,
-    const Cryptographer* cryptographer,
-    const ConflictProgress& progress,
-    sessions::StatusController* status) {
+bool ConflictResolver::ResolveConflicts(syncable::WriteTransaction* trans,

[Nicolas Zea, 2012/02/01 19:16:19]

I think it would be good to have a comment above here defining what a set is (we
still refer to sets in a couple comments) and why we we currently ignore the
possibility of sets.

[rlarocque, 2012/02/02 00:32:56]

I'm trying to remove the concept of conflict sets from our code base.  The
concept of hiearchy  conflicts is easier to understand, so we should use it
instead.  I intend to delete all comments referring to them, so let me know if
you find any.  

I could add a comment explaining why we don't try to process the hierarchy and
non-blocking/encryption conflicts here.  Alternatively, I could rename the
function to "ResolveSimpleConflicts()".  What do you think?

+                                        const Cryptographer* cryptographer,
+                                        const ConflictProgress& progress,
+                                        sessions::StatusController* status) {
   bool forward_progress = false;
-  // First iterate over simple conflict items (those that belong to no set).
+  // First iterate over simple conflict items.

[Nicolas Zea, 2012/02/01 19:16:19]

Remove first

[rlarocque, 2012/02/02 00:32:56]

Done.

   set<Id>::const_iterator conflicting_item_it;
   set<Id> processed_items;
   for (conflicting_item_it = progress.ConflictingItemsBegin();
        conflicting_item_it != progress.ConflictingItemsEnd();
        ++conflicting_item_it) {
     Id id = *conflicting_item_it;
     if (processed_items.count(id) > 0)
       continue;
-    map<Id, ConflictSet*>::const_iterator item_set_it =
-        progress.IdToConflictSetFind(id);
-    if (item_set_it == progress.IdToConflictSetEnd() ||
-        0 == item_set_it->second) {
-      // We have a simple conflict. In order check if positions have changed,
-      // we need to process conflicting predecessors before successors. Traverse
-      // backwards through all continuous conflicting predecessors, building a
-      // stack of items to resolve in predecessor->successor order, then process
-      // each item individually.
-      list<Id> predecessors;
-      Id prev_id = id;
-      do {
-        predecessors.push_back(prev_id);
-        Entry entry(trans, syncable::GET_BY_ID, prev_id);
-        // Any entry in conflict must be valid.
-        CHECK(entry.good());
-        Id new_prev_id = entry.Get(syncable::PREV_ID);
-        if (new_prev_id == prev_id)
+
+    // We have a simple conflict. In order check if positions have changed,

[rlarocque, 2012/01/31 19:12:17]

There are lots of indentation changes because I removed an if statement, but
otherwise the code on the left and right side of this diff should be identical.

+    // we need to process conflicting predecessors before successors. Traverse
+    // backwards through all continuous conflicting predecessors, building a
+    // stack of items to resolve in predecessor->successor order, then process
+    // each item individually.
+    list<Id> predecessors;
+    Id prev_id = id;
+    do {
+      predecessors.push_back(prev_id);
+      Entry entry(trans, syncable::GET_BY_ID, prev_id);
+      // Any entry in conflict must be valid.
+      CHECK(entry.good());
+      Id new_prev_id = entry.Get(syncable::PREV_ID);
+      if (new_prev_id == prev_id)
+        break;
+      prev_id = new_prev_id;
+    } while (processed_items.count(prev_id) == 0 &&
+             progress.HasSimpleConflictItem(prev_id));  // Excludes root.
+    while (!predecessors.empty()) {
+      id = predecessors.back();
+      predecessors.pop_back();
+      switch (ProcessSimpleConflict(trans, id, cryptographer, status)) {
+        case NO_SYNC_PROGRESS:
           break;
-        prev_id = new_prev_id;
-      } while (processed_items.count(prev_id) == 0 &&
-               progress.HasSimpleConflictItem(prev_id));  // Excludes root.
-      while (!predecessors.empty()) {
-        id = predecessors.back();
-        predecessors.pop_back();
-        switch (ProcessSimpleConflict(trans, id, cryptographer, status)) {
-          case NO_SYNC_PROGRESS:
-            break;
-          case SYNC_PROGRESS:
-            forward_progress = true;
-            break;
-        }
-        processed_items.insert(id);
+        case SYNC_PROGRESS:
+          forward_progress = true;
+          break;
       }
+      processed_items.insert(id);
     }
   }
   return forward_progress;
 }
 
-bool ConflictResolver::ResolveConflicts(syncable::WriteTransaction* trans,
-                                        const Cryptographer* cryptographer,
-                                        const ConflictProgress& progress,
-                                        sessions::StatusController* status) {
-  // TODO(rlarocque): A good amount of code related to the resolution of
-  // conflict sets has been deleted here.  This was done because the code had
-  // not been used in years.  An unrelated bug fix accidentally re-enabled the
-  // code, forcing us to make a decision about what we should do with the code.
-  // We decided to do the safe thing and delete it for now.  This restores the
-  // behaviour we've relied on for quite some time.  We should think about what
-  // that code was trying to do and consider re-enabling parts of it.
-
-  if (progress.ConflictSetsSize() > 0) {
-    DVLOG(1) << "Detected " << progress.IdToConflictSetSize()
-        << " non-simple conflicting entries in " << progress.ConflictSetsSize()
-        << " unprocessed conflict sets.";
-  }
-
-  return ResolveSimpleConflicts(trans, cryptographer, progress, status);
-}
-
 }  // namespace browser_sync