Bug 190762 - CSS Paint API should give a 2d rendering context
Summary: CSS Paint API should give a 2d rendering context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks: 190217
  Show dependency treegraph
 
Reported: 2018-10-19 14:29 PDT by Justin Michaud
Modified: 2018-10-25 13:15 PDT (History)
10 users (show)

See Also:


Attachments
Patch (54.81 KB, patch)
2018-10-19 14:53 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (55.40 KB, patch)
2018-10-19 15:20 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (55.96 KB, patch)
2018-10-19 19:56 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (53.21 KB, patch)
2018-10-22 14:50 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (53.75 KB, patch)
2018-10-22 16:00 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (53.92 KB, patch)
2018-10-22 17:07 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2018-10-19 14:29:42 PDT
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.
Comment 1 Justin Michaud 2018-10-19 14:53:40 PDT
Created attachment 352825 [details]
Patch
Comment 2 Justin Michaud 2018-10-19 15:20:40 PDT
Created attachment 352828 [details]
Patch
Comment 3 Dean Jackson 2018-10-19 16:20:38 PDT
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?
Comment 4 Justin Michaud 2018-10-19 19:56:31 PDT
Created attachment 352849 [details]
Patch
Comment 5 Justin Michaud 2018-10-19 20:09:02 PDT
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.
Comment 6 Simon Fraser (smfr) 2018-10-19 21:59:38 PDT
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.
Comment 7 Tim Horton 2018-10-19 22:06:06 PDT
(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?
Comment 8 Simon Fraser (smfr) 2018-10-19 22:11:39 PDT
I do.
Comment 9 Justin Michaud 2018-10-22 14:50:35 PDT
Created attachment 352911 [details]
Patch
Comment 10 Justin Michaud 2018-10-22 14:52:58 PDT
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.
Comment 11 Don Olmstead 2018-10-22 15:01:20 PDT
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.
Comment 12 Justin Michaud 2018-10-22 15:04:17 PDT
(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.
Comment 13 Dean Jackson 2018-10-22 15:39:29 PDT
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.
Comment 14 Justin Michaud 2018-10-22 16:00:31 PDT
Created attachment 352921 [details]
Patch
Comment 15 Justin Michaud 2018-10-22 17:07:06 PDT
Created attachment 352929 [details]
Patch
Comment 16 WebKit Commit Bot 2018-10-22 17:46:23 PDT
Comment on attachment 352929 [details]
Patch

Clearing flags on attachment: 352929

Committed r237344: <https://trac.webkit.org/changeset/237344>
Comment 17 WebKit Commit Bot 2018-10-22 17:46:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2018-10-22 17:47:46 PDT
<rdar://problem/45472741>
Comment 19 Darin Adler 2018-10-25 13:15:05 PDT
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.