Refactor Reading List Model to use URL as key.

components/reading_list/ios/reading_list_model_impl.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_model_impl.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "components/prefs/pref_service.h"
 #include "components/reading_list/ios/reading_list_model_storage.h"
 #include "components/reading_list/ios/reading_list_pref_names.h"
 #include "url/gurl.h"
 
+ReadingListModelImpl::Cache::Cache()
+    : read_entries(std::vector<GURL>()),
+      unread_entries(std::vector<GURL>()),
+      dirty(false) {}
+
+ReadingListModelImpl::Cache::~Cache() {}
+
 ReadingListModelImpl::ReadingListModelImpl()
     : ReadingListModelImpl(nullptr, nullptr) {}
 
 ReadingListModelImpl::ReadingListModelImpl(
     std::unique_ptr<ReadingListModelStorage> storage,
     PrefService* pref_service)
-    : pref_service_(pref_service),
+    : unread_entry_count_(0),
+      read_entry_count_(0),
+      cache_(base::MakeUnique<struct Cache>()),
+      pref_service_(pref_service),
       has_unseen_(false),
       loaded_(false),
       weak_ptr_factory_(this) {
   DCHECK(CalledOnValidThread());
   if (storage) {
     storage_layer_ = std::move(storage);
     storage_layer_->SetReadingListModel(this, this);
   } else {
     loaded_ = true;
-    read_ = base::MakeUnique<ReadingListEntries>();
-    unread_ = base::MakeUnique<ReadingListEntries>();
+    entries_ = base::MakeUnique<ReadingListEntries>();
+    cache_->dirty = true;
   }
   has_unseen_ = GetPersistentHasUnseen();
 }
 
 ReadingListModelImpl::~ReadingListModelImpl() {}
 
 void ReadingListModelImpl::StoreLoaded(
-    std::unique_ptr<ReadingListEntries> unread,
-    std::unique_ptr<ReadingListEntries> read) {
+    std::unique_ptr<ReadingListEntries> entries) {
   DCHECK(CalledOnValidThread());
-  read_ = std::move(read);
-  unread_ = std::move(unread);
+  entries_ = std::move(entries);
+  cache_->dirty = true;
+  for (auto& iterator : *entries_) {
+    if (iterator.second.IsRead()) {
+      read_entry_count_++;
+    } else {
+      unread_entry_count_++;
+    }
+  }
+  DCHECK(read_entry_count_ + unread_entry_count_ == entries_->size());
   loaded_ = true;
-  SortEntries();
   for (auto& observer : observers_)
     observer.ReadingListModelLoaded(this);
 }
 
 void ReadingListModelImpl::Shutdown() {
   DCHECK(CalledOnValidThread());
   for (auto& observer : observers_)
     observer.ReadingListModelBeingDeleted(this);
   loaded_ = false;
 }
 
 bool ReadingListModelImpl::loaded() const {
   DCHECK(CalledOnValidThread());
   return loaded_;
 }
 
+const ReadingListEntries::iterator ReadingListModelImpl::begin() const {
+  return entries_->begin();
+}
+
+const ReadingListEntries::iterator ReadingListModelImpl::end() const {
+  return entries_->end();
+}
+
+size_t ReadingListModelImpl::size() const {
+  DCHECK(CalledOnValidThread());
+  DCHECK(read_entry_count_ + unread_entry_count_ == entries_->size());
+  if (!loaded())
+    return 0;
+  return entries_->size();
+}
+
 size_t ReadingListModelImpl::unread_size() const {
   DCHECK(CalledOnValidThread());
+  DCHECK(read_entry_count_ + unread_entry_count_ == entries_->size());
   if (!loaded())
     return 0;
-  return unread_->size();
+  return unread_entry_count_;
 }
 
 size_t ReadingListModelImpl::read_size() const {
   DCHECK(CalledOnValidThread());
+  DCHECK(read_entry_count_ + unread_entry_count_ == entries_->size());
   if (!loaded())
     return 0;
-  return read_->size();
+  return read_entry_count_;
 }
 
 bool ReadingListModelImpl::HasUnseenEntries() const {
   DCHECK(CalledOnValidThread());
   if (!loaded())
     return false;
-  return unread_size() && has_unseen_;
+  return unread_entry_count_ > 0 && has_unseen_;
 }
 
 void ReadingListModelImpl::ResetUnseenEntries() {
   DCHECK(CalledOnValidThread());
   DCHECK(loaded());
   has_unseen_ = false;
   if (!IsPerformingBatchUpdates())
     SetPersistentHasUnseen(false);
 }
 
