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..fcab824196eb04ec05c947dc7a3af342d8961d4b 100644
--- a/components/reading_list/ios/reading_list_store.cc
+++ b/components/reading_list/ios/reading_list_store.cc
@@ -231,7 +231,7 @@ syncer::SyncError ReadingListStore::MergeSyncData(
 
       // Notify model about updated entry.
       delegate_->SyncAddEntry(std::move(entry));
-    } else if (existing_entry->UpdateTime() < entry->UpdateTime()) {
+    } else {
       // The entry from sync is more recent.

[gambard, 2016/12/06 17:02:22]

Update comment

[Olivier, 2016/12/06 17:16:21]

Done.

       // Merge the local data to it and store it.
       ReadingListEntry* merged_entry =
@@ -246,6 +246,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 +256,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,7 +320,7 @@ syncer::SyncError ReadingListStore::ApplySyncChanges(
 
         // Notify model about updated entry.
         delegate_->SyncAddEntry(std::move(entry));
-      } else if (existing_entry->UpdateTime() < entry->UpdateTime()) {
+      } else {
         // The entry from sync is more recent.

[gambard, 2016/12/06 17:02:22]

Update comment

[Olivier, 2016/12/06 17:16:21]

Done.

         // Merge the local data to it and store it.
         ReadingListEntry* merged_entry =
@@ -346,6 +335,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 +346,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 +417,45 @@ 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 entry is more recent, so it can be submitted to sync.

[gambard, 2016/12/06 17:02:22]

"The entry" here and below is not specific enough. Which one?
Update comment :)

[Olivier, 2016/12/06 17:16:21]

Done.

+    return true;
+  }
+  if (lhs.update_time_us() > rhs.update_time_us()) {
+    // The entry is older, so it cannot be submitted to sync.
+    return false;
+  }
+
+  if (lhs.url() != rhs.url() || lhs.title() != rhs.title() ||

[gambard, 2016/12/06 17:02:22]

Not sure the check on "url()" is necessary here.

[Olivier, 2016/12/06 17:16:21]

As far as sync is concerned, the key is entry_id. So checking url may be useful.

+      lhs.creation_time_us() != rhs.creation_time_us() ||
+      lhs.update_time_us() != rhs.update_time_us()) {

[gambard, 2016/12/06 17:02:22]

Actually I don't understand this condition.
If the update times are different on the two entries you will enter this block?
And you tested it for inequality above.

[Olivier, 2016/12/06 17:16:21]

This method ensures that you send correct data to the sync server.
It does not know the rules we use to merge entries.
If we want to change MergeEntryWith and break the strict order, this will catch
it and avoid sending the data to sync and loop.

+    // 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()) {

[gambard, 2016/12/06 17:02:22]

Maybe add a comment for this?

[Olivier, 2016/12/06 17:16:21]

Done.

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