[webkit-reviews] review granted: [Bug 225508] Add an experimental alternative display-list-based RemoteLayerBackingStore implementation : [Attachment 427988] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 225508: Add an experimental alternative display-list-based
RemoteLayerBackingStore implementation
https://bugs.webkit.org/show_bug.cgi?id=225508

Attachment 427988: Patch

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




--- Comment #3 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 427988
  --> https://bugs.webkit.org/attachment.cgi?id=427988
Patch

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

Really excellent work.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:416
> +#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
> +	   , [&] (IPC::SharedBufferCopy& buffer) {
> +	       ASSERT(m_type == Type::CGDisplayList);
> +	       ASSERT([layer isKindOfClass:[WKCompositingLayer class]]);
> +	       if (![layer isKindOfClass:[WKCompositingLayer class]])
> +		   return;
> +	       [layer setValue:@1 forKeyPath:WKCGDisplayListEnabledKey];
> +	       auto data = buffer.buffer()->createCFData();
> +	       [(WKCompositingLayer *)layer
_setWKContentsDisplayList:data.get()];
>	   }

Perhaps in the future we can make this function take a WKCompositingLayer
rather than a vanilla CALayer and just typedef WKCompositingLayer to CALayer on
platforms where they can be the same thing?

> Source/WebKit/UIProcess/RemoteLayerTree/cocoa/RemoteLayerTreeLayers.h:26
> +#if PLATFORM(COCOA)

#pragma once?

> Source/WebKit/UIProcess/RemoteLayerTree/cocoa/RemoteLayerTreeLayers.h:34
> + at property (nonatomic, retain,
setter=_setWKContentsDisplayList:)__attribute__((NSObject)) CFDataRef
_wkContentsDisplayList;

I think you want a space before the __attribute__. Also, this can probably be
direct.

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferBackendHandle.h:-40
>  #if PLATFORM(COCOA) // FIXME: This is really about IOSurface.
> -    MachSendRight,
> +    MachSendRight
> +#endif
> +    , ShareableBitmap::Handle
> +#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
> +    , IPC::SharedBufferCopy
>  #endif
> -    ShareableBitmap::Handle

I think this could be a bit neater if we made ShareableBitmap::Handle first
here.

using ImageBufferBackendHandle = Variant<
    ShareableBitmap::Handle
#if PLATFORM(COCOA) // really should be HAVE(IOSURFACE) or something like that
    , MachSendRight
#endif
#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
    , IPC::SharedBufferCopy
#endif
>;


Additionally, I think we should probably add struct wrappers for MachSendRight
and IPC::SharedBufferCopy in the future to indicate their use a bit better.

struct IOSurfaceImageBufferBackendHandle { MachSendRight handle };
struct CGDisplayListImageBufferBackendHandle { IPC::SharedBufferCopy handle };

using ImageBufferBackendHandle = Variant<
    ShareableBitmap::Handle
#if PLATFORM(COCOA)
    , IOSurfaceImageBufferBackendHandle
#endif
#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
    , CGDisplayListImageBufferBackendHandle
#endif
>;

This would let the type system tell the story a bit nicer. Not needed in this
pass though.