Add GetEntryFromURL method to the ReadingListModel.

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 a07f7f2b98851361e0189072cbb15d52b9c7df3f..dba2c4cec65b2f8e6af297a4cdaf633b10d58d5b 100644
--- a/ios/chrome/browser/reading_list/reading_list_model_impl.cc
+++ b/ios/chrome/browser/reading_list/reading_list_model_impl.cc
@@ -54,7 +54,6 @@ void ReadingListModelImpl::ResetUnseenEntries() {
     storageLayer_->SavePersistentHasUnseen(false);
 }
 
-// Returns a specific entry.
 const ReadingListEntry& ReadingListModelImpl::GetUnreadEntryAtIndex(
     size_t index) const {
   DCHECK(loaded());
@@ -67,20 +66,28 @@ const ReadingListEntry& ReadingListModelImpl::GetReadEntryAtIndex(
   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;