Summary: | base-palette can accept "light" or "dark" | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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, macpherson, menard, mmaxfield, simon.fraser, webkit-bug-importer, youennf | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 230800, 231029 | ||||||||||||||||||
Bug Blocks: | 230446 | ||||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2021-09-25 03:42:11 PDT
Created attachment 439273 [details]
Patch to make FontCreationContext optional
Created attachment 439274 [details]
Patch to implement light/dark
Comment on attachment 439273 [details] Patch to make FontCreationContext optional The FontCreationContext work is moved to https://bugs.webkit.org/show_bug.cgi?id=230800 Created attachment 439279 [details] Patch EWS will be red because this patch depends on https://bugs.webkit.org/show_bug.cgi?id=230800. This patch needs tests. Created attachment 439566 [details]
Patch
Created attachment 439636 [details]
Test font
Created attachment 439763 [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 439763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439763&action=review > LayoutTests/imported/w3c/ChangeLog:9 > + I will open a WPT PR for these posthaste. https://github.com/web-platform-tests/wpt/pull/31044 Created attachment 439805 [details]
Patch
Comment on attachment 439805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439805&action=review > Source/WebCore/ChangeLog:26 > + int64_t integer; Does the spec require these be 64-bit? > Source/WebCore/platform/graphics/FontPaletteValues.h:40 > + FontPaletteIndex() = default > Source/WebCore/platform/graphics/FontPaletteValues.h:83 > + enum class Type { : uint8_t ? > Source/WebCore/platform/graphics/FontPaletteValues.h:88 > + } type { Type::String }; I like to use 'const' for members that can only ever be set in the constructor. > Source/WebCore/platform/graphics/FontPaletteValues.h:90 > + unsigned integer { 0 }; You had int64_t in the changelog > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:472 > + int64_t rawIndex = basePalette.integer; // There is no kCFNumberUIntType. > + auto number = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt64Type, &rawIndex)); > + CFDictionaryAddValue(attributes, kCTFontPaletteAttribute, number.get()); Can author-supplied values conflict with kCTFontPaletteLight, kCTFontPaletteDark? Comment on attachment 439805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439805&action=review >> Source/WebCore/ChangeLog:26 >> + int64_t integer; > > Does the spec require these be 64-bit? Nope. In fact, our parser clamps to 32-bit. I'll update the changelog to be "unsigned" to match the implementation. >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:472 >> + CFDictionaryAddValue(attributes, kCTFontPaletteAttribute, number.get()); > > Can author-supplied values conflict with kCTFontPaletteLight, kCTFontPaletteDark? It depends if https://github.com/w3c/csswg-drafts/issues/6627 is accepted or not. If it's accepted (which I hope it is), there won't be a collision. Comment on attachment 439805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439805&action=review >> Source/WebCore/platform/graphics/FontPaletteValues.h:88 >> + } type { Type::String }; > > I like to use 'const' for members that can only ever be set in the constructor. It looks like that, because this is used as a key in a hash map, and our hash map infrastructure uses operator= to overwrite the buckets in the table, all the fields have to be assignable. :( Committed r283398 (242402@main): <https://commits.webkit.org/242402@main> |