Bug 190762 – CSS Paint API should give a 2d rendering context

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Justin Michaud]

As defined in the spec, the first parameter to the paint callback should be a PaintRenderingContext2D, which is a subset of a normal 2d rendering context.

[Justin Michaud]

Created attachment 352825 [details]
Patch

[Justin Michaud]

Created attachment 352828 [details]
Patch

[Dean Jackson]

Comment on attachment 352828 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Add a new type of canvas and 2d rendering context to support the CSS Painting API.
> +        Make many of the methods from HTMLCanvasElement virtual functions on CanvasBase, and

Thanks for doing this. It's been on my TODO for 12+ months.

> Source/WebCore/bindings/js/JSPaintRenderingContext2DCustom.cpp:18
> +/*
> + * Copyright (C) 2018 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */

Why this license?

> Source/WebCore/html/CanvasBase.cpp:44
> +    ASSERT(!m_context); // Should have been set to null by base class

Nit: .

> Source/WebCore/html/CustomPaintCanvas.cpp:89
> +    m_context = PaintRenderingContext2D::create(*this);
> +    if (!m_context)
> +        return { nullptr };

What would cause this to happen?

> Source/WebCore/html/CustomPaintCanvas.cpp:137
> +    if (!m_imageBuffer)
> +        return;

This would only happen if ImageBuffer::create fails. I wonder if you should reset m_hasCreatedImageBuffer too?

> Source/WebCore/html/CustomPaintCanvas.cpp:139
> +    m_imageBuffer->context().setShadowsIgnoreTransforms(true);
> +    m_imageBuffer->context().setStrokeThickness(1);

Could you add a comment here explaining why you need these?

> Source/WebCore/html/CustomPaintCanvas.cpp:146
> +    m_imageBuffer = WTFMove(buffer);
> +    if (m_imageBuffer && m_size != m_imageBuffer->internalSize())
> +        m_size = m_imageBuffer->internalSize();

Should you set m_size to (0,0) if the buffer was null?

> Source/WebCore/html/canvas/PaintRenderingContext2D.idl:31
> +CustomIsReachable,
> +EnabledAtRuntime=CSSPaintingAPI,
> +Conditional=CSS_PAINTING_API,
> +JSGenerateToJSObject,
> +JSCustomMarkFunction,

We usually indent these.

> Source/WebCore/html/canvas/WebMetalRenderPassAttachmentDescriptor.h:31
> -#include <wtf/Ref.h>
>  #include <wtf/RefCounted.h>
> +#include <wtf/RefPtr.h>

Drive-by!

> LayoutTests/fast/css-custom-paint/basic.html:79
> +      for (var i = 0; i < 6; i++){
> +        for (var j = 0; j < 6; j++){
> +          ctx.fillStyle = 'rgb(' + Math.floor(255 - 42.5 * i) + ',' +
> +                           Math.floor(255 - 42.5 * j) + ',0)';
> +          ctx.fillRect(j * 25, i * 25, 25, 25);
> +        }
> +      }

At this point can't you have a ref-test against a normal canvas object?

[Justin Michaud]

Created attachment 352849 [details]
Patch

[Justin Michaud]

Comment on attachment 352828 [details]
Patch

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

>> Source/WebCore/html/CustomPaintCanvas.cpp:139
>> +    m_imageBuffer->context().setStrokeThickness(1);
> 
> Could you add a comment here explaining why you need these?

No. I have no clue, I copied it from HTMLCanvasElement. The blame (git log --pretty=short -u -L 940,941:Source/WebCore/html/HTMLCanvasElement.cpp) isn't very helpful either, this line was there since well before 2010. Do you know who might know about this?

>> Source/WebCore/html/canvas/WebMetalRenderPassAttachmentDescriptor.h:31
>> +#include <wtf/RefPtr.h>
> 
> Drive-by!

We really need a bot to build without unified sources. This is yet another build error caused by them.

