Bug 179595 – Clean up gradient code in preparation for conic gradients

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Sam Weinig]

Clean up gradient code in preparation for conic gradients

[Sam Weinig]

Created attachment 326717 [details]
Patch

[Sam Weinig]

Created attachment 326718 [details]
Patch

[Sam Weinig]

Created attachment 326723 [details]
Patch

[Darin Adler]

Comment on attachment 326723 [details]
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?

[Sam Weinig]

Created attachment 327349 [details]
Patch

[Sam Weinig]

Created attachment 327350 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 327350 [details]
Patch

Clearing flags on attachment: 327350

Committed r225036: <https://trac.webkit.org/changeset/225036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35638901>