[webkit-reviews] review granted: [Bug 222584] Reduce the size of extended colors by storing the color space in free bits of the owning Color : [Attachment 421968] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 222584: Reduce the size of extended colors by storing the color space in
free bits of the owning Color
https://bugs.webkit.org/show_bug.cgi?id=222584

Attachment 421968: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 421968
  --> https://bugs.webkit.org/attachment.cgi?id=421968
Patch

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

> Source/WebCore/ChangeLog:21
> +	   Additional, talk the opertunity to remove the isInline/isExtended
and

The traditional spelling is "Additionally, take the opportunity". We were using
an extremely unconventional spelling here!

> Source/WebCore/ChangeLog:23
> +	   is still neecessary in some places, but can be replaced by a single

"necessary"

> Source/WebCore/platform/graphics/Color.h:143
> +    // Returns the underlying color if the color space is sRGB and the value
> +    // is representable as SRGBA<uint8_t>.

I think "representable" here is ambiguous. Does that include rounding and
clamping floating point values? Or it is it only representable if it can be
converted to/from 8-bit without losing precision?

I looked at the implementation and it turns out that this will never convert a
floating point value to an integer, even if that can be done without any loss,
so I don’t think representable is currently accurate.

> Source/WebCore/platform/graphics/Color.h:179
> +    // Out of line and inline colors will always be non-equal.

This is an annoying semantic that unduly emphasizes this internal difference in
external API.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:63
> +	   if (stop.color.colorSpace() != ColorSpace::SRGB)
>	       hasExtendedColors = true;

Slightly subtle that this color space check also guarantees the ranges are such
that we don’t needed the extended SRGB CG color space. Almost wish the comment
was explicit about that.