Bug 202574 – Implement [Transferable] property of OffscreenCanvas

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

[Chris Lord]

Spec: https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-interface

[Chris Lord]

Created attachment 380630 [details]
Patch

[Chris Lord]

Created attachment 385745 [details]
Patch

[Sam Weinig]

Comment on attachment 385745 [details]
Patch

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

> Source/WebCore/html/OffscreenCanvas.cpp:284
> +    return std::unique_ptr<DetachedOffscreenCanvas>(new DetachedOffscreenCanvas(takeImageBuffer(), size(), originClean()));

Please use makeUnique<>

[Ryosuke Niwa]

Comment on attachment 385745 [details]
Patch

r- due to build failures.

[Chris Lord]

Created attachment 385954 [details]
Patch

[Chris Lord]

argh, I think I've missed some OffscreenCanvas guards, will fix...

[Chris Lord]

Created attachment 385965 [details]
Patch

[Chris Lord]

I don't think the style test will pass without reformatting a not-trivial amount of the affected file - note, my additions are in keeping with what's in the file already.

[Chris Lord]

Created attachment 388978 [details]
Patch

[Antti Koivisto]

Comment on attachment 388978 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3153
> +#if ENABLE(OFFSCREEN_CANVAS)
> +                { },
> +#endif
>  #if ENABLE(WEBASSEMBLY)
>                  nullptr,
>  #endif

Maybe CloneDeserializer could have default argument values to avoid these?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3574
>      return adoptRef(*new SerializedScriptValue(WTFMove(buffer), blobURLs, nullptr, nullptr, { }
> +#if ENABLE(OFFSCREEN_CANVAS)
> +        , { }
> +#endif
>  #if ENABLE(WEBASSEMBLY)
>          , nullptr
>  #endif

Maybe SerializedScriptValue could have default argument values to avoid these?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3589
> +static bool offscreenCanvasesCanDetach(const Vector<RefPtr<OffscreenCanvas>>& offscreenCanvases)

The usual style is to start boolean functions with verb like is/are/has etc., maybe areAllOffscreenCanvasesDetachable or something like that.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3596
> +        if (!visited.add(offscreenCanvas.get()))
> +            return false;

What is this test about? Maybe add a comment?

[Antti Koivisto]

Created attachment 388986 [details]
again

[Antti Koivisto]

Comment on attachment 388986 [details]
again

oops wrong bug

[Chris Lord]

Created attachment 389010 [details]
Patch

[Chris Lord]

Created attachment 389015 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389015 [details]
Patch

Clearing flags on attachment: 389015

Committed r255315: <https://trac.webkit.org/changeset/255315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58970553>

[Darin Adler]

Comment on attachment 389015 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:567
>      static SerializationReturnCode serialize(JSGlobalObject* lexicalGlobalObject, JSValue value, Vector<RefPtr<MessagePort>>& messagePorts, Vector<RefPtr<JSC::ArrayBuffer>>& arrayBuffers, const Vector<RefPtr<ImageBitmap>>& imageBitmaps,
> +#if ENABLE(OFFSCREEN_CANVAS)
> +            const Vector<RefPtr<OffscreenCanvas>>& offscreenCanvases,
> +#endif
>  #if ENABLE(WEBASSEMBLY)
>              WasmModuleArray& wasmModules,
>  #endif
>          Vector<String>& blobURLs, Vector<uint8_t>& out, SerializationContext context, ArrayBufferContentsArray& sharedBuffers)

Seems to me that long arguments lists like these with compiler-conditioonal arguments in them are not a great pattern. We should look for something more elegant and easy to read. Maybe a structure would help in some way?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:969
> +    void dumpOffscreenCanvas(JSObject* obj, SerializationReturnCode& code)

WebKit coding style says use a word here, object, not an abbreviation, obj.

Also, we like to use references for things that can’t be null. Although I guess JavaScript programmers are holdouts on that one?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3595
> +    Vector<RefPtr<OffscreenCanvas>> offscreenCanvases;

I’d think Vector<Ref> would be the right type for these vectors. Seems unlikely we need to store nulls in them.

> Source/WebCore/bindings/js/SerializedScriptValue.h:52
> +#if ENABLE(OFFSCREEN_CANVAS)
> +class DetachedOffscreenCanvas;
> +#endif

Two coding style thoughts:

1) I’m not sure it’s worthwhile to put forward declarations inside #if blocks. I think it’s probably a reasonable practice to define them even if they aren’t used.

2) If we are going to put them in #if blocks, then the #if block should be a separate paragraph and not interspersed with other forward declarations. This is the same rule we follow for includes.

> Source/WebCore/html/OffscreenCanvas.cpp:90
> +    if (m_detached)
> +        return 0;

Can we just set m_size to 0 as part of the detaching process? Then we can let width remain a non-virtual function.

> Source/WebCore/html/OffscreenCanvas.cpp:97
> +    if (m_detached)
> +        return 0;

Ditto.