+const ReadingListEntry* ReadingListModelImpl::GetEntryByURL(
+    const GURL& gurl) const {
+  DCHECK(CalledOnValidThread());
+  DCHECK(loaded());
+  return GetMutableEntryFromURL(gurl);
+}
+
+const ReadingListEntry& ReadingListModelImpl::GetReadEntryAtIndex(
+    size_t index) const {
+  DCHECK(CalledOnValidThread());
+  DCHECK(loaded());
+  DCHECK(index < read_entry_count_);
+  if (cache_->dirty) {
+    RebuildIndex();
+  }
+  return *GetEntryByURL(cache_->read_entries[index]);
+}
+
 const ReadingListEntry& ReadingListModelImpl::GetUnreadEntryAtIndex(
     size_t index) const {
   DCHECK(CalledOnValidThread());
   DCHECK(loaded());
-  return unread_->at(index);
+  DCHECK(index < unread_entry_count_);
+  if (cache_->dirty) {
+    RebuildIndex();
+  }
+  return *GetEntryByURL(cache_->unread_entries[index]);
 }
 
-const ReadingListEntry& ReadingListModelImpl::GetReadEntryAtIndex(
-    size_t index) const {
+void ReadingListModelImpl::RebuildIndex() const {
   DCHECK(CalledOnValidThread());
   DCHECK(loaded());
-  return read_->at(index);
-}
-
-const ReadingListEntry* ReadingListModelImpl::GetEntryFromURL(
-    const GURL& gurl,
-    bool* read) const {
-  DCHECK(CalledOnValidThread());
-  DCHECK(loaded());
-  return GetMutableEntryFromURL(gurl, read);
+  if (!cache_->dirty) {
+    return;
+  }
+  cache_->dirty = false;
+  cache_->read_entries.clear();
+  cache_->unread_entries.clear();
+  for (auto& iterator : *entries_) {
+    if (iterator.second.IsRead()) {
+      cache_->read_entries.push_back(iterator.first);
+    } else {
+      cache_->unread_entries.push_back(iterator.first);
+    }
+  }
+  DCHECK(read_entry_count_ == cache_->read_entries.size());
+  DCHECK(unread_entry_count_ == cache_->unread_entries.size());
+  std::sort(cache_->read_entries.begin(), cache_->read_entries.end(),
+            [this](const GURL& left_url, const GURL& right_url) {
+              return this->entries_->at(left_url).UpdateTime() >

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

Note:
if this were not code that was going to be deleted, I'd prefer:
(*this->entries_)[left_url].UpdateTime
because there's no need to check the bounds (which at() does).

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

You cannot use operator[] on ReadingListEntries.

+                     this->entries_->at(right_url).UpdateTime();
+            });
+  std::sort(cache_->unread_entries.begin(), cache_->unread_entries.end(),
+            [this](const GURL& left_url, const GURL& right_url) {
+              return this->entries_->at(left_url).UpdateTime() >
+                     this->entries_->at(right_url).UpdateTime();
+            });
 }
 
 ReadingListEntry* ReadingListModelImpl::GetMutableEntryFromURL(
-    const GURL& gurl,
-    bool* read) const {
+    const GURL& url) const {
   DCHECK(CalledOnValidThread());
   DCHECK(loaded());
-  bool is_read;
-  ReadingListEntry entry(gurl, std::string());
-  auto it = std::find(read_->begin(), read_->end(), entry);
-  is_read = true;
-  if (it == read_->end()) {
-    it = std::find(unread_->begin(), unread_->end(), entry);
-    is_read = false;
-    if (it == unread_->end())
-      return nullptr;
+  auto iterator = entries_->find(url);
+  if (iterator == entries_->end()) {
+    return nullptr;
   }
-  if (read) {
-    *read = is_read;
-  }
-  return &(*it);
+  return &(iterator->second);
 }
 
-bool ReadingListModelImpl::CallbackEntryURL(
-    const GURL& url,
-    base::Callback<void(const ReadingListEntry&)> callback) const {
-  DCHECK(CalledOnValidThread());
-  DCHECK(loaded());
-  const ReadingListEntry* entry = GetMutableEntryFromURL(url, nullptr);
-  if (entry) {
-    callback.Run(*entry);
-    return true;
-  }
-  return false;
-}
-
-void ReadingListModelImpl::MoveEntryFrom(ReadingListEntries* entries,
-                                         const ReadingListEntry& entry,
-                                         bool read) {
-  auto result = std::find(entries->begin(), entries->end(), entry);
-  DCHECK(result != entries->end());
-  int index = std::distance(entries->begin(), result);
-  for (auto& observer : observers_)
-    observer.ReadingListWillMoveEntry(this, index, read);
-  entries->erase(result);
-}
-
-void ReadingListModelImpl::SyncAddEntry(std::unique_ptr<ReadingListEntry> entry,
-                                        bool read) {
+void ReadingListModelImpl::SyncAddEntry(
+    std::unique_ptr<ReadingListEntry> entry) {
   DCHECK(CalledOnValidThread());
   DCHECK(loaded());
   // entry must not already exist.
-  DCHECK(GetMutableEntryFromURL(entry->URL(), nullptr) == nullptr);
-  if (read) {
-    for (auto& observer : observers_)
-      observer.ReadingListWillAddReadEntry(this, *entry);
-    read_->insert(read_->begin(), std::move(*entry));
+  DCHECK(GetMutableEntryFromURL(entry->URL()) == nullptr);
+  for (auto& observer : observers_)
+    observer.ReadingListWillAddEntry(this, *entry);
+  if (entry->IsRead()) {
+    read_entry_count_++;
   } else {
-    for (auto& observer : observers_)
-      observer.ReadingListWillAddUnreadEntry(this, *entry);
-    has_unseen_ = true;
+    unread_entry_count_++;
     SetPersistentHasUnseen(true);
-    unread_->insert(unread_->begin(), std::move(*entry));
+  }
+  GURL url = entry->URL();

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

const GURL& url = entry->URL();

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

I need it for after the move, so I think I need a copy..

[jif, 2016/11/28 18:15:31]

oh wow, did not see that.

+  entries_->insert(std::make_pair(url, std::move(*entry)));

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

(*entries_)[url] = std::move(*entry); ?

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

You cannot use operator[] on entries.

[jif, 2016/11/28 18:15:31]

Acknowledged.

+  cache_->dirty = true;
+  for (auto& observer : observers_) {
+    observer.ReadingListDidAddEntry(this, url);
+    observer.ReadingListDidApplyChanges(this);
+  }
+}
+
+ReadingListEntry* ReadingListModelImpl::SyncMergeEntry(
+    std::unique_ptr<ReadingListEntry> entry) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(loaded());
+  ReadingListEntry* existing_entry = GetMutableEntryFromURL(entry->URL());
+  DCHECK(existing_entry);
+  DCHECK(existing_entry->UpdateTime() < entry->UpdateTime());
+
+  GURL url = entry->URL();

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

const GURL& ?

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

I need it for after the move, so I think I need a copy..

[jif, 2016/11/28 18:15:31]

Acknowledged.

+
+  for (auto& observer : observers_)
+    observer.ReadingListWillMoveEntry(this, url);
+
+  if (existing_entry->IsRead()) {
+    read_entry_count_--;
+  } else {
+    unread_entry_count_--;
+  }
+  // Merge local data in new entry.
+  entry->MergeLocalStateFrom(*existing_entry);
+
+  entries_->find(url)->second = std::move(*entry);

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

(*entries_)[url] = std::move(*entry); ?

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

You cannot use operator[] on entries.

[jif, 2016/11/28 18:15:31]

Acknowledged.

+  cache_->dirty = true;
+
+  existing_entry = GetMutableEntryFromURL(url);
+  if (existing_entry->IsRead()) {
+    read_entry_count_++;
+  } else {
+    unread_entry_count_++;
   }
   for (auto& observer : observers_)
     observer.ReadingListDidApplyChanges(this);
-}
-
-ReadingListEntry* ReadingListModelImpl::SyncMergeEntry(
-    std::unique_ptr<ReadingListEntry> entry,
-    bool read) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(loaded());
-  bool is_existing_entry_read;
-  ReadingListEntry* existing_entry =
-      GetMutableEntryFromURL(entry->URL(), &is_existing_entry_read);
-
-  DCHECK(existing_entry);
-  DCHECK(existing_entry->UpdateTime() < entry->UpdateTime());
-
-  // Merge local data in new entry.
-  entry->MergeLocalStateFrom(*existing_entry);
-
-  if (is_existing_entry_read) {
-    MoveEntryFrom(read_.get(), *existing_entry, true);
-  } else {
-    MoveEntryFrom(unread_.get(), *existing_entry, false);
-  }
-
-  ReadingListEntries::iterator added_iterator;
-  if (read) {
-    read_->push_back(std::move(*entry));
-    added_iterator = read_->end() - 1;
-  } else {
-    unread_->push_back(std::move(*entry));
-    added_iterator = unread_->end() - 1;
-  }
-  for (auto& observer : observers_)
-    observer.ReadingListDidApplyChanges(this);
-
-  ReadingListEntry& merged_entry = *added_iterator;
-  return &merged_entry;
+  return existing_entry;
 }
 
 void ReadingListModelImpl::SyncRemoveEntry(const GURL& url) {
   RemoveEntryByURLImpl(url, true);
 }
 
 void ReadingListModelImpl::RemoveEntryByURL(const GURL& url) {
   RemoveEntryByURLImpl(url, false);
 }
 
 void ReadingListModelImpl::RemoveEntryByURLImpl(const GURL& url,
                                                 bool from_sync) {
   DCHECK(CalledOnValidThread());
   DCHECK(loaded());
-  const ReadingListEntry entry(url, std::string());
+  const ReadingListEntry* entry = GetEntryByURL(url);
+  if (!entry)
+    return;
 
-  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));
+  for (auto& observer : observers_)
+    observer.ReadingListWillRemoveEntry(this, url);
 
-    if (storage_layer_ && !from_sync) {
-      storage_layer_->RemoveEntry(*result);
-    }
-    unread_->erase(result);
-
-    for (auto& observer : observers_)
-      observer.ReadingListDidApplyChanges(this);
-    return;
+  if (storage_layer_ && !from_sync) {
+    storage_layer_->RemoveEntry(*entry);
   }
-
-  result = std::find(read_->begin(), read_->end(), entry);
-  if (result != read_->end()) {
-    for (auto& observer : observers_)
-      observer.ReadingListWillRemoveReadEntry(
-          this, std::distance(read_->begin(), result));
-    if (storage_layer_ && !from_sync) {
-      storage_layer_->RemoveEntry(*result);
-    }
-    read_->erase(result);
-
-    for (auto& observer : observers_)
-      observer.ReadingListDidApplyChanges(this);
-    return;
+  if (entry->IsRead()) {
+    read_entry_count_--;
+  } else {
+    unread_entry_count_--;
   }
+  entries_->erase(url);
+  cache_->dirty = true;
+  for (auto& observer : observers_)
+    observer.ReadingListDidApplyChanges(this);
 }
 
 const ReadingListEntry& ReadingListModelImpl::AddEntry(
     const GURL& url,
     const std::string& title) {
   DCHECK(CalledOnValidThread());
   DCHECK(loaded());
   RemoveEntryByURL(url);
 
   std::string trimmedTitle(title);
   base::TrimWhitespaceASCII(trimmedTitle, base::TRIM_ALL, &trimmedTitle);
 
   ReadingListEntry entry(url, trimmedTitle);
   for (auto& observer : observers_)
-    observer.ReadingListWillAddUnreadEntry(this, entry);
+    observer.ReadingListWillAddEntry(this, entry);
   has_unseen_ = true;
   SetPersistentHasUnseen(true);
+  entries_->insert(std::make_pair(url, std::move(entry)));

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

(*entries)[url] = std::move(entry);

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

You cannot use operator[] on entries.

[jif, 2016/11/28 18:15:31]

Acknowledged.

+  unread_entry_count_++;
   if (storage_layer_) {
-    storage_layer_->SaveEntry(entry, false);
+    storage_layer_->SaveEntry(*GetEntryByURL(url));
   }
-  unread_->insert(unread_->begin(), std::move(entry));
+  cache_->dirty = true;
 
-  for (auto& observer : observers_)
+  for (auto& observer : observers_) {
+    observer.ReadingListDidAddEntry(this, url);
     observer.ReadingListDidApplyChanges(this);
-  return *unread_->begin();
+  }
+
+  return entries_->at(url);

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

return (*entries)[url];

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

You cannot use operator[] on entries.

[jif, 2016/11/28 18:15:31]

Acknowledged.

+}
+
+void ReadingListModelImpl::MarkReadByURL(const GURL& url) {
+  return SetReadStatus(url, true);
 }
 
 void ReadingListModelImpl::MarkUnreadByURL(const GURL& url) {
+  return SetReadStatus(url, false);
+}
+
+void ReadingListModelImpl::SetReadStatus(const GURL& url, bool read) {
   DCHECK(CalledOnValidThread());
   DCHECK(loaded());
-  ReadingListEntry entry(url, std::string());
-  auto result = std::find(read_->begin(), read_->end(), entry);
-  if (result == read_->end())
+  if (!entries_->count(url)) {
     return;
-
+  }
+  ReadingListEntry& entry = entries_->at(url);

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

(*entries)[url]

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

You cannot use operator[] on entries.

[jif, 2016/11/28 18:15:31]

Acknowledged.

+  if (entry.IsRead() == read) {
+    return;
+  }
   for (ReadingListModelObserver& observer : observers_) {
-    observer.ReadingListWillMoveEntry(
-        this, std::distance(read_->begin(), result), true);
+    observer.ReadingListWillMoveEntry(this, url);
   }
-
-  result->MarkEntryUpdated();
+  if (read) {
+    read_entry_count_++;
+    unread_entry_count_--;
+  } else {
+    unread_entry_count_++;
+    read_entry_count_--;
+  }
+  entry.SetRead(read);
+  entry.MarkEntryUpdated();
+  cache_->dirty = true;
   if (storage_layer_) {
-    storage_layer_->SaveEntry(*result, false);
+    storage_layer_->SaveEntry(entry);
   }
-
-  unread_->insert(unread_->begin(), std::move(*result));
-  read_->erase(result);
-
   for (ReadingListModelObserver& observer : observers_) {
     observer.ReadingListDidApplyChanges(this);
   }
 }
 
-void ReadingListModelImpl::MarkReadByURL(const GURL& url) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(loaded());
-  ReadingListEntry entry(url, std::string());
-  auto result = std::find(unread_->begin(), unread_->end(), entry);
-  if (result == unread_->end())
-    return;
-
-  for (auto& observer : observers_)
-    observer.ReadingListWillMoveEntry(
-        this, std::distance(unread_->begin(), result), false);
-
-  result->MarkEntryUpdated();
-  if (storage_layer_) {
-    storage_layer_->SaveEntry(*result, true);
-  }
-
-  read_->insert(read_->begin(), std::move(*result));
-  unread_->erase(result);
-
-  for (auto& observer : observers_)
-    observer.ReadingListDidApplyChanges(this);
-}
-
 void ReadingListModelImpl::SetEntryTitle(const GURL& url,
                                          const std::string& title) {
   DCHECK(CalledOnValidThread());
   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_)
