Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808433002:
Create a MostVisited cell for ContentSuggestions (Closed)
|
Created:
3 years, 8 months ago by gambard Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate a MostVisited cell for ContentSuggestions
This CL creates a MostVisited cell and item which can be used to display the
Most Visited sites in Content Suggestions.
BUG=707754
Review-Url: https://codereview.chromium.org/2808433002
Cr-Commit-Position: refs/heads/master@{#463949}
Committed: https://chromium.googlesource.com/chromium/src/+/d4ac95b13ea78688e1a8dfd6dbceac097e7fc968
Patch Set 1 #Patch Set 2 : Complete patch #Patch Set 3 : Split #
Total comments: 25
Patch Set 4 : Address comments #Patch Set 5 : Fix tests #
Total comments: 2
Patch Set 6 : Address comment #
Messages
Total messages: 21 (13 generated)
gambard@chromium.org changed reviewers: + lpromero@chromium.org, stkhapugin@chromium.org
PTAL!
lgtm https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h (right): https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h:21: NSArray<ContentSuggestionsMostVisited*>* mostVisiteds; mostVisitedItems or mostVisitedSuggestions https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h:26: @interface ContentSuggestionsMostVisitedCell : MDCCollectionViewCell nit: If you plan to add more logic into this class in the future, consider splitting out a custom view that just takes an array of most visited tiles and displays it properly, encapsulating all the layout logic, that you just put inside this cell. Right now it seems like this class is pretty much doing exactly that, but it'd be nice for it to not do anything else in the future either. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm (right): https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:80: - (instancetype)initWithFrame:(CGRect)frame { optional: add a comment here or above @implementation explaining that this cell: 1. displays two rows of content suggestions 2. uses a vertical stack of two horizontal stacks - one for each line 3. varies number of items per row depending on its width This might be obvious know, but if this class grows with animations or some other logic, it might start being confusing. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:178: - (NSUInteger)numberOfTilesPerLine { optional: it'd be nice to have a few tests for this part of layout logic, specifically: - on iPhone 7, in portrait, the number of tiles is 4 - on iPhone 7 Plus, in portrait, it's still 4 - in landscape it's 4 - on iPhone 5 it's 3 or something similar, with a few important cases that we actually need.
To ease up unittesting, some of the logic could be extracted to a simple class or functions. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn (right): https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn:14: "content_suggestions_most_visited_item.mm", Please add a third file for unittests. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h (right): https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h:21: NSArray<ContentSuggestionsMostVisited*>* mostVisiteds; On 2017/04/07 12:17:58, stkhapugin wrote: > mostVisitedItems or mostVisitedSuggestions Or |suggestions|? https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h:29: - (void)setMostVisiteds: Idem. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm (right): https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:52: NSMutableArray<ContentSuggestionsMostVisitedTile*>* tiles; Maybe add new lines between the property, as it is not easily readable (especially in the review tool). https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:64: @property(nonatomic, assign) NSUInteger oldNumberOfTilesPerLine; Optional nit: s/old/previous https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:123: for (ContentSuggestionsMostVisited* mostVisited : mostVisiteds) { s/:/in https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:132: - (void)layoutSubviews { Please add the comment we use usually to describe the two-passes layout. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:160: NSUInteger numberOfTilesfirstLine = s/numberOfTilesfirstLine/numberOfTilesFirstLine https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:192: // Returns the spacing of between tiles, based on the device. s/between/middle or interior or inner? Or maybe is it just about removing "of"?
Thanks, PTAL. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn (right): https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn:14: "content_suggestions_most_visited_item.mm", On 2017/04/07 12:46:56, lpromero wrote: > Please add a third file for unittests. Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h (right): https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h:21: NSArray<ContentSuggestionsMostVisited*>* mostVisiteds; On 2017/04/07 12:46:56, lpromero wrote: > On 2017/04/07 12:17:58, stkhapugin wrote: > > mostVisitedItems or mostVisitedSuggestions > > Or |suggestions|? Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h:26: @interface ContentSuggestionsMostVisitedCell : MDCCollectionViewCell On 2017/04/07 12:17:58, stkhapugin wrote: > nit: If you plan to add more logic into this class in the future, consider > splitting out a custom view that just takes an array of most visited tiles and > displays it properly, encapsulating all the layout logic, that you just put > inside this cell. Right now it seems like this class is pretty much doing > exactly that, but it'd be nice for it to not do anything else in the future > either. The only logic I am planing to add to this cell is a way to display it on one line only if needed. I think it fits in. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h:29: - (void)setMostVisiteds: On 2017/04/07 12:46:56, lpromero wrote: > Idem. Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm (right): https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:52: NSMutableArray<ContentSuggestionsMostVisitedTile*>* tiles; On 2017/04/07 12:46:57, lpromero wrote: > Maybe add new lines between the property, as it is not easily readable > (especially in the review tool). Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:64: @property(nonatomic, assign) NSUInteger oldNumberOfTilesPerLine; On 2017/04/07 12:46:57, lpromero wrote: > Optional nit: s/old/previous Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:80: - (instancetype)initWithFrame:(CGRect)frame { On 2017/04/07 12:17:58, stkhapugin wrote: > optional: add a comment here or above @implementation explaining that this cell: > 1. displays two rows of content suggestions > 2. uses a vertical stack of two horizontal stacks - one for each line > 3. varies number of items per row depending on its width > > This might be obvious know, but if this class grows with animations or some > other logic, it might start being confusing. Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:123: for (ContentSuggestionsMostVisited* mostVisited : mostVisiteds) { On 2017/04/07 12:46:57, lpromero wrote: > s/:/in Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:132: - (void)layoutSubviews { On 2017/04/07 12:46:57, lpromero wrote: > Please add the comment we use usually to describe the two-passes layout. Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:160: NSUInteger numberOfTilesfirstLine = On 2017/04/07 12:46:57, lpromero wrote: > s/numberOfTilesfirstLine/numberOfTilesFirstLine Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:178: - (NSUInteger)numberOfTilesPerLine { On 2017/04/07 12:17:58, stkhapugin wrote: > optional: it'd be nice to have a few tests for this part of layout logic, > specifically: > > - on iPhone 7, in portrait, the number of tiles is 4 > - on iPhone 7 Plus, in portrait, it's still 4 > - in landscape it's 4 > - on iPhone 5 it's 3 > > or something similar, with a few important cases that we actually need. Done. https://codereview.chromium.org/2808433002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.mm:192: // Returns the spacing of between tiles, based on the device. On 2017/04/07 12:46:57, lpromero wrote: > s/between/middle or interior or inner? > > Or maybe is it just about removing "of"? Done.
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: 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
Dry run: 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 to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2808433002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item_unittest.mm (right): https://codereview.chromium.org/2808433002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item_unittest.mm:57: // Setup. Return if IsIPadIdiom? Or rename the test in SizeRegularWidth?
Thanks! https://codereview.chromium.org/2808433002/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item_unittest.mm (right): https://codereview.chromium.org/2808433002/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item_unittest.mm:57: // Setup. On 2017/04/11 16:52:17, lpromero wrote: > Return if IsIPadIdiom? Or rename the test in SizeRegularWidth? Done.
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, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2808433002/#ps100001 (title: "Address comment")
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": 100001, "attempt_start_ts": 1491979846697240,
"parent_rev": "89b08748a1f0bef747dd1b15f3e80ad9dc70c7b9", "commit_rev":
"d4ac95b13ea78688e1a8dfd6dbceac097e7fc968"}
Message was sent while issue was closed.
Description was changed from ========== Create a MostVisited cell for ContentSuggestions This CL creates a MostVisited cell and item which can be used to display the Most Visited sites in Content Suggestions. BUG=707754 ========== to ========== Create a MostVisited cell for ContentSuggestions This CL creates a MostVisited cell and item which can be used to display the Most Visited sites in Content Suggestions. BUG=707754 Review-Url: https://codereview.chromium.org/2808433002 Cr-Commit-Position: refs/heads/master@{#463949} Committed: https://chromium.googlesource.com/chromium/src/+/d4ac95b13ea78688e1a8dfd6dbce... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d4ac95b13ea78688e1a8dfd6dbce... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
