Add GetEntryFromURL method to the ReadingListModel.

Add GetEntryFromURL method to the ReadingListModel.

BUG=659099
Committed: https://crrev.com/ad74ddb4727198767e957dd95d073969f92bcdb9
Cr-Commit-Position: refs/heads/master@{#427362}

[jif, 2016-10-25 13:00:28]

Description was changed from

==========
Foo.

BUG=
==========

to

==========
Add GetEntryFromURL method to the ReadingListModel.

BUG=None.
==========

[jif, 2016-10-25 13:00:28]

jif@chromium.org changed reviewers:
+ olivierrobin@chromium.org

[jif, 2016-10-25 13:00:38]

ptal

[jif, 2016-10-25 13:00:44]

The CQ bit was checked by jif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 13:01:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 13:09:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 13:09:36]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

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

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:71: auto it =
std::find_if(
DCHECK(loaded())

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:73: [&gurl](const
ReadingListEntry& entry) { return gurl == entry.URL(); });
This should not be needed as there is already a test on entry. Please use the
same test as in CallbackEntryURL()

[Olivier, 2016-10-25 13:11:26]

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_unittest.cc (right):

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_unittest.cc:181:
EXPECT_NE(nullptr, entry1)
;

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_unittest.cc:182:
EXPECT_EQ(entry1_title, entry1->Title());
mark read, then retest?

[gambard, 2016-10-25 13:18:33]

gambard@chromium.org changed reviewers:
+ gambard@chromium.org

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

lgtm after olivier comments but update CL description and bug (with 659099?)

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:73: [&gurl](const
ReadingListEntry& entry) { return gurl == entry.URL(); });
On 2016/10/25 13:10:09, Olivier Robin wrote:
> This should not be needed as there is already a test on entry. Please use the
> same test as in CallbackEntryURL()

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]

jif@chromium.org changed reviewers:
- gambard@chromium.org

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

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:71: auto it =
std::find_if(
On 2016/10/25 13:10:09, Olivier Robin wrote:
> DCHECK(loaded())

Done.

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:73: [&gurl](const
ReadingListEntry& entry) { return gurl == entry.URL(); });
On 2016/10/25 13:10:09, Olivier Robin wrote:
> This should not be needed as there is already a test on entry. Please use the
> same test as in CallbackEntryURL()

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.

[jif, 2016-10-25 13:19:10]

Description was changed from

==========
Add GetEntryFromURL method to the ReadingListModel.

BUG=None.
==========

to

==========
Add GetEntryFromURL method to the ReadingListModel.

BUG= 659099
==========

[Olivier, 2016-10-25 13:22:00]

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:73: [&gurl](const
ReadingListEntry& entry) { return gurl == entry.URL(); });
On 2016/10/25 13:18:44, jif wrote:
> On 2016/10/25 13:10:09, Olivier Robin wrote:
> > This should not be needed as there is already a test on entry. Please use
the
> > same test as in CallbackEntryURL()
> 
> 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.

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

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

jif@google.com changed reviewers:
+ jif@google.com

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

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:73: [&gurl](const
ReadingListEntry& entry) { return gurl == entry.URL(); });
On 2016/10/25 13:21:59, Olivier Robin wrote:
> On 2016/10/25 13:18:44, jif wrote:
> > On 2016/10/25 13:10:09, Olivier Robin wrote:
> > > This should not be needed as there is already a test on entry. Please use
> the
> > > same test as in CallbackEntryURL()
> > 
> > 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.
> 
> I don't think creating one ReadingListEntry is more coaty that creating the
> lambda.

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.

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_unittest.cc (right):

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_unittest.cc:181:
EXPECT_NE(nullptr, entry1)
On 2016/10/25 13:11:26, Olivier Robin wrote:
> ;

Done.

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_unittest.cc:182:
EXPECT_EQ(entry1_title, entry1->Title());
On 2016/10/25 13:11:26, Olivier Robin wrote:
> mark read, then retest?

Done.

[jif-google, 2016-10-25 13:53:23]

The CQ bit was checked by jif@google.com

[jif-google, 2016-10-25 13:53:23]

The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org
Link to the patchset: https://codereview.chromium.org/2436743002/#ps40001
(title: "Addressed comments.")

[jif-google, 2016-10-25 13:53:27]

The CQ bit was unchecked by jif@google.com

[jif, 2016-10-25 13:55:32]

The CQ bit was checked by jif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 13:55:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 14:02:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 14:02:59]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jif, 2016-10-25 15:19:21]

The CQ bit was checked by jif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 15:19:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Olivier, 2016-10-25 15:21:10]

LGTM once test is fixed.

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2436743002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:73: [&gurl](const
ReadingListEntry& entry) { return gurl == entry.URL(); });
On 2016/10/25 13:51:28, jif-google wrote:
> On 2016/10/25 13:21:59, Olivier Robin wrote:
> > On 2016/10/25 13:18:44, jif wrote:
> > > On 2016/10/25 13:10:09, Olivier Robin wrote:
> > > > This should not be needed as there is already a test on entry. Please
use
> > the
> > > > same test as in CallbackEntryURL()
> > > 
> > > 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.
> > 
> > I don't think creating one ReadingListEntry is more coaty that creating the
> > lambda.
> 
> 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.

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.

https://codereview.chromium.org/2436743002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_unittest.cc (right):

https://codereview.chromium.org/2436743002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_unittest.cc:184:
EXPECT_NE(nullptr, entry1);
model->GetEntryFromURL(url)

[commit-bot: I haz the power, 2016-10-25 15:27:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 15:27:39]

Dry run: This issue passed the CQ dry run.

[jif, 2016-10-25 15:36:16]

https://codereview.chromium.org/2436743002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_unittest.cc (right):

https://codereview.chromium.org/2436743002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_unittest.cc:184:
EXPECT_NE(nullptr, entry1);
On 2016/10/25 15:21:09, Olivier Robin wrote:
> model->GetEntryFromURL(url)

Done.

[jif, 2016-10-25 15:36:21]

The CQ bit was checked by jif@chromium.org

[jif, 2016-10-25 15:36:21]

The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org
Link to the patchset: https://codereview.chromium.org/2436743002/#ps60001
(title: "Fixed test.")

[commit-bot: I haz the power, 2016-10-25 15:36:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 15:41:23]

Description was changed from

==========
Add GetEntryFromURL method to the ReadingListModel.

BUG= 659099
==========

to

==========
Add GetEntryFromURL method to the ReadingListModel.

BUG= 659099
==========

[commit-bot: I haz the power, 2016-10-25 15:41:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-25 15:44:57]

Description was changed from

==========
Add GetEntryFromURL method to the ReadingListModel.

BUG= 659099
==========

to

==========
Add GetEntryFromURL method to the ReadingListModel.

BUG= 659099

Committed: https://crrev.com/ad74ddb4727198767e957dd95d073969f92bcdb9
Cr-Commit-Position: refs/heads/master@{#427362}
==========

[commit-bot: I haz the power, 2016-10-25 15:44:58]

Patchset 4 (id:??) landed as
https://crrev.com/ad74ddb4727198767e957dd95d073969f92bcdb9
Cr-Commit-Position: refs/heads/master@{#427362}