Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361393004:
Creates FaviconAttributesProvider. (Closed)
|
Created:
4 years, 3 months ago by stkhapugin Modified:
4 years, 2 months ago CC:
chromium-reviews, browser-components-watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreates FaviconAttributesProvider.
Creates FaviconAttributesProvider, FaviconAttributes and FaviconViewNew
classes to present favicons. These classes intend to replace FaviconView
and FaviconViewProvider downstream.
BUG=None
Committed: https://crrev.com/f0889fe15ef25c57dad4c68a729861c815e6c684
Cr-Commit-Position: refs/heads/master@{#422769}
Patch Set 1 #
Total comments: 10
Patch Set 2 : comments #Patch Set 3 : removed nullability annotations #
Total comments: 8
Patch Set 4 : comments & typo #
Messages
Total messages: 24 (7 generated)
stkhapugin@chromium.org changed reviewers: + jif@chromium.org, jyquinn@chromium.org, rohitrao@chromium.org
PTAL: - jif@ because this is going to be used in reading list - jyquinn@ because it is a replacement for history code - rohitrao@ as OWNER Please note that FaviconViewNew will be soon renamed to FaviconView in a followup CL.
Overall looks good, but I have a question about sizing for FaviconView. With the current FaviconView you can set the size property and the view will then be constrained to that size - is removing that intentional? https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... File ios/chrome/browser/ui/favicon_view.h (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... ios/chrome/browser/ui/favicon_view.h:15: + (CGFloat)defaultSize; Why make this a class method rather than a settable property? Or is this even needed? https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... File ios/chrome/browser/ui/favicon_view.mm (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... ios/chrome/browser/ui/favicon_view.mm:64: AddSameSizeConstraint(_faviconFallbackLabel, self); I think we may still need to constrain the subview centers to self's center? For orientation changes, etc.
We shouldn't start adding those annotations without consulting the rest of the team. For example, I think that the _Nonnull everywhere obnoxious, and using NS_ASSUME_NONNULL_BEGIN would make things much more readable. https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/favicon_attributes.h (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/favicon_attributes.h:10: // Attributes of a favicon. A favicon is represented either with an image or Attributes of a FaviconView. https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/favicon_attributes.mm (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/favicon_attributes.mm:60: [_faviconImage release]; We are supposed to use an ObjCPropertyReleaser https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/favicon_attributes_provider.mm (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/favicon_attributes_provider.mm:35: DCHECK(largeIconService); is the DCHECK necessary if largeIconService is _Nonnull ?
Thanks, PTAL Jackie, regarding sizing of favicon view - I changed it to expose a correct intrinsic content size (24x24) which is the only size currently used in Chrome iOS. If at some point somewhere we'll need a different size, it can be adjusted with auto layout constraints on its size. The size of images fetched is also easy to adjust by passing a different parameter to attribute provider. https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/favicon_attributes.h (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/favicon_attributes.h:10: // Attributes of a favicon. A favicon is represented either with an image or On 2016/09/26 10:58:12, jif wrote: > Attributes of a FaviconView. No, these are a representation of favicon, not the view. https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/favicon_attributes.mm (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/favicon_attributes.mm:60: [_faviconImage release]; On 2016/09/26 10:58:13, jif wrote: > We are supposed to use an ObjCPropertyReleaser Done. https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... File ios/chrome/browser/favicon/favicon_attributes_provider.mm (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... ios/chrome/browser/favicon/favicon_attributes_provider.mm:35: DCHECK(largeIconService); On 2016/09/26 10:58:13, jif wrote: > is the DCHECK necessary if largeIconService is _Nonnull ? Yes, because nullability markers are not tightly enforced, it's pretty easy to accidentally pass a nil value here. For example, calling this with "largeIconService:nil" will issue a warning, but calling this with "largeIconService:[LargeIconServiceFactory iconService]" would only issue a warning if LargeIconServiceFactory.h uses nullability markers afaik. https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... File ios/chrome/browser/ui/favicon_view.h (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... ios/chrome/browser/ui/favicon_view.h:15: + (CGFloat)defaultSize; On 2016/09/23 23:11:01, Jackie Quinn wrote: > Why make this a class method rather than a settable property? Or is this even > needed? You're right, this is useless. Exposing constants instead. https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... File ios/chrome/browser/ui/favicon_view.mm (right): https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... ios/chrome/browser/ui/favicon_view.mm:64: AddSameSizeConstraint(_faviconFallbackLabel, self); On 2016/09/23 23:11:01, Jackie Quinn wrote: > I think we may still need to constrain the subview centers to self's center? For > orientation changes, etc. You are right! I don't know why it even worked. Good catch!
On 2016/09/26 15:51:37, stkhapugin wrote: > Thanks, PTAL > > Jackie, regarding sizing of favicon view - I changed it to expose a correct > intrinsic content size (24x24) which is the only size currently used in Chrome > iOS. If at some point somewhere we'll need a different size, it can be adjusted > with auto layout constraints on its size. The size of images fetched is also > easy to adjust by passing a different parameter to attribute provider. > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > File ios/chrome/browser/favicon/favicon_attributes.h (right): > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > ios/chrome/browser/favicon/favicon_attributes.h:10: // Attributes of a favicon. > A favicon is represented either with an image or > On 2016/09/26 10:58:12, jif wrote: > > Attributes of a FaviconView. > > No, these are a representation of favicon, not the view. > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > File ios/chrome/browser/favicon/favicon_attributes.mm (right): > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > ios/chrome/browser/favicon/favicon_attributes.mm:60: [_faviconImage release]; > On 2016/09/26 10:58:13, jif wrote: > > We are supposed to use an ObjCPropertyReleaser > > Done. > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > File ios/chrome/browser/favicon/favicon_attributes_provider.mm (right): > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > ios/chrome/browser/favicon/favicon_attributes_provider.mm:35: > DCHECK(largeIconService); > On 2016/09/26 10:58:13, jif wrote: > > is the DCHECK necessary if largeIconService is _Nonnull ? > > Yes, because nullability markers are not tightly enforced, it's pretty easy to > accidentally pass a nil value here. For example, calling this with > "largeIconService:nil" will issue a warning, but calling this with > "largeIconService:[LargeIconServiceFactory iconService]" would only issue a > warning if LargeIconServiceFactory.h uses nullability markers afaik. > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... > File ios/chrome/browser/ui/favicon_view.h (right): > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... > ios/chrome/browser/ui/favicon_view.h:15: + (CGFloat)defaultSize; > On 2016/09/23 23:11:01, Jackie Quinn wrote: > > Why make this a class method rather than a settable property? Or is this even > > needed? > > You're right, this is useless. Exposing constants instead. > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... > File ios/chrome/browser/ui/favicon_view.mm (right): > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... > ios/chrome/browser/ui/favicon_view.mm:64: > AddSameSizeConstraint(_faviconFallbackLabel, self); > On 2016/09/23 23:11:01, Jackie Quinn wrote: > > I think we may still need to constrain the subview centers to self's center? > For > > orientation changes, etc. > > You are right! I don't know why it even worked. Good catch! Thanks! lgtm
On 2016/09/26 18:17:54, Jackie Quinn wrote: > On 2016/09/26 15:51:37, stkhapugin wrote: > > Thanks, PTAL > > > > Jackie, regarding sizing of favicon view - I changed it to expose a correct > > intrinsic content size (24x24) which is the only size currently used in Chrome > > iOS. If at some point somewhere we'll need a different size, it can be > adjusted > > with auto layout constraints on its size. The size of images fetched is also > > easy to adjust by passing a different parameter to attribute provider. > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > > File ios/chrome/browser/favicon/favicon_attributes.h (right): > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > > ios/chrome/browser/favicon/favicon_attributes.h:10: // Attributes of a > favicon. > > A favicon is represented either with an image or > > On 2016/09/26 10:58:12, jif wrote: > > > Attributes of a FaviconView. > > > > No, these are a representation of favicon, not the view. > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > > File ios/chrome/browser/favicon/favicon_attributes.mm (right): > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > > ios/chrome/browser/favicon/favicon_attributes.mm:60: [_faviconImage release]; > > On 2016/09/26 10:58:13, jif wrote: > > > We are supposed to use an ObjCPropertyReleaser > > > > Done. > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > > File ios/chrome/browser/favicon/favicon_attributes_provider.mm (right): > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/favicon/... > > ios/chrome/browser/favicon/favicon_attributes_provider.mm:35: > > DCHECK(largeIconService); > > On 2016/09/26 10:58:13, jif wrote: > > > is the DCHECK necessary if largeIconService is _Nonnull ? > > > > Yes, because nullability markers are not tightly enforced, it's pretty easy to > > accidentally pass a nil value here. For example, calling this with > > "largeIconService:nil" will issue a warning, but calling this with > > "largeIconService:[LargeIconServiceFactory iconService]" would only issue a > > warning if LargeIconServiceFactory.h uses nullability markers afaik. > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... > > File ios/chrome/browser/ui/favicon_view.h (right): > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... > > ios/chrome/browser/ui/favicon_view.h:15: + (CGFloat)defaultSize; > > On 2016/09/23 23:11:01, Jackie Quinn wrote: > > > Why make this a class method rather than a settable property? Or is this > even > > > needed? > > > > You're right, this is useless. Exposing constants instead. > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... > > File ios/chrome/browser/ui/favicon_view.mm (right): > > > > > https://codereview.chromium.org/2361393004/diff/1/ios/chrome/browser/ui/favic... > > ios/chrome/browser/ui/favicon_view.mm:64: > > AddSameSizeConstraint(_faviconFallbackLabel, self); > > On 2016/09/23 23:11:01, Jackie Quinn wrote: > > > I think we may still need to constrain the subview centers to self's center? > > For > > > orientation changes, etc. > > > > You are right! I don't know why it even worked. Good catch! > > Thanks! lgtm Oh the one place I could think of that may use favicons of a different size at some point is the NTP. But I'm fine with leaving size generalization to when it's needed :-)
Removed nullability annotations. PTAL jif, rohitrao
stkhapugin@chromium.org changed reviewers: + sdefresne@chromium.org - rohitrao@chromium.org
-rohitrao +sdefresne PTAL
jif@google.com changed reviewers: + jif@google.com
lgtm
stkhapugin@chromium.org changed reviewers: + noyau@chromium.org
+noyau for owners, ptal
https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/favicon_attributes.h (right): https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/favicon_attributes.h:11: // with a fallback monogram of a given color and background color. CL https://codereview.chromium.org/2374753002/ introduces the notion of favicon style which seems to encompass the same data. Can't you reuse it? https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/favicon_attributes.mm (right): https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/favicon_attributes.mm:25: _propertyReleaser_FaviconAttributes.Init(self, [FaviconAttributes class]); Another read-only/property releaser killer combo. https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/favicon_attributes_provider.h (right): https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/favicon_attributes_provider.h:20: @interface FaviconAttributesProvider : NSObject Can't this object be C++?
ptal https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/favicon_attributes.h (right): https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/favicon_attributes.h:11: // with a fallback monogram of a given color and background color. On 2016/10/03 14:52:10, noyau wrote: > CL https://codereview.chromium.org/2374753002/ introduces the notion of favicon > style which seems to encompass the same data. Can't you reuse it? There seems to only be a "fallback favicon style", not "favicon style". Also, this class exposes a UIKit-friendly API. I can create a convenience initializer that takes a FallbackIconStyle, but I'd prefer to keep this API to make view code cleaner. https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/favicon_attributes.mm (right): https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/favicon_attributes.mm:25: _propertyReleaser_FaviconAttributes.Init(self, [FaviconAttributes class]); On 2016/10/03 14:52:10, noyau wrote: > Another read-only/property releaser killer combo. Acknowledged. https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/favicon_attributes_provider.h (right): https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/favicon_attributes_provider.h:20: @interface FaviconAttributesProvider : NSObject On 2016/10/03 14:52:10, noyau wrote: > Can't this object be C++? This is basically a UIKit-friendly ObjC wrapper for favicon::LargeIconService to keep the rest of the code cleaner, as we don't have to do Skia->UIColor conversions and bitmap data to UIImage conversions elsewhere. Just look at the list of #includes in .mm that we mostly don't need elsewhere thanks to this class.
lgtm https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... File ios/chrome/browser/favicon/favicon_attributes_provider.h (right): https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/favicon_attributes_provider.h:9: #import "ios/chrome/browser/favicon/favicon_attributes.h" Probably not needed, could be replaced by @class. https://codereview.chromium.org/2361393004/diff/40001/ios/chrome/browser/favi... ios/chrome/browser/favicon/favicon_attributes_provider.h:20: @interface FaviconAttributesProvider : NSObject On 2016/10/04 11:47:18, stkhapugin wrote: > On 2016/10/03 14:52:10, noyau wrote: > > Can't this object be C++? > > This is basically a UIKit-friendly ObjC wrapper for favicon::LargeIconService to > keep the rest of the code cleaner, as we don't have to do Skia->UIColor > conversions and bitmap data to UIImage conversions elsewhere. Just look at the > list of #includes in .mm that we mostly don't need elsewhere thanks to this > class. I'm not arguing the utility of this class, which hides conversions as you said. All I'm saying is that the API of this class has no dependencies on ObjectiveC, the only reason you include UIKit it for NSObject. The .h could be C++ only. The implementation needs to be a .mm file, for sure, but it doesn't need to be an objectiveC class. But what I missed the first time is that Faviconattribute is full of ObjectiveC objects, this makes my point moot really.
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyquinn@chromium.org, jif@google.com Link to the patchset: https://codereview.chromium.org/2361393004/#ps60001 (title: "comments & typo")
thanks for review, everyone!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Creates FaviconAttributesProvider. Creates FaviconAttributesProvider, FaviconAttributes and FaviconViewNew classes to present favicons. These classes intend to replace FaviconView and FaviconViewProvider downstream. BUG=None ========== to ========== Creates FaviconAttributesProvider. Creates FaviconAttributesProvider, FaviconAttributes and FaviconViewNew classes to present favicons. These classes intend to replace FaviconView and FaviconViewProvider downstream. BUG=None Committed: https://crrev.com/f0889fe15ef25c57dad4c68a729861c815e6c684 Cr-Commit-Position: refs/heads/master@{#422769} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f0889fe15ef25c57dad4c68a729861c815e6c684 Cr-Commit-Position: refs/heads/master@{#422769} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