-      observer.ReadingListWillUpdateUnreadEntry(
-          this, std::distance(unread_->begin(), result));
-    result->SetTitle(title);
-
-    if (storage_layer_) {
-      storage_layer_->SaveEntry(*result, false);
-    }
-    for (auto& observer : observers_)
-      observer.ReadingListDidApplyChanges(this);
+  if (!entries_->count(url)) {

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

How about we search the entry only once, as opposed to twice (with count() and
at())?

auto it = entries_->find(url);
if (it == entries_.end())
 return;

ReadingListEntry& entry = truchmuche(it);

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

Done.

+    return;
+  }
+  ReadingListEntry& entry = entries_->at(url);
+  if (entry.Title() == title) {
     return;
   }
 
-  result = std::find(read_->begin(), read_->end(), entry);
-  if (result != read_->end()) {
-    for (auto& observer : observers_)
-      observer.ReadingListWillUpdateReadEntry(
-          this, std::distance(read_->begin(), result));
-    result->SetTitle(title);
-    if (storage_layer_) {
-      storage_layer_->SaveEntry(*result, true);
-    }
-    for (auto& observer : observers_)
-      observer.ReadingListDidApplyChanges(this);
-    return;
+  for (ReadingListModelObserver& observer : observers_) {
+    observer.ReadingListWillUpdateEntry(this, url);
+  }
+  entry.SetTitle(title);
+  if (storage_layer_) {
+    storage_layer_->SaveEntry(entry);
+  }
+  for (ReadingListModelObserver& observer : observers_) {
+    observer.ReadingListDidApplyChanges(this);
   }
 }
 
 void ReadingListModelImpl::SetEntryDistilledPath(
     const GURL& url,
     const base::FilePath& distilled_path) {
   DCHECK(CalledOnValidThread());
   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_)
