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
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

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].