Bug 179701 – Add a base class for HTMLCanvasElement and OffscreenCanvas

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

[Dean Jackson]

Add a base class for HTMLCanvasElement and OffscreenCanvas

[Radar WebKit Bug Importer]

<rdar://problem/35545195>

[Dean Jackson]

Created attachment 326938 [details]
Patch

[Sam Weinig]

Comment on attachment 326938 [details]
Patch

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

> Source/WebCore/html/CanvasBase.h:29
> +#include "IntSize.h"
> +#include "ScriptExecutionContext.h"

These can probably be forward declared.

> Source/WebCore/html/CanvasBase.h:40
> +public:
> +
> +    virtual ~CanvasBase() = default;

Extra newline.

[Dean Jackson]

Committed r224929: <https://trac.webkit.org/changeset/224929>

[Darin Adler]

Comment on attachment 326938 [details]
Patch

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

> Source/WebCore/html/CanvasBase.h:45
> +    HTMLCanvasElement* asHTMLCanvasElement();

I would suggest removing this function, and just having callers use is<HTMLCanvasElement> and downcast<HTMLCanvasElement> as needed. I don’t know many callers who want to get null when it’s the wrong type.

I just made this change to EventTarget, removing the toNode function and converting callers to use is<Node> and downcast<Node> instead.

> Source/WebCore/html/HTMLCanvasElement.h:86
> +    unsigned width() const override { return size().width(); }
> +    unsigned height() const override { return size().height(); }

final

> Source/WebCore/html/HTMLCanvasElement.h:91
> +    const IntSize& size() const override { return m_size; }

final

> Source/WebCore/html/HTMLCanvasElement.h:93
> +    void setSize(const IntSize& newSize) override

final

> Source/WebCore/html/HTMLCanvasElement.h:104
> +    bool isHTMLCanvasElement() const override { return true; }

final and private

> Source/WebCore/html/HTMLCanvasElement.h:156
> +    SecurityOrigin* securityOrigin() const override;

final