-      observer.ReadingListWillUpdateUnreadEntry(
-          this, std::distance(unread_->begin(), result));
-    result->SetDistilledPath(distilled_path);
-    if (storage_layer_) {
-      storage_layer_->SaveEntry(*result, false);
-    }
-    for (auto& observer : observers_)
-      observer.ReadingListDidApplyChanges(this);
+  if (!entries_->count(url)) {

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

same trick: only search for the entry once by using find().

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

Done.

+    return;
+  }
+  ReadingListEntry& entry = entries_->at(url);
+  if (entry.DistilledState() == ReadingListEntry::PROCESSED &&
+      entry.DistilledPath() == distilled_path) {
     return;
   }
 
-  result = std::find(read_->begin(), read_->end(), entry);
-  if (result != read_->end()) {
-    for (auto& observer : observers_)
-      observer.ReadingListWillUpdateReadEntry(
-          this, std::distance(read_->begin(), result));
-    result->SetDistilledPath(distilled_path);
-    if (storage_layer_) {
-      storage_layer_->SaveEntry(*result, true);
-    }
-    for (auto& observer : observers_)
-      observer.ReadingListDidApplyChanges(this);
-    return;
+  for (ReadingListModelObserver& observer : observers_) {
+    observer.ReadingListWillUpdateEntry(this, url);
+  }
+  entry.SetDistilledPath(distilled_path);
+  if (storage_layer_) {
+    storage_layer_->SaveEntry(entry);
+  }
+  for (ReadingListModelObserver& observer : observers_) {
+    observer.ReadingListDidApplyChanges(this);
   }
 }
 
 void ReadingListModelImpl::SetEntryDistilledState(
     const GURL& url,
     ReadingListEntry::DistillationState state) {
   DCHECK(CalledOnValidThread());
   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_)
