Bug 212231 – Extended Color Cleanup: Remove trivial uses of Color::rgb()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Extended Color Cleanup: Remove trivial uses of Color::rgb()

[Sam Weinig]

Created attachment 399980 [details]
Patch

[Darin Adler]

Comment on attachment 399980 [details]
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(style.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(style.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.

[Sam Weinig]

Created attachment 399986 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 399980 [details]
> 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(style.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(style.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.

Added. Made the code simplifications.

> 
> 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.

Removed this change. It's a use of rgb() that is local to Color family of files so we can make it a friend / member of Color when needed.

[EWS]

Committed r262035: <https://trac.webkit.org/changeset/262035>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399986 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63516688>