Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(707)

Issue 2673003002: [Reading List] Display the redirected URL's favicon. (Closed)

Created:
3 years, 10 months ago by Olivier
Modified:
3 years, 10 months ago
Reviewers:
gambard, stkhapugin
CC:
chromium-reviews, marq+watch_chromium.org, stkhapugin, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -9 lines) Patch
M ios/chrome/browser/reading_list/reading_list_distiller_page.h View 1 chunk +2 lines, -0 lines 3 comments Download
M ios/chrome/browser/reading_list/reading_list_distiller_page.mm View 3 chunks +14 lines, -5 lines 0 comments Download
M ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h View 1 chunk +2 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm View 2 chunks +10 lines, -4 lines 2 comments Download
M ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm View 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (6 generated)
Olivier
PTAL.
3 years, 10 months ago (2017-02-03 10:09:28 UTC) #2
stkhapugin
drive-by: There's a typo in the CL title. s/refirected/redirected https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_list/reading_list_distiller_page.h File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right): https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_list/reading_list_distiller_page.h#newcode85 ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: ...
3 years, 10 months ago (2017-02-03 10:37:13 UTC) #4
Olivier
https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_list/reading_list_distiller_page.h File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right): https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_list/reading_list_distiller_page.h#newcode85 ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: // Starts the fetching of |page_url|'s favicon. On 2017/02/03 ...
3 years, 10 months ago (2017-02-03 10:44:55 UTC) #6
gambard
https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm 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/reading_list/reading_list_collection_view_item.mm#newcode118 ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:118: if (self.attributes) { Not your doing but this code ...
3 years, 10 months ago (2017-02-03 10:57:49 UTC) #7
Olivier
https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm 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/reading_list/reading_list_collection_view_item.mm#newcode118 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: > ...
3 years, 10 months ago (2017-02-03 13:17:28 UTC) #8
gambard
lgtm
3 years, 10 months ago (2017-02-03 13:17:53 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2673003002/1
3 years, 10 months ago (2017-02-03 13:48:52 UTC) #11
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/528205c98e89a58848bc3d00c7411eb1130a1b8f
3 years, 10 months ago (2017-02-03 14:44:15 UTC) #14
stkhapugin
https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_list/reading_list_distiller_page.h File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right): https://codereview.chromium.org/2673003002/diff/1/ios/chrome/browser/reading_list/reading_list_distiller_page.h#newcode85 ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: // Starts the fetching of |page_url|'s favicon. On 2017/02/03 ...
3 years, 10 months ago (2017-02-03 14:48:53 UTC) #15
Olivier
3 years, 10 months ago (2017-02-03 15:04:13 UTC) #16
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698