Add FaviconAttributesProvider to ContentSuggestions

Add FaviconAttributesProvider to ContentSuggestions

This CL allows the ContentSuggestionsMediator to provide favicon attributes.

BUG=706296, 706427
Review-Url: https://codereview.chromium.org/2782613004
Cr-Commit-Position: refs/heads/master@{#461422}
Committed: https://chromium.googlesource.com/chromium/src/+/50148353a87f81026afab0ea2647ad66eb25cf8c

[gambard, 2017-03-29 12:54:49]

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

[gambard, 2017-03-29 12:54:49]

PTAL.

[lpromero, 2017-03-29 13:32:07]

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

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:178:
minFaviconSize:1
That's small, isn't it? Or why not 0?

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/BUILD.gn:22:
"//ios/chrome/browser/favicon",
I don't think the UI should know about favicons.

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

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:446:
[self.dataSource.attributesProvider
I would abstract this in a data source method.

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h
(right):

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:27:
FaviconAttributesProvider* attributesProvider;
I think this should not be know by the UI layer. You should instead add methods
that the mediator will implement.

[gambard, 2017-03-29 16:05:07]

Description was changed from

==========
Add FaviconAttributesProvider to ContentSuggestions

This CL allows the ContentSuggestionsMediator to provide favicon attributes.

BUG=706296, 694220
==========

to

==========
Add FaviconAttributesProvider to ContentSuggestions

This CL allows the ContentSuggestionsMediator to provide favicon attributes.

BUG=706296, 706427
==========

[gambard, 2017-03-30 08:07:11]

Thanks, PTAL.

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

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:178:
minFaviconSize:1
On 2017/03/29 13:32:07, lpromero wrote:
> That's small, isn't it? Or why not 0?

The idea is "the smallest possible if you have one"

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/BUILD.gn:22:
"//ios/chrome/browser/favicon",
On 2017/03/29 13:32:07, lpromero wrote:
> I don't think the UI should know about favicons.

Done.

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

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:446:
[self.dataSource.attributesProvider
On 2017/03/29 13:32:07, lpromero wrote:
> I would abstract this in a data source method. 

Done.

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h
(right):

https://codereview.chromium.org/2782613004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:27:
FaviconAttributesProvider* attributesProvider;
On 2017/03/29 13:32:07, lpromero wrote:
> I think this should not be know by the UI layer. You should instead add
methods
> that the mediator will implement.

Done.

[lpromero, 2017-03-31 08:30:26]

lgtm

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

https://codereview.chromium.org/2782613004/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:141:
@property(nonatomic, nullable, strong)
Optional nit: Could be nonnull.

[gambard, 2017-04-03 12:32:56]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-04-03 12:32:57]

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/2782613004/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-03 12:33:06]

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

[gambard, 2017-04-03 12:34:34]

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

https://codereview.chromium.org/2782613004/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:141:
@property(nonatomic, nullable, strong)
On 2017/03/31 08:30:26, lpromero wrote:
> Optional nit: Could be nonnull.

Yes but I think it is ok as is.

[commit-bot: I haz the power, 2017-04-03 14:07:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491222776873130,
"parent_rev": "e7182dca7960c0cbf1a6e0f0cc5f7b05ad2ef6a5", "commit_rev":
"50148353a87f81026afab0ea2647ad66eb25cf8c"}

[commit-bot: I haz the power, 2017-04-03 14:08:00]

Description was changed from

==========
Add FaviconAttributesProvider to ContentSuggestions

This CL allows the ContentSuggestionsMediator to provide favicon attributes.

BUG=706296, 706427
==========

to

==========
Add FaviconAttributesProvider to ContentSuggestions

This CL allows the ContentSuggestionsMediator to provide favicon attributes.

BUG=706296, 706427

Review-Url: https://codereview.chromium.org/2782613004
Cr-Commit-Position: refs/heads/master@{#461422}
Committed:
https://chromium.googlesource.com/chromium/src/+/50148353a87f81026afab0ea2647...
==========

[commit-bot: I haz the power, 2017-04-03 14:08:01]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/50148353a87f81026afab0ea2647...