Add GetEntryFromURL method to the ReadingListModel.

ios/chrome/browser/reading_list/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 "ios/chrome/browser/reading_list/reading_list_model_impl.h"
 
 #include "ios/chrome/browser/reading_list/reading_list_model_storage.h"
 #include "url/gurl.h"
 
 ReadingListModelImpl::ReadingListModelImpl() : ReadingListModelImpl(NULL) {}
@@ skipping 36 matching lines @@
   return unread_size() && hasUnseen_;
 }
 
 void ReadingListModelImpl::ResetUnseenEntries() {
   DCHECK(loaded());
   hasUnseen_ = false;
   if (storageLayer_ && !IsPerformingBatchUpdates())
     storageLayer_->SavePersistentHasUnseen(false);
 }
 
-// Returns a specific entry.
 const ReadingListEntry& ReadingListModelImpl::GetUnreadEntryAtIndex(
     size_t index) const {
   DCHECK(loaded());
   return unread_[index];
 }
 
 const ReadingListEntry& ReadingListModelImpl::GetReadEntryAtIndex(
     size_t index) const {
   DCHECK(loaded());
   return read_[index];
 }
 
+const ReadingListEntry* ReadingListModelImpl::GetEntryFromURL(
+    const GURL& gurl) const {
+  auto it = std::find_if(

[Olivier, 2016/10/25 13:10:09]

DCHECK(loaded())

[jif, 2016/10/25 13:18:45]

Done.

+      read_.begin(), read_.end(),
+      [&gurl](const ReadingListEntry& entry) { return gurl == entry.URL(); });

[Olivier, 2016/10/25 13:10:09]

This should not be needed as there is already a test on entry. Please use the
same test as in CallbackEntryURL()

[gambard, 2016/10/25 13:18:34]

I don't mind lambda but we already have a == operator for ReadingListEntry. I
expected a different equal function as you were using a custom lambda.
It is also making a very long line.

[jif, 2016/10/25 13:18:44]

CallbackEntryURL requires creating a fake ReadingListEntry (which is not super
lightweight). I think this sucks.

You could argue that the duplication of the lambda in |GetEntryFromURL| is bad,
but eventually the entries will be in only one container, so the std::find_if
will be called only once.

[Olivier, 2016/10/25 13:21:59]

I don't think creating one ReadingListEntry is more coaty that creating the
lambda.

[jif-google, 2016/10/25 13:51:28]

The lambda would be compiled as a struct containing one reference, and a ()
method.
Initializing the lambda would be equivalent to initializing a reference. Passing
the lambda would be equivalent to passing a reference.

On the other hand, creating the fake ReadingListEntry requires (at least) a copy
of the |gurl|.

Anyway, the performance difference is not very relevant.
What concerns me is that I don't think that semantically we are doing the right
thing:
We are supposed to search for an entry that has the URL |gurl|, but instead we
are searching for an entry that is equal to an new entry whose url is |gurl|.

That being said, I don't care, and I'll just use std::find.

[Olivier, 2016/10/25 15:21:09]

IMO, find_if should be used if you want to find an object by some other criteria
than the key.
Example: find_if(entry.title() = title).

Here, we use the key. It is an intrinsic property of the ReadingListEntry.

+  if (it == read_.end()) {
+    it = std::find_if(
+        unread_.begin(), unread_.end(),
+        [&gurl](const ReadingListEntry& entry) { return gurl == entry.URL(); });
+    if (it == unread_.end())
+      return nullptr;
+  }
+  return &(*it);
+}
+
 bool ReadingListModelImpl::CallbackEntryURL(
     const GURL& url,
     base::Callback<void(const ReadingListEntry&)> callback) const {
   DCHECK(loaded());
-  ReadingListEntry entry(url, std::string());
-  auto resultUnread = std::find(unread_.begin(), unread_.end(), entry);
-  if (resultUnread != unread_.end()) {
-    callback.Run(*resultUnread);
-    return true;
-  }
-
-  auto resultRead = std::find(read_.begin(), read_.end(), entry);
-  if (resultRead != read_.end()) {
-    callback.Run(*resultRead);
+  const ReadingListEntry* entry = GetEntryFromURL(url);
+  if (entry) {
+    callback.Run(*entry);
     return true;
   }
   return false;
 }
 
 void ReadingListModelImpl::RemoveEntryByUrl(const GURL& url) {
   DCHECK(loaded());
   const ReadingListEntry entry(url, std::string());
 
   auto result = std::find(unread_.begin(), unread_.end(), entry);
@@ skipping 196 matching lines @@
 
 void ReadingListModelImpl::EndBatchUpdates() {
   ReadingListModel::EndBatchUpdates();
   if (IsPerformingBatchUpdates() || !storageLayer_) {
     return;
   }
   storageLayer_->SavePersistentUnreadList(unread_);
   storageLayer_->SavePersistentReadList(read_);
   storageLayer_->SavePersistentHasUnseen(hasUnseen_);
 }