This function is about whether the aspect ratio value is applicable and set to a value other than auto and not whether we should ignore the aspect ratio (and we ignore the aspect ratio value for some other reasons)
Created attachment 424900 [details] Patch
Comment on attachment 424900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424900&action=review > Source/WebCore/rendering/RenderBox.cpp:5098 > +static inline bool hasAspectRatio(const RenderBox& renderer) This name is worse than the old one. It should be more like canUseAspectRatio() or aspectRatioApplies().
(In reply to Simon Fraser (smfr) from comment #2) > Comment on attachment 424900 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424900&action=review > > > Source/WebCore/rendering/RenderBox.cpp:5098 > > +static inline bool hasAspectRatio(const RenderBox& renderer) > > This name is worse than the old one. It should be more like > canUseAspectRatio() or aspectRatioApplies(). The function name certainly can be improved but it is definitely not as misleading as the initial name.
(In reply to zalan from comment #3) > (In reply to Simon Fraser (smfr) from comment #2) > > Comment on attachment 424900 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=424900&action=review > > > > > Source/WebCore/rendering/RenderBox.cpp:5098 > > > +static inline bool hasAspectRatio(const RenderBox& renderer) > > > > This name is worse than the old one. It should be more like > > canUseAspectRatio() or aspectRatioApplies(). > The function name certainly can be improved but it is definitely not as > misleading as the initial name. This was supposed to be just an extended version of RenderStyle::hasAspectRatio() taking the box type into account.
(In reply to zalan from comment #4) > (In reply to zalan from comment #3) > > (In reply to Simon Fraser (smfr) from comment #2) > > > Comment on attachment 424900 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=424900&action=review > > > > > > > Source/WebCore/rendering/RenderBox.cpp:5098 > > > > +static inline bool hasAspectRatio(const RenderBox& renderer) > > > > > > This name is worse than the old one. It should be more like > > > canUseAspectRatio() or aspectRatioApplies(). > > The function name certainly can be improved but it is definitely not as > > misleading as the initial name. > This was supposed to be just an extended version of > RenderStyle::hasAspectRatio() taking the box type into account. hasAspectRatio(const RenderBox& renderer) sounds like it's asking "does this box have aspect ratio", by which we really mean "does this box have dimensions controlled by the aspect-ratio property: But aspect-ratio has all these other constraints, like non-auto dimensions. I think it's different from transform in that way, so following hasTransform() is confusing.
(In reply to Simon Fraser (smfr) from comment #5) > (In reply to zalan from comment #4) > > (In reply to zalan from comment #3) > > > (In reply to Simon Fraser (smfr) from comment #2) > > > > Comment on attachment 424900 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=424900&action=review > > > > > > > > > Source/WebCore/rendering/RenderBox.cpp:5098 > > > > > +static inline bool hasAspectRatio(const RenderBox& renderer) > > > > > > > > This name is worse than the old one. It should be more like > > > > canUseAspectRatio() or aspectRatioApplies(). > > > The function name certainly can be improved but it is definitely not as > > > misleading as the initial name. > > This was supposed to be just an extended version of > > RenderStyle::hasAspectRatio() taking the box type into account. > > hasAspectRatio(const RenderBox& renderer) sounds like it's asking "does this > box have aspect ratio", by which we really mean "does this box have > dimensions controlled by the aspect-ratio property: But aspect-ratio has all > these other constraints, like non-auto dimensions. I think it's different > from transform in that way, so following hasTransform() is confusing. I agree it's not the same and that's why I moved it out of the class (to reduce visibility/misuse) I find it difficult to come up with a meaningful/proper name.
<rdar://problem/76403114>
I like the idea of the patch. Maybe something like canApplyAspectRatio?