Bug 225539 - Implement CanvasRenderingContext2D.createConicGradient
Summary: Implement CanvasRenderingContext2D.createConicGradient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2021-05-07 14:40 PDT by Yi Xu
Modified: 2021-05-17 15:15 PDT (History)
13 users (show)

See Also:


Attachments
Patch (11.68 KB, patch)
2021-05-10 17:24 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (25.10 KB, patch)
2021-05-11 14:20 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (25.35 KB, patch)
2021-05-12 12:42 PDT, Said Abou-Hallawa
sam: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (28.38 KB, patch)
2021-05-14 23:10 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yi Xu 2021-05-07 14:40:45 PDT
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
Comment 1 Said Abou-Hallawa 2021-05-10 17:24:34 PDT
Created attachment 428228 [details]
Patch
Comment 2 Sam Weinig 2021-05-10 22:15:27 PDT
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
Comment 3 Simon Fraser (smfr) 2021-05-11 11:01:17 PDT
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
Comment 4 Said Abou-Hallawa 2021-05-11 14:20:05 PDT
Created attachment 428310 [details]
Patch
Comment 5 Said Abou-Hallawa 2021-05-11 14:25:45 PDT
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.
Comment 6 Sam Weinig 2021-05-12 11:49:52 PDT
(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.
Comment 7 Said Abou-Hallawa 2021-05-12 12:42:31 PDT
Created attachment 428402 [details]
Patch
Comment 8 Sam Weinig 2021-05-12 16:24:04 PDT
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.
Comment 9 Radar WebKit Bug Importer 2021-05-14 14:41:17 PDT
<rdar://problem/78033646>
Comment 10 Said Abou-Hallawa 2021-05-14 23:10:20 PDT
Created attachment 428718 [details]
Patch
Comment 11 Said Abou-Hallawa 2021-05-15 13:40:39 PDT
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.
Comment 12 EWS 2021-05-15 13:47:04 PDT
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].
Comment 13 Sam Weinig 2021-05-16 08:46:36 PDT
(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.
Comment 14 Said Abou-Hallawa 2021-05-17 12:03:31 PDT
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.
Comment 15 Sam Weinig 2021-05-17 15:15:51 PDT
(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!