[webkit-reviews] review granted: [Bug 214521] Remove ColorBuilder : [Attachment 404644] 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 214521: Remove ColorBuilder
https://bugs.webkit.org/show_bug.cgi?id=214521

Attachment 404644: Patch

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




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

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

> Source/WebCore/platform/graphics/ColorTypes.h:45
> +template<typename Parent> struct ColorType {

Seems like this should be called CanApplyAlpha or AlphaHelper or something. The
generic name ColorType doesn't truly express what it currently does. But maybe
in future it will do more.

> Source/WebCore/platform/graphics/ColorTypes.h:48
> +    constexpr Parent colorWithAlpha(uint8_t overrideAlpha) const

Because this always takes uint8_t with no overloading, it is dangerous to pass
a 1.0 to this function. It will set alpha to 1/255, I think, without
necessarily warning.

> Source/WebCore/platform/graphics/ColorTypes.h:53
> +	   copy.alpha = overrideAlpha;

Will this do the right thing if the color type’s component type is float?

> Source/WebCore/platform/graphics/ColorTypes.h:106
> +    constexpr LinearSRGBA(T red, T green, T blue, T alpha =
ComponentTraits<T>::maxValue)

Super unhappy that we have to write all these constructors.