[webkit-reviews] review granted: [Bug 179595] Clean up gradient code in preparation for conic gradients : [Attachment 326723] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 179595: Clean up gradient code in preparation for conic gradients
https://bugs.webkit.org/show_bug.cgi?id=179595

Attachment 326723: Patch

https://bugs.webkit.org/attachment.cgi?id=326723&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 326723
  --> https://bugs.webkit.org/attachment.cgi?id=326723
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326723&action=review

> Source/WebCore/css/CSSGradientValue.cpp:138
> +struct LinearGradientAdaptor {

Unclear to me why this is a struct rather than a class.

> Source/WebCore/css/CSSGradientValue.cpp:139
> +    LinearGradientAdaptor(Gradient::LinearData& data)

Should mark this explicit since it’s not for type conversion.

> Source/WebCore/css/CSSGradientValue.cpp:161
> +	       data.point0 = FloatPoint(p0.x() + firstOffset * (p1.x() -
p0.x()), p0.y() + firstOffset * (p1.y() - p0.y()));
> +	       data.point1 = FloatPoint(p1.x() + (lastOffset - 1) * (p1.x() -
p0.x()), p1.y() + (lastOffset - 1) * (p1.y() - p0.y()));

Can we use { } instead of FloatPoint( ) here?

> Source/WebCore/css/CSSGradientValue.cpp:172
> +struct RadialGradientAdaptor {

Unclear to me why this is a struct rather than a class.

> Source/WebCore/css/CSSGradientValue.cpp:173
> +    RadialGradientAdaptor(Gradient::RadialData& data)

Should mark this explicit since it’s not for type conversion.

> Source/WebCore/css/CSSGradientValue.cpp:179
> +    FloatPoint startPoint() { return data.point0; }
> +    FloatPoint endPoint() { return data.point0 + FloatSize(data.endRadius,
0); }

Should be const. Might use FloatSize { } instead of FloatSize( ).

> Source/WebCore/css/CSSGradientValue.cpp:230
> +	   for (auto& stop : stops)
> +	       stop.offset /= scale;

Is it more efficient to invert scale and use *= inside the loop?

> Source/WebCore/css/CSSGradientValue.cpp:239
> +template<class GradientAdaptor>

I normally use typename rather than class. Also, I often put these on the same
line, but in this case that would be a long one so maybe not.

> Source/WebCore/css/CSSGradientValue.cpp:267
> +    FloatSize gradientSize(gradientStart - gradientEnd);

Consider this:

    auto gradientSize = gradientStart - gradientEnd;

> Source/WebCore/css/CSSGradientValue.cpp:827
> +    LinearGradientAdaptor adaptor { data };

What's the argument for adaptor vs. adapter in these names? I would be tempted
to just always use the more common spelling "adapter".

> Source/WebCore/css/CSSGradientValue.h:112
> +    template<class GradientAdaptor>

typename

> Source/WebCore/platform/graphics/Gradient.h:92
> +    enum class Type {
> +	   Linear,
> +	   Radial
> +    };

I like these kinds of things as one-liners.

> Source/WebCore/platform/graphics/Gradient.h:97
> +    static Ref<Gradient> create(LinearData&& data)
> +    {
> +	   return adoptRef(*new Gradient(WTFMove(data)));
> +    }

I’ve been learning towards putting these create function bodies in cpp files
and inlining the constructors instead of inlining the create functions. Smaller
at each call site, roughly the same number of function calls.

> Source/WebCore/platform/graphics/Gradient.h:111
> +    const Variant<LinearData, RadialData>& data() const { return m_data; }

Could consider:

    using Data = Variant<LinearData, RadialData>;

Then we would have members Data ("type member"), data (function member), and
m_data (data member). I did something similar in my recent work in another
class.

> Source/WebCore/platform/graphics/Gradient.h:124
> +    AffineTransform gradientSpaceTransform() { return
m_gradientSpaceTransformation; }

Make this const? Return a const& since AffineTransform is really big?