Bug 234583 – Encapsulate gradient color stops into a self contained class

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sam Weinig]

Rather than having the gradient color stop vector and a separate m_stopsSorted bit in Gradient, it would be slightly nicer to have a encapsulated object to represent the stops set.

[Sam Weinig]

Created attachment 447809 [details]
Patch

[Sam Weinig]

I think this is an improvement, but I would eventually like to make Gradient immutable after construction if we can, to simplify things further.

The only place that remains that uses mutation is CanvasGradient, but since mutation invalidates everything anyway, we just store both a GradientColorStops and a lazy created Gradient on CanvasGradient, and just invalidate the Gradient when ever a stop is added.

If we made Gradient immutable, we could simplify sorting and the hash cache by always sorting on creation.

[Sam Weinig]

I think this is an improvement, but I would eventually like to make Gradient immutable after construction if we can, to simplify things further.

The only place that remains that uses mutation is CanvasGradient, but since mutation invalidates everything anyway, we just store both a GradientColorStops and a lazy created Gradient on CanvasGradient, and just invalidate the Gradient when ever a stop is added.

If we made Gradient immutable, we could simplify sorting and the hash cache by always sorting on creation.

[Sam Weinig]

Created attachment 447811 [details]
Patch

[Sam Weinig]

Created attachment 447884 [details]
Patch

[EWS]

Committed r287411 (245546@main): <https://commits.webkit.org/245546@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447884 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86863966>

[Darin Adler]

Comment on attachment 447884 [details]
Patch

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

> Source/WebCore/platform/graphics/GradientColorStops.h:91
> +    template<typename MapFunction> GradientColorStops mapColors(MapFunction&& mapFunction) const

I suggest const MapFunction& here.

[Sam Weinig]

(In reply to Darin Adler from comment #8)
> Comment on attachment 447884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447884&action=review
> 
> > Source/WebCore/platform/graphics/GradientColorStops.h:91
> > +    template<typename MapFunction> GradientColorStops mapColors(MapFunction&& mapFunction) const
> 
> I suggest const MapFunction& here.

I think that would disallow stateful lambdas, which can be quite useful. What do you see as the benefit of a const reference over the universal reference here?

[Darin Adler]

(In reply to Sam Weinig from comment #9)
> (In reply to Darin Adler from comment #8)
> > I suggest const MapFunction& here.
> 
> I think that would disallow stateful lambdas, which can be quite useful.
> What do you see as the benefit of a const reference over the universal
> reference here?

I thought the const reference would make it clear that this function does not take ownership of the passed in function, thus the function can be reused elsewhere.

It didn’t occur to me that taking a const T& prevents the use of stateful lambdas or that this is a universal reference, rather than an rvalue reference. I wonder what our general design pattern should be. I was thinking about when we use an rvalue reference for WTF::Function: Function&& vs. const Function&.