> Source/WebCore/html/OffscreenCanvas.h:50
> +class OffscreenCanvas : public RefCounted<OffscreenCanvas>, public CanvasBase, public EventTargetWithInlineData {

Can this be final?

> Source/WebCore/html/OffscreenCanvas.h:67
> +    unsigned width() const override;

final

> Source/WebCore/html/OffscreenCanvas.h:69
> +    unsigned height() const override;

final

> Source/WebCore/html/OffscreenCanvas.h:72
> +    const IntSize& size() const override;

final

> Source/WebCore/html/OffscreenCanvas.h:73
> +    void setSize(const IntSize&) override;

final

> Source/WebCore/html/OffscreenCanvas.h:75
> +    bool isOffscreenCanvas() const override { return true; }

final, also should be private

Generally I suggest considering making some of the virtual function overrides private if they only need to be called on the base class.

> Source/WebCore/html/OffscreenCanvas.h:91
> +    ScriptExecutionContext* scriptExecutionContext() const override { return CanvasBase::scriptExecutionContext(); }

final

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:72
> +void CanvasRenderingContext::ref()
> +{
> +    if (m_canvas.isHTMLCanvasElement()) {
> +        auto* htmlCanvas = static_cast<HTMLCanvasElement*>(&m_canvas);
> +        htmlCanvas->ref();
> +        return;
> +    }
> +    if (is<OffscreenCanvas>(m_canvas)) {
> +        auto* offscreenCanvas = static_cast<OffscreenCanvas*>(&m_canvas);
> +        offscreenCanvas->ref();
> +        return;
> +    }
> +}
> +
> +void CanvasRenderingContext::deref()
> +{
> +    if (m_canvas.isHTMLCanvasElement()) {
> +        auto* htmlCanvas = static_cast<HTMLCanvasElement*>(&m_canvas);
> +        htmlCanvas->deref();
> +        return;
> +    }
> +    if (is<OffscreenCanvas>(m_canvas)) {
> +        auto* offscreenCanvas = static_cast<OffscreenCanvas*>(&m_canvas);
> +        offscreenCanvas->deref();
> +        return;
> +    }
> +}

A better pattern is to have a virtual ref/deref function in CanvasBase. EventTarget has an example of this: see refEventTarget/derefEventTarget.

Then these functions would just be m_canvas.ref() and m_canvas.deref().

> Source/WebCore/html/canvas/CanvasRenderingContext.h:52
> +    CanvasBase& canvasBase() const { return m_canvas; }

I don’t understand why this function is named canvasBase() instead of canvas().

> Source/WebCore/html/canvas/CanvasRenderingContext.h:73
> +    CanvasRenderingContext(CanvasBase&);

Should be explicit too.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:79
> +    HTMLCanvasElement& canvas() const { return static_cast<HTMLCanvasElement&>(canvasBase()); }

Should use downcast, not static_cast.

> Source/WebCore/html/canvas/ImageBitmapRenderingContext.h:51
> +    HTMLCanvasElement* canvas() const;

I think this should return a reference and code should be structured so this is called only when it’s know to be the correct type.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:120
> +    HTMLCanvasElement* canvas();

Same comment as elsewhere. I don’t think all these conversions from reference to pointer are a good idea. If we need checks for the type then we might need multiple functions, but I don’t think we should use pointers and null for this.

> Source/WebCore/html/canvas/WebGPURenderingContext.h:55
> +    HTMLCanvasElement* canvas() const;

Ditto.

> Source/WebCore/inspector/InspectorInstrumentation.h:1240
> +    auto* canvasElement = canvasRenderingContext.canvasBase().asHTMLCanvasElement();
> +    if (canvasElement) {

Better to write this with is/downcast.

> Source/WebCore/inspector/InspectorInstrumentation.h:1258
> +    if (auto* canvasElement = context.canvas()) {

Ditto, etc.

[Dean Jackson]

Comment on attachment 326938 [details]
Patch

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

>> Source/WebCore/html/CanvasBase.h:45
>> +    HTMLCanvasElement* asHTMLCanvasElement();
> 
> I would suggest removing this function, and just having callers use is<HTMLCanvasElement> and downcast<HTMLCanvasElement> as needed. I don’t know many callers who want to get null when it’s the wrong type.
> 
> I just made this change to EventTarget, removing the toNode function and converting callers to use is<Node> and downcast<Node> instead.

Done. With Darin's help over email, I had to add custom type traits to HTMLCanvasElement, so that is<> and downcast<> could be used - they needed to accept a CanvasBase reference.

>> Source/WebCore/html/HTMLCanvasElement.h:86
>> +    unsigned height() const override { return size().height(); }
> 
> final

Done. As were all the other similar comments.

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:72
>> +}
> 
> A better pattern is to have a virtual ref/deref function in CanvasBase. EventTarget has an example of this: see refEventTarget/derefEventTarget.
> 
> Then these functions would just be m_canvas.ref() and m_canvas.deref().

Done, although they had to be refCanvasBase() and derefCanvasBase(), since HTMLCanvasElement already has a ref/deref via Node.

>> Source/WebCore/html/canvas/CanvasRenderingContext.h:52
>> +    CanvasBase& canvasBase() const { return m_canvas; }
> 
> I don’t understand why this function is named canvasBase() instead of canvas().

This is because the Web-exposed interfaces which derive from this each have canvas() accessors that don't all return the same type. e.g. a CanvasRenderingContext2D always returns an HTMLCanvasElement, but a OffscreenCanvasRenderingContext returns a Variant<HTMLCanvasElement, OffscreenCanvas>.

So I could change all the IDL implementedAs, or just this one function.

>> Source/WebCore/html/canvas/CanvasRenderingContext.h:73
>> +    CanvasRenderingContext(CanvasBase&);
> 
> Should be explicit too.

Done.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:79
>> +    HTMLCanvasElement& canvas() const { return static_cast<HTMLCanvasElement&>(canvasBase()); }
> 
> Should use downcast, not static_cast.

Done.

>> Source/WebCore/html/canvas/ImageBitmapRenderingContext.h:51
>> +    HTMLCanvasElement* canvas() const;
> 
> I think this should return a reference and code should be structured so this is called only when it’s know to be the correct type.

I've left this as is for the moment, but I'll fix it in a subsequent patch (when other context types can return different canvas instances from canvas()).

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:120
>> +    HTMLCanvasElement* canvas();
> 
> Same comment as elsewhere. I don’t think all these conversions from reference to pointer are a good idea. If we need checks for the type then we might need multiple functions, but I don’t think we should use pointers and null for this.

OK.

>> Source/WebCore/inspector/InspectorInstrumentation.h:1240
>> +    if (canvasElement) {
> 
> Better to write this with is/downcast.

Done.

[Darin Adler]

Comment on attachment 326938 [details]
Patch

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

>>> Source/WebCore/html/canvas/CanvasRenderingContext.h:52
>>> +    CanvasBase& canvasBase() const { return m_canvas; }
>> 
>> I don’t understand why this function is named canvasBase() instead of canvas().
> 
> This is because the Web-exposed interfaces which derive from this each have canvas() accessors that don't all return the same type. e.g. a CanvasRenderingContext2D always returns an HTMLCanvasElement, but a OffscreenCanvasRenderingContext returns a Variant<HTMLCanvasElement, OffscreenCanvas>.
> 
> So I could change all the IDL implementedAs, or just this one function.

The one in CanvasRenderingContext2D is no problem, since we can override with a canvas function that returns a HTMLCanvasElement& and everything would come out clean.

But I can see how overriding to return a Variant would be messy. One way to handle that is to use [ImplementedAs=canvasForBinding] for the thing that returns the Variant. Or maybe "canvasForBindings", pretending that we care about more languages than just JavaScript.

It’s not critical, but I think it might be slightly nicer.

[Darin Adler]

Comment on attachment 326938 [details]
Patch

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

>>>> Source/WebCore/html/canvas/CanvasRenderingContext.h:52
>>>> +    CanvasBase& canvasBase() const { return m_canvas; }
>>> 
>>> I don’t understand why this function is named canvasBase() instead of canvas().
>> 
>> This is because the Web-exposed interfaces which derive from this each have canvas() accessors that don't all return the same type. e.g. a CanvasRenderingContext2D always returns an HTMLCanvasElement, but a OffscreenCanvasRenderingContext returns a Variant<HTMLCanvasElement, OffscreenCanvas>.
>> 
>> So I could change all the IDL implementedAs, or just this one function.
> 
> The one in CanvasRenderingContext2D is no problem, since we can override with a canvas function that returns a HTMLCanvasElement& and everything would come out clean.
> 
> But I can see how overriding to return a Variant would be messy. One way to handle that is to use [ImplementedAs=canvasForBinding] for the thing that returns the Variant. Or maybe "canvasForBindings", pretending that we care about more languages than just JavaScript.
> 
> It’s not critical, but I think it might be slightly nicer.

Note that when I say "override" I am not suggesting making the function virtual or using the "override" keyword. Just overriding the inherited function with a different one with the same name, which is not that uncommon a C++ idiom, particularly overriding to make the class of a return value more specific.

[Dean Jackson]

First post-review commit is https://trac.webkit.org/changeset/225119/webkit

[Dean Jackson]

Comment on attachment 326938 [details]
Patch

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

>>>>> Source/WebCore/html/canvas/CanvasRenderingContext.h:52
>>>>> +    CanvasBase& canvasBase() const { return m_canvas; }
>>>> 
>>>> I don’t understand why this function is named canvasBase() instead of canvas().
>>> 
>>> This is because the Web-exposed interfaces which derive from this each have canvas() accessors that don't all return the same type. e.g. a CanvasRenderingContext2D always returns an HTMLCanvasElement, but a OffscreenCanvasRenderingContext returns a Variant<HTMLCanvasElement, OffscreenCanvas>.
>>> 
>>> So I could change all the IDL implementedAs, or just this one function.
>> 
>> The one in CanvasRenderingContext2D is no problem, since we can override with a canvas function that returns a HTMLCanvasElement& and everything would come out clean.
>> 
>> But I can see how overriding to return a Variant would be messy. One way to handle that is to use [ImplementedAs=canvasForBinding] for the thing that returns the Variant. Or maybe "canvasForBindings", pretending that we care about more languages than just JavaScript.
>> 
>> It’s not critical, but I think it might be slightly nicer.
> 
> Note that when I say "override" I am not suggesting making the function virtual or using the "override" keyword. Just overriding the inherited function with a different one with the same name, which is not that uncommon a C++ idiom, particularly overriding to make the class of a return value more specific.

Sure. I had started down that path but decided to hold off in the original patch. I'll do it now as a follow-up (or as I actually change the return type in WebGLRenderingContext, which is the first one to require a variant).