Create a strict order in ReadingListSpecifics

components/reading_list/ios/reading_list_store.cc

 // Copyright 2016 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 "components/reading_list/ios/reading_list_store.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "components/reading_list/ios/proto/reading_list.pb.h"
@@ skipping 213 matching lines @@
 
     if (!existing_entry) {
       // This entry is new. Add it to the store and model.
       // Convert to local store format and write to store.
       std::unique_ptr<reading_list::ReadingListLocal> entry_pb =
           entry->AsReadingListLocal();
       batch_->WriteData(entry->URL().spec(), entry_pb->SerializeAsString());
 
       // 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 =
           delegate_->SyncMergeEntry(std::move(entry));
 
       // Write to the store.
       std::unique_ptr<reading_list::ReadingListLocal> entry_local_pb =
           merged_entry->AsReadingListLocal();
       batch_->WriteData(entry->URL().spec(),
                         entry_local_pb->SerializeAsString());
 
       // 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();
 
       // TODO(crbug.com/666232): Investigate if there is a risk of sync
       // ping-pong.
       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());
     }
   }
 
   // Commit local only entries to server.
   for (const auto& url : model_->Keys()) {
     const ReadingListEntry* entry = model_->GetEntryByURL(url);
     if (synced_entries.count(url.spec())) {
       // Entry already exists and has been merged above.
       continue;
     }
@@ skipping 44 matching lines @@
 
       if (!existing_entry) {
         // This entry is new. Add it to the store and model.
         // Convert to local store format and write to store.
         std::unique_ptr<reading_list::ReadingListLocal> entry_pb =
             entry->AsReadingListLocal();
         batch_->WriteData(entry->URL().spec(), entry_pb->SerializeAsString());
 
         // 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 =
             delegate_->SyncMergeEntry(std::move(entry));
 
         // Write to the store.
         std::unique_ptr<reading_list::ReadingListLocal> entry_local_pb =
             merged_entry->AsReadingListLocal();
         batch_->WriteData(merged_entry->URL().spec(),
                           entry_local_pb->SerializeAsString());
 
         // 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();
 
         // TODO(crbug.com/666232): Investigate if there is a risk of sync
         // ping-pong.
         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());
       }
     }
   }
 
   batch_->TransferMetadataChanges(std::move(metadata_change_list));
   return syncer::SyncError();
 }
 
 void ReadingListStore::GetData(StorageKeyList storage_keys,
                                DataCallback callback) {
@@ skipping 51 matching lines @@
 // called once when first encountering a remote entity. Local changes will
 // provide their storage keys directly to Put instead of using this method.
 // Theoretically this function doesn't need to be stable across multiple calls
 // on the same or different clients, but to keep things simple, it probably
 // should be.
 std::string ReadingListStore::GetStorageKey(
     const syncer::EntityData& entity_data) {
   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;
+}