Bug 188114

Comment on attachment 345986 [details]
Patch

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

Great direction.

I think we should reduce the use of WebGPURenderingContext even more. In fact, I think a better pattern might be to have the create functions take the GPUXXX&& arguments. The need for the rendering context argument drove a pattern where more of the logic was in the create functions, but I see no reason that callers, for example, couldn’t just create a GPUTexture and make a WebGPUTexture out of it. It would be nice if these WebGPU objects were closer to being pure wrappers.

> Source/WebCore/html/canvas/WebGPUBuffer.h:40
>      static RefPtr<WebGPUBuffer> create(WebGPURenderingContext&, const JSC::ArrayBufferView&);

Consider changing the first argument from a WebGPURenderingContext& to a const GPUDevice&.

> Source/WebCore/html/canvas/WebGPUCommandBuffer.h:59
> +    WebGPUCommandBuffer(const GPUCommandQueue&);

Should add explicit.

> Source/WebCore/html/canvas/WebGPUCommandQueue.h:42
>      static Ref<WebGPUCommandQueue> create(WebGPURenderingContext&);

Consider changing the argument from a WebGPURenderingContext& to a const GPUDevice&.

> Source/WebCore/html/canvas/WebGPUComputeCommandEncoder.h:50
> +    WebGPUComputeCommandEncoder(const GPUCommandBuffer&);

Should add explicit.

> Source/WebCore/html/canvas/WebGPUComputePipelineState.h:41
>      static Ref<WebGPUComputePipelineState> create(WebGPURenderingContext&, const GPUFunction&);

Consider changing the first argument from a WebGPURenderingContext& to a const GPUDevice&.

> Source/WebCore/html/canvas/WebGPUDepthStencilState.h:40
>      static Ref<WebGPUDepthStencilState> create(WebGPURenderingContext&, const GPUDepthStencilDescriptor&);

Consider changing the first argument from a WebGPURenderingContext& to a const GPUDevice&.

> Source/WebCore/html/canvas/WebGPUDrawable.h:42
>      static Ref<WebGPUDrawable> create(WebGPURenderingContext&);

Consider changing the argument from a WebGPURenderingContext& to a const GPUDevice&.

> Source/WebCore/html/canvas/WebGPUFunction.h:44
> +    WebGPUFunction(GPUFunction&&);

Should add explicit.

> Source/WebCore/html/canvas/WebGPULibrary.h:42
>      static Ref<WebGPULibrary> create(WebGPURenderingContext&, const String& sourceCode);

Consider changing the first argument from a WebGPURenderingContext& to a const GPUDevice&.

> Source/WebCore/html/canvas/WebGPURenderPassAttachmentDescriptor.h:56
> +    explicit WebGPURenderPassAttachmentDescriptor();

Should remove explicit now that this has no arguments.

> Source/WebCore/html/canvas/WebGPURenderPassColorAttachmentDescriptor.h:44
> +    WebGPURenderPassColorAttachmentDescriptor(GPURenderPassColorAttachmentDescriptor&&);

Should add explicit.

> Source/WebCore/html/canvas/WebGPURenderPassDepthAttachmentDescriptor.h:44
> +    WebGPURenderPassDepthAttachmentDescriptor(GPURenderPassDepthAttachmentDescriptor&&);

Should add explicit.

> Source/WebCore/html/canvas/WebGPURenderPipelineState.h:40
>      static Ref<WebGPURenderPipelineState> create(WebGPURenderingContext&, const GPURenderPipelineDescriptor&);

Consider changing the first argument from a WebGPURenderingContext& to a const GPUDevice&.

> Source/WebCore/html/canvas/WebGPUTexture.h:41
>      static Ref<WebGPUTexture> create(WebGPURenderingContext&, const GPUTextureDescriptor&);

Consider changing the first argument from a WebGPURenderingContext& to a const GPUDevice&.

Or consider eliminating this and promoting createFromDrawableTexture to be named create. The caller can create the GPUTexture.

Comment on attachment 345999 [details]
Patch

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

> Source/WebCore/html/canvas/WebGPURenderPassColorAttachmentDescriptor.cpp:43
> +    : WebGPURenderPassAttachmentDescriptor()

I don’t think explicit base class initialization is needed any more in this constructor.

> Source/WebCore/html/canvas/WebGPURenderPassDepthAttachmentDescriptor.cpp:41
> +    : WebGPURenderPassAttachmentDescriptor()

Ditto.

