Create a strict order in ReadingListSpecifics

components/reading_list/ios/reading_list_store.cc

Index: components/reading_list/ios/reading_list_store.cc
diff --git a/components/reading_list/ios/reading_list_store.cc b/components/reading_list/ios/reading_list_store.cc
index 2d81a19150cc4657e76341aee3cddc0eaebd3c11..bc39915874c065f117eb62738483bebcd7f7ad8e 100644
--- a/components/reading_list/ios/reading_list_store.cc
+++ b/components/reading_list/ios/reading_list_store.cc
@@ -231,9 +231,8 @@ syncer::SyncError ReadingListStore::MergeSyncData(
 
       // Notify model about updated entry.
       delegate_->SyncAddEntry(std::move(entry));
-    } else if (existing_entry->UpdateTime() < entry->UpdateTime()) {
-      // The entry from sync is more recent.
-      // Merge the local data to it and store it.
+    } else {
+      // Merge the local data and the sync data and store the result.
       ReadingListEntry* merged_entry =
           delegate_->SyncMergeEntry(std::move(entry));
 
@@ -246,6 +245,7 @@ syncer::SyncError ReadingListStore::MergeSyncData(
       // Send to sync
       std::unique_ptr<sync_pb::ReadingListSpecifics> entry_sync_pb =
           merged_entry->AsReadingListSpecifics();
+      DCHECK(CompareEntriesForSync(specifics, *entry_sync_pb));
       auto entity_data = base::MakeUnique<syncer::EntityData>();
       *(entity_data->specifics.mutable_reading_list()) = *entry_sync_pb;
       entity_data->non_unique_name = entry_sync_pb->entry_id();
@@ -255,18 +255,6 @@ syncer::SyncError ReadingListStore::MergeSyncData(
       change_processor()->Put(entry_sync_pb->entry_id(), std::move(entity_data),
                               metadata_change_list.get());
 
-    } else {
-      // The entry from sync is out of date.
-      // Send back the local more recent entry.
-      // No need to update
-      std::unique_ptr<sync_pb::ReadingListSpecifics> entry_pb =
-          existing_entry->AsReadingListSpecifics();
-      auto entity_data = base::MakeUnique<syncer::EntityData>();
-      *(entity_data->specifics.mutable_reading_list()) = *entry_pb;
-      entity_data->non_unique_name = entry_pb->entry_id();
-
-      change_processor()->Put(entry_pb->entry_id(), std::move(entity_data),
-                              metadata_change_list.get());
     }
   }
 
@@ -331,9 +319,8 @@ syncer::SyncError ReadingListStore::ApplySyncChanges(
 
         // Notify model about updated entry.
         delegate_->SyncAddEntry(std::move(entry));
-      } else if (existing_entry->UpdateTime() < entry->UpdateTime()) {
-        // The entry from sync is more recent.
-        // Merge the local data to it and store it.
+      } else {
+        // Merge the local data and the sync data and store the result.
         ReadingListEntry* merged_entry =
             delegate_->SyncMergeEntry(std::move(entry));
 
@@ -346,6 +333,7 @@ syncer::SyncError ReadingListStore::ApplySyncChanges(
         // Send to sync
         std::unique_ptr<sync_pb::ReadingListSpecifics> entry_sync_pb =
             merged_entry->AsReadingListSpecifics();
+        DCHECK(CompareEntriesForSync(specifics, *entry_sync_pb));
         auto entity_data = base::MakeUnique<syncer::EntityData>();
         *(entity_data->specifics.mutable_reading_list()) = *entry_sync_pb;
         entity_data->non_unique_name = entry_sync_pb->entry_id();
@@ -356,18 +344,6 @@ syncer::SyncError ReadingListStore::ApplySyncChanges(
                                 std::move(entity_data),
                                 metadata_change_list.get());
 
-      } else {
-        // The entry from sync is out of date.
-        // Send back the local more recent entry.
-        // No need to update
-        std::unique_ptr<sync_pb::ReadingListSpecifics> entry_pb =
-            existing_entry->AsReadingListSpecifics();
-        auto entity_data = base::MakeUnique<syncer::EntityData>();
-        *(entity_data->specifics.mutable_reading_list()) = *entry_pb;
-        entity_data->non_unique_name = entry_pb->entry_id();
-
-        change_processor()->Put(entry_pb->entry_id(), std::move(entity_data),
-                                metadata_change_list.get());
       }
     }
   }
@@ -439,3 +415,47 @@ std::string ReadingListStore::GetStorageKey(
   DCHECK(CalledOnValidThread());
   return entity_data.specifics.reading_list().entry_id();
 }
+
+bool ReadingListStore::CompareEntriesForSync(
+    const sync_pb::ReadingListSpecifics& lhs,
+    const sync_pb::ReadingListSpecifics& rhs) {
+  DCHECK(lhs.entry_id() == rhs.entry_id());
+  DCHECK(lhs.has_update_time_us());
+  DCHECK(rhs.has_update_time_us());
+  DCHECK(lhs.has_creation_time_us());
+  DCHECK(rhs.has_creation_time_us());
+  DCHECK(lhs.has_url());
+  DCHECK(rhs.has_url());
+  DCHECK(lhs.has_title());
+  DCHECK(rhs.has_title());
+  DCHECK(lhs.has_status());
+  DCHECK(rhs.has_status());
+  if (lhs.update_time_us() < rhs.update_time_us()) {
+    // The |rhs| more recent, so it can be submitted to sync.
+    return true;
+  }
+  if (lhs.update_time_us() > rhs.update_time_us()) {
+    // The |rhs| entry is older, so it cannot be submitted to sync.
+    return false;
+  }
+
+  if (lhs.url() != rhs.url() || lhs.title() != rhs.title() ||
+      lhs.creation_time_us() != rhs.creation_time_us() ||
+      lhs.update_time_us() != rhs.update_time_us()) {

[gambard, 2016/12/07 10:28:49]

lhs.update_time_us() != rhs.update_time_us() will always be true and is not
consistent with the comment below.

+    // A modification should not be submitted to the server without a change to
+    // update_time.
+    return false;
+  }
+  if (lhs.status() == sync_pb::ReadingListSpecifics::UNSEEN) {
+    // An entry can always be marked SEEN on a device.
+    return true;
+  }
+  if (lhs.status() != rhs.status()) {
+    // If the status is different and not UNSEEN, update_time should be

[gambard, 2016/12/07 10:28:49]

Reword this.
WDYT of: Update_time should be different if the status are different and |lhs|
is not UNSEEN.

+    // different.
+    return false;
+  }
+  // All cases should be handled. Entries must be the same.
+  DCHECK(lhs.SerializeAsString() == rhs.SerializeAsString());

[gambard, 2016/12/07 10:28:49]

Maybe return lhs.SerializeAsString() == rhs.SerializeAsString() instead of
DCHECK?

+  return true;
+}