Bug 216565

Created attachment 408828 [details] Patch

Created attachment 408833 [details] Patch

Created attachment 408961 [details] Patch

Created attachment 408969 [details] Patch

Created attachment 408975 [details] Patch

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.

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?

Created attachment 409241 [details] Patch

Created attachment 409245 [details] Patch

Created attachment 409258 [details] Patch

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.

<rdar://problem/69375838>

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?

Created attachment 409745 [details] Patch

Created attachment 409752 [details] Patch

Committed r267615: <https://trac.webkit.org/changeset/267615> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409752 [details].