Bug 190762

Created attachment 352825 [details] Patch

Created attachment 352828 [details] Patch

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?

Created attachment 352849 [details] Patch

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 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.

(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?

I do.

Created attachment 352911 [details] Patch

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 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.

(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 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.

Created attachment 352921 [details] Patch

Created attachment 352929 [details] Patch

Comment on attachment 352929 [details] Patch Clearing flags on attachment: 352929 Committed r237344: <https://trac.webkit.org/changeset/237344>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45472741>

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.