Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2803223002:
Create a Most Visited tile for ContentSuggestions (Closed)
|
Created:
3 years, 8 months ago by gambard Modified:
3 years, 6 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 Most Visited tile for ContentSuggestions
This CL creates a ContentSuggestionsMostVisitedTile view displaying a Most
Visited suggestion, with a favicon and a title.
It also creates a ContentSuggestionsMostVisited, containing the informations
for the tile.
BUG=707754
Review-Url: https://codereview.chromium.org/2803223002
Cr-Commit-Position: refs/heads/master@{#463258}
Committed: https://chromium.googlesource.com/chromium/src/+/cde4c9664fbafda7dffe1c61ac059d6bd80882b3
Patch Set 1 #Patch Set 2 : Reviewable #
Total comments: 29
Patch Set 3 : Address comments #
Total comments: 10
Patch Set 4 : Nothing changed due to clang format :) #
Total comments: 2
Patch Set 5 : Address comment #Messages
Total messages: 19 (9 generated)
gambard@chromium.org changed reviewers: + lpromero@chromium.org, stkhapugin@chromium.org
PTAL. Split of https://codereview.chromium.org/2808433002/#ps20001
lgtm https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn:40: "content_suggestions_most_visited_tile_unittest.mm", Can you add a simple CSMostVisited unittests file? I like the habit of creating files by 3 these days. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h:17: @property(nonatomic, assign) GURL url; I think this is a good occasion to test drive a solution without GURL. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:28: // Sets the text and the accessibility label of this View. *view https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:29: - (void)setText:(nullable NSString*)text; Prefer setTitle. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:49: @synthesize title = _title; Nit: Move these before the class methods. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:59: _title.preferredMaxLayoutWidth = kWidth; Should you reuse [[self class] width]? https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm:18: [ContentSuggestionsMostVisitedTile tileWithTitle:title attributes:nil]; Anything with the attributes you could test? https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm:21: } Nit: New line.
https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h:21: @property(nonatomic, strong, nullable) FaviconAttributes* attributes; Should this really be nullable, given that FaviconAttributes seem to support both favicon and monogram configurations? https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:17: + (NSInteger)width; CGFloat instead of NSInteger https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:64: [[MDCTypography fontLoader] regularFontOfSize:kFaviconSize / 2]; This looks weird to me: you're determining the font size from the size of the favicon? AFAIK font sizes are not really related to point sizes all that easily. Please verify with Pete. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:72: [_faviconView.widthAnchor constraintEqualToConstant:kFaviconSize], Optional nit You can instead modify your constraints below: Was: V:|[favicon]-(space)-[title]", @"H:|[title]|" Can replace with: V:|[favicon(kFaviconSize)]-(space)-[title]", @"H:|[title]|", @"[favicon(kFaviconSize)]" to replace first two lines of constraints here. You will still have to keep the centerX and the self.size https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:76: [self.widthAnchor constraintEqualToConstant:kWidth], It is preferred to use -(CGSize)intrinsicContentSize and not constraint the view's size inside the view directly.
Thanks, PTAL. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn:40: "content_suggestions_most_visited_tile_unittest.mm", On 2017/04/07 10:58:22, lpromero wrote: > Can you add a simple CSMostVisited unittests file? I like the habit of creating > files by 3 these days. Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h:17: @property(nonatomic, assign) GURL url; On 2017/04/07 10:58:22, lpromero wrote: > I think this is a good occasion to test drive a solution without GURL. Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h:21: @property(nonatomic, strong, nullable) FaviconAttributes* attributes; On 2017/04/07 11:15:18, stkhapugin wrote: > Should this really be nullable, given that FaviconAttributes seem to support > both favicon and monogram configurations? Yes, the fetch for the attributes are asynchronous, so at the beginning this can be nil. Configuring a favicon view with nil attributes ends up with a uniform grey background with no letter. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:17: + (NSInteger)width; On 2017/04/07 11:15:18, stkhapugin wrote: > CGFloat instead of NSInteger Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:28: // Sets the text and the accessibility label of this View. On 2017/04/07 10:58:22, lpromero wrote: > *view Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:29: - (void)setText:(nullable NSString*)text; On 2017/04/07 10:58:22, lpromero wrote: > Prefer setTitle. Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:49: @synthesize title = _title; On 2017/04/07 10:58:22, lpromero wrote: > Nit: Move these before the class methods. Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:59: _title.preferredMaxLayoutWidth = kWidth; On 2017/04/07 10:58:22, lpromero wrote: > Should you reuse [[self class] width]? Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:64: [[MDCTypography fontLoader] regularFontOfSize:kFaviconSize / 2]; On 2017/04/07 11:15:18, stkhapugin wrote: > This looks weird to me: you're determining the font size from the size of the > favicon? AFAIK font sizes are not really related to point sizes all that easily. > Please verify with Pete. Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:72: [_faviconView.widthAnchor constraintEqualToConstant:kFaviconSize], On 2017/04/07 11:15:18, stkhapugin wrote: > Optional nit > You can instead modify your constraints below: > > Was: > V:|[favicon]-(space)-[title]", @"H:|[title]|" > > Can replace with: > V:|[favicon(kFaviconSize)]-(space)-[title]", @"H:|[title]|", > @"[favicon(kFaviconSize)]" > > to replace first two lines of constraints here. You will still have to keep the > centerX and the self.size I considered doing this but I don't like constraining the width/height with visual constraints. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:76: [self.widthAnchor constraintEqualToConstant:kWidth], On 2017/04/07 11:15:18, stkhapugin wrote: > It is preferred to use -(CGSize)intrinsicContentSize and not constraint the > view's size inside the view directly. Done. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm:18: [ContentSuggestionsMostVisitedTile tileWithTitle:title attributes:nil]; On 2017/04/07 10:58:22, lpromero wrote: > Anything with the attributes you could test? Always the same problem... The faviconView is an intelligent view, not exposing directly its image. I don't see a way to test it. https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm:21: } On 2017/04/07 10:58:22, lpromero wrote: > Nit: New line. Done.
https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm:18: [ContentSuggestionsMostVisitedTile tileWithTitle:title attributes:nil]; On 2017/04/07 14:47:22, gambard wrote: > On 2017/04/07 10:58:22, lpromero wrote: > > Anything with the attributes you could test? > > Always the same problem... The faviconView is an intelligent view, not exposing > directly its image. I don't see a way to test it. The test you added is perfectly fine. For testing the favicon, maybe we should do screenshot testing (not the scope of this CL). https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h (right): https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h:21: + (nullable ContentSuggestionsMostVisited*) This method implementation seems to indicate it is not nullable. You could remove the annotation. https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h (right): https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:17: + (CGFloat)width; Is this still needed in the public interface? If not, you can also remove the impl. https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:21: + (nullable ContentSuggestionsMostVisitedTile*) Idem this can't fail. https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm (right): https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm:21: } Nit: new line. https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_unittest.mm (right): https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_unittest.mm:26: } Nit: new line.
Thanks, PTAL. https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h (right): https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h:21: + (nullable ContentSuggestionsMostVisited*) On 2017/04/07 15:16:12, lpromero wrote: > This method implementation seems to indicate it is not nullable. You could > remove the annotation. It is using an init. The init is nullable. How can I be sure it is nonnull? https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h (right): https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:17: + (CGFloat)width; On 2017/04/07 15:16:12, lpromero wrote: > Is this still needed in the public interface? If not, you can also remove the > impl. It is needed to determine how many tiles you can put in one line. https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h:21: + (nullable ContentSuggestionsMostVisitedTile*) On 2017/04/07 15:16:12, lpromero wrote: > Idem this can't fail. Same, it is using the return value of a nullable init. https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm (right): https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile_unittest.mm:21: } On 2017/04/07 15:16:12, lpromero wrote: > Nit: new line. This is actually removed by clang format https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_unittest.mm (right): https://codereview.chromium.org/2803223002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_unittest.mm:26: } On 2017/04/07 15:16:12, lpromero wrote: > Nit: new line. This is actually removed by clang format
lgtm with comments https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h:21: @property(nonatomic, strong, nullable) FaviconAttributes* attributes; On 2017/04/07 14:47:21, gambard wrote: > On 2017/04/07 11:15:18, stkhapugin wrote: > > Should this really be nullable, given that FaviconAttributes seem to support > > both favicon and monogram configurations? > > Yes, the fetch for the attributes are asynchronous, so at the beginning this can > be nil. > Configuring a favicon view with nil attributes ends up with a uniform grey > background with no letter. Please document this behavior in a comment here and/or on the cell class. https://codereview.chromium.org/2803223002/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm (right): https://codereview.chromium.org/2803223002/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:92: self.accessibilityLabel = title; |title| is nullable. Are you sure you want your a11y label to be empty?
Thanks! https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h (right): https://codereview.chromium.org/2803223002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited.h:21: @property(nonatomic, strong, nullable) FaviconAttributes* attributes; On 2017/04/10 11:17:37, stkhapugin wrote: > On 2017/04/07 14:47:21, gambard wrote: > > On 2017/04/07 11:15:18, stkhapugin wrote: > > > Should this really be nullable, given that FaviconAttributes seem to support > > > both favicon and monogram configurations? > > > > Yes, the fetch for the attributes are asynchronous, so at the beginning this > can > > be nil. > > Configuring a favicon view with nil attributes ends up with a uniform grey > > background with no letter. > > Please document this behavior in a comment here and/or on the cell class. Done. https://codereview.chromium.org/2803223002/diff/60001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm (right): https://codereview.chromium.org/2803223002/diff/60001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.mm:92: self.accessibilityLabel = title; On 2017/04/10 11:17:37, stkhapugin wrote: > |title| is nullable. Are you sure you want your a11y label to be empty? If there is no title, I don't see what I can put as accessibility label... :(
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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, stkhapugin@chromium.org Link to the patchset: https://codereview.chromium.org/2803223002/#ps80001 (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": 80001, "attempt_start_ts": 1491832285987350,
"parent_rev": "8c425270f28666a3fab38db1b6e68f54311990ea", "commit_rev":
"cde4c9664fbafda7dffe1c61ac059d6bd80882b3"}
Message was sent while issue was closed.
Description was changed from ========== Create a Most Visited tile for ContentSuggestions This CL creates a ContentSuggestionsMostVisitedTile view displaying a Most Visited suggestion, with a favicon and a title. It also creates a ContentSuggestionsMostVisited, containing the informations for the tile. BUG=707754 ========== to ========== Create a Most Visited tile for ContentSuggestions This CL creates a ContentSuggestionsMostVisitedTile view displaying a Most Visited suggestion, with a favicon and a title. It also creates a ContentSuggestionsMostVisited, containing the informations for the tile. BUG=707754 Review-Url: https://codereview.chromium.org/2803223002 Cr-Commit-Position: refs/heads/master@{#463258} Committed: https://chromium.googlesource.com/chromium/src/+/cde4c9664fbafda7dffe1c61ac05... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/cde4c9664fbafda7dffe1c61ac05... |
