Bug 232780 – Add context.roundRect support to 2D canvas

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

[Lauro Moura]

Recently, r285346 updated the html/ WPT folder and a number of new offscreencanvas tests use the roundRect method, which is not yet exposed.

List of tests:

imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.1.radius.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.1.radius.dompointinit.worker.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.2.radii.1.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.2.radii.1.dompointinit.worker.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.2.radii.2.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.2.radii.2.dompointinit.worker.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.3.radii.1.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.3.radii.2.dompointinit.worker.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.3.radii.3.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.3.radii.3.dompointinit.worker.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.4.radii.1.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.4.radii.1.dompointinit.worker.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.4.radii.2.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.4.radii.2.dompointinit.worker.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.4.radii.3.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.4.radii.3.dompointinit.worker.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.4.radii.4.dompointinit.html
imported/w3c/web-platform-tests/html/canvas/offscreen/path-objects/2d.path.roundrect.4.radii.4.dompointinit.worker.html

As they're new, the bots report missing results but the actual results happen to be FAIL messages.

I've got a working patch locally exposing roundRect (using Path::addRoundedRect, like path.rect()). Will do some cleanup before submitting.

[Lauro Moura]

Link to spec: https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-roundrect

[Lauro Moura]

Created attachment 443765 [details]
Patch

[Lauro Moura]

Comment on attachment 443765 [details]
Patch

Needs rebase.

[Lauro Moura]

Created attachment 443766 [details]
Rebased patch

[Cameron McCormack (:heycam)]

Nice, I just noticed today too that there are a bunch of tests for roundRect() (on OffscreenCanvas and regular canvas).

[Lauro Moura]

Created attachment 443834 [details]
Updated patch

Patch addressing EWS errors and moving expectations from platform/glib to root expectations

[Devin Rousso]

Comment on attachment 443834 [details]
Updated patch

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

I think this should also have some test coverage for the Web Inspector canvas recorder.  It'd likely involve just adding two `roundRect` calls in a new function at the end of `performActions` in `LayoutTests/inspector/canvas/resources/recording-2d.js` and then rerunning the tests inside `LayoutTests/inspector/canvas`.  While I would prefer that being done as part of this patch, I don't want Web Inspector to stand in the way of new features being added so I would accept a followup bug instead.

> Source/WebCore/inspector/InspectorCanvas.cpp:695
> +std::optional<InspectorCanvasCallTracer::ProcessedArgument> InspectorCanvas::processArgument(WTF::Vector<std::variant<double, WebCore::DOMPointInit> >& argument)

NIT: Can you please move this to be right below the `processArgument` that handles `Vector<int32_t>&` so that the order of functions matches what's in `FOR_EACH_INSPECTOR_CANVAS_CALL_TRACER_ARGUMENT`?

