Bug 230799 – Stop parsing font palette things that we can't implement

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

It's better to not parse it in the first place, so @supports works, rather than parse it but silently drop it on the floor.

[Radar WebKit Bug Importer]

<rdar://problem/83537772>

[Myles C. Maxfield]

This might actually be tricky because different ports will be able to implement different things. We might want to actually hold off on this until the spec is modified. (https://bugs.webkit.org/show_bug.cgi?id=230926)

[Myles C. Maxfield]

Reopening.

[Myles C. Maxfield]

Created attachment 440165 [details]
WIP

[Myles C. Maxfield]

It's probably okay for a missing base-palette to serialize out to base-palette:0.

[Myles C. Maxfield]

Created attachment 440168 [details]
WIP

[Myles C. Maxfield]

Created attachment 440172 [details]
WIP

[Myles C. Maxfield]

Created attachment 440175 [details]
Patch

[Myles C. Maxfield]

Created attachment 440176 [details]
Patch

[Myles C. Maxfield]

Created attachment 440177 [details]
Patch

[Myles C. Maxfield]

Created attachment 440178 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Cameron McCormack (:heycam)]

Comment on attachment 440178 [details]
Patch

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

Non-reviewer r=me.

> Source/WebCore/css/parser/CSSParserImpl.cpp:703
> +            unsigned key = 0;
>              const auto& pair = downcast<CSSFontPaletteValuesOverrideColorsValue>(item.get());
> -            if (pair.key().isString())
> -                key = pair.key().stringValue();
> -            else if (pair.key().isNumber())
> +            if (pair.key().isNumber())
>                  key = pair.key().value<unsigned>();
>              else
>                  continue;

Nit: Might look nicer as:

const auto& pair = ...;
if (!pair.key().isNumber())
    continue;
auto key = pair.key().value<unsigned>();
```

> Source/WebCore/platform/graphics/FontPaletteValues.h:77
>      enum class Type : uint8_t {
>          Light,
>          Dark,
> -        Integer,
> -        String
> -    } type { Type::String };
> +        Integer
> +    } type { Type::Integer };

(Side note: I find declaring and using types in the one declaration like this unidiomatic.)

[Myles C. Maxfield]

Created attachment 440471 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 440471 [details]
Patch

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

> Source/WebCore/css/StyleRule.cpp:333
> +StyleRuleFontPaletteValues::StyleRuleFontPaletteValues(const AtomString& name, const AtomString& fontFamily, std::optional<FontPaletteIndex>&& basePalette, Vector<FontPaletteValues::OverriddenColor>&& overrideColors)

I never know if r-value references are better for small types.

> Source/WebCore/css/StyleRule.h:184
> +    const std::optional<FontPaletteIndex>& basePalette() const

Probably still fine to return by value.

> Source/WebCore/css/parser/CSSParserImpl.cpp:702
> +            key = pair.key().value<unsigned>();

Just declare key on this line.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4860
> +        RefPtr<CSSPrimitiveValue> key = consumeInteger(range, 0);

auto

[Myles C. Maxfield]

Committed r283752 (242674@main): <https://commits.webkit.org/242674@main>