Suggestions UI - stack item

Suggestions UI - stack item

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

[gambard, 2017-01-16 14:15:11]

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

[gambard, 2017-01-16 14:15:11]

PTAL.

[gambard, 2017-01-16 14:33:53]

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

[gambard, 2017-01-16 14:33:53]

+marq@

[marq (ping after 24h), 2017-01-16 18:04:43]

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_stack_item.h (right):

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.h:11: // Item for a
suggestions item with a title and subtitle.
Describe what a StackItem is.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.h:14: // Init a
suggestions item with a |title| and a |subtitle|. |type| is the type
Initialize.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.h:23: // Corresponding
cell for a suggestion item.
s/suggestion item/suggestion stack item/

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.h:26:
@property(nonatomic, readonly, strong) UIButton* titleButton;
Do these need to be public properties?

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

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.mm:15: const NSInteger
kStackSpacing = 8;
If this is used to positions views, it should be a CGFloat.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.mm:74: UIView*
secondView = [[UIView alloc] init];
If these are placeholders, please add comments indicating as much.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.mm:92:
[self.contentView addSubview:itemDisplay];
Why not just reverse the order of these and use -addSubview: in each case?

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_stack_item_actions.h (right):

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item_actions.h:12: -
(void)topOfStackPressed:(id)sender;
That's not an action, that's an event. The intent of an action protocol is to
give a name (a selector) to an action like 'close tab' or 'add to list' so that
the action can be set as the target for a control. What should happen when the
top of the stack is pressed?

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

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:55:
isKindOfClass:[SuggestionsStackItem class]]) {
I don't like the runtime class membership test. 

lpromero@: I think it would be good if a collection view could declare all of
its items to be members of some CollectionViewItem subclass, and then
[CollectionViewModel itemAtIndexPath:] can return an Item with some known
additional API that the collection view controller can query without relying on
RTTI. 

e.g, if by default a CollectionViewController's |collectionViewModel| property
is a CollectionViewModel<CollectionViewItem*>*, a subclass can declare their
property to instead be a  CollectionViewModel<SuggestionCollectionViewItem*>*. 

This is all doable with ObjC generic annotations, but the downside is that the
implementation of CollectionViewModel has to use id internally for the type of
the object it contains.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:94: - (nullable
UIColor*)collectionView:(nonnull UICollectionView*)collectionView
I think we don't want nullability annotations on some methods and not on others.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:119:
cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) -
Taking edge inset width into account for cell height seems like something the
superclass should do.

https://codereview.chromium.org/2630313004/diff/20001/ios/showcase/suggestion...
File ios/showcase/suggestions/sc_suggestions_coordinator.mm (right):

https://codereview.chromium.org/2630313004/diff/20001/ios/showcase/suggestion...
ios/showcase/suggestions/sc_suggestions_coordinator.mm:39:
_suggestionViewController.suggestionCommandHandler = self;
Just make protocolAlerter the suggestionCommandHandler, then you don't need to
manually forward methods into it.

[lpromero, 2017-01-16 18:17:55]

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/BUILD.gn (right):

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/BUILD.gn:38: configs += [
"//build/config/compiler:enable_arc" ]
Can you separate the migration to ARC with the new code?

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

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:55:
isKindOfClass:[SuggestionsStackItem class]]) {
This should test the itemType property of the item instead of its class.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:119:
cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) -
On 2017/01/16 18:04:42, marq wrote:
> Taking edge inset width into account for cell height seems like something the
> superclass should do.

This is dependent on the cellStyle this controller wants for the given cell,
i.e. because of line 46.
Which superclass did you mean: CollectionViewController, or
MDCCollectionViewCell?

[gambard, 2017-01-17 13:28:12]

Thanks, PTAL.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/BUILD.gn (right):

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/BUILD.gn:38: configs += [
"//build/config/compiler:enable_arc" ]
On 2017/01/16 18:17:54, lpromero wrote:
> Can you separate the migration to ARC with the new code?

Done.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_stack_item.h (right):

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.h:11: // Item for a
suggestions item with a title and subtitle.
On 2017/01/16 18:04:42, marq wrote:
> Describe what a StackItem is.

Done.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.h:14: // Init a
suggestions item with a |title| and a |subtitle|. |type| is the type
On 2017/01/16 18:04:42, marq wrote:
> Initialize.

Done.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.h:23: // Corresponding
cell for a suggestion item.
On 2017/01/16 18:04:42, marq wrote:
> s/suggestion item/suggestion stack item/

Done.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.h:26:
@property(nonatomic, readonly, strong) UIButton* titleButton;
On 2017/01/16 18:04:42, marq wrote:
> Do these need to be public properties?

They are used by the item to set the title and the subtitle.

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

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.mm:15: const NSInteger
kStackSpacing = 8;
On 2017/01/16 18:04:42, marq wrote:
> If this is used to positions views, it should be a CGFloat.

Done.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.mm:74: UIView*
secondView = [[UIView alloc] init];
On 2017/01/16 18:04:42, marq wrote:
> If these are placeholders, please add comments indicating as much.

Done.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item.mm:92:
[self.contentView addSubview:itemDisplay];
On 2017/01/16 18:04:42, marq wrote:
> Why not just reverse the order of these and use -addSubview: in each case?

