[webkit-reviews] review granted: [Bug 190762] CSS Paint API should give a 2d rendering context : [Attachment 352828] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 190762: CSS Paint API should give a 2d rendering context
https://bugs.webkit.org/show_bug.cgi?id=190762

Attachment 352828: Patch

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




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 352828
  --> https://bugs.webkit.org/attachment.cgi?id=352828
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?