Makes -init and -setType: available in CollectionViewItem

Makes -init and -setType: available in CollectionViewItem

CollectionViewItem can be created with a default item type but it needs to have a valid item type to be inserted in the model. Only the CollectionViewController managing the item is allowed to set the item type.

BUG=711641
Review-Url: https://codereview.chromium.org/2814273004
Cr-Commit-Position: refs/heads/master@{#464943}
Committed: https://chromium.googlesource.com/chromium/src/+/62ecac299486dca3651febd14c7563a415d1da07

[lpromero, 2017-04-13 21:42:57]

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

[lpromero, 2017-04-13 21:42:59]

Comments but lg!

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/BUILD.gn (right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/BUILD.gn:17:
"collection_view_item+Private.h",
collection_view_item+collection_view_controller.h

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item+Private.h
(right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item+Private.h:12:
@interface CollectionViewItem (Private)
s/Private/CollectionViewController

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item+Private.h:12:
@interface CollectionViewItem (Private)
In the comment, specify that this is targetting only collection view controller
classes for their bookkeeping.

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item+Private.h:15: 
@property(nonatomic, readwrite, assign) NSInteger type;

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item.h (right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item.h:24: -
(instancetype)init;
You can then remove -init.

Ideally, we would have:

@interface CVI
@property(readonly) type
(no init redeclaration)
@end

@interface CVI (CVC)
@property(readwrite) type
- initWithType:
@end

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm
(right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm:49: _type =
type;
DCHECK type >= kItemTypeEnumStart.

[Moe, 2017-04-14 02:55:16]

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

[commit-bot: I haz the power, 2017-04-14 02:55:32]

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

[Moe, 2017-04-14 02:55:38]

Thank you for the feedback. PTAL.

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/BUILD.gn (right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/BUILD.gn:17:
"collection_view_item+Private.h",
On 2017/04/13 21:42:58, lpromero wrote:
> collection_view_item+collection_view_controller.h

Done.

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item+Private.h
(right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item+Private.h:15: 
On 2017/04/13 21:42:58, lpromero wrote:
> @property(nonatomic, readwrite, assign) NSInteger type;

Done.

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item.h (right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item.h:24: -
(instancetype)init;
On 2017/04/13 21:42:59, lpromero wrote:
> You can then remove -init.
> 
> Ideally, we would have:
> 
> @interface CVI
> @property(readonly) type
> (no init redeclaration)
> @end
> 
> @interface CVI (CVC)
> @property(readwrite) type
> - initWithType:
> @end

RE: -init Done.
I see a couple of problems with moving -initWithType: to the CVI (CVC)
extension:
- Items that inherit from CVI would have to include the header. So it won't be
exclusive to CVCs anymore.
- all existing CVCs and the new ones will have to include the header. Not only
that's a lot of change, it's also not obvious to the authors of new CVCs that
they need to include the header.

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm
(right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm:49: _type =
type;
On 2017/04/13 21:42:59, lpromero wrote:
> DCHECK type >= kItemTypeEnumStart.

This would create a dependency cycle:
//i/c/b/u/collection_view/cells -> //i/c/b/u/collection_view ->
//i/c/b/u/collection_view/cells
I don't think this check is necessary here since it's done in the
CollectionViewModel.

[commit-bot: I haz the power, 2017-04-14 04:47:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 04:47:41]

Dry run: This issue passed the CQ dry run.

[lpromero, 2017-04-14 11:33:58]

lgtm with tests.

https://codereview.chromium.org/2814273004/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/collection_view/cells/collection_view_item+collection_view_controller.h
(right):

https://codereview.chromium.org/2814273004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_item+collection_view_controller.h:13:
// CollectionViewController instances in order to facilitate their bookkeeping.
Maybe rephrase in a more active voice:

Items can be created with a default item type but they need to have a valid item
type to be inserted in the model. Only the CollectionViewController managing
those items is allowed to set the item type.

https://codereview.chromium.org/2814273004/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm
(right):

https://codereview.chromium.org/2814273004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm:47: 
Is this really needed, or does the synthesize take care of it?

If the compiler barfs, you could try:

@implementation CollectionViewItem (CollectionViewController)
@dynamic type;
@end

to tell the compiler to not freak out, there will be the proper getter/setter at
runtime.
Make sure that setting the type after init still works! → add a unit test :)

[lpromero, 2017-04-14 11:37:11]

+Don't forget to update the CL description

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item.h (right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item.h:24: -
(instancetype)init;
On 2017/04/14 02:55:38, moe wrote:
> On 2017/04/13 21:42:59, lpromero wrote:
> > You can then remove -init.
> > 
> > Ideally, we would have:
> > 
> > @interface CVI
> > @property(readonly) type
> > (no init redeclaration)
> > @end
> > 
> > @interface CVI (CVC)
> > @property(readwrite) type
> > - initWithType:
> > @end
> 
> RE: -init Done.
> I see a couple of problems with moving -initWithType: to the CVI (CVC)
> extension:
> - Items that inherit from CVI would have to include the header. So it won't be
> exclusive to CVCs anymore.
> - all existing CVCs and the new ones will have to include the header. Not only
> that's a lot of change, it's also not obvious to the authors of new CVCs that
> they need to include the header.

Acknowledged.

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm
(right):

https://codereview.chromium.org/2814273004/diff/1/ios/chrome/browser/ui/colle...
ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm:49: _type =
type;
On 2017/04/14 02:55:38, moe wrote:
> On 2017/04/13 21:42:59, lpromero wrote:
> > DCHECK type >= kItemTypeEnumStart.
> 
> This would create a dependency cycle:
> //i/c/b/u/collection_view/cells -> //i/c/b/u/collection_view ->
> //i/c/b/u/collection_view/cells
> I don't think this check is necessary here since it's done in the
> CollectionViewModel. 

Acknowledged.

[Moe, 2017-04-17 16:33:22]

Description was changed from

==========
WIP: Makes -init and -setType: available in CollectionViewItem
==========

to

==========
Makes -init and -setType: available in CollectionViewItem

CollectionViewItem can be created with a default item type but it needs to have
a valid item type to be inserted in the model. Only the CollectionViewController
managing the item is allowed to set the item type.
==========

[Moe, 2017-04-17 16:34:14]

Thank you Louis!

https://codereview.chromium.org/2814273004/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/collection_view/cells/collection_view_item+collection_view_controller.h
(right):

https://codereview.chromium.org/2814273004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_item+collection_view_controller.h:13:
// CollectionViewController instances in order to facilitate their bookkeeping.
On 2017/04/14 11:33:58, lpromero wrote:
> Maybe rephrase in a more active voice:
> 
> Items can be created with a default item type but they need to have a valid
item
> type to be inserted in the model. Only the CollectionViewController managing
> those items is allowed to set the item type.

Done.

https://codereview.chromium.org/2814273004/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm
(right):

https://codereview.chromium.org/2814273004/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_item.mm:47: 
On 2017/04/14 11:33:58, lpromero wrote:
> Is this really needed, or does the synthesize take care of it?
> 
> If the compiler barfs, you could try:
> 
> @implementation CollectionViewItem (CollectionViewController)
> @dynamic type;
> @end
> 
> to tell the compiler to not freak out, there will be the proper getter/setter
at
> runtime.
> Make sure that setting the type after init still works! → add a unit test :)

This setter is needed. I believe readonly tells the compiler not to synthesize a
setter method. So while @dynamic quiets the compiler, in runtime, setting the
type causes an
"unrecognized selector sent..." error because no synthesized setter exists.
Re: unit test, Done.

[Moe, 2017-04-17 16:36:28]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-04-17 16:36:28]

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/2814273004/#ps40001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-04-17 16:36:42]

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-04-17 17:32:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492446988248320,
"parent_rev": "6bb5dfd77a43f4c55e1bdc463a6568b27cd6de05", "commit_rev":
"62ecac299486dca3651febd14c7563a415d1da07"}

[commit-bot: I haz the power, 2017-04-17 17:32:13]

Description was changed from

==========
Makes -init and -setType: available in CollectionViewItem

CollectionViewItem can be created with a default item type but it needs to have
a valid item type to be inserted in the model. Only the CollectionViewController
managing the item is allowed to set the item type.
==========

to

==========
Makes -init and -setType: available in CollectionViewItem

CollectionViewItem can be created with a default item type but it needs to have
a valid item type to be inserted in the model. Only the CollectionViewController
managing the item is allowed to set the item type.

Review-Url: https://codereview.chromium.org/2814273004
Cr-Commit-Position: refs/heads/master@{#464943}
Committed:
https://chromium.googlesource.com/chromium/src/+/62ecac299486dca3651febd14c75...
==========

[commit-bot: I haz the power, 2017-04-17 17:32:14]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/62ecac299486dca3651febd14c75...

[Moe, 2017-04-17 20:54:05]

Description was changed from

==========
Makes -init and -setType: available in CollectionViewItem

CollectionViewItem can be created with a default item type but it needs to have
a valid item type to be inserted in the model. Only the CollectionViewController
managing the item is allowed to set the item type.

Review-Url: https://codereview.chromium.org/2814273004
Cr-Commit-Position: refs/heads/master@{#464943}
Committed:
https://chromium.googlesource.com/chromium/src/+/62ecac299486dca3651febd14c75...
==========

to

==========
Makes -init and -setType: available in CollectionViewItem

CollectionViewItem can be created with a default item type but it needs to have
a valid item type to be inserted in the model. Only the CollectionViewController
managing the item is allowed to set the item type.

BUG=711641

Review-Url: https://codereview.chromium.org/2814273004
Cr-Commit-Position: refs/heads/master@{#464943}
Committed:
https://chromium.googlesource.com/chromium/src/+/62ecac299486dca3651febd14c75...
==========