Bug 216565 – Move SerializationState from ImageBuffer to ImageBitmap

• Component: Canvas
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This structure belongs to ImageBitmap. It describes how the ImageBitmap can deal with its ImageBuffer.

[Said Abou-Hallawa]

Created attachment 408828 [details]
Patch

[Said Abou-Hallawa]

Created attachment 408833 [details]
Patch

[Said Abou-Hallawa]

Created attachment 408961 [details]
Patch

[Said Abou-Hallawa]

Created attachment 408969 [details]
Patch

[Said Abou-Hallawa]

Created attachment 408975 [details]
Patch

[Kenneth Russell]

Comment on attachment 408975 [details]
Patch

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

There are several nice aspects to this patch - the use of the OptionSet is a good cleanup. A few thoughts - happy to r+ this if you prefer things in the form presented here.

> Source/WebCore/html/ImageBitmap.cpp:839
>      m_bitmapData = nullptr;

It seems undesirable to me to reach up into the superclass's implementation for this operation. It will probably make it harder to add any GPU acceleration for ImageBitmap later, when the ImageBitmap's backing store is a texture rather than CPU-side bytes.

> Source/WebCore/html/SerializableImageBuffer.h:34
> +class SerializableImageBuffer {

This name is confusing to me because this class isn't an ImageBuffer. Perhaps call it something like "ImageBufferSerializationHelper"? (Also not a great name.)

Pulling so much of the ImageBitmap's functionality into a superclass, and using it for two purposes - constructing ImageBitmaps themselves, and as the data which is serialized/deserialized - makes the code harder to think about in my opinion. I recommend composition over inheritance.

> Source/WebCore/html/SerializableImageBuffer.h:61
> +    std::unique_ptr<ImageBuffer> m_bitmapData;

The fact that this data member is protected rather than private makes it more complicated to reason about correct states of this class and its subclass, ImageBitmap. I think it would be better to make it private and access it via accessors.

[Sam Weinig]

Comment on attachment 408975 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        std::pair<std::unique_ptr<ImageBuffer>, OptionSet<SerializationState>>
> +        can be replaced by a new class named SerializableImageBuffer. This class
> +        will also be a superclass of ImageBitmap. All the operations on ImageBuffer

What benefit is there in making it the superclass? Could it just be a member of ImageBitmap?

[Said Abou-Hallawa]

Created attachment 409241 [details]
Patch

[Said Abou-Hallawa]

Created attachment 409245 [details]
Patch

[Said Abou-Hallawa]

Created attachment 409258 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 408975 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        will also be a superclass of ImageBitmap. All the operations on ImageBuffer
> 
> What benefit is there in making it the superclass? Could it just be a member of ImageBitmap?

I added a new class named ImageBitmapSource and I made it a member of ImageBitmap.

>> Source/WebCore/html/ImageBitmap.cpp:839
>>      m_bitmapData = nullptr;
> 
> It seems undesirable to me to reach up into the superclass's implementation for this operation. It will probably make it harder to add any GPU acceleration for ImageBitmap later, when the ImageBitmap's backing store is a texture rather than CPU-side bytes.

I added a new class named ImageBitmapSource and I made it a member of ImageBitmap. In the future it can be made an abstract class and inherited by ImageBitmapBufferSource and ImageBitmapTextureSource.

>> Source/WebCore/html/SerializableImageBuffer.h:34
>> +class SerializableImageBuffer {
> 
> This name is confusing to me because this class isn't an ImageBuffer. Perhaps call it something like "ImageBufferSerializationHelper"? (Also not a great name.)
> 
> Pulling so much of the ImageBitmap's functionality into a superclass, and using it for two purposes - constructing ImageBitmaps themselves, and as the data which is serialized/deserialized - makes the code harder to think about in my opinion. I recommend composition over inheritance.

I named it ImageBitmapSource to make it more generic name so it can be extended for non CPU buffer back end.

>> Source/WebCore/html/SerializableImageBuffer.h:61
>> +    std::unique_ptr<ImageBuffer> m_bitmapData;
> 
> The fact that this data member is protected rather than private makes it more complicated to reason about correct states of this class and its subclass, ImageBitmap. I think it would be better to make it private and access it via accessors.

Done since this class is now a member of ImageBitmap.

[Radar WebKit Bug Importer]

<rdar://problem/69375838>

[Kenneth Russell]

Comment on attachment 409258 [details]
Patch

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

Looks good to me. A few minor comments. Please fix the style issues before landing. r+

> Source/WebCore/Headers.cmake:644
> +    html/ImageBitmapSource.h

Per below: since this is the backing store per the ChangeLog comment, I think ImageBitmapBacking would be a better name.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1046
> +        write(static_cast<uint8_t>(imageBitmap.serializationState().toRaw()));

Should this file #include ImageBitmapSource.h? It looks like it's getting it implicitly through the unified build.

> Source/WebCore/bindings/js/SerializedScriptValue.h:54
>  class ImageBitmap;

Is the ImageBitmap forward declaration needed?

> Source/WebCore/bindings/js/SerializedScriptValue.h:56
> +class ImageBitmapSource;

This header #includes ImageBuffer.h. Should that be removed now?

> Source/WebCore/html/ImageBitmap.h:29
> +#include "ImageBitmapSource.h"

Per below: I think "ImageBitmapBacking" would be a more descriptive name.

> Source/WebCore/html/ImageBitmap.h:91
> +    std::unique_ptr<ImageBuffer> takeImageBuffer();

Maybe worth a comment that this has the implicit side-effect of detaching the backing store, and that it returns nullptr if the ImageBitmap's already detached.

> Source/WebCore/html/ImageBitmap.h:101
> +    Optional<ImageBitmapSource> takeImageBitmapSource();

Suggest "takeImageBitmapBacking" as the name.

> Source/WebCore/html/ImageBitmap.h:130
> +    Optional<ImageBitmapSource> m_imageSource;

I think m_backing or m_backingStore would be a better choice of name.

> Source/WebCore/html/ImageBitmapSource.cpp:31
> +ImageBitmapSource::ImageBitmapSource(std::unique_ptr<ImageBuffer>&& bitmapData, OptionSet<SerializationState> serializationState)

See below regarding naming.

> Source/WebCore/html/ImageBitmapSource.h:39
> +class ImageBitmapSource {

Per the ChangeLog comment, since this is now acting as the ImageBitmap's backing store, I think "ImageBitmapBacking" would be a better name.

> Source/WebCore/html/ImageBitmapSource.h:41
> +    ImageBitmapSource() = default;

This constructor leaves m_serializationState thinking it's not origin-clean. Is that what was intended?

[Said Abou-Hallawa]

Created attachment 409745 [details]
Patch

[Said Abou-Hallawa]

Created attachment 409752 [details]
Patch

[EWS]

Committed r267615: <https://trac.webkit.org/changeset/267615>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409752 [details].