[Reading List] Display the redirected URL's favicon.

[Reading List] Display the redirected URL's favicon.

If the page is distilled and led to a redirection, the favicon to display
is the favicon of the final page.

BUG=688273
Review-Url: https://codereview.chromium.org/2673003002
Cr-Commit-Position: refs/heads/master@{#447997}
Committed: https://chromium.googlesource.com/chromium/src/+/528205c98e89a58848bc3d00c7411eb1130a1b8f

[Olivier, 2017-02-03 10:09:27]

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

[Olivier, 2017-02-03 10:09:28]

PTAL.

[stkhapugin, 2017-02-03 10:37:12]

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

[stkhapugin, 2017-02-03 10:37:13]

drive-by:
There's a typo in the CL title. s/refirected/redirected

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: // Starts the
fetching of |page_url|'s favicon.
Can you add a test for this?

[Olivier, 2017-02-03 10:44:27]

Description was changed from

==========
[Reading List] Display the refirected URL's favicon.

If the page is distilled and led to a redirection, the favicon to display
is the favicon of the final page.

BUG=688273
==========

to

==========
[Reading List] Display the redirected URL's favicon.

If the page is distilled and led to a redirection, the favicon to display
is the favicon of the final page.

BUG=688273
==========

[Olivier, 2017-02-03 10:44:55]

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: // Starts the
fetching of |page_url|'s favicon.
On 2017/02/03 10:37:13, stkhapugin wrote:
> Can you add a test for this?

This is a glue method.
Testing it would require far more than just deleting the method, so If you
prefer, I can just inline it.

[gambard, 2017-02-03 10:57:49]

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm
(right):

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:118: if
(self.attributes) {
Not your doing but this code might creates some weird things if you reuse cells
before the favicon has been fetched.

[Olivier, 2017-02-03 13:17:28]

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm
(right):

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:118: if
(self.attributes) {
On 2017/02/03 10:57:49, gambard wrote:
> Not your doing but this code might creates some weird things if you reuse
cells
> before the favicon has been fetched.

As it won't happen with current implementation and you have a fix, let's do it
in another CL.

[gambard, 2017-02-03 13:17:53]

lgtm

[Olivier, 2017-02-03 13:38:43]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2017-02-03 13:48:52]

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-02-03 14:43:44]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486129123111250, "parent_rev":
"9ea51a5ffa60046859be210ca5a76ae85abf5a94", "commit_rev":
"528205c98e89a58848bc3d00c7411eb1130a1b8f"}

[commit-bot: I haz the power, 2017-02-03 14:44:14]

Description was changed from

==========
[Reading List] Display the redirected URL's favicon.

If the page is distilled and led to a redirection, the favicon to display
is the favicon of the final page.

BUG=688273
==========

to

==========
[Reading List] Display the redirected URL's favicon.

If the page is distilled and led to a redirection, the favicon to display
is the favicon of the final page.

BUG=688273

Review-Url: https://codereview.chromium.org/2673003002
Cr-Commit-Position: refs/heads/master@{#447997}
Committed:
https://chromium.googlesource.com/chromium/src/+/528205c98e89a58848bc3d00c741...
==========

[commit-bot: I haz the power, 2017-02-03 14:44:15]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/528205c98e89a58848bc3d00c741...

[stkhapugin, 2017-02-03 14:48:53]

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):

https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: // Starts the
fetching of |page_url|'s favicon.
On 2017/02/03 10:44:55, Olivier Robin wrote:
> On 2017/02/03 10:37:13, stkhapugin wrote:
> > Can you add a test for this?
> 
> This is a glue method.
> Testing it would require far more than just deleting the method, so If you
> prefer, I can just inline it.

Sorry, I chose a bad place to leave this comment. I was trying to say: the
behavior of showing the favicon of the page redirected to should probably be
tested in one way or another. I can see how I could've changed this behavior if
I were told to refactor this code 3 years from now. Maybe in egtest for reading
list? Or at least leave some documentation somewhere about this behavior? 

Please don't see this as if I'm trying to stop you from landing this code, I'm
just trying to preserve knowledge that this non-trivial behavior is not lost
over time.

[Olivier, 2017-02-03 15:04:13]

On 2017/02/03 14:48:53, stkhapugin wrote:
>
https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_...
> File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):
> 
>
https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_...
> ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: // Starts
the
> fetching of |page_url|'s favicon.
> On 2017/02/03 10:44:55, Olivier Robin wrote:
> > On 2017/02/03 10:37:13, stkhapugin wrote:
> > > Can you add a test for this?
> > 
> > This is a glue method.
> > Testing it would require far more than just deleting the method, so If you
> > prefer, I can just inline it.
> 
> Sorry, I chose a bad place to leave this comment. I was trying to say: the
> behavior of showing the favicon of the page redirected to should probably be
> tested in one way or another. I can see how I could've changed this behavior
if
> I were told to refactor this code 3 years from now. Maybe in egtest for
reading
> list? Or at least leave some documentation somewhere about this behavior? 
> 
> Please don't see this as if I'm trying to stop you from landing this code, I'm
> just trying to preserve knowledge that this non-trivial behavior is not lost
> over time.

I think the logic of displaying the distilled icon is more a
ReadingListViewController thing.
The ReadingListDistillerPage just follows the redirections.

I will try to find the best place to add a test.
Thanks.