This shows there is an intended order. Using only -addSubview: let the reader
wonder if it is on purpose.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_stack_item_actions.h (right):

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_stack_item_actions.h:12: -
(void)topOfStackPressed:(id)sender;
On 2017/01/16 18:04:42, marq wrote:
> That's not an action, that's an event. The intent of an action protocol is to
> give a name (a selector) to an action like 'close tab' or 'add to list' so
that
> the action can be set as the target for a control. What should happen when the
> top of the stack is pressed?

Done.

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

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:55:
isKindOfClass:[SuggestionsStackItem class]]) {
On 2017/01/16 18:17:55, lpromero wrote:
> This should test the itemType property of the item instead of its class.

The ItemType property is unique per class for now but it might not always be
true.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:55:
isKindOfClass:[SuggestionsStackItem class]]) {
On 2017/01/16 18:04:42, marq wrote:
> I don't like the runtime class membership test. 
> 
> lpromero@: I think it would be good if a collection view could declare all of
> its items to be members of some CollectionViewItem subclass, and then
> [CollectionViewModel itemAtIndexPath:] can return an Item with some known
> additional API that the collection view controller can query without relying
on
> RTTI. 
> 
> e.g, if by default a CollectionViewController's |collectionViewModel| property
> is a CollectionViewModel<CollectionViewItem*>*, a subclass can declare their
> property to instead be a  CollectionViewModel<SuggestionCollectionViewItem*>*.

> 
> This is all doable with ObjC generic annotations, but the downside is that the
> implementation of CollectionViewModel has to use id internally for the type of
> the object it contains.

I used the item type.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:94: - (nullable
UIColor*)collectionView:(nonnull UICollectionView*)collectionView
On 2017/01/16 18:04:42, marq wrote:
> I think we don't want nullability annotations on some methods and not on
others.

Done.

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:119:
cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) -
On 2017/01/16 18:17:55, lpromero wrote:
> On 2017/01/16 18:04:42, marq wrote:
> > Taking edge inset width into account for cell height seems like something
the
> > superclass should do.
> 
> This is dependent on the cellStyle this controller wants for the given cell,
> i.e. because of line 46.
> Which superclass did you mean: CollectionViewController, or
> MDCCollectionViewCell?

Maybe we should add a method to CollectionViewController
-widthOfCellInSection:(NSInteger)section?

https://codereview.chromium.org/2630313004/diff/20001/ios/showcase/suggestion...
File ios/showcase/suggestions/sc_suggestions_coordinator.mm (right):

https://codereview.chromium.org/2630313004/diff/20001/ios/showcase/suggestion...
ios/showcase/suggestions/sc_suggestions_coordinator.mm:39:
_suggestionViewController.suggestionCommandHandler = self;
On 2017/01/16 18:04:43, marq wrote:
> Just make protocolAlerter the suggestionCommandHandler, then you don't need to
> manually forward methods into it.

I want to respond to some command (like "addEmptyItem") and forward some other
to the alerter.

[lpromero, 2017-01-17 13:47:20]

lgtm

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:74:
SuggestionsItem* item = [[SuggestionsItem alloc] initWithType:ItemTypeText
DCHECK(_collectionViewController)?

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:104: -
(BOOL)isCustomStyling:(NSInteger)section {
s/isCustomStyling/shouldUseCustomStyleForSection? Or something close.

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_commands.h (right):

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_commands.h:15: // Opens the first
page of the Reading List.
s/page/element?

[marq (ping after 24h), 2017-01-17 14:36:06]

lgtm

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

https://codereview.chromium.org/2630313004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:119:
cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) -
On 2017/01/16 18:17:55, lpromero wrote:
> On 2017/01/16 18:04:42, marq wrote:
> > Taking edge inset width into account for cell height seems like something
the
> > superclass should do.
> 
> This is dependent on the cellStyle this controller wants for the given cell,
> i.e. because of line 46.
> Which superclass did you mean: CollectionViewController, or
> MDCCollectionViewCell?

I meant CollectionViewController, since this is a method defined on that class,
but it doesn't get a superclass invocation so that won't really help.

[gambard, 2017-01-17 14:52:36]

Thanks!

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:74:
SuggestionsItem* item = [[SuggestionsItem alloc] initWithType:ItemTypeText
On 2017/01/17 13:47:20, lpromero wrote:
> DCHECK(_collectionViewController)?

Done.

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:104: -
(BOOL)isCustomStyling:(NSInteger)section {
On 2017/01/17 13:47:20, lpromero wrote:
> s/isCustomStyling/shouldUseCustomStyleForSection? Or something close.

Done.

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/suggestions/suggestions_commands.h (right):

https://codereview.chromium.org/2630313004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/suggestions/suggestions_commands.h:15: // Opens the first
page of the Reading List.
On 2017/01/17 13:47:20, lpromero wrote:
> s/page/element?

Reading List usually uses "pages".

[gambard, 2017-01-17 16:30:15]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-01-17 16:30:15]

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

[commit-bot: I haz the power, 2017-01-17 16:30:22]

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-17 17:29:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484670615240710,
"parent_rev": "2271432ba70129e37870842f007bfbc3efb2659b", "commit_rev":
"359779f98091859cf37500d5ae3505aee346f955"}

[commit-bot: I haz the power, 2017-01-17 17:30:23]

Description was changed from

==========
Suggestions UI - stack item

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

to

==========
Suggestions UI - stack item

BUG=679369

Review-Url: https://codereview.chromium.org/2630313004
Cr-Commit-Position: refs/heads/master@{#444080}
Committed:
https://chromium.googlesource.com/chromium/src/+/359779f98091859cf37500d5ae35...
==========

[commit-bot: I haz the power, 2017-01-17 17:30:24]

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