Bug 214221

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 404104 [details]
Patch

Committed r264280: <https://trac.webkit.org/changeset/264280>

<rdar://problem/65436807>