Simplify and improve Gradient, some other small color-related removals
Created attachment 404060 [details] Patch
Created attachment 404069 [details] Patch
Created attachment 404071 [details] Patch
Created attachment 404073 [details] Patch
Comment on attachment 404073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404073&action=review > Source/WebCore/platform/graphics/Gradient.h:107 > WEBCORE_EXPORT static Ref<Gradient> create(LinearData&&); > WEBCORE_EXPORT static Ref<Gradient> create(RadialData&&); > - WEBCORE_EXPORT static Ref<Gradient> create(ConicData&&); > + WEBCORE_EXPORT static Ref<Gradient> create(Data&&); The inconsistency here bugs me. Why do we still need the create functions taking LinearData or RadialData? > Source/WebCore/platform/graphics/Gradient.h:166 > > mutable unsigned m_cachedHash { 0 }; Might be able save a byte or two if we re-order things so m_stopsSorted, m_spreadMethod and m_cachedHash pack next to each other. > Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:238 > void RenderSVGResourceGradient::addStops(GradientData* gradientData, const Vector<Gradient::ColorStop>& stops, const RenderStyle& style) const > { > + ASSERT(gradientData); > ASSERT(gradientData->gradient); Not sure how far you want to go, but it looks like all the callers can prove GradientData to be non-null so this could be switched to pass by reference.
Comment on attachment 404073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404073&action=review >> Source/WebCore/platform/graphics/Gradient.h:107 >> + WEBCORE_EXPORT static Ref<Gradient> create(Data&&); > > The inconsistency here bugs me. Why do we still need the create functions taking LinearData or RadialData? We don’t need them; for some reason I just didn’t think of removing them. I can, and should. >> Source/WebCore/platform/graphics/Gradient.h:166 >> mutable unsigned m_cachedHash { 0 }; > > Might be able save a byte or two if we re-order things so m_stopsSorted, m_spreadMethod and m_cachedHash pack next to each other. Yes, right now they are in a sort of "logical" order rather than a packing order, since the hash isn’t part of the gradient’s state. I am not sure I want to make the change, but I really don’t object either. >> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:238 >> ASSERT(gradientData->gradient); > > Not sure how far you want to go, but it looks like all the callers can prove GradientData to be non-null so this could be switched to pass by reference. That’s what I had in mind when I wrote the assertion. Any time a function asserts a pointer argument is non-null, to me that says "need to change the interface". But I was trying to stick to Gradient.h for this patch.
Comment on attachment 404073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404073&action=review >>> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:238 >>> ASSERT(gradientData->gradient); >> >> Not sure how far you want to go, but it looks like all the callers can prove GradientData to be non-null so this could be switched to pass by reference. > > That’s what I had in mind when I wrote the assertion. Any time a function asserts a pointer argument is non-null, to me that says "need to change the interface". But I was trying to stick to Gradient.h for this patch. I started down the rabbit hole, but pulled back. Things I ran into: - GradientData struct uses WTF_MAKE_FAST_ALLOCATED but should use WTF_MAKE_STRUCT_FAST_ALLOCATED - RenderSVGResourceGradient::addStops should take Gradient& argument, not a GradientData*. - RenderSVGResourceGradient::buildGradient should return a Ref<Gradient>, not take a GradientData* argument, allowing us to remove unnecessary null check from RenderSVGResourceGradient::applyResource. - RenderSVGResourceGradient::applyResource should use ensure and a lambda construct the GradientData initialized rather than first constructing it and then initializing it. - RenderSVGResourceGradient::m_gradientMap should probably contain GradientData rather than std::unique_ptr<GradientData>.
Created attachment 404103 [details] Patch
Created attachment 404104 [details] Patch
Committed r264280: <https://trac.webkit.org/changeset/264280>
<rdar://problem/65436807>