Add an ID to the ContentSuggestion

Add an ID to the ContentSuggestion

This CL adds an ID for the ContentSuggestion, allowing them to be uniquely
identified.

BUG=693031
Review-Url: https://codereview.chromium.org/2708623002
Cr-Commit-Position: refs/heads/master@{#451768}
Committed: https://chromium.googlesource.com/chromium/src/+/d0bce4262751f95ad57a02065b15ba4c561922b6

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

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

[gambard, 2017-02-20 09:33:14]

Following the offline discussion about the ID. This more about high level
comments than code review.

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestion_id.h (right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestion_id.h:17:
@property(nonatomic, strong) ContentSuggestionsSectionInformation* sectionInfo;
I am not sure about using a ContentSuggestionsSectionInformation in suggestionID
(at least the name).
Do you have an opinion?

[gambard, 2017-02-21 10:11:17]

ping :)

[lpromero, 2017-02-21 10:33:34]

lgtm with comments

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/content_...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/content_...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:62:
contentSuggestion.id().id_within_category();
Out of curiosity: Here the lingo goes from category to section. Still ok?

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestion.h:13: @class
ContentSuggestionsSectionInformation;
Can remove.

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestion_id.h (right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestion_id.h:17:
@property(nonatomic, strong) ContentSuggestionsSectionInformation* sectionInfo;
On 2017/02/20 09:33:14, gambard wrote:
> I am not sure about using a ContentSuggestionsSectionInformation in
suggestionID
> (at least the name).
> Do you have an opinion?

I am more unsure about ContentSuggestionID.IDInSection. What about:

@interface ContentSuggestionIdentifier : NSObject
@property ContentSuggestionsSection* section;
@property std::string IDInSection;
@end

@protocol ContentSuggestionIdentification
@property ContentSuggestionIdentifier* suggestionIdentifier;
@end

This is the pattern used for UIAccessibilityIdentification and
accessibilityIdentifier for example.

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestion_id.mm (right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestion_id.mm:33:
self.IDInSection == other.IDInSection;
Unittests for equality.

[gambard, 2017-02-21 14:37:48]

Thanks!

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/content_...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/content_...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:62:
contentSuggestion.id().id_within_category();
On 2017/02/21 10:33:34, lpromero wrote:
> Out of curiosity: Here the lingo goes from category to section. Still ok?

For me it makes sense because this class is making a transition from the
C++/model world, where the suggestions are in categories, to the Objective-C/UI
world, where the suggestions are items in sections.
And maybe we will have a 1 to N mapping between Categories and Sections.

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestion.h:13: @class
ContentSuggestionsSectionInformation;
On 2017/02/21 10:33:34, lpromero wrote:
> Can remove.

Done.

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestion_id.h (right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestion_id.h:17:
@property(nonatomic, strong) ContentSuggestionsSectionInformation* sectionInfo;
On 2017/02/21 10:33:34, lpromero wrote:
> On 2017/02/20 09:33:14, gambard wrote:
> > I am not sure about using a ContentSuggestionsSectionInformation in
> suggestionID
> > (at least the name).
> > Do you have an opinion?
> 
> I am more unsure about ContentSuggestionID.IDInSection. What about:
> 
> @interface ContentSuggestionIdentifier : NSObject
> @property ContentSuggestionsSection* section;
> @property std::string IDInSection;
> @end
> 
> @protocol ContentSuggestionIdentification
> @property ContentSuggestionIdentifier* suggestionIdentifier;
> @end
> 
> This is the pattern used for UIAccessibilityIdentification and
> accessibilityIdentifier for example.

Done.

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestion_id.mm (right):

https://codereview.chromium.org/2708623002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestion_id.mm:33:
self.IDInSection == other.IDInSection;
On 2017/02/21 10:33:34, lpromero wrote:
> Unittests for equality.

Done.

[gambard, 2017-02-21 14:37:59]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-02-21 14:37:59]

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/2708623002/#ps60001
(title: "Add tests")

[commit-bot: I haz the power, 2017-02-21 14:38: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-02-21 15:50:42]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487687879168540,
"parent_rev": "6617e48c6e0733436a8fc430a17399038975926e", "commit_rev":
"d0bce4262751f95ad57a02065b15ba4c561922b6"}

[commit-bot: I haz the power, 2017-02-21 15:51:37]

Description was changed from

==========
Add an ID to the ContentSuggestion

This CL adds an ID for the ContentSuggestion, allowing them to be uniquely
identified.

BUG=693031
==========

to

==========
Add an ID to the ContentSuggestion

This CL adds an ID for the ContentSuggestion, allowing them to be uniquely
identified.

BUG=693031

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

[commit-bot: I haz the power, 2017-02-21 15:51:38]

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