Suggestions UI - Add Article items

Suggestions UI - Add Article items

BUG=679369
Review-Url: https://codereview.chromium.org/2618333005
Cr-Commit-Position: refs/heads/master@{#443266}
Committed: https://chromium.googlesource.com/chromium/src/+/d1d73f239ba994051cf60f02d0531cef7875094a

[gambard, 2017-01-10 13:04:54]

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

[gambard, 2017-01-10 13:04:54]

PTAL.
Split 3/4 of https://codereview.chromium.org/2618343002/

[lpromero, 2017-01-11 13:00:39]

Can you make a test file and a configureItem test for the item?

https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_article_item.mm (right):

https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:19: NSString*
_subtitle;
Idem as previous CL, prefer properties with copy attributes.

https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:30: _title =
title;
Should be copy. Same below.

https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:103: _title =
title;
Do you need the cell to keep those? You could just query the titleLabel for its
text when needed.

[lpromero, 2017-01-11 13:00:52]

lgtm with test file

[gambard, 2017-01-11 17:14:45]

Thanks, but what do you want to test? The |configureCell| method of the item?

https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_article_item.mm (right):

https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:19: NSString*
_subtitle;
On 2017/01/11 13:00:39, lpromero wrote:
> Idem as previous CL, prefer properties with copy attributes.

Done.

https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:30: _title =
title;
On 2017/01/11 13:00:39, lpromero wrote:
> Should be copy. Same below.

Done.

https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:103: _title =
title;
On 2017/01/11 13:00:39, lpromero wrote:
> Do you need the cell to keep those? You could just query the titleLabel for
its
> text when needed.

Done.

[lpromero, 2017-01-11 18:11:06]

On 2017/01/11 17:14:45, gambard wrote:
> Thanks, but what do you want to test? The |configureCell| method of the item?
> 
>
https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
> File ios/chrome/browser/ui/suggestions/suggestions_article_item.mm (right):
> 
>
https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
> ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:19: NSString*
> _subtitle;
> On 2017/01/11 13:00:39, lpromero wrote:
> > Idem as previous CL, prefer properties with copy attributes.
> 
> Done.
> 
>
https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
> ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:30: _title =
> title;
> On 2017/01/11 13:00:39, lpromero wrote:
> > Should be copy. Same below.
> 
> Done.
> 
>
https://codereview.chromium.org/2618333005/diff/20001/ios/chrome/browser/ui/s...
> ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:103: _title =
> title;
> On 2017/01/11 13:00:39, lpromero wrote:
> > Do you need the cell to keep those? You could just query the titleLabel for
> its
> > text when needed.
> 
> Done.

You can have a look at the other item's test files. The minimum is testing that
the item is hooked to the proper cellClass. Then that configureCell sets a
minimum on the cell.
More importantly, I think that we should have as a policy to always create a
test file for every new class file we add.
It's a good practice, will help us when we enforce code coverage and especially
lowers the barrier for people to add tests when debugging/fixing a class.

[gambard, 2017-01-12 09:50:52]

Thanks, test added.

[lpromero, 2017-01-12 09:57:21]

lgtm

https://codereview.chromium.org/2618333005/diff/60001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_article_item.mm (right):

https://codereview.chromium.org/2618333005/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:55: UIView*
_imageContainer;
Nit: you only need the container in the init. Consider just making it a local
variable.

[gambard, 2017-01-12 12:55:12]

Thanks

https://codereview.chromium.org/2618333005/diff/60001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_article_item.mm (right):

https://codereview.chromium.org/2618333005/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_article_item.mm:55: UIView*
_imageContainer;
On 2017/01/12 09:57:21, lpromero wrote:
> Nit: you only need the container in the init. Consider just making it a local
> variable.

Done.

[marq (ping after 24h), 2017-01-12 15:05:16]

https://codereview.chromium.org/2618333005/diff/80001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_data_source.mm (right):

https://codereview.chromium.org/2618333005/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_data_source.mm:25: @implementation
SuggestionsDataSource {
I'm confused; where's the @interface for this declared?

[gambard, 2017-01-12 15:28:19]

https://codereview.chromium.org/2618333005/diff/80001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_data_source.mm (right):

https://codereview.chromium.org/2618333005/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_data_source.mm:25: @implementation
SuggestionsDataSource {
On 2017/01/12 15:05:16, marq wrote:
> I'm confused; where's the @interface for this declared?

This is an error during rebase...

[marq (ping after 24h), 2017-01-12 15:39:34]

lgtm

[gambard, 2017-01-12 15:39:54]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-01-12 15:39:56]

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/2618333005/#ps90001
(title: "Remove data source")

[commit-bot: I haz the power, 2017-01-12 15:40:20]

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-12 17:06:52]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1484235594952030,
"parent_rev": "eef32b1c356569392ecea7b75f44fc6619d97620", "commit_rev":
"d1d73f239ba994051cf60f02d0531cef7875094a"}

[commit-bot: I haz the power, 2017-01-12 17:07:26]

Description was changed from

==========
Suggestions UI - Add Article items

BUG=679369
==========

to

==========
Suggestions UI - Add Article items

BUG=679369

Review-Url: https://codereview.chromium.org/2618333005
Cr-Commit-Position: refs/heads/master@{#443266}
Committed:
https://chromium.googlesource.com/chromium/src/+/d1d73f239ba994051cf60f02d053...
==========

[commit-bot: I haz the power, 2017-01-12 17:07:32]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/d1d73f239ba994051cf60f02d053...