Implement CanvasRenderingContext2D.createConicGradient

• Component: Canvas
• Status: RESOLVED FIXED

[Yi Xu]

Proposal document:
https://github.com/fserb/canvas2D/blob/master/spec/conic-gradient.md

Syntax:
const grad = ctx.createConicGradient(0, 100, 100);

grad.addColorStop(0, "red");
grad.addColorStop(0.25, "orange");
grad.addColorStop(0.5, "yellow");
grad.addColorStop(0.75, "green");
grad.addColorStop(1, "blue");

Corresponding bugs:
Chromium: crbug.com/1097034
Mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1627014

[Said Abou-Hallawa]

Created attachment 428228 [details]
Patch

[Sam Weinig]

Comment on attachment 428228 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasFillStrokeStyles.idl:48
> +    CanvasGradient createConicGradient(float angle, float x, float y);

I believe these should be ‘unrestricted double’ according to the linked spec

[Simon Fraser (smfr)]

Comment on attachment 428228 [details]
Patch

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

Can we write some ref tests for this?

> Source/WebCore/html/canvas/CanvasGradient.cpp:48
> +CanvasGradient::CanvasGradient(const FloatPoint& p, float angleInRadians, CanvasBase& canvasBase)

Can we call p centerPoint instead?

> Source/WebCore/html/canvas/CanvasGradient.cpp:64
> +Ref<CanvasGradient> CanvasGradient::create(const FloatPoint& p, float angleInRadians, CanvasBase& canvasBase)

Same

[Said Abou-Hallawa]

Created attachment 428310 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 428228 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasFillStrokeStyles.idl:48
>> +    CanvasGradient createConicGradient(float angle, float x, float y);
> 
> I believe these should be ‘unrestricted double’ according to the linked spec

Yes this is true. But the radial and linear gradient need to be ‘unrestricted double’ as well; see https://html.spec.whatwg.org/multipage/canvas.html#canvasfillstrokestyles.

The problem is the Gradient::Data members are floats and FloatPoints. So we need to change the data types of all gradients to be double instead of float. I think we should address this in a separate patch.

>> Source/WebCore/html/canvas/CanvasGradient.cpp:48
>> +CanvasGradient::CanvasGradient(const FloatPoint& p, float angleInRadians, CanvasBase& canvasBase)
> 
> Can we call p centerPoint instead?

Fixed.

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #5)
> Comment on attachment 428228 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428228&action=review
> 
> >> Source/WebCore/html/canvas/CanvasFillStrokeStyles.idl:48
> >> +    CanvasGradient createConicGradient(float angle, float x, float y);
> > 
> > I believe these should be ‘unrestricted double’ according to the linked spec
> 
> Yes this is true. But the radial and linear gradient need to be
> ‘unrestricted double’ as well; see
> https://html.spec.whatwg.org/multipage/canvas.html#canvasfillstrokestyles.
> 
> The problem is the Gradient::Data members are floats and FloatPoints. So we
> need to change the data types of all gradients to be double instead of
> float. I think we should address this in a separate patch.

I think those two things can be handled separately. The IDL can be in terms of unrestricted double and do the narrowing I the c++ instead (we certainly do that elsewhere). But, if you plan to do a follow up soon after this, doing it in another pass seems fine. If you don't plan on doing that work soon, I think making its type match should be done now.

[Said Abou-Hallawa]

Created attachment 428402 [details]
Patch

[Sam Weinig]

Comment on attachment 428402 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasFillStrokeStyles.idl:47
> +    CanvasGradient createConicGradient(unrestricted double angle, unrestricted double x, unrestricted double y);

Probably have some old tests to update, but looks good.

[Radar WebKit Bug Importer]

<rdar://problem/78033646>

[Said Abou-Hallawa]

Created attachment 428718 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 428718 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasFillStrokeStyles.idl:47
> +    CanvasGradient createLinearGradient(double x0, double y0, double x1, double y1);
> +    CanvasGradient createRadialGradient(double x0, double y0, double r0, double x1, double y1, double r1);
> +    CanvasGradient createConicGradient(double angle, double x, double y);

I had to make these parameter 'double' instead of 'unrestricted double' to pass the some of the canvas gradient layout tests. I checked the sources of Chrome and FireFox and I found they make them 'double' also.

[EWS]

Committed r277547 (237775@main): <https://commits.webkit.org/237775@main>

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

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #11)
> Comment on attachment 428718 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428718&action=review
> 
> > Source/WebCore/html/canvas/CanvasFillStrokeStyles.idl:47
> > +    CanvasGradient createLinearGradient(double x0, double y0, double x1, double y1);
> > +    CanvasGradient createRadialGradient(double x0, double y0, double r0, double x1, double y1, double r1);
> > +    CanvasGradient createConicGradient(double angle, double x, double y);
> 
> I had to make these parameter 'double' instead of 'unrestricted double' to
> pass the some of the canvas gradient layout tests. I checked the sources of
> Chrome and FireFox and I found they make them 'double' also.

Please file a bug on the HTML spec. If we can't implement something as specified, we should be trying to get the spec changed to match reality.

[Said Abou-Hallawa]

Comment on attachment 428718 [details]
Patch

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

>>> Source/WebCore/html/canvas/CanvasFillStrokeStyles.idl:47
>>> +    CanvasGradient createConicGradient(double angle, double x, double y);
>> 
>> I had to make these parameter 'double' instead of 'unrestricted double' to pass the some of the canvas gradient layout tests. I checked the sources of Chrome and FireFox and I found they make them 'double' also.
> 
> Please file a bug on the HTML spec. If we can't implement something as specified, we should be trying to get the spec changed to match reality.

I checked the link https://html.spec.whatwg.org/multipage/canvas.html#canvasfillstrokestyles and I think the types were changed recently to be all just 'double'. I was referring to it above on May 11, 2021. And it has changed on May 12, 2021.

So right now we implement createLinearGradient() and createRadialGradient() as they are specified.

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #14)
> Comment on attachment 428718 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428718&action=review
> 
> >>> Source/WebCore/html/canvas/CanvasFillStrokeStyles.idl:47
> >>> +    CanvasGradient createConicGradient(double angle, double x, double y);
> >> 
> >> I had to make these parameter 'double' instead of 'unrestricted double' to pass the some of the canvas gradient layout tests. I checked the sources of Chrome and FireFox and I found they make them 'double' also.
> > 
> > Please file a bug on the HTML spec. If we can't implement something as specified, we should be trying to get the spec changed to match reality.
> 
> I checked the link
> https://html.spec.whatwg.org/multipage/canvas.html#canvasfillstrokestyles
> and I think the types were changed recently to be all just 'double'. I was
> referring to it above on May 11, 2021. And it has changed on May 12, 2021.
> 
> So right now we implement createLinearGradient() and createRadialGradient()
> as they are specified.

Excellent!