Bug 212231 - Extended Color Cleanup: Remove trivial uses of Color::rgb()
Summary: Extended Color Cleanup: Remove trivial uses of Color::rgb()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks: 212184
  Show dependency treegraph
 
Reported: 2020-05-21 14:17 PDT by Sam Weinig
Modified: 2020-05-24 13:38 PDT (History)
20 users (show)

See Also:


Attachments
Patch (25.72 KB, patch)
2020-05-21 14:29 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (25.44 KB, patch)
2020-05-21 15:26 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-05-21 14:17:19 PDT
Extended Color Cleanup: Remove trivial uses of Color::rgb()
Comment 1 Sam Weinig 2020-05-21 14:29:13 PDT
Created attachment 399980 [details]
Patch
Comment 2 Darin Adler 2020-05-21 15:12:47 PDT
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.
Comment 3 Sam Weinig 2020-05-21 15:26:41 PDT
Created attachment 399986 [details]
Patch
Comment 4 Sam Weinig 2020-05-21 15:28:46 PDT
(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.
Comment 5 EWS 2020-05-21 16:33:21 PDT
Committed r262035: <https://trac.webkit.org/changeset/262035>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399986 [details].
Comment 6 Radar WebKit Bug Importer 2020-05-21 16:34:14 PDT
<rdar://problem/63516688>