Bug 202394 – ImageBitmap should be serializable

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Lord]

Currently an ImageBitmap can be sent in sendMessage if it's transferred, but it can't be copied. As it is serializable in the spec[1], this should work.

[1] https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#imagebitmap

[Chris Lord]

Created attachment 379887 [details]
Patch

[Chris Lord]

Created attachment 380208 [details]
Patch

[Zan Dobersek]

Comment on attachment 380208 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:915
> +        auto buffer = imageBitmap.buffer();

It's usually preferable to use `auto*` to explicitly denote that a pointer of some sorts is being returned.

Given the pointer, what should happen if it is null? It can be, at least theoretically. Should a validation error be reported in this case. or should serialization still succeed?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:927
> +        write(static_cast<int32_t>(imageBitmap.originClean()));

origin-clean being a boolean, I think using uint8_t type here and in the deserialization makes more sense, if possible.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:932
> +        write(arrayBuffer->byteLength());

Nit: perhaps this should be explicitly casted to uint32_t to make the intended type clear, even when `unsigned` as returned by byteLength() is an equivalent.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2769
> +        read(originClean);
> +        read(logicalWidth);
> +        read(logicalHeight);
> +        read(resolutionScale);

These returns should also probable be checked, perhaps in the same if-statement to keep things compact.

[Chris Lord]

Created attachment 380211 [details]
Patch

[Zan Dobersek]

Comment on attachment 380211 [details]
Patch

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

Patch looks good, just some additional checks required to cover the odd cases.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:923
> +        auto imageData = buffer->getPremultipliedImageData(IntRect(0, 0, logicalSize.width(), logicalSize.height()));

Looked it up only now -- getPremultipliedImageData() can possibly return null, so we have to null-check around that as well.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2776
> +        auto imageData = Uint8ClampedArray::tryCreate(WTFMove(arrayBuffer), 0, arrayBuffer->byteLength());

tryCreate() possibly returns null, so we have to null-check it.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2777
> +        auto buffer = ImageBuffer::create(FloatSize(logicalWidth, logicalHeight), Unaccelerated, resolutionScale);

Similar here -- ImageBuffer::create() can return null if internally the initialization doesn't succeed (e.g. memory allocation or some other step fails).

[Chris Lord]

Created attachment 380218 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 380218 [details]
Patch

Clearing flags on attachment: 380218

Committed r250721: <https://trac.webkit.org/changeset/250721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55982648>