Remove remaining makeSimpleColorFrom* variants
Created attachment 403017 [details] Patch
Created attachment 403020 [details] Patch
Comment on attachment 403020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403020&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:918 > + if (state().strokeStyle.isValid() && state().strokeStyle.isEquivalent(color)) Seems awkward that we need to explicitly check isValid. Why does isEquivalent ever return true when isValid is false? Wish invalid color was a little more like Optional's nullopt. > Source/WebCore/html/canvas/CanvasStyle.h:74 > CMYKAColor(const CMYKAColor&) = default; Why is this needed? > Source/WebCore/platform/graphics/ColorUtilities.h:38 > +template<typename> struct CMYKA; Alphabetical order, please > Source/WebCore/platform/graphics/ColorUtilities.h:67 > - return std::clamp(static_cast<int>(std::lroundf(f)), 0, 255); > + return std::clamp(static_cast<int>(std::round(f)), 0, 255); I think maybe std::lround would be better than std::round. Since the library can round and convert to int in a single operation, why not do it? Or if we are trying to make this work for values larger or smaller than an int, then we should not cast to int, and need a clamp that takes a floating point value as an argument. > Source/WebCore/platform/graphics/ColorUtilities.h:72 > - return std::clamp(static_cast<int>(std::lroundf(f * 255.0f)), 0, 255); > + return std::clamp(static_cast<int>(std::round(f * 255.0f)), 0, 255); Ditto.
(In reply to Darin Adler from comment #3) > Comment on attachment 403020 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403020&action=review > > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:918 > > + if (state().strokeStyle.isValid() && state().strokeStyle.isEquivalent(color)) > > Seems awkward that we need to explicitly check isValid. Why does > isEquivalent ever return true when isValid is false? Wish invalid color was > a little more like Optional's nullopt. Looks like this isValid() is totally not needed. isEquivalent() will return false already is the CanvasStyle is in the invalid state. Removed a bunch of these. > > > Source/WebCore/html/canvas/CanvasStyle.h:74 > > CMYKAColor(const CMYKAColor&) = default; > > Why is this needed? I am not sure. Will try removing it. > > > Source/WebCore/platform/graphics/ColorUtilities.h:38 > > +template<typename> struct CMYKA; > > Alphabetical order, please Fixed. > > > Source/WebCore/platform/graphics/ColorUtilities.h:67 > > - return std::clamp(static_cast<int>(std::lroundf(f)), 0, 255); > > + return std::clamp(static_cast<int>(std::round(f)), 0, 255); > > I think maybe std::lround would be better than std::round. Since the library > can round and convert to int in a single operation, why not do it? > > Or if we are trying to make this work for values larger or smaller than an > int, then we should not cast to int, and need a clamp that takes a floating > point value as an argument. > > > Source/WebCore/platform/graphics/ColorUtilities.h:72 > > - return std::clamp(static_cast<int>(std::lroundf(f * 255.0f)), 0, 255); > > + return std::clamp(static_cast<int>(std::round(f * 255.0f)), 0, 255); > > Ditto. Fixed.
Created attachment 403088 [details] Patch
Committed r263688: <https://trac.webkit.org/changeset/263688> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403088 [details].
<rdar://problem/64906405>