[webkit-reviews] review granted: [Bug 212231] Extended Color Cleanup: Remove trivial uses of Color::rgb() : [Attachment 399980] 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 212231: Extended Color Cleanup: Remove trivial uses of Color::rgb()
https://bugs.webkit.org/show_bug.cgi?id=212231

Attachment 399980: Patch

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




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

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:381
> +	   if (style.hasOverrideAlpha())
> +	       style =
CanvasStyle(currentColor(canvasBase()).colorWithAlphaUsingAlternativeRounding(s
tyle.overrideAlpha()));
> +	   else
>	       style = CanvasStyle(currentColor(canvasBase()));

If colorWithAlphaUsingAlternativeRounding took an Optional then we could write
this code in a more straightforward way. The style could give us an optional
overriding value for the alpha.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:407
> +	   if (style.hasOverrideAlpha())
> +	       style =
CanvasStyle(currentColor(canvasBase()).colorWithAlphaUsingAlternativeRounding(s
tyle.overrideAlpha()));
> +	   else
>	       style = CanvasStyle(currentColor(canvasBase()));

Would be nice here.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1313
> +    if (alpha)
> +	   color = color.colorWithAlphaUsingAlternativeRounding(*alpha);
> +    setShadow(FloatSize(width, height), blur, color);

And here the code is getting less simple because we don’t have that.

I wish we were keeping it.

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:126
> +    if (equalIgnoringSemanticColor(color, Color::transparent)) {

I hope performance is still great. The old version was pleasantly bit-twiddly
and inlined in a way that gave me confidence it was efficient.

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:130
> +    if (Color::isBlackColor(color)) {

I find this Color::isBlackColor function vexing. Should it return true when an
extended color is black? For the use here, I think we’d want the super-fast one
even if it gets false negatives.

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:134
> +    if (Color::isWhiteColor(color)) {

Ditto.