[webkit-reviews] review granted: [Bug 226810] Parse attributes' HTML length values by HTMLDimension : [Attachment 431060] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted cathiechen <cathiechen at igalia.com>'s
request for review:
Bug 226810: Parse attributes' HTML length values by HTMLDimension
https://bugs.webkit.org/show_bug.cgi?id=226810

Attachment 431060: Patch

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




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

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

> Source/WebCore/html/HTMLElement.cpp:1019
> +void HTMLElement::addHTMLLengthToStyleWithConfigs(MutableStyleProperties&
style, CSSPropertyID propertyID, const String& value, AllowPercentage
allowPercentage, UseCSSPXAsUnitType useCSSPX, IsMultiLength isMultiLengh)

Typo here in isMultiLengh

Not thrilled with the word "configs" here. I think the function name doesn’t
have to change just because we have three more arguments; it can just be an
overload. Think of the arguments as having default values.

> Source/WebCore/html/HTMLElement.h:155
> +    enum class AllowPercentage : bool { No, Yes };
> +    enum class UseCSSPXAsUnitType : bool { No, Yes };
> +    enum class IsMultiLength : bool { No, Yes };
> +    void addHTMLLengthToStyleWithConfigs(MutableStyleProperties&,
CSSPropertyID, const String& value, AllowPercentage, UseCSSPXAsUnitType,
IsMultiLength);

These all can and should be private instead of protected. Also not sure that
these options are best represented as three separate booleans, but if we make
that private it’s easy to change it later.

This function should take a StringView, not a const String&.

> Source/WebCore/html/HTMLElement.h:159
>      void addHTMLLengthToStyle(MutableStyleProperties&, CSSPropertyID, const
String& value);
> +    void addHTMLMultiLengthToStyle(MutableStyleProperties&, CSSPropertyID,
const String& value);
> +    void addHTMLPixelsToStyle(MutableStyleProperties&, CSSPropertyID, const
String& value);
> +    void addHTMLNumberToStyle(MutableStyleProperties&, CSSPropertyID, const
String& value);

These functions should take StringView, not const String&.