[webkit-reviews] review granted: [Bug 217095] [WebGPU] Add platform guards : [Attachment 410026] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Don Olmstead
<don.olmstead at sony.com>'s request for review:
Bug 217095: [WebGPU] Add platform guards
https://bugs.webkit.org/show_bug.cgi?id=217095

Attachment 410026: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 410026
  --> https://bugs.webkit.org/attachment.cgi?id=410026
Patch

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

Not totally thrilled with details of the approach. Happy that we are adding the
#if USE(METAL).

> Source/WebCore/platform/graphics/gpu/GPUBindGroupAllocator.h:40
> +#if USE(METAL)
> +#include <objc/NSObjCRuntime.h>
>  #include <wtf/RetainPtr.h>
>  
>  OBJC_PROTOCOL(MTLArgumentEncoder);
>  OBJC_PROTOCOL(MTLBuffer);
> +#endif

I’d prefer we use two separate #if/#endif pairs here. Same feedback over and
over again. Nicer to have all includes together, then all forward declarations
together, rather than trying to group into platforms.

> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:62
>  #else
> -using PlatformBuffer = void;
> +#error Unsupported configuration

I think we’ll get errors without an explicit #error, so maybe don’t bother with
the #else.

Not the biggest fan of platform-specifics with a chain of #if/#elif/#endif.
Seems like we can just define these types separately as needed.

But more, generally speaking, I am not so fond of these types; how much
platform-independent code to handle platform-specific types really needs to
exist at all? I’d rather see #if around multiple definitions of
m_platformBuffer, because I don’t really see this as a platform-independent
abstraction. Very little platform-independent code needs to deal with these;
the platform-specific code does need to.

One exception is the constructor, I guess?

We can do better.

Same comment repeatedly about other cases below.

> Source/WebCore/platform/graphics/gpu/GPUSwapChain.h:35
> +// PlatformLayer implementation needed otherwise compiling derived sources
will fail

Please add a period.