[webkit-reviews] review granted: [Bug 222665] Allow CSS font-styling for canvas without RenderStyle : [Attachment 422222] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 4 13:39:32 PST 2021


Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 222665: Allow CSS font-styling for canvas without RenderStyle
https://bugs.webkit.org/show_bug.cgi?id=222665

Attachment 422222: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 422222
  --> https://bugs.webkit.org/attachment.cgi?id=422222
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:194
> +    static double computeUnzoomedNonCalcLengthDouble(CSSUnitType, double
value, CSSPropertyID, const FontMetrics* = nullptr, const
FontCascadeDescription* = nullptr, const FontCascadeDescription*
rootFontDescription = nullptr, const RenderView* = nullptr);

I worry about the long list of arguments. Is there any way for us to cut that
complexity down to size? This is why we end up creating objects to group the
context for calls like this.

Also wonder if there’s a more terse way to name these variations.

> Source/WebCore/css/CSSToLengthConversionData.h:78
> +    CSSPropertyID propertyToCompute() const { return m_propertyToCompute ?
*m_propertyToCompute : CSSPropertyInvalid; }

Can use valueOr for this sort of thing.

> Source/WebCore/style/StyleFontSizeFunctions.h:28
> +#include "Settings.h"

Really annoying that Settings::Values creates these additional header
dependencies. I think we should consider defining SettingsValues at the
namespace level to avoid the need for all these additional header includes.

> Source/WebCore/style/StyleFontSizeFunctions.h:41
> +enum class MinimumFontSizeRule {
> +    None,
> +    Absolute,
> +    AbsoluteAndRelative
> +};

Comment on "just moved" code: I may be the only person who feels this way, but
I like the one-line versions of this kind of small enum rather than the
vertical version.

    enum class MinimumFontSizeRule : uint8_t { None, Absolute,
AbsoluteAndRelative };

It’s also a tiny bit helpful, I think, to mark the underlying type uint8_t
rather than letting it default to signed 32-bit integer.

> Source/WebCore/style/StyleResolveForFontRaw.cpp:202
> +    // Line height is non-existent on FontCascade, so is left unhandled.

Is there a slightly more precise way to say what we mean by "left unhandled"?


More information about the webkit-reviews mailing list