Add Category wrapper for ContentSuggestions on iOS

Add Category wrapper for ContentSuggestions on iOS

This CL adds an Objective-C wrapper for the ntp_snippets::Category.
It also adds a SectionInformation containing the informations relative to a
section, like the title, layout, ordering and cell to display when the section
is empty.

BUG=686728
Review-Url: https://codereview.chromium.org/2689953003
Cr-Commit-Position: refs/heads/master@{#450012}
Committed: https://chromium.googlesource.com/chromium/src/+/79a649cdaa78cd9f9b6c040b1bccc89c7efcb7e5

[gambard, 2017-02-13 09:25:37]

gambard@chromium.org changed reviewers:
+ baxley@chromium.org, lpromero@chromium.org, stkhapugin@chromium.org

[gambard, 2017-02-13 09:25:38]

PTAL.
+baxley@ for ios/chrome/test/BUILD.gn

[gambard, 2017-02-13 09:31:33]

This is used in https://codereview.chromium.org/2691593002/

[stkhapugin, 2017-02-13 12:19:19]

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h:19:
@property(nonatomic, assign, readonly) ntp_snippets::Category category;
This is not a property, you create a new object every time. Use a getter
instead. I think you can still use dot notation in this case.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:14:
int _categoryID;
Since this is a primitive and not a C++ object, you can use a property instead
of an ivar for a more modern syntax.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:30:
if (![object isKindOfClass:[ContentSuggestionsCategoryWrapper class]]) {
s/isKindOfClass/isMemberOfClass

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:40:
- (NSUInteger)hash {
-hash and -isEqual are usually put in a #pragma mark - comparison

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper_unittest.mm
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper_unittest.mm:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
Excellent unit tests, thank you.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:12:
// Layout for the section.
Is it "layout for the section" (= the way this section is laid out) or "layout
for items of a section"?

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:14:
// No specific layout.


Optional: Does this match some other enum? Somehow, I expect "custom" layout to
be the last in the list, as it is custom since it doesn't match any other
category, so logically I'd expect to see all previous categories before this
one.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:28:
ContentSuggestionsSectionNotImplemented
nit: make this "ContentSuggestionsSectionCount" and replace DCHECK in init from 

DCHECK(ID!=ContentSuggestionsSectionNotImplemented);

to:

DCHECK(ID < ContentSuggestionsSectionCount);

[lpromero, 2017-02-13 12:34:55]

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:30:
if (![object isKindOfClass:[ContentSuggestionsCategoryWrapper class]]) {
On 2017/02/13 12:19:18, stkhapugin wrote:
> s/isKindOfClass/isMemberOfClass

Why? Subclassing would mean two objects couldn't be equals as is.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper_unittest.mm
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper_unittest.mm:49:
}
Add a test that exercises the class check in the isEqual: implementation.
Compare a Wrapper and an NSObject for example, or more involved stuff like
subclassing.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:37:
// If this not nil, the section is displaying this cell when it is empty.
"If this is not nil"

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:38:
// If it is nil, the section is not displayed when empty.
No action needed: Technically, these comments are not related to this object but
its client, right? For example, this behavior can't be unit tested with this
class alone.

[stkhapugin, 2017-02-13 13:31:38]

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:30:
if (![object isKindOfClass:[ContentSuggestionsCategoryWrapper class]]) {
On 2017/02/13 12:34:55, lpromero wrote:
> On 2017/02/13 12:19:18, stkhapugin wrote:
> > s/isKindOfClass/isMemberOfClass
> 
> Why? Subclassing would mean two objects couldn't be equals as is.

Since subclasses are generally different from their superclasses, I've been
always taught that the default behaviour should be subclass != parent, unless
you know you have a special case. But I see now that both opinions are popular
and there's no particular consensus in ObjC community.

[lpromero, 2017-02-13 13:55:32]

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:30:
if (![object isKindOfClass:[ContentSuggestionsCategoryWrapper class]]) {
On 2017/02/13 13:31:38, stkhapugin wrote:
> On 2017/02/13 12:34:55, lpromero wrote:
> > On 2017/02/13 12:19:18, stkhapugin wrote:
> > > s/isKindOfClass/isMemberOfClass
> > 
> > Why? Subclassing would mean two objects couldn't be equals as is.
> 
> Since subclasses are generally different from their superclasses, I've been
> always taught that the default behaviour should be subclass != parent, unless
> you know you have a special case. But I see now that both opinions are popular
> and there's no particular consensus in ObjC community. 

I don't have any particular point of view on this. It's what semantic you want
to give to equality.

If self and class are both instances of a same subclass, the isMemberOfClass
impl breaks too. If the semantic you want is having the same class, then the
check should be self.class == object.class.
One issue I have with that is that it means that subclassing breaks the parent
behavior. It breaks the Open/closed principle I think.

(http://nshipster.com/equality/ uses isKIndOfClass, which like here, is check to
early return before casting.)

[baxley, 2017-02-13 14:26:29]

ios/chrome/test/BUILD.gn LGTM

[gambard, 2017-02-13 14:51:41]

Thanks, PTAL.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h:19:
@property(nonatomic, assign, readonly) ntp_snippets::Category category;
On 2017/02/13 12:19:18, stkhapugin wrote:
> This is not a property, you create a new object every time. Use a getter
> instead. I think you can still use dot notation in this case. 

Done.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:14:
int _categoryID;
On 2017/02/13 12:19:18, stkhapugin wrote:
> Since this is a primitive and not a C++ object, you can use a property instead
> of an ivar for a more modern syntax. 

Done.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:30:
if (![object isKindOfClass:[ContentSuggestionsCategoryWrapper class]]) {
On 2017/02/13 13:55:32, lpromero wrote:
> On 2017/02/13 13:31:38, stkhapugin wrote:
> > On 2017/02/13 12:34:55, lpromero wrote:
> > > On 2017/02/13 12:19:18, stkhapugin wrote:
> > > > s/isKindOfClass/isMemberOfClass
> > > 
> > > Why? Subclassing would mean two objects couldn't be equals as is.
> > 
> > Since subclasses are generally different from their superclasses, I've been
> > always taught that the default behaviour should be subclass != parent,
unless
> > you know you have a special case. But I see now that both opinions are
popular
> > and there's no particular consensus in ObjC community. 
> 
> I don't have any particular point of view on this. It's what semantic you want
> to give to equality.
> 
> If self and class are both instances of a same subclass, the isMemberOfClass
> impl breaks too. If the semantic you want is having the same class, then the
> check should be self.class == object.class.
> One issue I have with that is that it means that subclassing breaks the parent
> behavior. It breaks the Open/closed principle I think.
> 
> (http://nshipster.com/equality/ uses isKIndOfClass, which like here, is check
to
> early return before casting.)

I keep it this way.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:40:
- (NSUInteger)hash {
On 2017/02/13 12:19:18, stkhapugin wrote:
> -hash and -isEqual are usually put in a #pragma mark - comparison

Done.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper_unittest.mm
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper_unittest.mm:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/02/13 12:19:18, stkhapugin wrote:
> Excellent unit tests, thank you. 

:)

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper_unittest.mm:49:
}
On 2017/02/13 12:34:55, lpromero wrote:
> Add a test that exercises the class check in the isEqual: implementation.
> Compare a Wrapper and an NSObject for example, or more involved stuff like
> subclassing.

Done.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h
(right):

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:12:
// Layout for the section.
On 2017/02/13 12:19:18, stkhapugin wrote:
> Is it "layout for the section" (= the way this section is laid out) or "layout
> for items of a section"?

Done.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:14:
// No specific layout.
On 2017/02/13 12:19:18, stkhapugin wrote:
> 
> 
> Optional: Does this match some other enum? Somehow, I expect "custom" layout
to
> be the last in the list, as it is custom since it doesn't match any other
> category, so logically I'd expect to see all previous categories before this
> one. 

Not really. In the current implementation card = MDCCollectionViewCellStyleCard

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:28:
ContentSuggestionsSectionNotImplemented
On 2017/02/13 12:19:18, stkhapugin wrote:
> nit: make this "ContentSuggestionsSectionCount" and replace DCHECK in init
from 
> 
> DCHECK(ID!=ContentSuggestionsSectionNotImplemented);
> 
> to:
> 
> DCHECK(ID < ContentSuggestionsSectionCount);

Done.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:37:
// If this not nil, the section is displaying this cell when it is empty.
On 2017/02/13 12:34:55, lpromero wrote:
> "If this is not nil"

Done.

https://codereview.chromium.org/2689953003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:38:
// If it is nil, the section is not displayed when empty.
On 2017/02/13 12:34:55, lpromero wrote:
> No action needed: Technically, these comments are not related to this object
but
> its client, right? For example, this behavior can't be unit tested with this
> class alone.

I rewrote the comment.

[lpromero, 2017-02-13 14:57:32]

lgtm

[stkhapugin, 2017-02-13 15:42:50]

lgtm

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h
(right):

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:22:
// When adding a new kind of suggestions, add a new corresponding section.
nit: I know it looks obvious, but can you add a comment saying "these values are
never persisted between launches" if this is the case, or otherwise note that
they may be persisted?

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:40:
@property(nonatomic, strong) CollectionViewItem* emptyCell;
optional: I'm almost certain you will never write into this property, so you
might make it readonly and change designated initializer to
initWithID:emptyCell:.

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:42:
@property(nonatomic, assign) ContentSuggestionsSectionLayout layout;
Optional: same here, I'm almost sure you'll never write to this, so you can
stick |layout| into designated initialzier and make this readonly.

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:46:
@property(nonatomic, copy) NSString* title;
Optional: And this one.

[gambard, 2017-02-13 16:27:53]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-02-13 16:27:53]

The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org,
stkhapugin@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2689953003/#ps130001
(title: "Fix title copu")

[gambard, 2017-02-13 16:28:04]

Thanks!

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h
(right):

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:22:
// When adding a new kind of suggestions, add a new corresponding section.
On 2017/02/13 15:42:49, stkhapugin wrote:
> nit: I know it looks obvious, but can you add a comment saying "these values
are
> never persisted between launches" if this is the case, or otherwise note that
> they may be persisted?

Done.

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:40:
@property(nonatomic, strong) CollectionViewItem* emptyCell;
On 2017/02/13 15:42:49, stkhapugin wrote:
> optional: I'm almost certain you will never write into this property, so you
> might make it readonly and change designated initializer to
> initWithID:emptyCell:. 

Done.

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:42:
@property(nonatomic, assign) ContentSuggestionsSectionLayout layout;
On 2017/02/13 15:42:49, stkhapugin wrote:
> Optional: same here, I'm almost sure you'll never write to this, so you can
> stick |layout| into designated initialzier and make this readonly. 

Done.

https://codereview.chromium.org/2689953003/diff/90001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:46:
@property(nonatomic, copy) NSString* title;
On 2017/02/13 15:42:49, stkhapugin wrote:
> Optional: And this one. 

Done.

[commit-bot: I haz the power, 2017-02-13 16:28:26]

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-02-13 18:15:32]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1487003273571960,
"parent_rev": "ede219e4e7644e62d0965894e16ff40629549484", "commit_rev":
"79a649cdaa78cd9f9b6c040b1bccc89c7efcb7e5"}

[commit-bot: I haz the power, 2017-02-13 18:16:17]

Description was changed from

==========
Add Category wrapper for ContentSuggestions on iOS

This CL adds an Objective-C wrapper for the ntp_snippets::Category.
It also adds a SectionInformation containing the informations relative to a
section, like the title, layout, ordering and cell to display when the section
is empty.

BUG=686728
==========

to

==========
Add Category wrapper for ContentSuggestions on iOS

This CL adds an Objective-C wrapper for the ntp_snippets::Category.
It also adds a SectionInformation containing the informations relative to a
section, like the title, layout, ordering and cell to display when the section
is empty.

BUG=686728

Review-Url: https://codereview.chromium.org/2689953003
Cr-Commit-Position: refs/heads/master@{#450012}
Committed:
https://chromium.googlesource.com/chromium/src/+/79a649cdaa78cd9f9b6c040b1bcc...
==========

[commit-bot: I haz the power, 2017-02-13 18:16:18]

Committed patchset #8 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/79a649cdaa78cd9f9b6c040b1bcc...