Bug 225286 – Add preliminary support for specifying a color space for 2D canvas

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

[Sam Weinig]

As a first step toward implementing https://github.com/WICG/canvas-color-space/blob/main/CanvasColorSpaceProposal.md, let's start with adding support for specifying a color space via the settings object and rendering using that color space. To keep things small, we will leave out ImageData access from this first change.

[Sam Weinig]

Created attachment 427564 [details]
Patch

[Sam Weinig]

Created attachment 427614 [details]
Patch

[Sam Weinig]

Created attachment 427615 [details]
Patch

[Sam Weinig]

Created attachment 427617 [details]
Patch

[Sam Weinig]

Created attachment 427628 [details]
Patch

[Sam Weinig]

Created attachment 427674 [details]
Patch

[Dean Jackson]

Comment on attachment 427674 [details]
Patch

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

> LayoutTests/fast/canvas/CanvasRenderingContext2DSettings-colorSpace-disabled.html:11
> +        let canvas = document.createElement("canvas");
> +        let context = canvas.getContext("2d", settings);

Nit: const
(and elsewhere)

> LayoutTests/fast/canvas/canvas-color-space-display-p3.html:5
> +    <p>This test passes if the square below looks to be all one color</p>

Nice test!

[Simon Fraser (smfr)]

Comment on attachment 427674 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:951
> +        else
> +            return std::pair { DestinationColorSpace::SRGB, PixelFormat::BGRA8 };

No else after return.

> Source/WebCore/html/canvas/PredefinedColorSpace.h:34
> +enum class PredefinedColorSpace {

Maybe this should be CanvasPredefinedColorSpace?

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 427674 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427674&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:951
> > +        else
> > +            return std::pair { DestinationColorSpace::SRGB, PixelFormat::BGRA8 };
> 
> No else after return.

Yeah. (I kind of hate this rule. The symmetry seems so much nicer in cases like this, but if I want to change this rule I will bring it up separately). Fixed.

> 
> > Source/WebCore/html/canvas/PredefinedColorSpace.h:34
> > +enum class PredefinedColorSpace {
> 
> Maybe this should be CanvasPredefinedColorSpace?

I'd like to stick to the spec name if I can. Perhaps we should suggest changing it HTML?

[Sam Weinig]

Created attachment 427770 [details]
Patch

[EWS]

Committed r277024 (237338@main): <https://commits.webkit.org/237338@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/77561169>