Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779843006:
Add favicon view to ContentSuggestionsArticle (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. |
DescriptionAdd favicon view to ContentSuggestionsArticle
BUG=706427
Review-Url: https://codereview.chromium.org/2779843006
Cr-Commit-Position: refs/heads/master@{#461411}
Committed: https://chromium.googlesource.com/chromium/src/+/ca7e8fffb3a38d5fdb83f987cb2bc3f8c9909a8a
Patch Set 1 #Patch Set 2 : Fix build.gn #
Total comments: 2
Patch Set 3 : Rebase #
Total comments: 10
Patch Set 4 : Rebase #Patch Set 5 : Remove comments #Patch Set 6 : Rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 23 (10 generated)
gambard@chromium.org changed reviewers: + olivierrobin@chromium.org
PTAL.
lpromero@chromium.org changed reviewers: + lpromero@chromium.org
https://codereview.chromium.org/2779843006/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn (right): https://codereview.chromium.org/2779843006/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn:29: "//ios/chrome/browser/favicon", Don't you miss //ios/chrome/browser/ui/favicon for FaviconViewNew? As we said IRL, please move attributes to ui/.
Thanks, PTAL. https://codereview.chromium.org/2779843006/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn (right): https://codereview.chromium.org/2779843006/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/BUILD.gn:29: "//ios/chrome/browser/favicon", On 2017/03/29 14:05:14, lpromero wrote: > Don't you miss //ios/chrome/browser/ui/favicon for FaviconViewNew? > > As we said IRL, please move attributes to ui/. Done.
lgtm https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:23: // When updating this, make sure to update |layoutSubviews|. Why? Here and below. https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:89: if (self.attributes) If there are no attributes, the favicon view is not reconfigured, nor deconfigured. Does it means it can keep the configuration from a previous item when the cell is reused?
https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:25: // When updating this, make sure to update |layoutSubviews|. why? https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:268: [_faviconView.heightAnchor constraintEqualToConstant:kFaviconSize], If attributes is nil (supported), this view will still take kFaviconSize x kFaviconSize. Is this expected?
Description was changed from ========== Add favicon view to ContentSuggestionsArticle BUG=694220 ========== to ========== Add favicon view to ContentSuggestionsArticle BUG=706427 ==========
Thanks, PTAL. https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:23: // When updating this, make sure to update |layoutSubviews|. On 2017/03/29 14:33:01, lpromero wrote: > Why? Here and below. Because it is the spacing between the labels and the elements on the side. https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:25: // When updating this, make sure to update |layoutSubviews|. On 2017/03/29 14:38:24, Olivier Robin wrote: > why? Because it is the spacing between the labels and the elements on the side. https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:89: if (self.attributes) On 2017/03/29 14:33:01, lpromero wrote: > If there are no attributes, the favicon view is not reconfigured, nor > deconfigured. Does it means it can keep the configuration from a previous item > when the cell is reused? In theory yes. But the call to self.attributes = ... is "very quick", so it never happens (the same pattern is used in Reading List). I have plan to fix this :) https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:268: [_faviconView.heightAnchor constraintEqualToConstant:kFaviconSize], On 2017/03/29 14:38:24, Olivier Robin wrote: > If attributes is nil (supported), this view will still take kFaviconSize x > kFaviconSize. Is this expected? Yes. The callback should be quick enough. I plan to fix this.
https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:23: // When updating this, make sure to update |layoutSubviews|. On 2017/03/29 16:25:14, gambard wrote: > On 2017/03/29 14:33:01, lpromero wrote: > > Why? Here and below. > > Because it is the spacing between the labels and the elements on the side. I don't get why this comment is here.
https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2779843006/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_article_item.mm:23: // When updating this, make sure to update |layoutSubviews|. On 2017/03/29 16:35:47, lpromero wrote: > On 2017/03/29 16:25:14, gambard wrote: > > On 2017/03/29 14:33:01, lpromero wrote: > > > Why? Here and below. > > > > Because it is the spacing between the labels and the elements on the side. > > I don't get why this comment is here. Well... Yes, the comment does not make sense as is.
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/2779843006/#ps80001 (title: "Remove comments")
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...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2779843006/#ps100001 (title: "Rebase")
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": 1491218112030670,
"parent_rev": "fb80b96a036920cd61a7e2cea33a1e797bc96816", "commit_rev":
"ca7e8fffb3a38d5fdb83f987cb2bc3f8c9909a8a"}
Message was sent while issue was closed.
Description was changed from ========== Add favicon view to ContentSuggestionsArticle BUG=706427 ========== to ========== Add favicon view to ContentSuggestionsArticle BUG=706427 Review-Url: https://codereview.chromium.org/2779843006 Cr-Commit-Position: refs/heads/master@{#461411} Committed: https://chromium.googlesource.com/chromium/src/+/ca7e8fffb3a38d5fdb83f987cb2b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ca7e8fffb3a38d5fdb83f987cb2b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
