Created attachment 412868 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Created attachment 412877 [details] Patch

Created attachment 412879 [details] Patch

Created attachment 412880 [details] Patch

Created attachment 412881 [details] Patch

Created attachment 412885 [details] Patch

Created attachment 412887 [details] Patch

Created attachment 412890 [details] Patch

Created attachment 412891 [details] Patch

Created attachment 412893 [details] Patch

Created attachment 412894 [details] Patch

Comment on attachment 412894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412894&action=review > Source/WebCore/platform/graphics/NativeImage.h:74 > + RenderingResourceIdentifier m_renderingResourceIdentifier { RenderingResourceIdentifier::generate() }; It's a bit surprising the Image owns this itself, rather than a higher level > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:460 > + subImage = tileImage->platformImage(); I wish there was a way to get rid of this extra typing. Requiring it doesn't seem to actually add much clarity to the code. Can we do an operator PlatformImagePtr()? > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:87 > + image = NativeImage::create(CGBitmapContextCreateImage(context.get())); I wish we could make this implicit. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:864 > +DrawNativeImage::DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options) Most of this patch is an introduction and migration to a new type, but this looks like an intentional behavior change. It seems like a good idea to split this out, just in case.

Comment on attachment 412894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412894&action=review > Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:369 > + m_decodedImage = NativeImage::create(CGBitmapContextCreateImage(bitmapContext.get())); Does this leak the image? > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:73 > + return NativeImage::create(CGImageCreateWithImageInRect(image->platformImage().get(), CGRectMake(0, 0, backendSize.width(), backendSize.height()))); (Ditto about the leak) >> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:87 >> + image = NativeImage::create(CGBitmapContextCreateImage(context.get())); > > I wish we could make this implicit. (Ditto about the leak)

Comment on attachment 412894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412894&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:159 > + image = NativeImage::create(CGImageCreate(pixelArrayDimensions.width(), pixelArrayDimensions.height(), 8, 32, 4 * pixelArrayDimensions.width(), sRGBColorSpaceRef(), kCGBitmapByteOrderDefault | kCGImageAlphaNoneSkipLast, dataProvider.get(), 0, false, kCGRenderingIntentDefault)); IIUC, these are going to leak as well since CGImageCreate returns a +1 object. We could use adoptCF here to fix this. > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:169 > + image = NativeImage::create(CGBitmapContextCreateImage(context.get())); (Ditto) > Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:114 > + return NativeImage::create(CGBitmapContextCreateImage(context().platformContext())); (Ditto) > Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:117 > + return NativeImage::create(CGImageCreate( (Ditto)

Created attachment 413507 [details] Patch

Created attachment 413508 [details] Patch

Created attachment 413511 [details] Patch

Created attachment 413517 [details] Patch

Created attachment 413518 [details] Patch

Created attachment 413521 [details] Patch

Comment on attachment 413521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413521&action=review > Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:369 > + m_decodedImage = NativeImage::create(CGBitmapContextCreateImage(bitmapContext.get())); Nit - still have a leak here.

<rdar://problem/71167282>

Created attachment 413566 [details] Patch

Comment on attachment 412894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412894&action=review >> Source/WebCore/platform/graphics/NativeImage.h:74 >> + RenderingResourceIdentifier m_renderingResourceIdentifier { RenderingResourceIdentifier::generate() }; > > It's a bit surprising the Image owns this itself, rather than a higher level The identifier is removed for now. We agreed on having a RenderingResourceIdentifier for ImageBuffer regardless it is a remote ImageBuffer or not. This solved the problem of recording and replaying resource items such as DrawImageBuffer. A HashMap of Ref<ImageBuffer> is filled while recording and is passed when replaying back. DrawImageBuffer contains only the RenderingResourceIdentifier of the ImageBuffer. So I may use the technique with NativeImage. >> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:460 >> + subImage = tileImage->platformImage(); > > I wish there was a way to get rid of this extra typing. Requiring it doesn't seem to actually add much clarity to the code. Can we do an operator PlatformImagePtr()? I added it at some point but I realized that I need it in cases like CGImageGetWidth(tileImage->platformImage().get()). It was a little bit confusing when you need and when you do not need it. The RefPtr has its operator to NativeImage and this makes difficult to differentiate between them. >> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:369 >> + m_decodedImage = NativeImage::create(CGBitmapContextCreateImage(bitmapContext.get())); > > Does this leak the image? Yes. Thanks for catching this. I think I could avoid writing adoptCF. I did not realized is going to increment the ref count instead of adopting the existing one. >> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:73 >> + return NativeImage::create(CGImageCreateWithImageInRect(image->platformImage().get(), CGRectMake(0, 0, backendSize.width(), backendSize.height()))); > > (Ditto about the leak) Yes searching for "NativeImage::create(CG" should get this as well. >> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:864 >> +DrawNativeImage::DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options) > > Most of this patch is an introduction and migration to a new type, but this looks like an intentional behavior change. It seems like a good idea to split this out, just in case. Done.

Comment on attachment 413566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413566&action=review > Source/WebCore/platform/graphics/NativeImage.h:37 > +class NativeImage : public RefCounted<NativeImage> { WTF_MAKE_FAST_ALLOCATED > Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:341 > + RetainPtr<CGImageRef> image = m_cgImage->platformImage(); The member variable isn't a CGImage any more, so maybe rename it. > Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:376 > + image = m_cgImage->platformImage(); Hard to see where 'image' comes from. > Source/WebKitLegacy/mac/DOM/DOM.mm:539 > + RetainPtr<CGImageRef> contentImage = image->nativeImage()->platformImage(); auto?

Created attachment 413639 [details] Patch

Created attachment 413645 [details] Patch

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