Summary: | Extended Color Cleanup: Remove trivial uses of Color::rgb() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | calvaris, cdumez, changseok, darin, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, mifenton, pdr, philipj, sabouhallawa, schenney, sergio, simon.fraser, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=198628 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 212184 | ||||||||
Attachments: |
|
Description
Sam Weinig
2020-05-21 14:17:19 PDT
Created attachment 399980 [details]
Patch
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. Created attachment 399986 [details]
Patch
(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. Committed r262035: <https://trac.webkit.org/changeset/262035> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399986 [details]. |