Bug 224050 – Rename RenderBox::shouldIgnoreAspectRatio

• Status: NEW
• Component: Layout and Rendering

[alan]

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)

[alan]

Created attachment 424900 [details]
Patch

[Simon Fraser (smfr)]

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().

[alan]

(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.

[alan]

(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.

[Simon Fraser (smfr)]

(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.

[alan]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/76403114>

[Rob Buis]

I like the idea of the patch. Maybe something like canApplyAspectRatio?