[webkit-reviews] review granted: [Bug 236671] Allow downcast<> to work with ImageBufferBackends that support sharing : [Attachment 452095] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 236671: Allow downcast<> to work with ImageBufferBackends that support
sharing
https://bugs.webkit.org/show_bug.cgi?id=236671

Attachment 452095: Patch

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




--- Comment #2 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 452095
  --> https://bugs.webkit.org/attachment.cgi?id=452095
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452095&action=review

> Source/WebCore/platform/graphics/ImageBufferBackend.h:143
> +    virtual ImageBufferBackendSharing* toBackendSharing() { return nullptr;
}

It is sad that we can't make ImageBufferBackend inherit
ImageBufferBackendSharing and have to introduce such function to avoid the
diamond inheritance problem.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:119
> +    auto handleFromBuffer = [](WebCore::ImageBuffer& buffer) ->
std::optional<ImageBufferBackendHandle> {

This is definitely cleaner and safer than the existing code. But we still have
to deal with the backend and have to downcast it. Is there a way to move this
call to ImageBuffer such that this code can be replaced by a single call?

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferBackendHandleSharing.h:37
> +    virtual ImageBufferBackendHandle createBackendHandle() const = 0;

Can these functions in ImageBuffer/ImageBufferBackend also be moved to this
class?

    virtual bool isInUse() const = 0;
    virtual void releaseGraphicsContext() = 0;
    virtual void releaseBufferToPool() = 0;

    // Returns true on success.
    virtual bool setVolatile() = 0;
    virtual VolatilityState setNonVolatile() = 0;

    virtual std::unique_ptr<ThreadSafeImageBufferFlusher> createFlusher() = 0;

I think they are specific to WebKit and they are only used by
RemoteLayerBackingStore.

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferBackendHandleSharing.h:45
>
+SPECIALIZE_TYPE_TRAITS_IMAGE_BUFFER_BACKEND_SHARING(WebKit::ImageBufferBackend
HandleSharing, isImageBufferBackendHandleSharing())

SPECIALIZE_TYPE_TRAITS_IMAGE_BUFFER_BACKEND_SHARING() is defined in
ImageBufferBackend.h and it is only used here. Why do not we move the macro
here also?