Bug 230792 – base-palette can accept "light" or "dark"

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

[Myles C. Maxfield]

-

[Radar WebKit Bug Importer]

<rdar://problem/83530228>

[Myles C. Maxfield]

https://github.com/w3c/csswg-drafts/commit/1c74611151b452930609627a0de2412c0cb86175

[Myles C. Maxfield]

Created attachment 439273 [details]
Patch to make FontCreationContext optional

[Myles C. Maxfield]

Created attachment 439274 [details]
Patch to implement light/dark

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 439279 [details]
Patch

EWS will be red because this patch depends on https://bugs.webkit.org/show_bug.cgi?id=230800.

[Myles C. Maxfield]

This patch needs tests.

[Myles C. Maxfield]

Created attachment 439566 [details]
Patch

[Myles C. Maxfield]

Created attachment 439636 [details]
Test font

[Myles C. Maxfield]

Created attachment 439763 [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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 439805 [details]
Patch

[Simon Fraser (smfr)]

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?

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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. :(

[Myles C. Maxfield]

Committed r283398 (242402@main): <https://commits.webkit.org/242402@main>