Bug 226770 – CSSOM test for serializing font-variant fails

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

CSSOM test for serializing font-variant fails, see css/cssom/serialize-values.html.

[Rob Buis]

Created attachment 430850 [details]
Patch

[Rob Buis]

Created attachment 430866 [details]
Patch

[Rob Buis]

Created attachment 430882 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79342847>

[Rob Buis]

Created attachment 431785 [details]
Patch

[Rob Buis]

Created attachment 431791 [details]
Patch

[Rob Buis]

Created attachment 431793 [details]
Patch

[Darin Adler]

Comment on attachment 431793 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:758
> +    if (variant) {
> +        switch (*variant) {

Could write this instead:

    switch (variant.value_or(FontVariantEastAsianVariant::Normal)) {

Then we don’t have to nest the entire switch statement inside an if statement. It’s also such a mess how these functions map an enumeration value to a CSS value but don’t share any of the code to create the identifier and append. We could make this a lot tidier with a bit of refactoring.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4969
> +        addProperty(CSSPropertyFontVariantCaps, CSSPropertyFontVariant, CSSValuePool::singleton().createIdentifierValue(CSSValueNormal), important, true);
> +        addProperty(CSSPropertyFontVariantEastAsian, CSSPropertyFontVariant, CSSValuePool::singleton().createIdentifierValue(CSSValueNormal), important, true);
> +        addProperty(CSSPropertyFontVariantPosition, CSSPropertyFontVariant, CSSValuePool::singleton().createIdentifierValue(CSSValueNormal), important, true);

This ", true" stuff is unreadable. This is why we don}t use booleans for arguments where we pass constants.

[Rob Buis]

Created attachment 431872 [details]
Patch

[Rob Buis]

Comment on attachment 431793 [details]
Patch

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

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:758
>> +        switch (*variant) {
> 
> Could write this instead:
> 
>     switch (variant.value_or(FontVariantEastAsianVariant::Normal)) {
> 
> Then we don’t have to nest the entire switch statement inside an if statement. It’s also such a mess how these functions map an enumeration value to a CSS value but don’t share any of the code to create the identifier and append. We could make this a lot tidier with a bit of refactoring.

That does seem nicer, fixed.

[EWS]

Committed r279070 (238990@main): <https://commits.webkit.org/238990@main>

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