Bug 226770 - CSSOM test for serializing font-variant fails
Summary: CSSOM test for serializing font-variant fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-08 08:02 PDT by Rob Buis
Modified: 2021-06-21 11:19 PDT (History)
9 users (show)

See Also:


Attachments
Patch (11.01 KB, patch)
2021-06-08 09:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.33 KB, patch)
2021-06-08 11:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (24.83 KB, patch)
2021-06-08 13:18 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (30.62 KB, patch)
2021-06-18 13:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (30.92 KB, patch)
2021-06-19 01:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (33.28 KB, patch)
2021-06-19 03:10 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (30.79 KB, patch)
2021-06-21 10:11 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].