Add Store+Sync to reading list.

ios/chrome/browser/reading_list/reading_list_model_impl.cc

Index: ios/chrome/browser/reading_list/reading_list_model_impl.cc
diff --git a/ios/chrome/browser/reading_list/reading_list_model_impl.cc b/ios/chrome/browser/reading_list/reading_list_model_impl.cc
index 1066206e65d581c687a5e366dfc55b1008cc2f4d..5101a262af3df3e69b5327932fbcda0636552273 100644
--- a/ios/chrome/browser/reading_list/reading_list_model_impl.cc
+++ b/ios/chrome/browser/reading_list/reading_list_model_impl.cc
@@ -4,76 +4,122 @@
 
 #include "ios/chrome/browser/reading_list/reading_list_model_impl.h"
 
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "components/prefs/pref_service.h"
 #include "ios/chrome/browser/reading_list/reading_list_model_storage.h"
+#include "ios/chrome/browser/reading_list/reading_list_pref_names.h"
+#include "ios/web/public/web_thread.h"
 #include "url/gurl.h"
 
-ReadingListModelImpl::ReadingListModelImpl() : ReadingListModelImpl(NULL) {}
+ReadingListModelImpl::ReadingListModelImpl()
+    : ReadingListModelImpl(NULL, NULL) {}
 
 ReadingListModelImpl::ReadingListModelImpl(
-    std::unique_ptr<ReadingListModelStorage> storage)
-    : hasUnseen_(false) {
+    std::unique_ptr<ReadingListModelStorage> storage,
+    PrefService* pref_service)
+    : pref_service_(pref_service),
+      has_unseen_(false),
+      loaded_(false),
+      weak_ptr_factory_(this) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   if (storage) {
-    storageLayer_ = std::move(storage);
-    read_ = storageLayer_->LoadPersistentReadList();
-    unread_ = storageLayer_->LoadPersistentUnreadList();
-    hasUnseen_ = storageLayer_->LoadPersistentHasUnseen();
+    storage_layer_ = std::move(storage);
+    storage_layer_->SetReadingListModel(this);
+  } else {
+    loaded_ = true;
+    read_ = base::MakeUnique<ReadingListEntries>();
+    unread_ = base::MakeUnique<ReadingListEntries>();
   }
-  loaded_ = true;
+  has_unseen_ = LoadPersistentHasUnseen();
 }
 ReadingListModelImpl::~ReadingListModelImpl() {}
 
