Bug 207233

Created attachment 389723 [details] Patch

Created attachment 389737 [details] Patch for review

Comment on attachment 389737 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=389737&action=review > Source/WebKit/WebProcess/GPU/RenderingBackend/ImageBufferShareableBitmapBackend.cpp:67 > + if (WTF::holds_alternative<ShareableBitmap::Handle>(handle)) { This should be if (!WTF::holds_alternative<ShareableBitmap::Handle>(handle)) {

Created attachment 391841 [details] Patch

Created attachment 391863 [details] Patch

The GKT port fails while compiling WebKit/WebProcess/WebPage/WebFrame.cpp because the following definition Display* m_display { nullptr }; in the header file WebKit/WebProcess/WebPage/gtk/AcceleratedSurfaceX11.h is ambiguous. The candidates are the global definition (not inside any namespace) typedef struct _XDisplay Display; which is defined in WebKit/WebProcess/WebPage/gtk/AcceleratedSurfaceX11.h and the WebCore namespace namespace WebCore::Display { } which is defined in WebCore/layout/displaytree/DisplayLineBox.h It looks like there is a global "using namespace WebCore" (not inside any namespace) in one of the WebKit sources or the headers which compile with WebKit/WebProcess/WebPage/WebFrame.cpp in a unified source.

Created attachment 391880 [details] Patch

Created attachment 391882 [details] Patch

Created attachment 391883 [details] Patch

Comment on attachment 391883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391883&action=review > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:62 > + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); make_unique<> Do you need to pass both bitmap and context in? The context can be created by the bitmap. > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80 > + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); make_unique<> > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:64 > + RefPtr<ShareableBitmap> m_bitmap; Can this be a Ref<>... > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:65 > + std::unique_ptr<WebCore::GraphicsContext> m_context; And this a GraphicsContext&? > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:49 > + RetainPtr<CGContextRef> cgContext = surface->ensurePlatformContext(nullptr); ensurePlatformContext has a nullptr default argument. > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:53 > + CGContextClearRect(cgContext.get(), FloatRect(FloatPoint::zero(), backendSize)); Shame that we have eagerly clear here. > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:55 > + return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(surface))); make_unique<> > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:69 > + return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(logicalSize, internalSize, resolutionScale, colorSpace, WTFMove(surface))); make_unique<>

Comment on attachment 391883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391883&action=review >> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:62 >> + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); > > make_unique<> > > Do you need to pass both bitmap and context in? The context can be created by the bitmap. If createGraphicsContext() returns nullptr for any reason, the ImageBuffer will be in a weird state. It is allocated but it can't function probably. I wanted to have two states for the returned value: 1. nullptr: Something went wrong when allocating the data members needed for the ImageBuffer. 2. valid pointer: Nothing went wrong when allocating all the data member needed for the ImageBuffer. So the ImageBuffer can function properly. >> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80 >> + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); > > make_unique<> Fixed. But I have to make the constructor public. >> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:65 >> + std::unique_ptr<WebCore::GraphicsContext> m_context; > > And this a GraphicsContext&? No this can't be because it is created by the create() method by calling ShareableBitmap::createGraphicsContext(). The ownership of this pointer is transferred to this class in the constructor by WTFMove(context). The destructor of this class is responsible for releasing the std::unique_ptr. >> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:49 >> + RetainPtr<CGContextRef> cgContext = surface->ensurePlatformContext(nullptr); > > ensurePlatformContext has a nullptr default argument. default argument was removed. >> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:53 >> + CGContextClearRect(cgContext.get(), FloatRect(FloatPoint::zero(), backendSize)); > > Shame that we have eagerly clear here. This what we used to do with IOSurface ImageBuffer before the refactoring. >> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:55 >> + return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(surface))); > > make_unique<> Fixed. >> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:69 >> + return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(logicalSize, internalSize, resolutionScale, colorSpace, WTFMove(surface))); > > make_unique<> Fixed.

Created attachment 391932 [details] Patch

Comment on attachment 391883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391883&action=review >>> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80 >>> + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); >> >> make_unique<> > > Fixed. But I have to make the constructor public. You could keep it private and use a friend declaration. See ContentFilter.h for an example.

Comment on attachment 391932 [details] Patch Clearing flags on attachment: 391932 Committed r257606: <https://trac.webkit.org/changeset/257606>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59871107>