Bug 226810 – Parse attributes' HTML length values by HTMLDimension

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[cathiechen]

`addHTMLLengthToStyle()` is using CSS parser to get the values. We should reuse `parseHTMLDimension()` here.

[cathiechen]

Created attachment 431060 [details]
Patch

[cathiechen]

Comment on attachment 431060 [details]
Patch

Hi,
I think this patch is ready for review now:)
It applies HTMLDimension to other length parsing scenarios.
Please take a look, thanks!

[Darin Adler]

Comment on attachment 431060 [details]
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&.

[cathiechen]

Comment on attachment 431060 [details]
Patch

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

Hi Darin,
Thanks for the 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.

Got it! Done, thanks!

>> Source/WebCore/html/HTMLElement.h:155
>> +    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&.

Done!

>> Source/WebCore/html/HTMLElement.h:159
>> +    void addHTMLNumberToStyle(MutableStyleProperties&, CSSPropertyID, const String& value);
> 
> These functions should take StringView, not const String&.

Ah, done, thanks!

[cathiechen]

Created attachment 431198 [details]
Patch

[EWS]

Committed r278765 (238725@main): <https://commits.webkit.org/238725@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431198 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79205778>