-      observer.ReadingListWillUpdateUnreadEntry(
-          this, std::distance(unread_->begin(), result));
-    result->SetDistilledState(state);
-    if (storage_layer_) {
-      storage_layer_->SaveEntry(*result, false);
-    }
-    for (auto& observer : observers_)
-      observer.ReadingListDidApplyChanges(this);
+  if (!entries_->count(url)) {

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

search for the entry once by using find().

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

Done.

+    return;
+  }
+  ReadingListEntry& entry = entries_->at(url);
+  if (entry.DistilledState() == state) {
     return;
   }
 
-  result = std::find(read_->begin(), read_->end(), entry);
-  if (result != read_->end()) {
-    for (auto& observer : observers_)
-      observer.ReadingListWillUpdateReadEntry(
-          this, std::distance(read_->begin(), result));
-    result->SetDistilledState(state);
-    if (storage_layer_) {
-      storage_layer_->SaveEntry(*result, true);
-    }
-    for (auto& observer : observers_)
-      observer.ReadingListDidApplyChanges(this);
-    return;
+  for (ReadingListModelObserver& observer : observers_) {
+    observer.ReadingListWillUpdateEntry(this, url);
   }
-};
+  entry.SetDistilledState(state);
+  if (storage_layer_) {
+    storage_layer_->SaveEntry(entry);
+  }
+  for (ReadingListModelObserver& observer : observers_) {
+    observer.ReadingListDidApplyChanges(this);
+  }
+}
 
 std::unique_ptr<ReadingListModel::ScopedReadingListBatchUpdate>
 ReadingListModelImpl::CreateBatchToken() {
   return base::MakeUnique<ReadingListModelImpl::ScopedReadingListBatchUpdate>(
       this);
 }
 
 ReadingListModelImpl::ScopedReadingListBatchUpdate::
     ScopedReadingListBatchUpdate(ReadingListModelImpl* model)
     : ReadingListModel::ScopedReadingListBatchUpdate::
           ScopedReadingListBatchUpdate(model) {
   if (model->StorageLayer()) {
     storage_token_ = model->StorageLayer()->EnsureBatchCreated();
   }
 }
 
 ReadingListModelImpl::ScopedReadingListBatchUpdate::
     ~ScopedReadingListBatchUpdate() {
   storage_token_.reset();
 }
 
 void ReadingListModelImpl::LeavingBatchUpdates() {
   DCHECK(CalledOnValidThread());
   if (storage_layer_) {
     SetPersistentHasUnseen(has_unseen_);
-    SortEntries();
   }
   ReadingListModel::LeavingBatchUpdates();
 }
 
 void ReadingListModelImpl::EnteringBatchUpdates() {
   DCHECK(CalledOnValidThread());
   ReadingListModel::EnteringBatchUpdates();
 }
 
 void ReadingListModelImpl::SetPersistentHasUnseen(bool has_unseen) {
@@ skipping 14 matching lines @@
       reading_list::prefs::kReadingListHasUnseenEntries);
 }
 
 syncer::ModelTypeSyncBridge* ReadingListModelImpl::GetModelTypeSyncBridge() {
   DCHECK(loaded());
   if (!storage_layer_)
     return nullptr;
   return storage_layer_->GetModelTypeSyncBridge();
 }
 
-void ReadingListModelImpl::SortEntries() {
-  DCHECK(CalledOnValidThread());
-  DCHECK(loaded());
-  std::sort(read_->begin(), read_->end(),
-            ReadingListEntry::CompareEntryUpdateTime);
-  std::sort(unread_->begin(), unread_->end(),
-            ReadingListEntry::CompareEntryUpdateTime);
-}
-
 ReadingListModelStorage* ReadingListModelImpl::StorageLayer() {
   return storage_layer_.get();
 }