Download favicon from server for suggested articles

Download favicon from server for suggested articles

This CL uses the FetchFavicon method of the ContentSuggestionsService to fetches
favicon for suggested articles if they are not available in history.
Only the favicon of suggested articles are fetched because the download
service can only access public URL.

BUG=706427
Review-Url: https://codereview.chromium.org/2818453002
Cr-Commit-Position: refs/heads/master@{#464721}
Committed: https://chromium.googlesource.com/chromium/src/+/7d2c9cff70f35b667e59349d8a3520c058da6593

[gambard, 2017-04-12 10:56:06]

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

[gambard, 2017-04-12 10:56:07]

PTAL.

[lpromero, 2017-04-14 13:19:40]

lgtm

https://codereview.chromium.org/2818453002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2818453002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:244:
completion([image.ToUIImage() copy]);
Is the copy necessary? UIImage is immutable.

https://codereview.chromium.org/2818453002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:254:
identifier, 1, kDefaultFaviconSize, base::BindBlockArc(imageCallback));
Optional nit: Can you add something like: "identifier, 1 /*
minimum_size_in_pixel */, kDefaut…"

https://codereview.chromium.org/2818453002/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2818453002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:483:
weakItem.attributes =
I'd advise to return early if one of the weak items are nil, so that the
FaviconAttributes method doesn’t get called.
It will also avoid [weakSelf reconfigure:nil].

[gambard, 2017-04-14 13:28:10]

Thanks!

https://codereview.chromium.org/2818453002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2818453002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:244:
completion([image.ToUIImage() copy]);
On 2017/04/14 13:19:40, lpromero wrote:
> Is the copy necessary? UIImage is immutable.

It is a memory management thing. You can ask stkhapugin for more information.

https://codereview.chromium.org/2818453002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:254:
identifier, 1, kDefaultFaviconSize, base::BindBlockArc(imageCallback));
On 2017/04/14 13:19:40, lpromero wrote:
> Optional nit: Can you add something like: "identifier, 1 /*
> minimum_size_in_pixel */, kDefaut…"

Done.

[gambard, 2017-04-14 13:28:15]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-04-14 13:28:15]

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

[commit-bot: I haz the power, 2017-04-14 13:28:25]

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-04-14 13:42:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492176495414720,
"parent_rev": "d44c5d06f60bbecd5404e17cc9e2350313ed5693", "commit_rev":
"7d2c9cff70f35b667e59349d8a3520c058da6593"}

[commit-bot: I haz the power, 2017-04-14 13:43:22]

Description was changed from

==========
Download favicon from server for suggested articles

This CL uses the FetchFavicon method of the ContentSuggestionsService to fetches
favicon for suggested articles if they are not available in history.
Only the favicon of suggested articles are fetched because the download
service can only access public URL.

BUG=706427
==========

to

==========
Download favicon from server for suggested articles

This CL uses the FetchFavicon method of the ContentSuggestionsService to fetches
favicon for suggested articles if they are not available in history.
Only the favicon of suggested articles are fetched because the download
service can only access public URL.

BUG=706427

Review-Url: https://codereview.chromium.org/2818453002
Cr-Commit-Position: refs/heads/master@{#464721}
Committed:
https://chromium.googlesource.com/chromium/src/+/7d2c9cff70f35b667e59349d8a35...
==========

[commit-bot: I haz the power, 2017-04-14 13:43:24]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7d2c9cff70f35b667e59349d8a35...