Bug 230799 - Stop parsing font palette things that we can't implement
Summary: Stop parsing font palette things that we can't implement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 230446
  Show dependency treegraph
 
Reported: 2021-09-25 17:16 PDT by Myles C. Maxfield
Modified: 2021-10-07 16:06 PDT (History)
12 users (show)

See Also:


Attachments
WIP (9.40 KB, patch)
2021-10-04 23:40 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (10.86 KB, patch)
2021-10-04 23:57 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (18.36 KB, patch)
2021-10-05 00:14 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (22.09 KB, patch)
2021-10-05 00:30 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (22.10 KB, patch)
2021-10-05 00:32 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (22.10 KB, patch)
2021-10-05 00:33 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (22.09 KB, patch)
2021-10-05 00:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (22.18 KB, patch)
2021-10-06 23:24 PDT, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-09-25 17:16:58 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-09-25 17:17:19 PDT
<rdar://problem/83537772>
Comment 2 Myles C. Maxfield 2021-09-28 23:19:11 PDT
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)
Comment 3 Myles C. Maxfield 2021-10-04 23:22:24 PDT
Reopening.
Comment 4 Myles C. Maxfield 2021-10-04 23:40:16 PDT
Created attachment 440165 [details]
WIP
Comment 5 Myles C. Maxfield 2021-10-04 23:40:39 PDT
It's probably okay for a missing base-palette to serialize out to base-palette:0.
Comment 6 Myles C. Maxfield 2021-10-04 23:57:23 PDT
Created attachment 440168 [details]
WIP
Comment 7 Myles C. Maxfield 2021-10-05 00:14:06 PDT
Created attachment 440172 [details]
WIP
Comment 8 Myles C. Maxfield 2021-10-05 00:30:08 PDT
Created attachment 440175 [details]
Patch
Comment 9 Myles C. Maxfield 2021-10-05 00:32:07 PDT
Created attachment 440176 [details]
Patch
Comment 10 Myles C. Maxfield 2021-10-05 00:33:55 PDT
Created attachment 440177 [details]
Patch
Comment 11 Myles C. Maxfield 2021-10-05 00:38:21 PDT
Created attachment 440178 [details]
Patch
Comment 12 EWS Watchlist 2021-10-05 00:39:50 PDT
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
Comment 13 Cameron McCormack (:heycam) 2021-10-06 17:51:45 PDT
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.)
Comment 14 Myles C. Maxfield 2021-10-06 23:24:15 PDT
Created attachment 440471 [details]
Patch
Comment 15 Simon Fraser (smfr) 2021-10-07 15:06:43 PDT
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
Comment 16 Myles C. Maxfield 2021-10-07 16:06:11 PDT
Committed r283752 (242674@main): <https://commits.webkit.org/242674@main>