Add CSDataSource in showcase

Add CSDataSource in showcase

This CL adds a fake data source for ContentSuggestions in showcase.

BUG=704493
Review-Url: https://codereview.chromium.org/2769063005
Cr-Commit-Position: refs/heads/master@{#476683}
Committed: https://chromium.googlesource.com/chromium/src/+/6858b7fa2ae42b6c687589f39dd8c70509cb44ac

[gambard, 2017-03-24 10:47:25]

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

[gambard, 2017-03-24 10:47:25]

PTAL.

[lpromero, 2017-03-24 12:10:17]

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/DEPS
File ios/showcase/DEPS (right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/DEPS#newcode8
ios/showcase/DEPS:8: "+ui/gfx",
I would like to not have the UI depend on gfx. That means changing the data
source protocol to use UIImage only.

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
File ios/showcase/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/BUILD.gn:21: "//url",
Idem, only NSString should crossover. We need to keep ContentSuggestion out of
the UI layer and create intermediary objects. The mediator can do the
translation.

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
File ios/showcase/content_suggestions/sc_content_suggestions_data_source.h
(right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.h:11: 
Comment.

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
File ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm
(right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm:25:
@property(nonatomic, strong) ContentSuggestionsSectionInformation* sectionInfo;
Comment.

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm:36: -
(nonnull NSArray<ContentSuggestion*>*)allSuggestions {
No need for nullability annotations in the impl file. Here and below.

[gambard, 2017-03-24 15:12:49]

Thanks, PTAL.

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/DEPS
File ios/showcase/DEPS (right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/DEPS#newcode8
ios/showcase/DEPS:8: "+ui/gfx",
On 2017/03/24 12:10:17, lpromero wrote:
> I would like to not have the UI depend on gfx. That means changing the data
> source protocol to use UIImage only.

Done.

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
File ios/showcase/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/BUILD.gn:21: "//url",
On 2017/03/24 12:10:17, lpromero wrote:
> Idem, only NSString should crossover. We need to keep ContentSuggestion out of
> the UI layer and create intermediary objects. The mediator can do the
> translation.

So, based on your doc, should I keep it? :)

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
File ios/showcase/content_suggestions/sc_content_suggestions_data_source.h
(right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.h:11: 
On 2017/03/24 12:10:17, lpromero wrote:
> Comment.

Done.

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
File ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm
(right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm:25:
@property(nonatomic, strong) ContentSuggestionsSectionInformation* sectionInfo;
On 2017/03/24 12:10:17, lpromero wrote:
> Comment.

Done.

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm:36: -
(nonnull NSArray<ContentSuggestion*>*)allSuggestions {
On 2017/03/24 12:10:17, lpromero wrote:
> No need for nullability annotations in the impl file. Here and below.

Done.

[lpromero, 2017-03-24 15:22:54]

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
File ios/showcase/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2769063005/diff/1/ios/showcase/content_sugges...
ios/showcase/content_suggestions/BUILD.gn:21: "//url",
On 2017/03/24 15:12:49, gambard wrote:
> On 2017/03/24 12:10:17, lpromero wrote:
> > Idem, only NSString should crossover. We need to keep ContentSuggestion out
of
> > the UI layer and create intermediary objects. The mediator can do the
> > translation.
> 
> So, based on your doc, should I keep it? :)

As of today, I'd say no, as people pushed back before I started the doc. So I
advise waiting a couple days so we get consensus.

[lpromero, 2017-05-02 13:54:31]

Should this CL be revived to incorporate the latest design decisions?

[gambard, 2017-05-02 13:56:43]

It will be once the dataSource will return CollectionViewItem directly and not
ContentSuggestion.

[gambard, 2017-05-18 11:21:16]

PTAL!

[lpromero, 2017-05-22 13:01:50]

lgtm

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/DEPS
File ios/showcase/DEPS (right):

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/DEPS#newcode8
ios/showcase/DEPS:8: "+components/strings/grit",
Remove this line.

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/content_su...
File ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm
(right):

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/content_su...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm:164: //
Returns a favicon attributes with a random background color and a random
Nit: "Returns favicon attributes".

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/content_su...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm:188: //
Returns and article with the fields set except the IDInSection.
Nit: s/and/an

[gambard, 2017-06-02 14:46:03]

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

[gambard, 2017-06-02 14:46:04]

Thanks.
sdefresne@: PTAL for DEPS include of components/strings

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/DEPS
File ios/showcase/DEPS (right):

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/DEPS#newcode8
ios/showcase/DEPS:8: "+components/strings/grit",
On 2017/05/22 13:01:50, lpromero wrote:
> Remove this line.

Done.

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/content_su...
File ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm
(right):

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/content_su...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm:164: //
Returns a favicon attributes with a random background color and a random
On 2017/05/22 13:01:50, lpromero wrote:
> Nit: "Returns favicon attributes".

Done.

https://codereview.chromium.org/2769063005/diff/40001/ios/showcase/content_su...
ios/showcase/content_suggestions/sc_content_suggestions_data_source.mm:188: //
Returns and article with the fields set except the IDInSection.
On 2017/05/22 13:01:50, lpromero wrote:
> Nit: s/and/an

Done.

[gambard, 2017-06-02 14:46:26]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 14:46:54]

Dry run: CQ is trying da patch.

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

[sdefresne, 2017-06-02 15:16:31]

lgtm

[gambard, 2017-06-02 15:17:22]

The CQ bit was unchecked by gambard@chromium.org

[gambard, 2017-06-02 15:17:23]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-06-02 15:17:23]

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/2769063005/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2017-06-02 15:17:44]

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-06-02 16:19:05]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496416643284170,
"parent_rev": "ea4ad773e8a3b6fefb566abc3aa396eb2f4058fe", "commit_rev":
"6858b7fa2ae42b6c687589f39dd8c70509cb44ac"}

[commit-bot: I haz the power, 2017-06-02 16:19:15]

Description was changed from

==========
Add CSDataSource in showcase

This CL adds a fake data source for ContentSuggestions in showcase.

BUG=704493
==========

to

==========
Add CSDataSource in showcase

This CL adds a fake data source for ContentSuggestions in showcase.

BUG=704493

Review-Url: https://codereview.chromium.org/2769063005
Cr-Commit-Position: refs/heads/master@{#476683}
Committed:
https://chromium.googlesource.com/chromium/src/+/6858b7fa2ae42b6c687589f39dd8...
==========

[commit-bot: I haz the power, 2017-06-02 16:19:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6858b7fa2ae42b6c687589f39dd8...