(In reply to Darin Adler from comment #5)
> Comment on attachment 345986 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345986&action=review
> 
> Great direction.
> 
> I think we should reduce the use of WebGPURenderingContext even more. In
> fact, I think a better pattern might be to have the create functions take
> the GPUXXX&& arguments. The need for the rendering context argument drove a
> pattern where more of the logic was in the create functions, but I see no
> reason that callers, for example, couldn’t just create a GPUTexture and make
> a WebGPUTexture out of it. It would be nice if these WebGPU objects were
> closer to being pure wrappers.

Yeah, that does sound better. Updated the patch to do that.

> 
> > Source/WebCore/html/canvas/WebGPUBuffer.h:40
> >      static RefPtr<WebGPUBuffer> create(WebGPURenderingContext&, const JSC::ArrayBufferView&);
> 
> Consider changing the first argument from a WebGPURenderingContext& to a
> const GPUDevice&.

Changed to take a GPUBuffer&& instead.

> 
> > Source/WebCore/html/canvas/WebGPUCommandBuffer.h:59
> > +    WebGPUCommandBuffer(const GPUCommandQueue&);
> 
> Should add explicit.

Done.

> 
> > Source/WebCore/html/canvas/WebGPUCommandQueue.h:42
> >      static Ref<WebGPUCommandQueue> create(WebGPURenderingContext&);
> 
> Consider changing the argument from a WebGPURenderingContext& to a const
> GPUDevice&.

Changed to take a GPUCommandQueue&& instead.

> 
> > Source/WebCore/html/canvas/WebGPUComputeCommandEncoder.h:50
> > +    WebGPUComputeCommandEncoder(const GPUCommandBuffer&);
> 
> Should add explicit.

Done.

> 
> > Source/WebCore/html/canvas/WebGPUComputePipelineState.h:41
> >      static Ref<WebGPUComputePipelineState> create(WebGPURenderingContext&, const GPUFunction&);
> 
> Consider changing the first argument from a WebGPURenderingContext& to a
> const GPUDevice&.

Changed to take a GPUComputePipelineState&& instead.

> 
> > Source/WebCore/html/canvas/WebGPUDepthStencilState.h:40
> >      static Ref<WebGPUDepthStencilState> create(WebGPURenderingContext&, const GPUDepthStencilDescriptor&);
> 
> Consider changing the first argument from a WebGPURenderingContext& to a
> const GPUDevice&.

Changed to take a GPUDepthStencilState&& instead.

> 
> > Source/WebCore/html/canvas/WebGPUDrawable.h:42
> >      static Ref<WebGPUDrawable> create(WebGPURenderingContext&);
> 
> Consider changing the argument from a WebGPURenderingContext& to a const
> GPUDevice&.

Changed to take a GPUDrawable.h&& instead.

> 
> > Source/WebCore/html/canvas/WebGPUFunction.h:44
> > +    WebGPUFunction(GPUFunction&&);
> 
> Should add explicit.

Done.

> 
> > Source/WebCore/html/canvas/WebGPULibrary.h:42
> >      static Ref<WebGPULibrary> create(WebGPURenderingContext&, const String& sourceCode);
> 
> Consider changing the first argument from a WebGPURenderingContext& to a
> const GPUDevice&.

Changed to take a GPULibrary&& instead.

> 
> > Source/WebCore/html/canvas/WebGPURenderPassAttachmentDescriptor.h:56
> > +    explicit WebGPURenderPassAttachmentDescriptor();
> 
> Should remove explicit now that this has no arguments.

Done.

> 
> > Source/WebCore/html/canvas/WebGPURenderPassColorAttachmentDescriptor.h:44
> > +    WebGPURenderPassColorAttachmentDescriptor(GPURenderPassColorAttachmentDescriptor&&);
> 
> Should add explicit.

Done.

> 
> > Source/WebCore/html/canvas/WebGPURenderPassDepthAttachmentDescriptor.h:44
> > +    WebGPURenderPassDepthAttachmentDescriptor(GPURenderPassDepthAttachmentDescriptor&&);
> 
> Should add explicit.

Done.

> 
> > Source/WebCore/html/canvas/WebGPURenderPipelineState.h:40
> >      static Ref<WebGPURenderPipelineState> create(WebGPURenderingContext&, const GPURenderPipelineDescriptor&);
> 
> Consider changing the first argument from a WebGPURenderingContext& to a
> const GPUDevice&.

Changed to take a GPURenderPipelineState&& instead.

> 
> > Source/WebCore/html/canvas/WebGPUTexture.h:41
> >      static Ref<WebGPUTexture> create(WebGPURenderingContext&, const GPUTextureDescriptor&);
> 
> Consider changing the first argument from a WebGPURenderingContext& to a
> const GPUDevice&.
> 
> Or consider eliminating this and promoting createFromDrawableTexture to be
> named create. The caller can create the GPUTexture.

I did that latter.

(In reply to Darin Adler from comment #7)
> Comment on attachment 345999 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345999&action=review
> 
> > Source/WebCore/html/canvas/WebGPURenderPassColorAttachmentDescriptor.cpp:43
> > +    : WebGPURenderPassAttachmentDescriptor()
> 
> I don’t think explicit base class initialization is needed any more in this
> constructor.
> 
> > Source/WebCore/html/canvas/WebGPURenderPassDepthAttachmentDescriptor.cpp:41
> > +    : WebGPURenderPassAttachmentDescriptor()
> 
> Ditto.

Removed the base class initialization in both cases.

Comment on attachment 346042 [details]
Patch

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

> Source/WebCore/html/canvas/WebGPURenderPipelineDescriptor.h:32
> +#include <wtf/Ref.h>

RefPtr.h happens to include Ref.h, so we never have to include both. Not sure that’s an elegant thing, but I tend to go with it and leave out the Ref.h include when both are needed.

Created attachment 346073 [details]
Patch

Attachment 346073 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/WebGPULibrary.cpp:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/canvas/WebGPUDrawable.cpp:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 346554 [details]
Patch

Attachment 346554 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/WebGPULibrary.cpp:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/canvas/WebGPUDrawable.cpp:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 346554 [details]
Patch

Clearing flags on attachment: 346554

Committed r234564: <https://trac.webkit.org/changeset/234564>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/42920504>