Store the distilled_url in Reading List entry in Reading List on iOS.

Store the distilled_url in Reading List entry in Reading List on iOS.

The URL of the distilled page is not necessarily the URL of the Reading
List entry (in case of redirection).
Store the URL of the page that was distilled in the database.
This CL adds the field in ReadingListEntry.
This field will be used in a next CL.

BUG=682666
Review-Url: https://codereview.chromium.org/2647763005
Cr-Commit-Position: refs/heads/master@{#445063}
Committed: https://chromium.googlesource.com/chromium/src/+/695d0c65669b7af00f8daec96d497519c0e0cb46

[Olivier, 2017-01-20 08:50:14]

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

[Olivier, 2017-01-20 08:50:20]

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

[commit-bot: I haz the power, 2017-01-20 08:50:34]

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

[Olivier, 2017-01-20 09:02:39]

Description was changed from

==========
Store the distilled_url in Reading List entry.

The URL of the distilled page is not necessarily the URL of the Reading
List entry (incase of redirection).
Store the URL of the page that was distilled in the database.
This CL adds the field in ReadingListEntry.
This field will be used in a next CL.

BUG=682666
==========

to

==========
Store the distilled_url in Reading List entry in Reading List on iOS.

The URL of the distilled page is not necessarily the URL of the Reading
List entry (incase of redirection).
Store the URL of the page that was distilled in the database.
This CL adds the field in ReadingListEntry.
This field will be used in a next CL.

BUG=682666
==========

[commit-bot: I haz the power, 2017-01-20 09:34:47]

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

[commit-bot: I haz the power, 2017-01-20 09:34:47]

Dry run: This issue passed the CQ dry run.

[Olivier, 2017-01-20 09:49:29]

Description was changed from

==========
Store the distilled_url in Reading List entry in Reading List on iOS.

The URL of the distilled page is not necessarily the URL of the Reading
List entry (incase of redirection).
Store the URL of the page that was distilled in the database.
This CL adds the field in ReadingListEntry.
This field will be used in a next CL.

BUG=682666
==========

to

==========
Store the distilled_url in Reading List entry in Reading List on iOS.

The URL of the distilled page is not necessarily the URL of the Reading
List entry (in case of redirection).
Store the URL of the page that was distilled in the database.
This CL adds the field in ReadingListEntry.
This field will be used in a next CL.

BUG=682666
==========

[gambard, 2017-01-20 14:01:25]

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
File components/reading_list/ios/reading_list_entry.cc (right):

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_entry.cc:199: DCHECK(!path.empty());
Maybe DCHECK(distilled_url.is_valid());?

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_entry.cc:321: GURL distilled_url = url;
Do you want to initialize it to url?
Why not empty init?

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model.h:121: virtual void
SetEntryDistilledInfo(const GURL& url,
Put it in the same order than the SetEntryDistilledPath to be coherent with the
implementation.

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
File components/reading_list/ios/reading_list_model_unittest.mm (right):

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model_unittest.mm:373:
TEST_F(ReadingListModelTest, SyncMergeEntry) {
Maybe out of scope, but can you add comments to all the tests in this file?
It is hard to know what is tested in each function.

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model_unittest.mm:616:
EXPECT_EQ(base::FilePath("distilled/page.html"), entry.DistilledPath());
Add test for the distilled url.

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model_unittest.mm:656:
EXPECT_EQ(base::FilePath("distilled/page.html"), entry->DistilledPath());
Add test for distilled url.

https://codereview.chromium.org/2647763005/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm
(left):

https://codereview.chromium.org/2647763005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm:90:
// Tests that loading an online version of an entry.
Can you update this comments with more details?

https://codereview.chromium.org/2647763005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm:113:
// Tests that loading a distilled version of an entry from a commited entry.
Same

https://codereview.chromium.org/2647763005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm:138:
// Tests that loading a distilled version of an entry.
Same

[Olivier, 2017-01-20 14:43:51]

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
File components/reading_list/ios/reading_list_entry.cc (right):

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_entry.cc:199: DCHECK(!path.empty());
On 2017/01/20 14:01:24, gambard wrote:
> Maybe DCHECK(distilled_url.is_valid());?

Done.

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_entry.cc:321: GURL distilled_url = url;
On 2017/01/20 14:01:24, gambard wrote:
> Do you want to initialize it to url?
> Why not empty init?

It was to handle existing entry.
But actually, I handle the case already in offline_url_utils.

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model.h:121: virtual void
SetEntryDistilledInfo(const GURL& url,
On 2017/01/20 14:01:24, gambard wrote:
> Put it in the same order than the SetEntryDistilledPath to be coherent with
the
> implementation.

Not sure I understand.
Do you mean change order of methods declaration?
I sorted reading_list_model_impl.h as the comment above is relative to both 
SetEntryTitle and SetEntryDistilledState.
If this is something else, cannot clarify the comment?

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
File components/reading_list/ios/reading_list_model_unittest.mm (right):

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model_unittest.mm:373:
TEST_F(ReadingListModelTest, SyncMergeEntry) {
On 2017/01/20 14:01:25, gambard wrote:
> Maybe out of scope, but can you add comments to all the tests in this file?
> It is hard to know what is tested in each function.

Done.

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model_unittest.mm:616:
EXPECT_EQ(base::FilePath("distilled/page.html"), entry.DistilledPath());
On 2017/01/20 14:01:25, gambard wrote:
> Add test for the distilled url.

Done.

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model_unittest.mm:656:
EXPECT_EQ(base::FilePath("distilled/page.html"), entry->DistilledPath());
On 2017/01/20 14:01:24, gambard wrote:
> Add test for distilled url.

Done.

https://codereview.chromium.org/2647763005/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm
(left):

https://codereview.chromium.org/2647763005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm:90:
// Tests that loading an online version of an entry.
On 2017/01/20 14:01:25, gambard wrote:
> Can you update this comments with more details?

Done.

https://codereview.chromium.org/2647763005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm:113:
// Tests that loading a distilled version of an entry from a commited entry.
On 2017/01/20 14:01:25, gambard wrote:
> Same

Done.

https://codereview.chromium.org/2647763005/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm:138:
// Tests that loading a distilled version of an entry.
On 2017/01/20 14:01:25, gambard wrote:
> Same

Done.

[gambard, 2017-01-20 14:53:32]

lgtm

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2647763005/diff/1/components/reading_list/ios...
components/reading_list/ios/reading_list_model.h:121: virtual void
SetEntryDistilledInfo(const GURL& url,
On 2017/01/20 14:43:51, Olivier Robin wrote:
> On 2017/01/20 14:01:24, gambard wrote:
> > Put it in the same order than the SetEntryDistilledPath to be coherent with
> the
> > implementation.
> 
> Not sure I understand.
> Do you mean change order of methods declaration?
> I sorted reading_list_model_impl.h as the comment above is relative to both 
> SetEntryTitle and SetEntryDistilledState.
> If this is something else, cannot clarify the comment?

Order of methods declaration, sorry.
Your change was what I expected, thanks!

[jif, 2017-01-20 15:01:09]

lgtm

[Olivier, 2017-01-20 15:29:18]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2017-01-20 15:29:30]

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, 2017-01-20 15:42:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484926158592970,
"parent_rev": "ea33b332c6d28c050b657baada979a22ae5ad3a6", "commit_rev":
"695d0c65669b7af00f8daec96d497519c0e0cb46"}

[commit-bot: I haz the power, 2017-01-20 15:43:29]

Description was changed from

==========
Store the distilled_url in Reading List entry in Reading List on iOS.

The URL of the distilled page is not necessarily the URL of the Reading
List entry (in case of redirection).
Store the URL of the page that was distilled in the database.
This CL adds the field in ReadingListEntry.
This field will be used in a next CL.

BUG=682666
==========

to

==========
Store the distilled_url in Reading List entry in Reading List on iOS.

The URL of the distilled page is not necessarily the URL of the Reading
List entry (in case of redirection).
Store the URL of the page that was distilled in the database.
This CL adds the field in ReadingListEntry.
This field will be used in a next CL.

BUG=682666

Review-Url: https://codereview.chromium.org/2647763005
Cr-Commit-Position: refs/heads/master@{#445063}
Committed:
https://chromium.googlesource.com/chromium/src/+/695d0c65669b7af00f8daec96d49...
==========

[commit-bot: I haz the power, 2017-01-20 15:43:30]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/695d0c65669b7af00f8daec96d49...