[webkit-reviews] review granted: [Bug 213948] SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them : [Attachment 403529] Part 1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 213948: SimpleColor and SRGBA<uint8_t> are essentially the same - let's
converge them
https://bugs.webkit.org/show_bug.cgi?id=213948

Attachment 403529: Part 1

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




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 403529
  --> https://bugs.webkit.org/attachment.cgi?id=403529
Part 1

View in context: https://bugs.webkit.org/attachment.cgi?id=403529&action=review

Love the direction this is going.

> Source/WebCore/ChangeLog:9
> +	   Begin converging SimpleColor and SRGBA<uint8_t>, starting with
removing usages that
> +	   were getting SimpleColors to access or operate on the color's
components.

Converging seems great. The strangest thing about SimpleColor is probably how
it can seems to prefer to represent all 4 channels in a single integer.

> Source/WebCore/ChangeLog:12
> +	   - Replace toSRGBASimpleColorLossy() with toSRGBALossy<uint8_t>
> +	   - Replace toSRGBALossy() with toSRGBALossy<float>().

It worries me that the integer vs. floating point type also implies [0-255] vs.
[0-1]. Kind of wish there was a "uint8_t to mean [0-1]" type instead of actual
uint8_t, but that would also ruin the code I guess. But it would be good to
prevent accidental conversion between that and floating point. Also ignores the
fact that floating point [0-255] is a thing.

> Source/WebCore/platform/graphics/Color.cpp:152
> +    return { makeSimpleColor(toSRGBALossy<uint8_t>()), Semantic };

This isn’t so pretty. The makeSimpleColor call here seems strange.

> Source/WebCore/platform/graphics/ColorTypes.h:243
> +struct ARGB {

Not sure if this is a specific enough name to imply "stored as a 32-bit integer
with 8-bit components". Seems like either 32 or 8 might need to be in the name.
SRGBA also defines the color space more precisely by including the letter "S".