Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691593002:
Connect ContentSuggestionsMediator to the ContentService (Closed)
|
Created:
3 years, 10 months ago by gambard Modified:
3 years, 10 months ago CC:
chromium-reviews, marq+watch_chromium.org, sdefresne+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConnect ContentSuggestionsMediator to the ContentService
ContentSuggestionsMediator expose the suggestions from the ContentService
instead of empty ones.
BUG=686728
Review-Url: https://codereview.chromium.org/2691593002
Cr-Commit-Position: refs/heads/master@{#451586}
Committed: https://chromium.googlesource.com/chromium/src/+/907dd7c27ec76ec75fad3c9659ba323e83376fd7
Patch Set 1 #Patch Set 2 : Fix task runner #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Remove unused method #
Total comments: 31
Patch Set 6 : Address comments #
Total comments: 23
Patch Set 7 : Address comments #Patch Set 8 : Split CL #
Total comments: 7
Patch Set 9 : Address comments #
Total comments: 18
Patch Set 10 : Address comments #
Total comments: 14
Patch Set 11 : Address comments #Dependent Patchsets: Messages
Total messages: 27 (9 generated)
Description was changed from ========== Use data in Content Suggestions Connect the article suggestions from the ContentService with the suggestions UI. BUG=686728 ========== to ========== Display articles in Content Suggestions Connect the article suggestions from the ContentService with the suggestions UI. BUG=686728 ==========
gambard@chromium.org changed reviewers: + lpromero@chromium.org, stkhapugin@chromium.org
PTAL.
Should there be more tests? Much of what is changed in this CL is in implememtation files, so not much candidate for testing. Should things be refactored to be more testable? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:43: return ContentSuggestionsSectionCount; If this is used as a section identifier, it probably shouldn't be called count, but maybe default or other? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:198: sectionInfo.emptyCell = [[ContentSuggestionsItem alloc] initWithType:0 item type 0 should trigger a breakpoint in the model. Can you check? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:30: @property(nonatomic, copy) NSString* text; Please add comments, as this is becoming a larger API. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:34: @property(nonatomic, assign) ContentSuggestionsItemType type; I am not a fan of how the collection view world gets put into this pure model object (pure model as opposed to the CollectionViewModel, which is more of a ViewData or ViewModel kind of model). Cant it be just a NSInteger, and the updater passes here an item type value? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:36: @property(nonatomic, strong) ContentSuggestionsImageUpdater* imageUpdater; Please add a comment for how it is used in the context of this class. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestion.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:30: imageUpdater.delegate = self; To be cleaner, should you also unassigned yourself as delegate of the old updater? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:39: self.image = image; Does this object need to notify to observers/delegate that its image changed? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:37: SectionIdentifier SectionIdentifierForInfo( I would have seen such an "adapter" for the item type too. Why not? I know it's a pain and boilerplate, but at least it keeps things separate. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:14: // ImageUpdater receive an image. receives https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:27: @property(nonatomic, assign) NSInteger sectionIdentifier; Why are these needed? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:31: - (void)receivedImage:(UIImage*)image; Unit test that the delegate is called, with the proper image.
https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:43: return ContentSuggestionsSectionCount; On 2017/02/14 10:52:27, lpromero wrote: > If this is used as a section identifier, it probably shouldn't be called count, > but maybe default or other? It is not a section identifier in regard of the CollectionViewModel meaning. It is a sectionID as in ContentSuggestionsSectionInformation. If you have a better name, please tell :) https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:198: sectionInfo.emptyCell = [[ContentSuggestionsItem alloc] initWithType:0 On 2017/02/14 10:52:27, lpromero wrote: > item type 0 should trigger a breakpoint in the model. Can you check? A breakpoint or a DCHECK? I don't see when it should trigger one. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:30: @property(nonatomic, copy) NSString* text; On 2017/02/14 10:52:27, lpromero wrote: > Please add comments, as this is becoming a larger API. Done. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:34: @property(nonatomic, assign) ContentSuggestionsItemType type; On 2017/02/14 10:52:27, lpromero wrote: > I am not a fan of how the collection view world gets put into this pure model > object (pure model as opposed to the CollectionViewModel, which is more of a > ViewData or ViewModel kind of model). > > Cant it be just a NSInteger, and the updater passes here an item type value? Not sure I understand. Can you clarify? I am using item type to get compilation indications. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:36: @property(nonatomic, strong) ContentSuggestionsImageUpdater* imageUpdater; On 2017/02/14 10:52:27, lpromero wrote: > Please add a comment for how it is used in the context of this class. Done. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestion.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:30: imageUpdater.delegate = self; On 2017/02/14 10:52:27, lpromero wrote: > To be cleaner, should you also unassigned yourself as delegate of the old > updater? It cannot happen for now but it is cleaner. Done. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:39: self.image = image; On 2017/02/14 10:52:27, lpromero wrote: > Does this object need to notify to observers/delegate that its image changed? ContentSuggestion does not have observers/delegate. What do you mean? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:37: SectionIdentifier SectionIdentifierForInfo( On 2017/02/14 10:52:27, lpromero wrote: > I would have seen such an "adapter" for the item type too. Why not? > I know it's a pain and boilerplate, but at least it keeps things separate. Because the sectionID of the SectionInfo is used for ordering also. It is not only a section identifier. The item type is never used as something else than an item type. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:14: // ImageUpdater receive an image. On 2017/02/14 10:52:27, lpromero wrote: > receives Done. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:27: @property(nonatomic, assign) NSInteger sectionIdentifier; On 2017/02/14 10:52:27, lpromero wrote: > Why are these needed? To know the item associated with the ImageUpdater (to update the item and reconfigure the associated cell). https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:31: - (void)receivedImage:(UIImage*)image; On 2017/02/14 10:52:27, lpromero wrote: > Unit test that the delegate is called, with the proper image. Done.
https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:43: return ContentSuggestionsSectionCount; On 2017/02/14 14:03:42, gambard wrote: > On 2017/02/14 10:52:27, lpromero wrote: > > If this is used as a section identifier, it probably shouldn't be called > count, > > but maybe default or other? > > It is not a section identifier in regard of the CollectionViewModel meaning. > It is a sectionID as in ContentSuggestionsSectionInformation. > If you have a better name, please tell :) Does it act as a "Unknown" category then? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:198: sectionInfo.emptyCell = [[ContentSuggestionsItem alloc] initWithType:0 On 2017/02/14 14:03:42, gambard wrote: > On 2017/02/14 10:52:27, lpromero wrote: > > item type 0 should trigger a breakpoint in the model. Can you check? > > A breakpoint or a DCHECK? > I don't see when it should trigger one. Sorry, yes, a DCHECK. https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/collection_view/co... https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:34: @property(nonatomic, assign) ContentSuggestionsItemType type; On 2017/02/14 14:03:42, gambard wrote: > On 2017/02/14 10:52:27, lpromero wrote: > > I am not a fan of how the collection view world gets put into this pure model > > object (pure model as opposed to the CollectionViewModel, which is more of a > > ViewData or ViewModel kind of model). > > > > Cant it be just a NSInteger, and the updater passes here an item type value? > > Not sure I understand. Can you clarify? > I am using item type to get compilation indications. I know it helps with compiler, but it makes this model class depend on this implementation detail, and literally depend on collection_view_model. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestion.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:39: self.image = image; On 2017/02/14 14:03:42, gambard wrote: > On 2017/02/14 10:52:27, lpromero wrote: > > Does this object need to notify to observers/delegate that its image changed? > > ContentSuggestion does not have observers/delegate. What do you mean? I know they don't. The question is: should they? How will users of a ContentSuggestion know when to requery? https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:27: @property(nonatomic, assign) NSInteger sectionIdentifier; On 2017/02/14 14:03:43, gambard wrote: > On 2017/02/14 10:52:27, lpromero wrote: > > Why are these needed? > > To know the item associated with the ImageUpdater (to update the item and > reconfigure the associated cell). Ack.
This CL is too big, please break down into several. Should be relatively straightforward, since you can add a lot of this code without calling it or add classes without instantiating it. I'm asking for this because I've started reviewing it in detail but at some point gave up and went to big picture, where I gave up and went to offline discussion where I gave up and went to design doc. Too much complexity for one code review. Sorry for extra work! Please also link these smaller CLs to the design doc, which is a great way to make sure the big picture is seen. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:62: @property(nonatomic, strong) NSMutableDictionary* sectionInformationByCategory; Needs comment. What are the keys, objects and how this needs to be updated. It's also nice to use the modern generic syntax: @property(nonatomic, strong) NSMutableDictionary<KeyType*, ValueType*>* dictName; Bonus points for nullability on this property. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:69: // Converts the |contentsuggestion| to a ContentSuggestion. Update the arg name with same capitalization. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:83: @synthesize sectionInformationByCategory = _sectionInformationByCategory; Is it necessary? I keep forgetting :( https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:108: if (![self.sectionInformationByCategory Use modern syntax: if (!self.sectionInformationByCategory[...]) Bonus points for introducing a convenience initializer in ContentSUggestionsCategoryWrapper: + (ContentSuggestionsCategoryWrapper*)wrapperWithCategory:(...)category { return [[self alloc] initWithCategory:category]; // needs autorelease if not built with ARC } https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:167: - (ContentSuggestion*)convertContentSuggestion: This should be a method on ContentSuggestion (possibly you can have a category on ContentSuggestion that initializes it with ntp_snippets::ContentSuggestion. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:194: sectionInfo.layout = SectionLayoutForLayout(categoryInfo->card_layout()); All this setup of ContentSuggestionsSectionInformation with ntp_snippets::CategoryInfo really belongs to ContentSuggestionsSectionInformation methods. Seems like your code will benefit from: + (ContentSuggestionsSectionInformation*)sectionInformationWithCategory:(ntp_snippets::Category)category categoryInfo(base::Optional<...>)sectionInfo { // move code form this method there } https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:207: forKey:[[ContentSuggestionsCategoryWrapper alloc] Use modern syntax: self.sectionInformationByCategory[key] = value; https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:29: @property(nonatomic, copy) NSString* title; Optional: this is a really small header, why don't you add nullability? :) https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:44: @property(nonatomic, strong) ContentSuggestionsImageUpdater* imageUpdater; Discussed offline https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:33: - (void)receivedImage:(UIImage*)image; Per offline discussion, this is not a great way of doing this plumbing. Can you hold off with the image logic for this CL altogether? This CL is really huge.
Patchset #8 (id:140001) has been deleted
Description was changed from ========== Display articles in Content Suggestions Connect the article suggestions from the ContentService with the suggestions UI. BUG=686728 ========== to ========== Connect ContentSuggestionsMediator to the ContentService ContentSuggestionsMediator expose the suggestions from the ContentService instead of empty ones. BUG=686728 ==========
Thanks. I have split this CL, PTAL. 2nd CL: https://codereview.chromium.org/2697843003 https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:43: return ContentSuggestionsSectionCount; On 2017/02/14 15:43:32, lpromero wrote: > On 2017/02/14 14:03:42, gambard wrote: > > On 2017/02/14 10:52:27, lpromero wrote: > > > If this is used as a section identifier, it probably shouldn't be called > > count, > > > but maybe default or other? > > > > It is not a section identifier in regard of the CollectionViewModel meaning. > > It is a sectionID as in ContentSuggestionsSectionInformation. > > If you have a better name, please tell :) > > Does it act as a "Unknown" category then? Kind of. The name was "NotImplemented" before. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/cont... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:198: sectionInfo.emptyCell = [[ContentSuggestionsItem alloc] initWithType:0 On 2017/02/14 15:43:32, lpromero wrote: > On 2017/02/14 14:03:42, gambard wrote: > > On 2017/02/14 10:52:27, lpromero wrote: > > > item type 0 should trigger a breakpoint in the model. Can you check? > > > > A breakpoint or a DCHECK? > > I don't see when it should trigger one. > > Sorry, yes, a DCHECK. > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/collection_view/co... Yes. I am removing it and adding a TODO for now. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:34: @property(nonatomic, assign) ContentSuggestionsItemType type; On 2017/02/14 15:43:32, lpromero wrote: > On 2017/02/14 14:03:42, gambard wrote: > > On 2017/02/14 10:52:27, lpromero wrote: > > > I am not a fan of how the collection view world gets put into this pure > model > > > object (pure model as opposed to the CollectionViewModel, which is more of a > > > ViewData or ViewModel kind of model). > > > > > > Cant it be just a NSInteger, and the updater passes here an item type value? > > > > Not sure I understand. Can you clarify? > > I am using item type to get compilation indications. > > I know it helps with compiler, but it makes this model class depend on this > implementation detail, and literally depend on collection_view_model. Done. https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestion.mm (right): https://codereview.chromium.org/2691593002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:39: self.image = image; On 2017/02/14 15:43:32, lpromero wrote: > On 2017/02/14 14:03:42, gambard wrote: > > On 2017/02/14 10:52:27, lpromero wrote: > > > Does this object need to notify to observers/delegate that its image > changed? > > > > ContentSuggestion does not have observers/delegate. What do you mean? > > I know they don't. The question is: should they? How will users of a > ContentSuggestion know when to requery? Ok, the imageUpdater thing is not clear at all :) As stepan suggested, I will add it in a separate CL. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:62: @property(nonatomic, strong) NSMutableDictionary* sectionInformationByCategory; On 2017/02/14 16:34:18, stkhapugin wrote: > Needs comment. What are the keys, objects and how this needs to be updated. It's > also nice to use the modern generic syntax: > > @property(nonatomic, strong) NSMutableDictionary<KeyType*, ValueType*>* > dictName; > > Bonus points for nullability on this property. Done. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:69: // Converts the |contentsuggestion| to a ContentSuggestion. On 2017/02/14 16:34:18, stkhapugin wrote: > Update the arg name with same capitalization. Done. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:83: @synthesize sectionInformationByCategory = _sectionInformationByCategory; On 2017/02/14 16:34:18, stkhapugin wrote: > Is it necessary? I keep forgetting :( Seems so :) https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:108: if (![self.sectionInformationByCategory On 2017/02/14 16:34:18, stkhapugin wrote: > Use modern syntax: if (!self.sectionInformationByCategory[...]) > > Bonus points for introducing a convenience initializer in > ContentSUggestionsCategoryWrapper: > > + (ContentSuggestionsCategoryWrapper*)wrapperWithCategory:(...)category { > return [[self alloc] initWithCategory:category]; // needs autorelease if not > built with ARC > } Done. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:167: - (ContentSuggestion*)convertContentSuggestion: On 2017/02/14 16:34:18, stkhapugin wrote: > This should be a method on ContentSuggestion (possibly you can have a category > on ContentSuggestion that initializes it with ntp_snippets::ContentSuggestion. ContentSuggestion should not know about ntp_snippets::*. I am moving it to a C function. It will allow easier refactoring if needed. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:194: sectionInfo.layout = SectionLayoutForLayout(categoryInfo->card_layout()); On 2017/02/14 16:34:18, stkhapugin wrote: > All this setup of ContentSuggestionsSectionInformation with > ntp_snippets::CategoryInfo really belongs to > ContentSuggestionsSectionInformation methods. Seems like your code will benefit > from: > > + > (ContentSuggestionsSectionInformation*)sectionInformationWithCategory:(ntp_snippets::Category)category > categoryInfo(base::Optional<...>)sectionInfo { > // move code form this method there > } Same. UI code should not know about ntp_snippets::*. I am moving the creation code to a C function. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:207: forKey:[[ContentSuggestionsCategoryWrapper alloc] On 2017/02/14 16:34:18, stkhapugin wrote: > Use modern syntax: > > self.sectionInformationByCategory[key] = value; Done. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:29: @property(nonatomic, copy) NSString* title; On 2017/02/14 16:34:18, stkhapugin wrote: > Optional: this is a really small header, why don't you add nullability? :) Done. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:44: @property(nonatomic, strong) ContentSuggestionsImageUpdater* imageUpdater; On 2017/02/14 16:34:19, stkhapugin wrote: > Discussed offline Done. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_image_updater.h:33: - (void)receivedImage:(UIImage*)image; On 2017/02/14 16:34:19, stkhapugin wrote: > Per offline discussion, this is not a great way of doing this plumbing. Can you > hold off with the image logic for this CL altogether? This CL is really huge. Done.
https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:167: - (ContentSuggestion*)convertContentSuggestion: On 2017/02/15 10:17:56, gambard wrote: > On 2017/02/14 16:34:18, stkhapugin wrote: > > This should be a method on ContentSuggestion (possibly you can have a category > > on ContentSuggestion that initializes it with ntp_snippets::ContentSuggestion. > > ContentSuggestion should not know about ntp_snippets::*. > I am moving it to a C function. It will allow easier refactoring if needed. A common Objecitve-C pattern is: @class ModelClass; @interface ViewClass : ... - (instancetype)initWithUIThingie:(Thingie*)thingie otherThingie:(Thingie*)thingie; @end // Possibly in a different file @interface ViewClass (InitWitModelClass) - (instancetype)initWithModel:(ModelClass*)model; @end @implementation ViewClass(InitWithModelCLass) - (instancetype)initWithModel:(ModelClass*)model { Thingie* firstUIThingieFromModel = helper(model); Thingie* secondUIThingieFromModel = other_helper(model); return [self initWithUIThingie:firstUIThingieFromModel otherThingie:secondUIThingieFromModel]; } @end This way you have a clear separation between UI and model code, but you keep the code on the boundary in a separate file that can be included anywhere such convertion is necessary. This pattern is used all the time for UICollectionViewCell to be configured with a model object. Your case is really similar. But you don't have to do this. https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:19: // Data for a suggestions item, compatible with Objective-C. Optional: maybe something like - objc wrapper for ntp_snippets::suggestions_item? This way it's easy to understand what is a suggestions item (I hope the ntp:: class actually has a reasonable description) https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.mm (right): https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:21: remove newline https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h (right): https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:42: @property(nonatomic, strong) CollectionViewItem* emptyCell; Why are these not readonly anymore? Can these be modified after creation?
Thanks, PTAL. https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:167: - (ContentSuggestion*)convertContentSuggestion: On 2017/02/15 15:35:42, stkhapugin wrote: > On 2017/02/15 10:17:56, gambard wrote: > > On 2017/02/14 16:34:18, stkhapugin wrote: > > > This should be a method on ContentSuggestion (possibly you can have a > category > > > on ContentSuggestion that initializes it with > ntp_snippets::ContentSuggestion. > > > > ContentSuggestion should not know about ntp_snippets::*. > > I am moving it to a C function. It will allow easier refactoring if needed. > > A common Objecitve-C pattern is: > > @class ModelClass; > > @interface ViewClass : ... > > - (instancetype)initWithUIThingie:(Thingie*)thingie > otherThingie:(Thingie*)thingie; > > @end > > // Possibly in a different file > > @interface ViewClass (InitWitModelClass) > > - (instancetype)initWithModel:(ModelClass*)model; > > @end > > @implementation ViewClass(InitWithModelCLass) > > - (instancetype)initWithModel:(ModelClass*)model { > Thingie* firstUIThingieFromModel = helper(model); > Thingie* secondUIThingieFromModel = other_helper(model); > > return [self initWithUIThingie:firstUIThingieFromModel > otherThingie:secondUIThingieFromModel]; > } > > @end > > > > This way you have a clear separation between UI and model code, but you keep the > code on the boundary in a separate file that can be included anywhere such > convertion is necessary. > > This pattern is used all the time for UICollectionViewCell to be configured with > a model object. Your case is really similar. But you don't have to do this. Acknowledged. https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:19: // Data for a suggestions item, compatible with Objective-C. On 2017/02/15 15:35:42, stkhapugin wrote: > Optional: maybe something like - objc wrapper for > ntp_snippets::suggestions_item? This way it's easy to understand what is a > suggestions item (I hope the ntp:: class actually has a reasonable description) Well, the description is about the same :D Done. https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.mm (right): https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:21: On 2017/02/15 15:35:43, stkhapugin wrote: > remove newline Done. https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h (right): https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:42: @property(nonatomic, strong) CollectionViewItem* emptyCell; On 2017/02/15 15:35:43, stkhapugin wrote: > Why are these not readonly anymore? Can these be modified after creation? No, but the creation is in multiple steps (see ContentSuggestionsMediator's SectionInformationFromCategoryInfo), it is easier to set the property this way. And none of them are mandatory.
https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h (right): https://codereview.chromium.org/2691593002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.h:42: @property(nonatomic, strong) CollectionViewItem* emptyCell; On 2017/02/16 10:06:30, gambard wrote: > On 2017/02/15 15:35:43, stkhapugin wrote: > > Why are these not readonly anymore? Can these be modified after creation? > > No, but the creation is in multiple steps (see ContentSuggestionsMediator's > SectionInformationFromCategoryInfo), it is easier to set the property this way. > And none of them are mandatory. So you agree that it's better and safer to make everything readonly, but you won't do this because in one method where you instantiate this class you don't want to create a few local variables? :) https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:75: return sectionInfo; Does this make sense to return an empty section info at all? Are you presenting this empty section? If not, return nil here (and don't add it to the dictionary later) https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.mm (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:12: Unless you add something here in a future CL, remove the anonymous extension. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm (left): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm:28: _layout = layout; I really find this change weird. You start with great code, and change it to become okay code.
gambard@chromium.org changed reviewers: + marq@chromium.org
+marq for content_suggestions_section_information.h API change :)
https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/100001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:83: @synthesize sectionInformationByCategory = _sectionInformationByCategory; On 2017/02/15 10:17:56, gambard wrote: > On 2017/02/14 16:34:18, stkhapugin wrote: > > Is it necessary? I keep forgetting :( > > Seems so :) We have not yet turned on property autosynthesis. It has been discussed, and feel free to bring it up again. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h:23: // The category wrapped by this object. Should this be a readonly property? https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:61: ContentSuggestionsSectionInformation* SectionInformationFromCategoryInfo( Comment for this function. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:16: ContentSuggestionTypeArticle Set an initial value for the first enum value. Any reason why this doesn't follow Chromium's MACRO_STYLE for enums? https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:19: // Data for a suggestions item, compatible with Objective-C. Most acts as a Most -> Mostly https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm:17: @synthesize ID = _ID; Usually I like shorter names, but 'ID' (and especially '_ID') looks like it might mean something special (like _cmd does), even though it actually doesn't. I'd find it a bit more readable if it were 'sectionID' instead.
Thanks, PTAL. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h:23: // The category wrapped by this object. On 2017/02/16 13:14:35, marq wrote: > Should this be a readonly property? I moved from a readonly property to a method because I am not really backing it by any ivar. +stkhapugin. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:61: ContentSuggestionsSectionInformation* SectionInformationFromCategoryInfo( On 2017/02/16 13:14:35, marq wrote: > Comment for this function. Done. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:75: return sectionInfo; On 2017/02/16 10:53:27, stkhapugin wrote: > Does this make sense to return an empty section info at all? Are you presenting > this empty section? If not, return nil here (and don't add it to the dictionary > later) Presenting an empty section make sense. For now it should not happen but I don't mind if it happens. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:16: ContentSuggestionTypeArticle On 2017/02/16 13:14:35, marq wrote: > Set an initial value for the first enum value. > > Any reason why this doesn't follow Chromium's MACRO_STYLE for enums? Because almost all "typedef NS_ENUM" are using MacroStyle in our current code. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:19: // Data for a suggestions item, compatible with Objective-C. Most acts as a On 2017/02/16 13:14:35, marq wrote: > Most -> Mostly Done. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.mm (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.mm:12: On 2017/02/16 10:53:27, stkhapugin wrote: > Unless you add something here in a future CL, remove the anonymous extension. Done. https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm (left): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm:28: _layout = layout; On 2017/02/16 10:53:27, stkhapugin wrote: > I really find this change weird. You start with great code, and change it to > become okay code. Can we have your opinion on this marq@? https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm:17: @synthesize ID = _ID; On 2017/02/16 13:14:36, marq wrote: > Usually I like shorter names, but 'ID' (and especially '_ID') looks like it > might mean something special (like _cmd does), even though it actually doesn't. > I'd find it a bit more readable if it were 'sectionID' instead. Done.
lgtm https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h (right): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h:23: // The category wrapped by this object. On 2017/02/16 13:45:53, gambard wrote: > On 2017/02/16 13:14:35, marq wrote: > > Should this be a readonly property? > > I moved from a readonly property to a method because I am not really backing it > by any ivar. > +stkhapugin. I prefer it as a method, since a property really is necessary for either a) synthesizing the ivar or b) using this as a keypath; neither is useful so no need for property. Even readonly property has to have an (implicit) storage classifier, which is logically incompatible with having no backing ivar.
lgtm https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm (left): https://codereview.chromium.org/2691593002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestions_section_information.mm:28: _layout = layout; On 2017/02/16 13:45:53, gambard wrote: > On 2017/02/16 10:53:27, stkhapugin wrote: > > I really find this change weird. You start with great code, and change it to > > become okay code. > > Can we have your opinion on this marq@? I'm fine with the change.
lgtm https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h (right): https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h:19: NS_DESIGNATED_INITIALIZER; Optional: Do you need this initializer anywhere (outside of +wrapperWithCategory:)? You could keep -init NS_UNAVAILABLE and force everyone to use +wrapperWithCategory: instead I guess. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm (right): https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:54: return self.category == other.category; This is not a real property. Please don't use dot syntax here. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:65: [[[self class] allocWithZone:zone] initWithCategory:self.category]; Idem, use [self category] instead. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:27: // Returns the ItemType for this |category|. The returned value is not an ItemType is it? https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:29: return ContentSuggestionTypeArticle; Optional: Maybe add a comment saying: For now, only Article is a relevant type. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:50: // Converts the |contentSuggestion| to a ContentSuggestion. Optional: Maybe mentioning only the types would be clearer? Like: Converts a ntp_snippets::ContentSuggestion to a(n Objective-C) ContentSuggestion. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:32: // Section information in which this suggestion should be. As discusses, this could be in a protocol (like ContentSuggestionIdentification), or in an object you compose with.
Thanks! https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h (right): https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.h:19: NS_DESIGNATED_INITIALIZER; On 2017/02/17 15:47:51, lpromero wrote: > Optional: Do you need this initializer anywhere (outside of > +wrapperWithCategory:)? You could keep -init NS_UNAVAILABLE and force everyone > to use +wrapperWithCategory: instead I guess. Is there a benefit to put the init private? https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm (right): https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:54: return self.category == other.category; On 2017/02/17 15:47:51, lpromero wrote: > This is not a real property. Please don't use dot syntax here. Done. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_category_wrapper.mm:65: [[[self class] allocWithZone:zone] initWithCategory:self.category]; On 2017/02/17 15:47:51, lpromero wrote: > Idem, use [self category] instead. Done. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm (right): https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:27: // Returns the ItemType for this |category|. On 2017/02/17 15:47:51, lpromero wrote: > The returned value is not an ItemType is it? Done. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:29: return ContentSuggestionTypeArticle; On 2017/02/17 15:47:51, lpromero wrote: > Optional: Maybe add a comment saying: For now, only Article is a relevant type. Done. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/con... ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:50: // Converts the |contentSuggestion| to a ContentSuggestion. On 2017/02/17 15:47:51, lpromero wrote: > Optional: Maybe mentioning only the types would be clearer? > Like: Converts a ntp_snippets::ContentSuggestion to a(n Objective-C) > ContentSuggestion. Done. https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/content_suggestions/content_suggestion.h (right): https://codereview.chromium.org/2691593002/diff/200001/ios/chrome/browser/ui/... ios/chrome/browser/ui/content_suggestions/content_suggestion.h:32: // Section information in which this suggestion should be. On 2017/02/17 15:47:51, lpromero wrote: > As discusses, this could be in a protocol (like > ContentSuggestionIdentification), or in an object you compose with. I will land this CL as is and create the object when I will use it.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stkhapugin@chromium.org, marq@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2691593002/#ps220001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487582899990300,
"parent_rev": "592d703b32a81520da604cfd6e7d680aaa4b8fc1", "commit_rev":
"907dd7c27ec76ec75fad3c9659ba323e83376fd7"}
Message was sent while issue was closed.
Description was changed from ========== Connect ContentSuggestionsMediator to the ContentService ContentSuggestionsMediator expose the suggestions from the ContentService instead of empty ones. BUG=686728 ========== to ========== Connect ContentSuggestionsMediator to the ContentService ContentSuggestionsMediator expose the suggestions from the ContentService instead of empty ones. BUG=686728 Review-Url: https://codereview.chromium.org/2691593002 Cr-Commit-Position: refs/heads/master@{#451586} Committed: https://chromium.googlesource.com/chromium/src/+/907dd7c27ec76ec75fad3c9659ba... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/907dd7c27ec76ec75fad3c9659ba... |
