Bug 186344

I thought the spec was in the process of changing the syntax fairly dramatically? I mean, we can flip the order super easily, but if we're going to be changing our implementation substantially, we may want to wait.

Looks like we're gaining interoperability on that ordering, so I guess we should:

https://wpt.fyi/results/css/css-fonts/parsing/font-synthesis-valid.html?label=master&label=experimental&product=chrome&product=firefox&product=webkitgtk&product=safari&aligned

Created attachment 443153 [details]
Patch

Comment on attachment 443153 [details]
Patch

Layout test results will be updated with the next patchset.

Comment on attachment 443153 [details]
Patch

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

r=mews

> Source/WebCore/css/StyleProperties.cpp:173
> +        case CSSPropertyFontSynthesis:
> +            return fontSynthesisPropertyValue(*value);

This is a surprising place for this code. Why is this here and not below (or maybe even in another function?)

Comment on attachment 443153 [details]
Patch

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

>> Source/WebCore/css/StyleProperties.cpp:173
>> +            return fontSynthesisPropertyValue(*value);
> 
> This is a surprising place for this code. Why is this here and not below (or maybe even in another function?)

Falling through from CSSPropertyStrokeOpacity and returning a fontSynthesisPropertyValue() doesn't seem right.

Comment on attachment 443153 [details]
Patch

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

>>> Source/WebCore/css/StyleProperties.cpp:173
>>> +            return fontSynthesisPropertyValue(*value);
>> 
>> This is a surprising place for this code. Why is this here and not below (or maybe even in another function?)
> 
> Falling through from CSSPropertyStrokeOpacity and returning a fontSynthesisPropertyValue() doesn't seem right.

I moved this callsite to above. Originally I placed this here because it was right before default switch but I didn't see the FALLTHROUGH.
And it seems that below place is for shorthand properties, so I will place this call as below.

if (auto value = getPropertyCSSValue(propertyID)) {
        switch (propertyID) {
        case CSSPropertyFontSynthesis:
            return fontSynthesisPropertyValue(*value);
        case CSSPropertyFillOpacity:
        case CSSPropertyFloodOpacity:
...

Created attachment 443174 [details]
Patch

I updated layout test result on Ubuntu 21.10
and will update layout test result on macOS 12.0.1 too in the next patchset.

Created attachment 443195 [details]
Patch

Could you please review this patch again? I think this patch is ready for review now.

Created attachment 443281 [details]
Update layout test results for ios and common

Hi, please review this patch when you are available. It seems that ews has passed.

Comment on attachment 443281 [details]
Update layout test results for ios and common

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

> Source/WebCore/css/StyleProperties.cpp:505
> +String StyleProperties::fontSynthesisPropertyValue(CSSValue& value) const
> +{
> +    String valueStr = value.cssText();
> +    if (isCSSWideValueKeyword(valueStr) || is<CSSPrimitiveValue>(value))
> +        return valueStr;
> +
> +    if (is<CSSValueList>(value)) {
> +        bool hasWeight = false;
> +        bool hasStyle = false;
> +        if (downcast<CSSValueList>(value).hasValue(CSSValuePool::singleton().createIdentifierValue(CSSValueWeight).ptr()))
> +            hasWeight = true;
> +        if (downcast<CSSValueList>(value).hasValue(CSSValuePool::singleton().createIdentifierValue(CSSValueStyle).ptr()))
> +            hasStyle = true;
> +        return makeString(hasWeight ? "weight" : "", (hasWeight && hasStyle) ? " " : "", hasStyle ? "style" : "");
> +    }
> +
> +    return String();
> +}

I think, instead of writing this function, we should change consumeFontSynthesis() in CSSPropertyParser.cpp to just canonicalize the list at parsing time. Canonicalizing at parse time would be a simpler solution, and there would not have to be another special case here - StyleProperties::getPropertyValue() would be entirely unmodified.

Maybe something like this:

    bool foundWeight = false;
    bool foundStyle = false;
    bool foundSmallCaps = false;
    while (true) {
        auto ident = consumeIdent<CSSValueWeight, CSSValueStyle, CSSValueSmallCaps>(range);
        if (!ident)
            break;
        switch (ident->valueID()) {
        case CSSValueWeight:
            if (foundWeight)
                return nullptr;
            foundWeight = true;
        case CSSValueStyle:
            if (foundStyle)
                return nullptr;
            foundStyle = true;
        case CSSValueSmallCaps:
            if (foundSmallCaps)
                return nullptr;
            foundSmallCaps = true;
        }
    }

    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
    if (foundWeight)
        list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueWeight));
    if (foundStyle)
        list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueStyle));
    if (foundSmallCaps)
        list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueSmallCaps));

    if (!list->length())
        return nullptr;
    return list;

