[webkit-reviews] review granted: [Bug 230792] base-palette can accept "light" or "dark" : [Attachment 439805] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 230792: base-palette can accept "light" or "dark"
https://bugs.webkit.org/show_bug.cgi?id=230792

Attachment 439805: Patch

https://bugs.webkit.org/attachment.cgi?id=439805&action=review




--- Comment #14 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 439805
  --> https://bugs.webkit.org/attachment.cgi?id=439805
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?