Bug 226770

Summary: CSSOM test for serializing font-variant fails
Product: WebKit Reporter: Rob Buis <rbuis>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Rob Buis 2021-06-08 08:02:40 PDT 
CSSOM test for serializing font-variant fails, see css/cssom/serialize-values.html.
Comment 1 Rob Buis 2021-06-08 09:33:28 PDT 
Created attachment 430850 [details]
Patch
Comment 2 Rob Buis 2021-06-08 11:12:44 PDT 
Created attachment 430866 [details]
Patch
Comment 3 Rob Buis 2021-06-08 13:18:49 PDT 
Created attachment 430882 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-06-15 08:03:16 PDT 
<rdar://problem/79342847>
Comment 5 Rob Buis 2021-06-18 13:09:06 PDT 
Created attachment 431785 [details]
Patch
Comment 6 Rob Buis 2021-06-19 01:07:36 PDT 
Created attachment 431791 [details]
Patch
Comment 7 Rob Buis 2021-06-19 03:10:58 PDT 
Created attachment 431793 [details]
Patch
Comment 8 Darin Adler 2021-06-20 19:19:25 PDT 
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.
Comment 9 Rob Buis 2021-06-21 10:11:43 PDT 
Created attachment 431872 [details]
Patch
Comment 10 Rob Buis 2021-06-21 10:15:46 PDT 
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.
Comment 11 EWS 2021-06-21 11:19:02 PDT 
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].