Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640473002:
Suggestions UI - favicon item (Closed)
|
Created:
3 years, 11 months ago by gambard Modified:
3 years, 11 months ago CC:
chromium-reviews, marq+watch_chromium.org, lpromero+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSuggestions UI - favicon item
BUG=679369
Review-Url: https://codereview.chromium.org/2640473002
Cr-Commit-Position: refs/heads/master@{#445069}
Committed: https://chromium.googlesource.com/chromium/src/+/7de29d98ff473630649264e9837bfc4235d0921d
Patch Set 1 #Patch Set 2 : Fix tests #
Total comments: 42
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Fix compilation #
Total comments: 10
Patch Set 6 : Address marq comments #Patch Set 7 : Change function name #Patch Set 8 : Fix build #Messages
Total messages: 30 (10 generated)
gambard@chromium.org changed reviewers: + lpromero@chromium.org, marq@chromium.org
PTAL.
https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.h (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.h:20: ItemTypeFavicon, No action needed: At some point you'll want to distance yourself from calling these based on their look and start calling them based on the feature they display. Since here you have only placeholders, like a catalog of special cells, it still makes sense. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:13: #import "ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h" Should it be SuggestionsFaviconsItem? https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:40: toSectionWithIdentifier:sectionIdentifier++]; I am not a fan of using incrementing sectionIdentifier for static lists of sections like here. Let say you conditionally add or not the favicon section. You have no way to know the section identifier of the section that holds the standard, article and expandable items. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:127: return section == 0 || section == 1; Here you should translate to sectionIdentifier and then check for the particular sectionIdentifier value. That's where having a static enum for sections is useful. In the example of the conditionally added favicon section, this would break here, because in the case of the favicon section not added, you shouldn't return YES for section 1. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_commands.h (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_commands.h:18: - (void)openFaviconAtIndexPath:(NSIndexPath*)indexPath; This should just be an index. The fact that it is backed by a cell, at a given index path is an implementation detail. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.h (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.h:10: @interface SuggestionsFaviconInternalCell : UICollectionViewCell Comments on the class. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.h:13: @property(nonatomic, strong) UILabel* title; s/title/titleLabel https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:29: _title.font = [[MDFRobotoFontLoader sharedInstance] lightFontOfSize:10]; It is not available via MDCTypography? https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:43: constraintEqualToAnchor:self.contentView.centerXAnchor], Since you set the leading and trailing for the title below, you don't need this, do you? https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:17: - (void)openFaviconAtIndexPath:(NSIndexPath*)indexPath; No action needed: It might be fine here to pass an index path, as the cell publicly exposes its collection view. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:23: // this item. The collection view scroll horizontally. scrolls https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:28: // Adds a favicon with an |favicon| image and a |title| to this item. s/an/a https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:29: - (void)addFavicon:(UIImage*)favicon withTitle:(NSString*)title; Should you instead make the API accept an array? It would pair nicely with the index you get back from the delegate. (You might need to create a dat holder class that holds the title and favicon. It might just be what the mediator would give to the view controller in input, right?) https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:25: const CGFloat kInternalLeftSpacing = 16; s/left/leading, idem in the comment. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:81: cell.delegate = self.delegate; Add a prepareForReuse to the cell, where these are unset. Also reset the faviconView.image and title.text. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:118: _collectionView.translatesAutoresizingMaskIntoConstraints = NO; You might need to add that before adding as subview? I find it cleaner and avoids the system to resize the view to satisfy the autoresizing masks constraints when the view is added, for in fine removing the constraints. As a general question, do you get unsatisfiable constraints errors in the console when you display this? https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item_unittest.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item_unittest.mm:41: ASSERT_TRUE([cell isMemberOfClass:[SuggestionsFaviconCell class]]); Optional: I almost commented on line 26 to use isMemberOfClass, but actually, line 26 has an advantage when the classes don't match: the log tells you the expected class and the actual class. Whereas here, you'll get: Expected: YES, Actual: NO. Which doesn't quickly help. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:80: [self.suggestionCommandHandler openFaviconAtIndexPath:indexPath]; This is where I think the suggestionCommandHandler should only get an index back, not an index path.
Thanks, PTAL. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.h (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.h:20: ItemTypeFavicon, On 2017/01/18 09:34:38, lpromero wrote: > No action needed: At some point you'll want to distance yourself from calling > these based on their look and start calling them based on the feature they > display. Since here you have only placeholders, like a catalog of special cells, > it still makes sense. Yes, for now this is only demo. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:13: #import "ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h" On 2017/01/18 09:34:38, lpromero wrote: > Should it be SuggestionsFaviconsItem? In all class names favicon is used. So I followed the trend :) https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:40: toSectionWithIdentifier:sectionIdentifier++]; On 2017/01/18 09:34:38, lpromero wrote: > I am not a fan of using incrementing sectionIdentifier for static lists of > sections like here. Let say you conditionally add or not the favicon section. > You have no way to know the section identifier of the section that holds the > standard, article and expandable items. I know, but once again, this is for demo :) The content of this function should be completely removed once we use a mediator to pipe the data in. I am waiting to see the interface with the data providers to construct a meaningful mediator. Once the mediator is built, the data will come from it and the ItemType and the SectionIdentifier will (hopefully) be more meaningful. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:127: return section == 0 || section == 1; On 2017/01/18 09:34:38, lpromero wrote: > Here you should translate to sectionIdentifier and then check for the particular > sectionIdentifier value. That's where having a static enum for sections is > useful. In the example of the conditionally added favicon section, this would > break here, because in the case of the favicon section not added, you shouldn't > return YES for section 1. Same as the other comment. This will be rewritten once the mediator comes in. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_commands.h (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_commands.h:18: - (void)openFaviconAtIndexPath:(NSIndexPath*)indexPath; On 2017/01/18 09:34:38, lpromero wrote: > This should just be an index. The fact that it is backed by a cell, at a given > index path is an implementation detail. Done. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.h (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.h:10: @interface SuggestionsFaviconInternalCell : UICollectionViewCell On 2017/01/18 09:34:38, lpromero wrote: > Comments on the class. Done. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.h:13: @property(nonatomic, strong) UILabel* title; On 2017/01/18 09:34:38, lpromero wrote: > s/title/titleLabel Done. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:29: _title.font = [[MDFRobotoFontLoader sharedInstance] lightFontOfSize:10]; On 2017/01/18 09:34:38, lpromero wrote: > It is not available via MDCTypography? Done. It is but showcase does not load the roboto font in MDCTypography. [MDCTypography setFontLoader:[MDFRobotoFontLoader sharedInstance]]; should probably be added to showcase app delegate. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:43: constraintEqualToAnchor:self.contentView.centerXAnchor], On 2017/01/18 09:34:38, lpromero wrote: > Since you set the leading and trailing for the title below, you don't need this, > do you? Done. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:17: - (void)openFaviconAtIndexPath:(NSIndexPath*)indexPath; On 2017/01/18 09:34:38, lpromero wrote: > No action needed: It might be fine here to pass an index path, as the cell > publicly exposes its collection view. Acknowledged. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:23: // this item. The collection view scroll horizontally. On 2017/01/18 09:34:38, lpromero wrote: > scrolls Done. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:28: // Adds a favicon with an |favicon| image and a |title| to this item. On 2017/01/18 09:34:38, lpromero wrote: > s/an/a Done. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:29: - (void)addFavicon:(UIImage*)favicon withTitle:(NSString*)title; On 2017/01/18 09:34:38, lpromero wrote: > Should you instead make the API accept an array? It would pair nicely with the > index you get back from the delegate. (You might need to create a dat holder > class that holds the title and favicon. It might just be what the mediator would > give to the view controller in input, right?) My original intention was to pass an array of data holder. But as this API might change when the mediator is created, I went for the simplest way to add a favicon. I don't know if the favicons will all be created at the same time or not, how they are added... I will keep the API as is for now and see how I can optimize once I know which information can be passed to it. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:25: const CGFloat kInternalLeftSpacing = 16; On 2017/01/18 09:34:38, lpromero wrote: > s/left/leading, idem in the comment. Done. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:81: cell.delegate = self.delegate; On 2017/01/18 09:34:38, lpromero wrote: > Add a prepareForReuse to the cell, where these are unset. Also reset the > faviconView.image and title.text. Done. To reset the image and the title, I have added it to the inner cells. I have not used -prepareForReuse on any other cell. Should I? https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:118: _collectionView.translatesAutoresizingMaskIntoConstraints = NO; On 2017/01/18 09:34:38, lpromero wrote: > You might need to add that before adding as subview? I find it cleaner and > avoids the system to resize the view to satisfy the autoresizing masks > constraints when the view is added, for in fine removing the constraints. > > As a general question, do you get unsatisfiable constraints errors in the > console when you display this? Done. I don't unsatisfiable constraints. I have some ambiguous constraints on other type of cell (SuggestionsArticleCell). In lot of places it is done after adding it to the view, so if it would create conflicts we would have seen it before. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item_unittest.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item_unittest.mm:41: ASSERT_TRUE([cell isMemberOfClass:[SuggestionsFaviconCell class]]); On 2017/01/18 09:34:39, lpromero wrote: > Optional: I almost commented on line 26 to use isMemberOfClass, but actually, > line 26 has an advantage when the classes don't match: the log tells you the > expected class and the actual class. Whereas here, you'll get: > Expected: YES, Actual: NO. Which doesn't quickly help. Done. This was the way it is done in most of the other "CellIsConfigured" tests. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:80: [self.suggestionCommandHandler openFaviconAtIndexPath:indexPath]; On 2017/01/18 09:34:39, lpromero wrote: > This is where I think the suggestionCommandHandler should only get an index > back, not an index path. Done.
https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:81: cell.delegate = self.delegate; On 2017/01/18 12:37:58, gambard wrote: > On 2017/01/18 09:34:38, lpromero wrote: > > Add a prepareForReuse to the cell, where these are unset. Also reset the > > faviconView.image and title.text. > > Done. > To reset the image and the title, I have added it to the inner cells. > I have not used -prepareForReuse on any other cell. Should I? Usually I reset everything I can have set in configureCell: and -collectionView:cellForItemAtIndexPath:. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:118: _collectionView.translatesAutoresizingMaskIntoConstraints = NO; On 2017/01/18 12:37:58, gambard wrote: > On 2017/01/18 09:34:38, lpromero wrote: > > You might need to add that before adding as subview? I find it cleaner and > > avoids the system to resize the view to satisfy the autoresizing masks > > constraints when the view is added, for in fine removing the constraints. > > > > As a general question, do you get unsatisfiable constraints errors in the > > console when you display this? > > Done. > I don't unsatisfiable constraints. I have some ambiguous constraints on other > type of cell (SuggestionsArticleCell). > In lot of places it is done after adding it to the view, so if it would create > conflicts we would have seen it before. What kind of ambiguous constraints log do you get? https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item_unittest.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item_unittest.mm:41: ASSERT_TRUE([cell isMemberOfClass:[SuggestionsFaviconCell class]]); On 2017/01/18 12:37:58, gambard wrote: > On 2017/01/18 09:34:39, lpromero wrote: > > Optional: I almost commented on line 26 to use isMemberOfClass, but actually, > > line 26 has an advantage when the classes don't match: the log tells you the > > expected class and the actual class. Whereas here, you'll get: > > Expected: YES, Actual: NO. Which doesn't quickly help. > > Done. > This was the way it is done in most of the other "CellIsConfigured" tests. Sure, I only thought about it now :) https://codereview.chromium.org/2640473002/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_commands.h (right): https://codereview.chromium.org/2640473002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_commands.h:18: - (void)openFaviconAtIndexPath:(NSInteger)index; s/indexPath/index, in the method and comment. https://codereview.chromium.org/2640473002/diff/40001/ios/showcase/common/pro... File ios/showcase/common/protocol_alerter.mm (right): https://codereview.chromium.org/2640473002/diff/40001/ios/showcase/common/pro... ios/showcase/common/protocol_alerter.mm:203: // Returns a string describing an argument at |index| that is know to be a long s/know/known https://codereview.chromium.org/2640473002/diff/40001/ios/showcase/common/pro... ios/showcase/common/protocol_alerter.mm:206: long long longLong; s/longLong/value.
Thanks, PTAL. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:81: cell.delegate = self.delegate; On 2017/01/18 13:14:02, lpromero wrote: > On 2017/01/18 12:37:58, gambard wrote: > > On 2017/01/18 09:34:38, lpromero wrote: > > > Add a prepareForReuse to the cell, where these are unset. Also reset the > > > faviconView.image and title.text. > > > > Done. > > To reset the image and the title, I have added it to the inner cells. > > I have not used -prepareForReuse on any other cell. Should I? > > Usually I reset everything I can have set in configureCell: and > -collectionView:cellForItemAtIndexPath:. Acknowledged. https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.mm:118: _collectionView.translatesAutoresizingMaskIntoConstraints = NO; On 2017/01/18 13:14:02, lpromero wrote: > On 2017/01/18 12:37:58, gambard wrote: > > On 2017/01/18 09:34:38, lpromero wrote: > > > You might need to add that before adding as subview? I find it cleaner and > > > avoids the system to resize the view to satisfy the autoresizing masks > > > constraints when the view is added, for in fine removing the constraints. > > > > > > As a general question, do you get unsatisfiable constraints errors in the > > > console when you display this? > > > > Done. > > I don't unsatisfiable constraints. I have some ambiguous constraints on other > > type of cell (SuggestionsArticleCell). > > In lot of places it is done after adding it to the view, so if it would create > > conflicts we would have seen it before. > > What kind of ambiguous constraints log do you get? The title and description of the article are pinned to the bottom of the article, which must be lower than the image bottom. If the image is bigger than the text, one of the label must be expanded, which one is ambiguous. I was aware of this, I will fix it in a future CL. https://codereview.chromium.org/2640473002/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_commands.h (right): https://codereview.chromium.org/2640473002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_commands.h:18: - (void)openFaviconAtIndexPath:(NSInteger)index; On 2017/01/18 13:14:02, lpromero wrote: > s/indexPath/index, in the method and comment. Done. https://codereview.chromium.org/2640473002/diff/40001/ios/showcase/common/pro... File ios/showcase/common/protocol_alerter.mm (right): https://codereview.chromium.org/2640473002/diff/40001/ios/showcase/common/pro... ios/showcase/common/protocol_alerter.mm:203: // Returns a string describing an argument at |index| that is know to be a long On 2017/01/18 13:14:02, lpromero wrote: > s/know/known Done. https://codereview.chromium.org/2640473002/diff/40001/ios/showcase/common/pro... ios/showcase/common/protocol_alerter.mm:206: long long longLong; On 2017/01/18 13:14:02, lpromero wrote: > s/longLong/value. Done.
The CQ bit was checked by lpromero@chromium.org
lgtm https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm (right): https://codereview.chromium.org/2640473002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:29: _title.font = [[MDFRobotoFontLoader sharedInstance] lightFontOfSize:10]; On 2017/01/18 12:37:57, gambard wrote: > On 2017/01/18 09:34:38, lpromero wrote: > > It is not available via MDCTypography? > > Done. > It is but showcase does not load the roboto font in MDCTypography. > [MDCTypography setFontLoader:[MDFRobotoFontLoader sharedInstance]]; should > probably be added to showcase app delegate. https://codereview.chromium.org/2646483002/.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Waiting for marq@ review before commit. I am asking you a review because I am trying to use the new architecture patterns, but if you don't have time/don't want to review it, it is OK.
https://codereview.chromium.org/2640473002/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm (right): https://codereview.chromium.org/2640473002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:29: _titleLabel.font = [[MDCTypography fontLoader] lightFontOfSize:10]; Either make this a constant or use a font size constant from a header (I think we have those somewhere). https://codereview.chromium.org/2640473002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:57: - (void)prepareForReuse { nit: #pragma mark - UICollectionReusableView https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h (right): https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:15: // Opens the favicon associated with |indexPath|. Called when the inner cell of So which collection is |indexPath| for, the overall collection view, or the internal one inside the cell? https://codereview.chromium.org/2640473002/diff/80001/ios/showcase/suggestion... File ios/showcase/suggestions/sc_suggestions_coordinator.mm (right): https://codereview.chromium.org/2640473002/diff/80001/ios/showcase/suggestion... ios/showcase/suggestions/sc_suggestions_coordinator.mm:63: [static_cast<id<SuggestionsCommands>>(self.alerter) openFaviconAtIndex:index]; I'm going to again suggest you just directly assign the alerter as _suggestionViewController.suggestionCommandHandler instead of manually forwarding each method in this way. That's the intended use, specifically so that when commands are added to a protocol they are supported in the showcase without any additional code. https://codereview.chromium.org/2640473002/diff/80001/ios/showcase/suggestion... ios/showcase/suggestions/sc_suggestions_coordinator.mm:63: [static_cast<id<SuggestionsCommands>>(self.alerter) openFaviconAtIndex:index]; Oh, and a minor nit: my current understanding is that reinterpret_cast<> is the right choice here.
Thanks, marq PTAL. https://codereview.chromium.org/2640473002/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm (right): https://codereview.chromium.org/2640473002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:29: _titleLabel.font = [[MDCTypography fontLoader] lightFontOfSize:10]; On 2017/01/19 16:20:26, marq wrote: > Either make this a constant or use a font size constant from a header (I think > we have those somewhere). Done. https://codereview.chromium.org/2640473002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_internal_cell.mm:57: - (void)prepareForReuse { On 2017/01/19 16:20:25, marq wrote: > nit: #pragma mark - UICollectionReusableView Done. https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h (right): https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:15: // Opens the favicon associated with |indexPath|. Called when the inner cell of On 2017/01/19 16:20:26, marq wrote: > So which collection is |indexPath| for, the overall collection view, or the > internal one inside the cell? The one inside the cell. The second part of the comment should have make this understandable. I have updated it. https://codereview.chromium.org/2640473002/diff/80001/ios/showcase/suggestion... File ios/showcase/suggestions/sc_suggestions_coordinator.mm (right): https://codereview.chromium.org/2640473002/diff/80001/ios/showcase/suggestion... ios/showcase/suggestions/sc_suggestions_coordinator.mm:63: [static_cast<id<SuggestionsCommands>>(self.alerter) openFaviconAtIndex:index]; On 2017/01/19 16:20:26, marq wrote: > Oh, and a minor nit: my current understanding is that reinterpret_cast<> is the > right choice here. Done. https://codereview.chromium.org/2640473002/diff/80001/ios/showcase/suggestion... ios/showcase/suggestions/sc_suggestions_coordinator.mm:63: [static_cast<id<SuggestionsCommands>>(self.alerter) openFaviconAtIndex:index]; On 2017/01/19 16:20:26, marq wrote: > I'm going to again suggest you just directly assign the alerter as > _suggestionViewController.suggestionCommandHandler instead of manually > forwarding each method in this way. That's the intended use, specifically so > that when commands are added to a protocol they are supported in the showcase > without any additional code. Done.
https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h (right): https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:15: // Opens the favicon associated with |indexPath|. Called when the inner cell of On 2017/01/19 17:37:32, gambard wrote: > On 2017/01/19 16:20:26, marq wrote: > > So which collection is |indexPath| for, the overall collection view, or the > > internal one inside the cell? > > The one inside the cell. The second part of the comment should have make this > understandable. > I have updated it. Would something like the following be helpful? - openFaviConAtInnerIndexPath:()innerIndexPath;
https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h (right): https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:15: // Opens the favicon associated with |indexPath|. Called when the inner cell of On 2017/01/19 18:51:05, lpromero wrote: > On 2017/01/19 17:37:32, gambard wrote: > > On 2017/01/19 16:20:26, marq wrote: > > > So which collection is |indexPath| for, the overall collection view, or the > > > internal one inside the cell? > > > > The one inside the cell. The second part of the comment should have make this > > understandable. > > I have updated it. > > Would something like the following be helpful? > - openFaviConAtInnerIndexPath:()innerIndexPath; This is maybe too much. What about - openFaviconAtIndexPath:()innerIndexPath; ?
https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h (right): https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:15: // Opens the favicon associated with |indexPath|. Called when the inner cell of On 2017/01/20 07:38:49, gambard wrote: > On 2017/01/19 18:51:05, lpromero wrote: > > On 2017/01/19 17:37:32, gambard wrote: > > > On 2017/01/19 16:20:26, marq wrote: > > > > So which collection is |indexPath| for, the overall collection view, or > the > > > > internal one inside the cell? > > > > > > The one inside the cell. The second part of the comment should have make > this > > > understandable. > > > I have updated it. > > > > Would something like the following be helpful? > > - openFaviConAtInnerIndexPath:()innerIndexPath; > > This is maybe too much. What about - openFaviconAtIndexPath:()innerIndexPath; ? lg
https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h (right): https://codereview.chromium.org/2640473002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_favicon_item.h:15: // Opens the favicon associated with |indexPath|. Called when the inner cell of On 2017/01/20 08:58:39, lpromero wrote: > On 2017/01/20 07:38:49, gambard wrote: > > On 2017/01/19 18:51:05, lpromero wrote: > > > On 2017/01/19 17:37:32, gambard wrote: > > > > On 2017/01/19 16:20:26, marq wrote: > > > > > So which collection is |indexPath| for, the overall collection view, or > > the > > > > > internal one inside the cell? > > > > > > > > The one inside the cell. The second part of the comment should have make > > this > > > > understandable. > > > > I have updated it. > > > > > > Would something like the following be helpful? > > > - openFaviConAtInnerIndexPath:()innerIndexPath; > > > > This is maybe too much. What about - openFaviconAtIndexPath:()innerIndexPath; > ? > > lg Done.
lgtm
The CQ bit was checked by gambard@chromium.org
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/2640473002/#ps120001 (title: "Change function name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gambard@chromium.org
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/2640473002/#ps140001 (title: "Fix build")
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": 140001, "attempt_start_ts": 1484926155596760,
"parent_rev": "746baec6650e544f81bc8a7def3b13da13fbef51", "commit_rev":
"7de29d98ff473630649264e9837bfc4235d0921d"}
Message was sent while issue was closed.
Description was changed from ========== Suggestions UI - favicon item BUG=679369 ========== to ========== Suggestions UI - favicon item BUG=679369 Review-Url: https://codereview.chromium.org/2640473002 Cr-Commit-Position: refs/heads/master@{#445069} Committed: https://chromium.googlesource.com/chromium/src/+/7de29d98ff473630649264e9837b... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/7de29d98ff473630649264e9837b... |