>> LayoutTests/fast/css-custom-paint/basic.html:79
>> +      }
> 
> At this point can't you have a ref-test against a normal canvas object?

Yep. In the patch that starts off the Worklets implementation (which should be next), I am hoping to re-write all of these tests so they can work without the dom. Then, I am going to start adding some ref tests.

[Simon Fraser (smfr)]

Comment on attachment 352849 [details]
Patch

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

> Source/WebCore/html/CustomPaintCanvas.h:75
> +    bool hasCreatedImageBuffer() const final { return m_hasCreatedImageBuffer; }
> +    ImageBuffer* buffer() const final;
> +    Image* copiedImage() const final;

I don't think this new canvas type should necessarily be backed by an image buffer. It should be display-list based.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 352849 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352849&action=review
> 
> > Source/WebCore/html/CustomPaintCanvas.h:75
> > +    bool hasCreatedImageBuffer() const final { return m_hasCreatedImageBuffer; }
> > +    ImageBuffer* buffer() const final;
> > +    Image* copiedImage() const final;
> 
> I don't think this new canvas type should necessarily be backed by an image
> buffer. It should be display-list based.

From talking to Justin today I think this is a stop-gap to get things working, but display lists are the endgame. Are you thinking he should just go straight to display lists?

[Simon Fraser (smfr)]

I do.

[Justin Michaud]

Created attachment 352911 [details]
Patch

[Justin Michaud]

This patch uses display lists to do the drawing. I think drawImage is the only supported function that won't work right now with display lists, and I would like to fix it in a different patch. This also still calls the paint callback during paint, but I think fixing that should probably go in another patch too.

[Don Olmstead]

Comment on attachment 352849 [details]
Patch

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

> Source/WebCore/html/canvas/PaintRenderingContext2D.idl:33
> +[
> +    CustomIsReachable,
> +    EnabledAtRuntime=CSSPaintingAPI,
> +    Conditional=CSS_PAINTING_API,
> +    JSGenerateToJSObject,
> +    JSCustomMarkFunction,
> +] interface PaintRenderingContext2D {
> +};

Does the IDL need to be taught about workers? When I was looking at the CSS Typed OM stuff I saw Exposed=(Window,LayoutWorklet,PaintWorklet) in Chrome's IDL.

[Justin Michaud]

(In reply to Don Olmstead from comment #11)
> Comment on attachment 352849 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352849&action=review
> 
> > Source/WebCore/html/canvas/PaintRenderingContext2D.idl:33
> > +[
> > +    CustomIsReachable,
> > +    EnabledAtRuntime=CSSPaintingAPI,
> > +    Conditional=CSS_PAINTING_API,
> > +    JSGenerateToJSObject,
> > +    JSCustomMarkFunction,
> > +] interface PaintRenderingContext2D {
> > +};
> 
> Does the IDL need to be taught about workers? When I was looking at the CSS
> Typed OM stuff I saw Exposed=(Window,LayoutWorklet,PaintWorklet) in Chrome's
> IDL.

I think it will eventually. I am going to see how much of the Worklets spec needs to be implemented for custom paint to work next.

[Dean Jackson]

Comment on attachment 352911 [details]
Patch

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

> Source/WebCore/html/CustomPaintCanvas.cpp:89
> +    downcast<PaintRenderingContext2D>(*m_context).setUsesDisplayListDrawing(true);
> +    downcast<PaintRenderingContext2D>(*m_context).setTracksDisplayListReplay(false);

smfr++ for recommending this. I didn't know it was stable enough.

[Justin Michaud]

Created attachment 352921 [details]
Patch

[Justin Michaud]

Created attachment 352929 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 352929 [details]
Patch

Clearing flags on attachment: 352929

Committed r237344: <https://trac.webkit.org/changeset/237344>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45472741>

[Darin Adler]

Comment on attachment 352929 [details]
Patch

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

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:66
> +    UNUSED_PARAM(destContext);

Why is this here? The code below seems to use destContext.