Bug 217095

Created attachment 410026 [details]
Patch

Comment on attachment 410026 [details]
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.

Comment on attachment 410026 [details]
Patch

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

>> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:62
>> +#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.

+1 to what Darin said. I find the PlatformType stuff fairly confusing, because it means you have to redirect to work out what the actual type is.

Created attachment 410071 [details]
Patch

(In reply to Dean Jackson from comment #6)
> Comment on attachment 410026 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410026&action=review
> 
> >> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:62
> >> +#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.
> 
> +1 to what Darin said. I find the PlatformType stuff fairly confusing,
> because it means you have to redirect to work out what the actual type is.

When I'm going through the actual implementation with Dawn I'll revisit the PlatformType stuff.

Committed r267777: <https://trac.webkit.org/changeset/267777>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410071 [details].

<rdar://problem/69773470>