[webkit-reviews] review granted: [Bug 216565] Move SerializationState from ImageBuffer to ImageBitmap : [Attachment 409258] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 216565: Move SerializationState from ImageBuffer to ImageBitmap
https://bugs.webkit.org/show_bug.cgi?id=216565

Attachment 409258: Patch

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




--- Comment #13 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 409258
  --> https://bugs.webkit.org/attachment.cgi?id=409258
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?