Refactor Reading List Model to use URL as key.

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 53 matching lines @@
   DCHECK(CalledOnValidThread());
   pending_transaction_count_--;
   if (pending_transaction_count_ == 0) {
     store_->CommitWriteBatch(
         std::move(batch_),
         base::Bind(&ReadingListStore::OnDatabaseSave, base::AsWeakPtr(this)));
     batch_.reset();
   }
 }
 
-void ReadingListStore::SaveEntry(const ReadingListEntry& entry, bool read) {
+void ReadingListStore::SaveEntry(const ReadingListEntry& entry) {
   DCHECK(CalledOnValidThread());
   auto token = EnsureBatchCreated();
 
   std::unique_ptr<reading_list::ReadingListLocal> pb_entry =
-      entry.AsReadingListLocal(read);
+      entry.AsReadingListLocal();
 
   batch_->WriteData(entry.URL().spec(), pb_entry->SerializeAsString());
 
   if (!change_processor()->IsTrackingMetadata()) {
     return;
   }
   std::unique_ptr<sync_pb::ReadingListSpecifics> pb_entry_sync =
-      entry.AsReadingListSpecifics(read);
+      entry.AsReadingListSpecifics();
 
   std::unique_ptr<syncer::MetadataChangeList> metadata_change_list =
       CreateMetadataChangeList();
 
   std::unique_ptr<syncer::EntityData> entity_data(new syncer::EntityData());
   *entity_data->specifics.mutable_reading_list() = *pb_entry_sync;
   entity_data->non_unique_name = pb_entry_sync->entry_id();
 
   change_processor()->Put(entry.URL().spec(), std::move(entity_data),
                           metadata_change_list.get());
@@ skipping 19 matching lines @@
     syncer::ModelTypeStore::Result result,
     std::unique_ptr<syncer::ModelTypeStore::RecordList> entries) {
   DCHECK(CalledOnValidThread());
   if (result != syncer::ModelTypeStore::Result::SUCCESS) {
     change_processor()->OnMetadataLoaded(
         change_processor()->CreateAndUploadError(
             FROM_HERE, "Cannot load Reading List Database."),
         nullptr);
     return;
   }
-  auto read = base::MakeUnique<ReadingListEntries>();
-  auto unread = base::MakeUnique<ReadingListEntries>();
+  auto loaded_entries = base::MakeUnique<ReadingListEntries>();
 
   for (const syncer::ModelTypeStore::Record& r : *entries.get()) {
     // for (const reading_list::ReadingListLocal& pb_entry : *entries) {
     reading_list::ReadingListLocal proto;
     if (!proto.ParseFromString(r.value)) {
       continue;
       // TODO(skym, crbug.com/582460): Handle unrecoverable initialization
       // failure.
     }
 
     std::unique_ptr<ReadingListEntry> entry(
         ReadingListEntry::FromReadingListLocal(proto));
     if (!entry) {
       continue;
     }
-    if (proto.status() == reading_list::ReadingListLocal::READ) {
-      read->push_back(std::move(*entry));
-    } else {
-      unread->push_back(std::move(*entry));
-    }
+    GURL url = entry->URL();
+    DCHECK(!loaded_entries->count(url));
+    loaded_entries->insert(std::make_pair(url, std::move(*entry)));

[jif-google, 2016/11/28 16:28:21]

use operator[] if possible

[Olivier, 2016/11/28 17:58:25]

Not possible

   }
 
-  delegate_->StoreLoaded(std::move(unread), std::move(read));
+  delegate_->StoreLoaded(std::move(loaded_entries));
 
   store_->ReadAllMetadata(
       base::Bind(&ReadingListStore::OnReadAllMetadata, base::AsWeakPtr(this)));
 }
 
 void ReadingListStore::OnReadAllMetadata(
     syncer::SyncError sync_error,
     std::unique_ptr<syncer::MetadataBatch> metadata_batch) {
   DCHECK(CalledOnValidThread());
   change_processor()->OnMetadataLoaded(sync_error, std::move(metadata_batch));
@@ skipping 50 matching lines @@
   std::set<std::string> synced_entries;
   std::unique_ptr<ReadingListModel::ScopedReadingListBatchUpdate>
       model_batch_updates = model_->BeginBatchUpdates();
 
   // Merge sync to local data.
   for (const auto& kv : entity_data_map) {
     synced_entries.insert(kv.first);
     const sync_pb::ReadingListSpecifics& specifics =
         kv.second.value().specifics.reading_list();
     // Deserialize entry.
-    bool read = specifics.status() == sync_pb::ReadingListSpecifics::READ;
     std::unique_ptr<ReadingListEntry> entry(
         ReadingListEntry::FromReadingListSpecifics(specifics));
 
-    bool was_read;
     const ReadingListEntry* existing_entry =
-        model_->GetEntryFromURL(entry->URL(), &was_read);
+        model_->GetEntryByURL(entry->URL());
 
     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(read);
+          entry->AsReadingListLocal();
       batch_->WriteData(entry->URL().spec(), entry_pb->SerializeAsString());
 
       // Notify model about updated entry.
-      delegate_->SyncAddEntry(std::move(entry), read);
+      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.
       ReadingListEntry* merged_entry =
-          delegate_->SyncMergeEntry(std::move(entry), read);
+          delegate_->SyncMergeEntry(std::move(entry));
 
       // Write to the store.
       std::unique_ptr<reading_list::ReadingListLocal> entry_local_pb =
-          merged_entry->AsReadingListLocal(read);
+          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(read);
+          merged_entry->AsReadingListSpecifics();
       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(was_read);
+          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.
-  int unread_count = model_->unread_size();
-  int read_count = model_->read_size();
-  for (int index = 0; index < unread_count + read_count; index++) {
-    bool read = index >= unread_count;
-    const ReadingListEntry& entry =
-        read ? model_->GetReadEntryAtIndex(index - unread_count)
-             : model_->GetUnreadEntryAtIndex(index);
+  for (const auto& iterator : *model_) {
+    // for (int index = 0; index < count; index++) {

[jif-google, 2016/11/28 16:28:21]

remove

[Olivier, 2016/11/28 17:58:25]

Done.

+    const ReadingListEntry& entry = iterator.second;
     if (synced_entries.count(entry.URL().spec())) {
       // Entry already exists and has been merged above.
       continue;
     }
 
     // Local entry has later timestamp. It should be committed to server.
     std::unique_ptr<sync_pb::ReadingListSpecifics> entry_pb =
-        entry.AsReadingListSpecifics(read);
+        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));
 
@@ skipping 15 matching lines @@
 
   for (syncer::EntityChange& change : entity_changes) {
     if (change.type() == syncer::EntityChange::ACTION_DELETE) {
       batch_->DeleteData(change.storage_key());
       // Need to notify model that entry is deleted.
       delegate_->SyncRemoveEntry(GURL(change.storage_key()));
     } else {
       // Deserialize entry.
       const sync_pb::ReadingListSpecifics& specifics =
           change.data().specifics.reading_list();
-      bool read = specifics.status() == sync_pb::ReadingListSpecifics::READ;
       std::unique_ptr<ReadingListEntry> entry(
           ReadingListEntry::FromReadingListSpecifics(specifics));
 
-      bool was_read;
       const ReadingListEntry* existing_entry =
-          model_->GetEntryFromURL(entry->URL(), &was_read);
+          model_->GetEntryByURL(entry->URL());
 
       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(read);
+            entry->AsReadingListLocal();
         batch_->WriteData(entry->URL().spec(), entry_pb->SerializeAsString());
 
         // Notify model about updated entry.
-        delegate_->SyncAddEntry(std::move(entry), read);
+        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.
         ReadingListEntry* merged_entry =
-            delegate_->SyncMergeEntry(std::move(entry), read);
+            delegate_->SyncMergeEntry(std::move(entry));
 
         // Write to the store.
         std::unique_ptr<reading_list::ReadingListLocal> entry_local_pb =
-            merged_entry->AsReadingListLocal(read);
+            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(read);
+            merged_entry->AsReadingListSpecifics();
         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(was_read);
+            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) {
   DCHECK(CalledOnValidThread());
   auto batch = base::MakeUnique<syncer::MutableDataBatch>();
   for (std::string url_string : storage_keys) {

[jif-google, 2016/11/28 16:28:21]

const std::string& url_string

[Olivier, 2016/11/28 17:58:25]

Done.

-    bool read;
-    const ReadingListEntry* entry =
-        model_->GetEntryFromURL(GURL(url_string), &read);
+    const ReadingListEntry* entry = model_->GetEntryByURL(GURL(url_string));
     if (entry) {
-      AddEntryToBatch(batch.get(), *entry, read);
+      AddEntryToBatch(batch.get(), *entry);
     }
   }
 
   callback.Run(syncer::SyncError(), std::move(batch));
 }
 
 void ReadingListStore::GetAllData(DataCallback callback) {
   DCHECK(CalledOnValidThread());
   auto batch = base::MakeUnique<syncer::MutableDataBatch>();
-  int unread_count = model_->unread_size();
-  int read_count = model_->read_size();
-  for (int index = 0; index < unread_count + read_count; index++) {
-    bool read = index >= unread_count;
-    const ReadingListEntry& entry =
-        read ? model_->GetReadEntryAtIndex(index - unread_count)
-             : model_->GetUnreadEntryAtIndex(index);
-    AddEntryToBatch(batch.get(), entry, read);
+
+  for (const auto& iterator : *model_) {
+    AddEntryToBatch(batch.get(), iterator.second);
   }
 
   callback.Run(syncer::SyncError(), std::move(batch));
 }
 
 void ReadingListStore::AddEntryToBatch(syncer::MutableDataBatch* batch,
-                                       const ReadingListEntry& entry,
-                                       bool read) {
+                                       const ReadingListEntry& entry) {
   DCHECK(CalledOnValidThread());
   std::unique_ptr<sync_pb::ReadingListSpecifics> entry_pb =
-      entry.AsReadingListSpecifics(read);
+      entry.AsReadingListSpecifics();
 
   std::unique_ptr<syncer::EntityData> entity_data(new syncer::EntityData());
   *(entity_data->specifics.mutable_reading_list()) = *entry_pb;
   entity_data->non_unique_name = entry_pb->entry_id();
 
   batch->Put(entry_pb->entry_id(), std::move(entity_data));
 }
 
 // Get or generate a client tag for |entity_data|. This must be the same tag
 // that was/would have been generated in the SyncableService/Directory world
@@ skipping 12 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();
 }