+void ReadingListModelImpl::ModelLoaded(
+    std::unique_ptr<ReadingListEntries> unread,
+    std::unique_ptr<ReadingListEntries> read) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  read_ = std::move(read);
+  unread_ = std::move(unread);
+  SortEntries();
+  loaded_ = true;
+  for (auto& observer : observers_)
+    observer.ReadingListModelLoaded(this);
+}
+
 void ReadingListModelImpl::Shutdown() {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   for (auto& observer : observers_)
     observer.ReadingListModelBeingDeleted(this);
   loaded_ = false;
 }
 
 bool ReadingListModelImpl::loaded() const {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   return loaded_;
 }
 
 size_t ReadingListModelImpl::unread_size() const {
-  DCHECK(loaded());
-  return unread_.size();
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  if (!loaded())
+    return 0;
+  return unread_->size();
 }
 
 size_t ReadingListModelImpl::read_size() const {
-  DCHECK(loaded());
-  return read_.size();
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  if (!loaded())
+    return 0;
+  return read_->size();
 }
 
 bool ReadingListModelImpl::HasUnseenEntries() const {
-  DCHECK(loaded());
-  return unread_size() && hasUnseen_;
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  if (!loaded())
+    return false;
+  return unread_size() && has_unseen_;
 }
 
 void ReadingListModelImpl::ResetUnseenEntries() {
   DCHECK(loaded());
-  hasUnseen_ = false;
-  if (storageLayer_ && !IsPerformingBatchUpdates())
-    storageLayer_->SavePersistentHasUnseen(false);
+  has_unseen_ = false;
+  if (!IsPerformingBatchUpdates())
+    SavePersistentHasUnseen(false);
 }
 
 const ReadingListEntry& ReadingListModelImpl::GetUnreadEntryAtIndex(
     size_t index) const {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
-  return unread_[index];
+  return unread_->at(index);
 }
 
 const ReadingListEntry& ReadingListModelImpl::GetReadEntryAtIndex(
     size_t index) const {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
-  return read_[index];
+  return read_->at(index);
 }
 
 const ReadingListEntry* ReadingListModelImpl::GetEntryFromURL(
     const GURL& gurl) const {
   DCHECK(loaded());
+  bool read;
+  return GetMutableEntryFromURL(gurl, read);
+}
+
+ReadingListEntry* ReadingListModelImpl::GetMutableEntryFromURL(
+    const GURL& gurl,
+    bool& read) const {

[skym, 2016/11/01 18:27:25]

I think this is against Google C++ style,
https://google.github.io/styleguide/cppguide.html#Reference_Arguments , raw
pointer is preferred when modifying.

[Olivier, 2016/11/02 14:57:10]

Done.

+  DCHECK(loaded());
   ReadingListEntry entry(gurl, std::string());
-  auto it = std::find(read_.begin(), read_.end(), entry);
-  if (it == read_.end()) {
-    it = std::find(unread_.begin(), unread_.end(), entry);
-    if (it == unread_.end())
+  auto it = std::find(read_->begin(), read_->end(), entry);
+  read = true;
+  if (it == read_->end()) {
+    it = std::find(unread_->begin(), unread_->end(), entry);
+    read = false;
+    if (it == unread_->end())
       return nullptr;
   }
   return &(*it);
@@ -82,8 +128,10 @@ const ReadingListEntry* ReadingListModelImpl::GetEntryFromURL(
 bool ReadingListModelImpl::CallbackEntryURL(
     const GURL& url,
     base::Callback<void(const ReadingListEntry&)> callback) const {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
-  const ReadingListEntry* entry = GetEntryFromURL(url);
+  bool read;
+  const ReadingListEntry* entry = GetMutableEntryFromURL(url, read);
   if (entry) {
     callback.Run(*entry);
     return true;
@@ -91,33 +139,118 @@ bool ReadingListModelImpl::CallbackEntryURL(
   return false;
 }
 
+bool ReadingListModelImpl::CallbackEntryReadStatusURL(
+    const GURL& url,
+    base::Callback<void(const ReadingListEntry&, bool read)> callback) const {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  DCHECK(loaded());
+  bool read;
+  const ReadingListEntry* entry = GetMutableEntryFromURL(url, read);
+  if (entry) {
+    callback.Run(*entry, read);
+    return true;
+  }
+  return false;
+}
+
+void ReadingListModelImpl::SyncAddEntry(std::unique_ptr<ReadingListEntry> entry,
+                                        bool read) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  DCHECK(loaded());
+  bool is_entry_read;

[skym, 2016/11/01 18:27:25]

Can you rename to make it clear this is for the old/existing one?

[Olivier, 2016/11/02 14:57:10]

Done.

+  ReadingListEntry* existing_entry =
+      GetMutableEntryFromURL(entry->URL(), is_entry_read);
+  if (!existing_entry) {
+    if (storage_layer_) {
+      storage_layer_->SaveEntry(*entry, read, true);
+    }
+    if (read) {
+      for (auto& observer : observers_)
+        observer.ReadingListWillAddReadEntry(this, *entry);
+      read_->insert(read_->begin(), std::move(*entry));
+    } else {
+      for (auto& observer : observers_)
+        observer.ReadingListWillAddUnreadEntry(this, *entry);
+      has_unseen_ = true;
+      SavePersistentHasUnseen(true);
+      unread_->insert(unread_->begin(), std::move(*entry));
+    }
+    for (auto& observer : observers_)
+      observer.ReadingListDidApplyChanges(this);

[skym, 2016/11/01 18:27:25]

I don't understand what these various observer methods are, but it seems odd
that nothing gets sent to the observers when updating an existing element. Only
on new elements. If this is intentional, can you add a comment?

[Olivier, 2016/11/02 14:57:10]

You are right. Added ReadingListWillMoveEntry

+    return;
+  }
+
+  if (existing_entry->UpdateTime() > entry->UpdateTime()) {
+    // Existing entry is newer thatn sync one. Do not update it.
+    return;
+  }
+
+  // Merge local data in new entry.
+  entry->MergeLocalStateFrom(*existing_entry);
+
+  if (is_entry_read) {
+    auto result = std::find(read_->begin(), read_->end(), *existing_entry);

[skym, 2016/11/01 18:27:25]

There's a lot of std::find calls in here. They're all O(n), right? Is the
assumption that the total number of elements should always be fairly small?

[Olivier, 2016/11/02 14:57:10]

Yes, the number of entries should stay small

+    if (result != read_->end()) {
+      read_->erase(result);

[skym, 2016/11/01 18:27:25]

If |is_entry_read| == |read|, then updating the existing record would be more
efficient. Though keeping the code as simple as possible may be more valuable,
especially since the above find is already O(n).

+    }
+  } else {
+    auto result = std::find(unread_->begin(), unread_->end(), *existing_entry);
+    if (result != unread_->end()) {
+      unread_->erase(result);
+    }
+  }
+
+  if (storage_layer_) {
+    storage_layer_->SaveEntry(*entry, true, read);

[skym, 2016/11/01 18:27:25]

Are the arguments |true| and |read| swapped here?

[skym, 2016/11/01 18:27:25]

Also, kind of weird that the storage call is in the middle here. Is there a
specific reason? I feel like it'd make more sense to keep the |read_| and
|unread_| updating as close together as possible. Unless I'm missing something.

[Olivier, 2016/11/02 14:57:10]

Yes, thanks! Done.

[Olivier, 2016/11/02 14:57:10]

It is there because there is a std::move on the next call.

+  }
+
+  if (read) {
+    read_->push_back(std::move(*entry));
+  } else {
+    unread_->push_back(std::move(*entry));
+  }
+}
+
+void ReadingListModelImpl::SyncRemoveEntry(const GURL& gurl) {
+  RemoveEntryByUrlImpl(gurl, true);
+}
+
 void ReadingListModelImpl::RemoveEntryByUrl(const GURL& url) {
+  RemoveEntryByUrlImpl(url, false);
+}
+
+void ReadingListModelImpl::RemoveEntryByUrlImpl(const GURL& url,
+                                                bool from_sync) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
   const ReadingListEntry entry(url, std::string());
 
-  auto result = std::find(unread_.begin(), unread_.end(), entry);
-  if (result != unread_.end()) {
-    for (auto& observer : observers_) {
+  auto result = std::find(unread_->begin(), unread_->end(), entry);
+  if (result != unread_->end()) {
+    for (auto& observer : observers_)
       observer.ReadingListWillRemoveUnreadEntry(
-          this, std::distance(unread_.begin(), result));
+          this, std::distance(unread_->begin(), result));
+
+    if (storage_layer_) {
+      storage_layer_->RemoveEntry(*result, from_sync);
     }
-    unread_.erase(result);
-    if (storageLayer_ && !IsPerformingBatchUpdates())
-      storageLayer_->SavePersistentUnreadList(unread_);
+    unread_->erase(result);
+
     for (auto& observer : observers_)
       observer.ReadingListDidApplyChanges(this);
     return;
   }
 
-  result = std::find(read_.begin(), read_.end(), entry);
-  if (result != read_.end()) {
-    for (auto& observer : observers_) {
+  result = std::find(read_->begin(), read_->end(), entry);
+  if (result != read_->end()) {
+    for (auto& observer : observers_)
       observer.ReadingListWillRemoveReadEntry(
-          this, std::distance(read_.begin(), result));
+          this, std::distance(read_->begin(), result));
+    if (storage_layer_) {
+      storage_layer_->RemoveEntry(*result, from_sync);
     }
-    read_.erase(result);
-    if (storageLayer_ && !IsPerformingBatchUpdates())
-      storageLayer_->SavePersistentReadList(read_);
+    read_->erase(result);
+
     for (auto& observer : observers_)
       observer.ReadingListDidApplyChanges(this);
     return;
@@ -127,97 +260,103 @@ void ReadingListModelImpl::RemoveEntryByUrl(const GURL& url) {
 const ReadingListEntry& ReadingListModelImpl::AddEntry(
     const GURL& url,
     const std::string& title) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
   RemoveEntryByUrl(url);
   ReadingListEntry entry(url, title);
   for (auto& observer : observers_)
     observer.ReadingListWillAddUnreadEntry(this, entry);
-  unread_.insert(unread_.begin(), std::move(entry));
-  hasUnseen_ = true;
-  if (storageLayer_ && !IsPerformingBatchUpdates()) {
-    storageLayer_->SavePersistentUnreadList(unread_);
-    storageLayer_->SavePersistentHasUnseen(true);
+  has_unseen_ = true;
+  SavePersistentHasUnseen(true);
+  if (storage_layer_) {
+    storage_layer_->SaveEntry(entry, false, false);
   }
+  unread_->insert(unread_->begin(), std::move(entry));
+
   for (auto& observer : observers_)
     observer.ReadingListDidApplyChanges(this);
-  return *unread_.begin();
+  return *unread_->begin();
 }
 
 void ReadingListModelImpl::MarkUnreadByURL(const GURL& url) {
   DCHECK(loaded());
   ReadingListEntry entry(url, std::string());
-  auto result = std::find(read_.begin(), read_.end(), entry);
-  if (result == read_.end())
+  auto result = std::find(read_->begin(), read_->end(), entry);
+  if (result == read_->end())
     return;
 
   for (ReadingListModelObserver& observer : observers_) {
     observer.ReadingListWillMoveEntry(this,
-                                      std::distance(read_.begin(), result));
+                                      std::distance(read_->begin(), result));
   }
 
-  unread_.insert(unread_.begin(), std::move(*result));
-  read_.erase(result);
-
-  if (storageLayer_ && !IsPerformingBatchUpdates()) {
-    storageLayer_->SavePersistentUnreadList(read_);
-    storageLayer_->SavePersistentReadList(unread_);
+  result->MarkEntryUpdated();
+  if (storage_layer_) {
+    storage_layer_->SaveEntry(*result, false, false);
   }
+
+  unread_->insert(unread_->begin(), std::move(*result));
+  read_->erase(result);
+
   for (ReadingListModelObserver& observer : observers_) {
     observer.ReadingListDidApplyChanges(this);
   }
 }
 
 void ReadingListModelImpl::MarkReadByURL(const GURL& url) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
   ReadingListEntry entry(url, std::string());
-  auto result = std::find(unread_.begin(), unread_.end(), entry);
-  if (result == unread_.end())
+  auto result = std::find(unread_->begin(), unread_->end(), entry);
+  if (result == unread_->end())
     return;
 
-  for (auto& observer : observers_) {
+  for (auto& observer : observers_)
     observer.ReadingListWillMoveEntry(this,
-                                      std::distance(unread_.begin(), result));
+                                      std::distance(unread_->begin(), result));
+
+  result->MarkEntryUpdated();
+  if (storage_layer_) {
+    storage_layer_->SaveEntry(*result, true, false);
   }
 
-  read_.insert(read_.begin(), std::move(*result));
-  unread_.erase(result);
+  read_->insert(read_->begin(), std::move(*result));
+  unread_->erase(result);
 
-  if (storageLayer_ && !IsPerformingBatchUpdates()) {
-    storageLayer_->SavePersistentUnreadList(unread_);
-    storageLayer_->SavePersistentReadList(read_);
-  }
   for (auto& observer : observers_)
     observer.ReadingListDidApplyChanges(this);
 }
 
 void ReadingListModelImpl::SetEntryTitle(const GURL& url,
                                          const std::string& title) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
   const ReadingListEntry entry(url, std::string());
 
-  auto result = std::find(unread_.begin(), unread_.end(), entry);
-  if (result != unread_.end()) {
-    for (auto& observer : observers_) {
+  auto result = std::find(unread_->begin(), unread_->end(), entry);
+  if (result != unread_->end()) {
+    for (auto& observer : observers_)
       observer.ReadingListWillUpdateUnreadEntry(
-          this, std::distance(unread_.begin(), result));
-    }
+          this, std::distance(unread_->begin(), result));
     result->SetTitle(title);
-    if (storageLayer_ && !IsPerformingBatchUpdates())
-      storageLayer_->SavePersistentUnreadList(unread_);
+
+    if (storage_layer_) {
+      storage_layer_->SaveEntry(*result, false, false);
+    }
     for (auto& observer : observers_)
       observer.ReadingListDidApplyChanges(this);
     return;
   }
 
-  result = std::find(read_.begin(), read_.end(), entry);
-  if (result != read_.end()) {
-    for (auto& observer : observers_) {
+  result = std::find(read_->begin(), read_->end(), entry);
+  if (result != read_->end()) {
+    for (auto& observer : observers_)
       observer.ReadingListWillUpdateReadEntry(
-          this, std::distance(read_.begin(), result));
-    }
+          this, std::distance(read_->begin(), result));
     result->SetTitle(title);
-    if (storageLayer_ && !IsPerformingBatchUpdates())
-      storageLayer_->SavePersistentReadList(read_);
+    if (storage_layer_) {
+      storage_layer_->SaveEntry(*result, true, false);
+    }
     for (auto& observer : observers_)
       observer.ReadingListDidApplyChanges(this);
     return;
@@ -226,32 +365,33 @@ void ReadingListModelImpl::SetEntryTitle(const GURL& url,
 
 void ReadingListModelImpl::SetEntryDistilledURL(const GURL& url,
                                                 const GURL& distilled_url) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
   const ReadingListEntry entry(url, std::string());
 
-  auto result = std::find(unread_.begin(), unread_.end(), entry);
-  if (result != unread_.end()) {
-    for (auto& observer : observers_) {
+  auto result = std::find(unread_->begin(), unread_->end(), entry);
+  if (result != unread_->end()) {
+    for (auto& observer : observers_)
       observer.ReadingListWillUpdateUnreadEntry(
-          this, std::distance(unread_.begin(), result));
-    }
+          this, std::distance(unread_->begin(), result));
     result->SetDistilledURL(distilled_url);
-    if (storageLayer_ && !IsPerformingBatchUpdates())
-      storageLayer_->SavePersistentUnreadList(unread_);
+    if (storage_layer_) {
+      storage_layer_->SaveEntry(*result, false, false);
+    }
     for (auto& observer : observers_)
       observer.ReadingListDidApplyChanges(this);
     return;
   }
 
-  result = std::find(read_.begin(), read_.end(), entry);
-  if (result != read_.end()) {
-    for (auto& observer : observers_) {
+  result = std::find(read_->begin(), read_->end(), entry);
+  if (result != read_->end()) {
+    for (auto& observer : observers_)
       observer.ReadingListWillUpdateReadEntry(
-          this, std::distance(read_.begin(), result));
-    }
+          this, std::distance(read_->begin(), result));
     result->SetDistilledURL(distilled_url);
-    if (storageLayer_ && !IsPerformingBatchUpdates())
-      storageLayer_->SavePersistentReadList(read_);
+    if (storage_layer_) {
+      storage_layer_->SaveEntry(*result, true, false);
+    }
     for (auto& observer : observers_)
       observer.ReadingListDidApplyChanges(this);
     return;
@@ -261,44 +401,82 @@ void ReadingListModelImpl::SetEntryDistilledURL(const GURL& url,
 void ReadingListModelImpl::SetEntryDistilledState(
     const GURL& url,
     ReadingListEntry::DistillationState state) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(loaded());
   const ReadingListEntry entry(url, std::string());
 
-  auto result = std::find(unread_.begin(), unread_.end(), entry);
-  if (result != unread_.end()) {
-    for (auto& observer : observers_) {
+  auto result = std::find(unread_->begin(), unread_->end(), entry);
+  if (result != unread_->end()) {
+    for (auto& observer : observers_)
       observer.ReadingListWillUpdateUnreadEntry(
-          this, std::distance(unread_.begin(), result));
-    }
+          this, std::distance(unread_->begin(), result));
     result->SetDistilledState(state);
-    if (storageLayer_ && !IsPerformingBatchUpdates())
-      storageLayer_->SavePersistentUnreadList(unread_);
+    if (storage_layer_) {
+      storage_layer_->SaveEntry(*result, false, false);
+    }
     for (auto& observer : observers_)
       observer.ReadingListDidApplyChanges(this);
     return;
   }
 
-  result = std::find(read_.begin(), read_.end(), entry);
-  if (result != read_.end()) {
-    for (auto& observer : observers_) {
+  result = std::find(read_->begin(), read_->end(), entry);
+  if (result != read_->end()) {
+    for (auto& observer : observers_)
       observer.ReadingListWillUpdateReadEntry(
-          this, std::distance(read_.begin(), result));
-    }
+          this, std::distance(read_->begin(), result));
     result->SetDistilledState(state);
-    if (storageLayer_ && !IsPerformingBatchUpdates())
-      storageLayer_->SavePersistentReadList(read_);
+    if (storage_layer_) {
+      storage_layer_->SaveEntry(*result, true, false);
+    }
     for (auto& observer : observers_)
       observer.ReadingListDidApplyChanges(this);
     return;
   }
 };
 
-void ReadingListModelImpl::EndBatchUpdates() {
-  ReadingListModel::EndBatchUpdates();
-  if (IsPerformingBatchUpdates() || !storageLayer_) {
+void ReadingListModelImpl::LeavingBatchUpdates() {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  ReadingListModel::LeavingBatchUpdates();
+  if (storage_layer_) {
+    SavePersistentHasUnseen(has_unseen_);
+    storage_layer_->CommitTransaction();
+    SortEntries();
+  }
+}
+
+void ReadingListModelImpl::EnteringBatchUpdates() {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  if (storage_layer_) {
+    storage_layer_->BeginTransaction();
+  }
+  ReadingListModel::EnteringBatchUpdates();
+}
+
+void ReadingListModelImpl::SavePersistentHasUnseen(bool has_unseen) {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  if (!pref_service_) {
     return;
   }
-  storageLayer_->SavePersistentUnreadList(unread_);
-  storageLayer_->SavePersistentReadList(read_);
-  storageLayer_->SavePersistentHasUnseen(hasUnseen_);
+  pref_service_->SetBoolean(reading_list::prefs::kReadingListHasUnseenEntries,
+                            has_unseen);
+}
+
+bool ReadingListModelImpl::LoadPersistentHasUnseen() {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  if (!pref_service_) {
+    return false;
+  }
+  return pref_service_->GetBoolean(
+      reading_list::prefs::kReadingListHasUnseenEntries);
+}
+
+syncer::ModelTypeService* ReadingListModelImpl::GetModelTypeService() {
+  return storage_layer_->GetModelTypeService();
+}
+
+void ReadingListModelImpl::SortEntries() {
+  std::sort(read_->begin(), read_->end(),
+            ReadingListEntry::CompareEntryUpdateTime);
+  std::sort(unread_->begin(), unread_->end(),
+            ReadingListEntry::CompareEntryUpdateTime);
 }