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()) {
-      // 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));
 
       // 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()) {
-        // 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));
 
         // 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 (rhs.url() != lhs.url() || rhs.title().compare(lhs.title()) < 0 ||
+      rhs.creation_time_us() < lhs.creation_time_us() ||
+      rhs.update_time_us() < lhs.update_time_us()) {
+    return false;
+  }
+  if (rhs.update_time_us() == lhs.update_time_us()) {
+    if ((rhs.status() == sync_pb::ReadingListSpecifics::UNSEEN &&
+         lhs.status() != sync_pb::ReadingListSpecifics::UNSEEN) ||
+        (rhs.status() == sync_pb::ReadingListSpecifics::UNREAD &&
+         lhs.status() == sync_pb::ReadingListSpecifics::READ))
+      return false;

[gambard, 2016/12/07 13:49:54]

Why not returning the condition directly?

[Olivier, 2016/12/08 15:23:22]

Because we may add other cases after

+  }
+  return true;
+}