It's probably even possible to make this more concise by using lambdas.

Comment on attachment 443281 [details]
Update layout test results for ios and common

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

>> Source/WebCore/css/StyleProperties.cpp:505
>> +}
> 
> I think, instead of writing this function, we should change consumeFontSynthesis() in CSSPropertyParser.cpp to just canonicalize the list at parsing time. Canonicalizing at parse time would be a simpler solution, and there would not have to be another special case here - StyleProperties::getPropertyValue() would be entirely unmodified.
> 
> Maybe something like this:
> 
>     bool foundWeight = false;
>     bool foundStyle = false;
>     bool foundSmallCaps = false;
>     while (true) {
>         auto ident = consumeIdent<CSSValueWeight, CSSValueStyle, CSSValueSmallCaps>(range);
>         if (!ident)
>             break;
>         switch (ident->valueID()) {
>         case CSSValueWeight:
>             if (foundWeight)
>                 return nullptr;
>             foundWeight = true;
>         case CSSValueStyle:
>             if (foundStyle)
>                 return nullptr;
>             foundStyle = true;
>         case CSSValueSmallCaps:
>             if (foundSmallCaps)
>                 return nullptr;
>             foundSmallCaps = true;
>         }
>     }
> 
>     RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
>     if (foundWeight)
>         list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueWeight));
>     if (foundStyle)
>         list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueStyle));
>     if (foundSmallCaps)
>         list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueSmallCaps));
> 
>     if (!list->length())
>         return nullptr;
>     return list;
> 
> It's probably even possible to make this more concise by using lambdas.

Thank you for your review, Myles:) I applied your comment in the next patchset and will upload it soon.

Created attachment 443477 [details]
Canonicalize CSSValueList at parsing stage in consumeFontSynthesis

Comment on attachment 443477 [details]
Canonicalize CSSValueList at parsing stage in consumeFontSynthesis

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:995
> +    auto makeCanonicalizedList = [](CSSParserTokenRange& range) -> RefPtr<CSSValueList> {

The fact that this code is in a lambda doesn't seem to be useful. When I mentioned "lambda" I was suggesting using it as a tool to reduce (nearly-)duplicated code.

If it's not possible to reduce (nearly-)duplicated code, I suggest we promote this code to no longer be within a lambda.

Comment on attachment 443477 [details]
Canonicalize CSSValueList at parsing stage in consumeFontSynthesis

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

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:995
>> +    auto makeCanonicalizedList = [](CSSParserTokenRange& range) -> RefPtr<CSSValueList> {
> 
> The fact that this code is in a lambda doesn't seem to be useful. When I mentioned "lambda" I was suggesting using it as a tool to reduce (nearly-)duplicated code.
> 
> If it's not possible to reduce (nearly-)duplicated code, I suggest we promote this code to no longer be within a lambda.

I added a small tiny lambda below, except that I promoted all the codes to no longer be within a lambda in the next patchset. Thank you for your review.

auto checkAndMarkExistence = [](bool* found) {
    if (*found)
        return false;
    return *found = true;
};

Created attachment 443482 [details]
Patch for landing

Committed r285383 (243940@main): <https://commits.webkit.org/243940@main>

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

<rdar://problem/85105405>

Committed r285384 (243941@main): <https://commits.webkit.org/243941@main>