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>