sync: Remove the remaining conflict sets code

chrome/browser/sync/sessions/session_state.h

 // 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.
 
 // The 'sessions' namespace comprises all the pieces of state that are
 // combined to form a SyncSession instance. In that way, it can be thought of
 // as an extension of the SyncSession type itself. Session scoping gives
 // context to things like "conflict progress", "update progress", etc, and the
 // separation this file provides allows clients to only include the parts they
 // need rather than the entire session stack.
@@ skipping 143 matching lines @@
   const int64 unsynced_count;
   const int num_blocking_conflicting_updates;
   const int num_conflicting_updates;
   const bool did_commit_items;
   const SyncSourceInfo source;
   const size_t num_entries;
   base::Time sync_start_time;
   const bool retry_scheduled;
 };
 
-// Tracks progress of conflicts and their resolution using conflict sets.
+// Tracks progress of conflicts and their resolution.

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

resolutions

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

Done.

 class ConflictProgress {
  public:
   explicit ConflictProgress(bool* dirty_flag);
   ~ConflictProgress();
-  // Various iterators, size, and retrieval functions for conflict sets.
-  IdToConflictSetMap::const_iterator IdToConflictSetBegin() const;
-  IdToConflictSetMap::const_iterator IdToConflictSetEnd() const;
-  IdToConflictSetMap::size_type IdToConflictSetSize() const;
-  IdToConflictSetMap::const_iterator IdToConflictSetFind(
-      const syncable::Id& the_id) const;
-  const ConflictSet* IdToConflictSetGet(const syncable::Id& the_id);
-  std::set<ConflictSet*>::const_iterator ConflictSetsBegin() const;
-  std::set<ConflictSet*>::const_iterator ConflictSetsEnd() const;
-  std::set<ConflictSet*>::size_type ConflictSetsSize() const;
-  bool HasSimpleConflictItem(const syncable::Id& id) const;
+
+  bool HasSimpleConflictItem(const syncable::Id &id) const;
 
   // Various mutators for tracking commit conflicts.
   void AddConflictingItemById(const syncable::Id& the_id);
   void EraseConflictingItemById(const syncable::Id& the_id);
   int ConflictingItemsSize() const { return conflicting_item_ids_.size(); }
   std::set<syncable::Id>::const_iterator ConflictingItemsBegin() const;
   std::set<syncable::Id>::const_iterator ConflictingItemsEnd() const;
 
   // Mutators for nonblocking conflicting items (see description below).
   void AddNonblockingConflictingItemById(const syncable::Id& the_id);
-  void EraseNonblockingConflictingItemById(const syncable::Id& the_id);
   int NonblockingConflictingItemsSize() const {
     return nonblocking_conflicting_item_ids_.size();
   }
 
-  void MergeSets(const syncable::Id& set1, const syncable::Id& set2);
-  void CleanupSets();
+  void AddHierarchyConflictingItemById(const syncable::Id& id);
+  int HierarchyConflictingItemsSize() const {
+    return hierarchy_conflicting_item_ids_.size();
+  }
 
  private:
-  // TODO(sync): move away from sets if it makes more sense.
+  // Conflicts that occur when an local and server changes collide, minus

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

remove an

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

Done.

+  // the nonblocking and hierarchy conflicts.
   std::set<syncable::Id> conflicting_item_ids_;
-  std::map<syncable::Id, ConflictSet*> id_to_conflict_set_;
-  std::set<ConflictSet*> conflict_sets_;
 
   // Nonblocking conflicts are not processed by the conflict resolver, but
   // they will be processed in the APPLY_UDPATES_TO_RESOLVE_CONFLICTS step.
   std::set<syncable::Id> nonblocking_conflicting_item_ids_;
 
+  // Hiearchy conflicts are updates that would violate tree invariants if
+  // they were applied.
+  std::set<syncable::Id> hierarchy_conflicting_item_ids_;

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

I'm not sure this is the right thing to do.  I decided to maintain symmetry with
the nonblocking_conflicting_item_ids, even though it doesn't make a lot of sense
to me.  

We never access the members of this set.  I think the set is intended to allow
us to count more accurately (by avoiding double-counting), but I'm not sure
that's actually necessary.  

Alternative approaches:
A. Track both encryption and hierarchy conflicts as nonblocking conflicts
because we treat them the same way.  (this may end up limiting the amount of
information we could export to about:sync)
B. Use counters instead of sets to track these items.  (This makes the most
sense to me, but I'm reluctant to delete code I don't understand.)
C. Some combination of the two: one set, two counters?

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

I prefer C, probably by adding an extra parameter to the nonblocking conflict
methods specifying the type of nonblocking conflict (e.g. CONFLICT_ENCRYPTION 
or CONFLICT_HIERARCHY).

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

Minor point: If we're going with option C, I'd prefer to use separate functions.
 It's harder to misuse two functions than one function with an enum parameter.  

Implemented in the following patch set.

+
   // Whether a conflicting item was added or removed since
   // the last call to reset_progress_changed(), if any. In practice this
   // points to StatusController::is_dirty_.
   bool* dirty_;
 };
 
 typedef std::pair<VerifyResult, sync_pb::SyncEntity> VerifiedUpdate;
 typedef std::pair<UpdateAttemptResponse, syncable::Id> AppliedUpdate;
 
 // Tracks update application and verification.
@@ skipping 109 matching lines @@
   ~PerModelSafeGroupState();
 
   UpdateProgress update_progress;
   ConflictProgress conflict_progress;
 };
 
 }  // namespace sessions
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_SESSIONS_SESSION_STATE_H_