[webkit-reviews] review denied: [Bug 204955] Make GPU process create/release a shared IOSurface : [Attachment 385072] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 204955: Make GPU process create/release a shared IOSurface
https://bugs.webkit.org/show_bug.cgi?id=204955

Attachment 385072: Patch

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




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

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

IOSurface is an macOS/iOS class/concept, so I don't think it should be used in
these ostensibly cross platform classes.  In most other cases we create an
abstraction around it and use IOSurface as the implementation of that
abstraction on Apple platforms.

> Source/WebCore/ChangeLog:4
> +	   Make GPU process create/release a shared IOSurface
> +	   https://bugs.webkit.org/show_bug.cgi?id=204955

How are you testing this?

> Source/WebCore/page/Page.h:280
> +#if ENABLE(GPU_PROCESS)
> +    RefPtr<RenderingBackend> renderingBackend();
> +#endif

Leaking the notion of a GPU process into the page seems very unfortunate. I
think this would be much cleaner/clearer if all pages had a RenderingBackend,
and one implementation of the backend was a GPU process.

> Source/WebCore/platform/graphics/ColorSpace.h:33
> +enum class ColorSpace : uint8_t {
> +    SRGB,
> +    LinearRGB,
> +    DisplayP3

Can you make this change in its own change prior to this?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in:26
> +    void CreateSharedIOSurface(WebCore::IntSize size, WebCore::IntSize
contextSize, enum:uint8_t WebCore::ColorSpace colorSpace) -> (uint64_t
surfaceID, MachSendRight sendRight) Synchronous

Please do not introduce any new sync messages, they almost always cause
performance issues down the line.