> Source/WebCore/inspector/InspectorCanvas.cpp:698
> +        if (std::holds_alternative<DOMPointInit>(item)) {

NIT: it would be slightly preferable to use `WTF::switchOn` instead of `std::holds_alternative` with `std::get` so that if a new type is added to this `std::variant` then there's a compile-time guarantee that this codepath wont be missed

> Source/WebCore/inspector/InspectorCanvas.cpp:705
> +            return object;

While I do think this will work, a primary goal of this entire system is to avoid duplicating data since all of it has to be serialized and sent over the Web Inspector connection (which could be communicating with an entire other device, such as when inspecting an iOS device from a macOS machine).  As such, we probably want to create either a new `RecordingSwizzleType::DOMPointInit` or a more generic `RecordingSwizzleType::Object` so that if multiple calls to `roundRect` are made with the same `radii` argument we don't encode the same data more than once.  As such, can you please file a followup bug about improving/optimizing this and include a FIXME comment here?

> Source/WebCore/inspector/InspectorCanvas.cpp:709
> +    auto array = JSON::ArrayOf<JSON::Value>::create();

you should be able to use `buildArrayForVector` instead of creating this yourself

> Source/WebCore/inspector/InspectorCanvasCallTracer.h:181
> +    macro(SINGLE_ARG(Vector<std::variant<double, DOMPointInit> >&)) \

Rather than create a whole new macro for this, could we just wrap the type in parenthesis (e.g. `macro((Vector<std::variant<double, DOMPointInit>>&))`)?

Alternatively, you could define a `using RadiusVariant = std::variant<double, DOMPointInit>` inside `CanvasPath` and then use that here as `Vector<CanvasPath::RadiusVariant>&` (see `CanvasRenderingContext2DBase::StyleVariant` above as an example).

[Lauro Moura]

Created attachment 443893 [details]
Updated patch

Patch addressing Devin's comments and more EWS errors.

[Lauro Moura]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 443834 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443834&action=review
> 
> I think this should also have some test coverage for the Web Inspector
> canvas recorder.  It'd likely involve just adding two `roundRect` calls in a
> new function at the end of `performActions` in
> `LayoutTests/inspector/canvas/resources/recording-2d.js` and then rerunning
> the tests inside `LayoutTests/inspector/canvas`.  While I would prefer that
> being done as part of this patch, I don't want Web Inspector to stand in the
> way of new features being added so I would accept a followup bug instead.

Added. In the end it was only the recording-2d-full.html that failed.

> 
> > Source/WebCore/inspector/InspectorCanvas.cpp:695
> > +std::optional<InspectorCanvasCallTracer::ProcessedArgument> InspectorCanvas::processArgument(WTF::Vector<std::variant<double, WebCore::DOMPointInit> >& argument)
> 
> NIT: Can you please move this to be right below the `processArgument` that
> handles `Vector<int32_t>&` so that the order of functions matches what's in
> `FOR_EACH_INSPECTOR_CANVAS_CALL_TRACER_ARGUMENT`?

Done.

> 
> > Source/WebCore/inspector/InspectorCanvas.cpp:698
> > +        if (std::holds_alternative<DOMPointInit>(item)) {
> 
> NIT: it would be slightly preferable to use `WTF::switchOn` instead of
> `std::holds_alternative` with `std::get` so that if a new type is added to
> this `std::variant` then there's a compile-time guarantee that this codepath
> wont be missed

Updated in the inspector and canvaspath code.

> 
> > Source/WebCore/inspector/InspectorCanvas.cpp:705
> > +            return object;
> 
> While I do think this will work, a primary goal of this entire system is to
> avoid duplicating data since all of it has to be serialized and sent over
> the Web Inspector connection (which could be communicating with an entire
> other device, such as when inspecting an iOS device from a macOS machine). 
> As such, we probably want to create either a new
> `RecordingSwizzleType::DOMPointInit` or a more generic
> `RecordingSwizzleType::Object` so that if multiple calls to `roundRect` are
> made with the same `radii` argument we don't encode the same data more than
> once.  As such, can you please file a followup bug about
> improving/optimizing this and include a FIXME comment here?

Sure, I added the comment and can open a bug about it.

> 
> > Source/WebCore/inspector/InspectorCanvas.cpp:709
> > +    auto array = JSON::ArrayOf<JSON::Value>::create();
> 
> you should be able to use `buildArrayForVector` instead of creating this
> yourself

I ended up keeping the local implementation due to WTFMov'ing the item, as JSON::Value's copy constructor was deleted. Is there any way to use the existing function? 

> 
> > Source/WebCore/inspector/InspectorCanvasCallTracer.h:181
> > +    macro(SINGLE_ARG(Vector<std::variant<double, DOMPointInit> >&)) \
> 
> Rather than create a whole new macro for this, could we just wrap the type
> in parenthesis (e.g. `macro((Vector<std::variant<double, DOMPointInit>>&))`)?
> 
> Alternatively, you could define a `using RadiusVariant =
> std::variant<double, DOMPointInit>` inside `CanvasPath` and then use that
> here as `Vector<CanvasPath::RadiusVariant>&` (see
> `CanvasRenderingContext2DBase::StyleVariant` above as an example).

Yeah, creating the alias made it more readable (and more future-proof).

Also updated the baselines for some idlharness failures and removed the designated initializers windows build were complaining about.

Thanks for the feedback!

[Devin Rousso]

Comment on attachment 443893 [details]
Updated patch

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

> Source/WebCore/inspector/InspectorCanvas.cpp:486
> +                // FIXME We'd likely want to either create a new RecordingSwizzleType::DOMPointInit or RecordingSwizzleType::Object to avoid
> +                // encoding the same data multiple times

Can you please also file a new bugzilla bug for this? :)

> Source/WebCore/inspector/InspectorCanvas.cpp:497
> +    // Did not use buildArrayForVector due to WTFMov'ing the Ref<Value> to the vector as Value copy constructor was deleted.
> +    auto array = JSON::ArrayOf<JSON::Value>::create();
> +    for (auto& item : processed)
> +        array->addItem(WTFMove(item));
> +    return { { array, RecordingSwizzleType::Array } };

Sorry I should've been clearer.  I think you should be able to replace all of this with just
```
    return {{ buildArrayForVector(WTFMove(processed), RecordingSwizzleType::Array }};
```

(as a side note for my future self, it would actually be even better to have `buildArrayForVector` take a lambda so that we can skip the `.map` calls and therefore avoid another `Vector` allocation)

[Radar WebKit Bug Importer]

<rdar://problem/85366210>

[Lauro Moura]

(In reply to Devin Rousso from comment #10)
> Comment on attachment 443893 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443893&action=review
> 
> > Source/WebCore/inspector/InspectorCanvas.cpp:486
> > +                // FIXME We'd likely want to either create a new RecordingSwizzleType::DOMPointInit or RecordingSwizzleType::Object to avoid
> > +                // encoding the same data multiple times
> 
> Can you please also file a new bugzilla bug for this? :)

Opened bug233255.

> 
> > Source/WebCore/inspector/InspectorCanvas.cpp:497
> > +    // Did not use buildArrayForVector due to WTFMov'ing the Ref<Value> to the vector as Value copy constructor was deleted.
> > +    auto array = JSON::ArrayOf<JSON::Value>::create();
> > +    for (auto& item : processed)
> > +        array->addItem(WTFMove(item));
> > +    return { { array, RecordingSwizzleType::Array } };
> 
> Sorry I should've been clearer.  I think you should be able to replace all
> of this with just
> ```
>     return {{ buildArrayForVector(WTFMove(processed),
> RecordingSwizzleType::Array }};
> ```
> 
> (as a side note for my future self, it would actually be even better to have
> `buildArrayForVector` take a lambda so that we can skip the `.map` calls and
> therefore avoid another `Vector` allocation)

Tried this and it complains as, IIUC, the current `buildArrayForVector` impl doesn't expect to move the list contents into the created array, while ArrayOf<T>->addItem for JSON::Value expects them to be movable.

Maybe add a `buildArrayForVector` overload that would allow moving the data like the one below?

```
template<typename T> static Ref<JSON::ArrayOf<JSON::Value>> buildArrayForVector(Vector<T>&& vector)
{
    auto array = JSON::ArrayOf<JSON::Value>::create();
    for (auto& item : vector)
        array->addItem(WTFMove(item));
    return array;
}
```

[Lauro Moura]

Created attachment 445246 [details]
Updated patch with EWS fix

Patch fixing issue with infinite/nan radius caught by debug-enabled EWS bots.

[Lauro Moura]

mips failure related to a build issue fixed in bug233551

[Lauro Moura]

Comment on attachment 445246 [details]
Updated patch with EWS fix

The patch needs a minor update after Chromium added new tests to WPT where `radii` can be a single `double` or `DOMPointInit` unwrapped in a list.[1]

PS: This case doesn't seem to be covered by the spec yet, though. Might need to fill an issue there too.

[1] https://github.com/web-platform-tests/wpt/pull/32251

[Lauro Moura]

Created attachment 450310 [details]
Rebased and updated patch

[Lauro Moura]

Created attachment 450312 [details]
Rebased and updated patch fixed

Previous patch had a couple of .orig files that were mistankely added to it.

[Lauro Moura]

(In reply to Lauro Moura from comment #17)
> Created attachment 450312 [details]
> Rebased and updated patch fixed
> 
> Previous patch had a couple of .orig files that were mistankely added to it.

This version differs from the December patch by adding an overload that receives a single radius without being wrapped in a list, as explained in #c15. Also minor rebases to some expected files.

Devin, regarding RadiusVariant swizzling, I've added a `processArgument` receiving it and handling the float variant but the case receiving a DOMPointInit I left as a stub waiting for bug233255 (proper DOMPointInit or object swizzling support)

[Lauro Moura]

Created attachment 450324 [details]
Fixed unused param

Also updated idlharness for gtk/wpe. Will wait EWS to get the results for the other testers.

[Darin Adler]

Comment on attachment 450324 [details]
Fixed unused param

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

Looking good; will look to review again after we have all tests updated and passing

> Source/WebCore/html/canvas/CanvasPath.cpp:243
> +    return roundRect(x, y, width, height, Vector { radii });

There’s a more efficient way to do this. The roundRect function can take a Span<RadiusVariant> instead of const Vector<RadiusVariant>&. We can construct a span here without doing memory allocation, whereas the Vector will allocate a block on the heap.

> Source/WebCore/html/canvas/CanvasPath.cpp:259
> +    auto normalizedRadii = Vector<DOMPointInit>();

I think we could just write:

    Vector<DOMPointInit> normalizedRadii;

Also, I suggest using inline capacity to help performance:

    Vector<DOMPointInit, 16> normalizedRadii;

Choose a number large enough that the vector will almost never be larger than that.

> Source/WebCore/html/canvas/CanvasPath.cpp:264
> +        auto ret = WTF::switchOn(radius,

Please use words instead of abbreviations in WebKit code. No need for "ret". Can use "result" or "returnValue" or "value" or whatever you like, or could even call it "exception" in this case.

> Source/WebCore/html/canvas/CanvasPath.cpp:303
> +    // Degenerated case, fallback to regular rect.

"Degenerated" -> "Degenerate", also "fall back" rather than "fallback" since we want the verb form.

> Source/WebCore/html/canvas/CanvasPath.cpp:311
> +    DOMPointInit upperLeft, upperRight, lowerRight, lowerLeft;

Why do we use DOMPointInit here instead of FloatPoint? Do we need a DoublePoint here? I don’t understand why we narrow everything to float when calling in from DOM to canvas, if we then need double for intermediates.

> Source/WebCore/html/canvas/CanvasPath.cpp:343
> +        RELEASE_ASSERT_NOT_REACHED();

What guarantees we can’t hit this assertion? Seems like radii could be a vector of a large size, so I don’t see how we guarantee normalizedRadii is not. I’m sure it’s there, I just couldn’t spot it.

> Source/WebCore/html/canvas/CanvasPath.cpp:395
> +    m_path.moveTo(FloatPoint(x + upperLeft.x, y));

Can use { } syntax instead of FloatPoint() if you like, here and below.

> Source/WebCore/html/canvas/CanvasPath.h:31
> +#include "DOMPointInit.h"

I think we can just forward-declare DOMPointInit in this header rather that including DOMPointInit.h.

> Source/WebCore/html/canvas/CanvasPath.h:35
> +#include <wtf/Vector.h>

<wtf/Forward.h> is sufficient to define a function with an argument of the Vector, don’t need to pull in the whole header in the .h file.

> Source/WebCore/html/canvas/CanvasPath.h:55
> +    ExceptionOr<void> roundRect(float x, float y, float width, float height, const Vector<RadiusVariant>& radii);
> +    ExceptionOr<void> roundRect(float x, float y, float width, float height, const RadiusVariant& radii);

I don’t understand why we convert double to float here.

> Source/WebCore/inspector/InspectorCanvasCallTracer.h:33
> +#include <variant>

I don’t see any uses of std::variant directly below; seems unlikely this include is needed.

[Darin Adler]

Comment on attachment 450324 [details]
Fixed unused param

Will need another try with more expected test result updates.

[Lauro Moura]

Created attachment 451856 [details]
Rebased and updated patch

[Lauro Moura]

Created attachment 451861 [details]
Fix forward declaration

[Lauro Moura]

(In reply to Darin Adler from comment #20)
> Comment on attachment 450324 [details]
> Fixed unused param
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450324&action=review
> 
> Looking good; will look to review again after we have all tests updated and
> passing
> 

Thanks!

> > Source/WebCore/html/canvas/CanvasPath.cpp:243
> > +    return roundRect(x, y, width, height, Vector { radii });
> 
> There’s a more efficient way to do this. The roundRect function can take a
> Span<RadiusVariant> instead of const Vector<RadiusVariant>&. We can
> construct a span here without doing memory allocation, whereas the Vector
> will allocate a block on the heap.

Nice catch. In the first update I had added an overload specifically for Span besides the Vector one but now I've removed the latter, leaving just the Span and the single-valued ones.

> 
> > Source/WebCore/html/canvas/CanvasPath.cpp:343
> > +        RELEASE_ASSERT_NOT_REACHED();
> 
> What guarantees we can’t hit this assertion? Seems like radii could be a
> vector of a large size, so I don’t see how we guarantee normalizedRadii is
> not. I’m sure it’s there, I just couldn’t spot it.

The check in bellow the point "// 2." (line 255) bails out if radii is >4 or empty.
 
> > Source/WebCore/html/canvas/CanvasPath.h:55
> > +    ExceptionOr<void> roundRect(float x, float y, float width, float height, const Vector<RadiusVariant>& radii);
> > +    ExceptionOr<void> roundRect(float x, float y, float width, float height, const RadiusVariant& radii);
> 
> I don’t understand why we convert double to float here.

Do you mean from the double in the idl file to float in the corresponding C++ code? Good question, this seems to be the norm at least in this file for a long, long time and I couldn't find the reasoning yet.

[Lauro Moura]

For the record, this issue was opened recently for the spec update with the single-valued overload: https://github.com/whatwg/html/issues/7592

[Darin Adler]

Comment on attachment 451861 [details]
Fix forward declaration

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

> Source/WebCore/html/canvas/CanvasPath.h:37
> +#include <wtf/Forward.h>
> +
>  
>  namespace WebCore {

Please don’t add that extra blank line.

> Source/WebCore/html/canvas/CanvasPath.h:72
> +private:

Please don’t add this.

> Source/WebCore/html/canvas/CanvasPath.idl:40
> +    undefined roundRect(unrestricted double x, unrestricted double y, unrestricted double w, unrestricted double h, sequence<(unrestricted double or DOMPointInit)> radii);
> +    undefined roundRect(unrestricted double x, unrestricted double y, unrestricted double w, unrestricted double h, (unrestricted double or DOMPointInit) radii);

Still so puzzled about why we say these functions all take double in the IDL file and then convert them all to float in the bindings by passing the double to an internal function that takes a float. Not new to this patch, but not good for the future.

[Lauro Moura]

Created attachment 454312 [details]
Patch for landing

[EWS]

Committed r291106 (248268@main): <https://commits.webkit.org/248268@main>

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

[Said Abou-Hallawa]

This should have added support for roundRect to all 2D canvas contexts.

[Said Abou-Hallawa]

*** Bug 236839 has been marked as a duplicate of this bug. ***