[webkit-reviews] review granted: [Bug 195180] [css-values-4] Support font-relative lh and rlh unit : [Attachment 395487] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Tyler Wilcock
<twilco.o at protonmail.com>'s request for review:
Bug 195180: [css-values-4] Support font-relative lh and rlh unit
https://bugs.webkit.org/show_bug.cgi?id=195180

Attachment 395487: Patch

https://bugs.webkit.org/attachment.cgi?id=395487&action=review




--- Comment #11 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 395487
  --> https://bugs.webkit.org/attachment.cgi?id=395487
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395487&action=review

Looks good!

> Source/WebCore/css/CSSToLengthConversionData.h:44
> +enum class ComputingProperty {
> +    FontSize,
> +    LineHeight,
> +    OtherOrUnknown
> +};

An alternative approach would be to use Optional<CSSPropertyID> instead of this
(passing CSSPropertyFontSize/CSSPropertyLineHeight where appropriate).

If you keep the enum it should move to CSSToLengthConversionData namespace.
PropertyToCompute or just PropertyType might read better.

> Source/WebCore/css/CSSToLengthConversionData.h:48
> +    CSSToLengthConversionData(const RenderStyle* style, const RenderStyle*
rootStyle, const RenderStyle* parentStyle, const RenderView* renderView, float
zoom, ComputingProperty computingProperty = ComputingProperty::OtherOrUnknown)

'computingProperty' is an awkward name for variables and fields too