Summary: | Stop parsing font palette things that we can't implement | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||
Component: | Text | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | clopez, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, heycam, macpherson, menard, mmaxfield, simon.fraser, webkit-bug-importer, youennf | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=230926 | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 230446 | ||||||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2021-09-25 17:16:58 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) Reopening. Created attachment 440165 [details]
WIP
It's probably okay for a missing base-palette to serialize out to base-palette:0. Created attachment 440168 [details]
WIP
Created attachment 440172 [details]
WIP
Created attachment 440175 [details]
Patch
Created attachment 440176 [details]
Patch
Created attachment 440177 [details]
Patch
Created attachment 440178 [details]
Patch
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 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.) Created attachment 440471 [details]
Patch
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 Committed r283752 (242674@main): <https://commits.webkit.org/242674@main> |