Bug 187333

Summary: Improve WebGPU implementation, including using Metal Objective-C protocols more simply and correctly
Product: WebKit Reporter: Darin Adler <darin>
Component: WebGPUAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cdumez, cmarcelo, dbates, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, kondapallykalyan, mitz, rniwa, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
sam: review+
Patch
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews201 for win-future
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews101 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews107 for mac-sierra-wk2 none

Darin Adler
Reported 2018-07-04 15:58:00 PDT
Improve WebGPU implementation, including using Metal Objective-C protocols more simply and correctly
Attachments
Patch (274.61 KB, patch)
2018-07-04 17:03 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (274.86 KB, patch)
2018-07-04 17:23 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (274.94 KB, patch)
2018-07-04 18:15 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (283.80 KB, patch)
2018-07-04 19:18 PDT, Darin Adler 		sam: review+
DetailsFormatted DiffDiff
Patch (285.53 KB, patch)
2018-07-13 21:30 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (368.09 KB, patch)
2018-07-13 21:50 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (374.67 KB, patch)
2018-07-13 22:16 PDT, Darin Adler 		ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews201 for win-future (12.77 MB, application/zip)
2018-07-14 00:21 PDT, EWS Watchlist 		no flags
Details
Patch (375.73 KB, patch)
2018-07-24 21:38 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (375.54 KB, patch)
2018-07-25 21:06 PDT, Darin Adler 		ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews101 for mac-sierra (2.34 MB, application/zip)
2018-07-25 22:33 PDT, EWS Watchlist 		no flags
Details
Patch (375.58 KB, patch)
2018-07-25 22:39 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.19 MB, application/zip)
2018-07-25 22:40 PDT, EWS Watchlist 		no flags
Details
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2018-07-04 17:03:52 PDT Comment hidden (obsolete)
Darin Adler
Comment 2 2018-07-04 17:04:54 PDT
Patch ended up huge; I can break it down into smaller bites if needed. However, since this is all in WebGPU so not on by default I think maybe we can just test it all together.
Darin Adler
Comment 3 2018-07-04 17:05:27 PDT
I was led to tackle this by problems compiling the Metal code under ARC that this patch resolves.
EWS Watchlist
Comment 4 2018-07-04 17:05:32 PDT Comment hidden (obsolete)
Darin Adler
Comment 5 2018-07-04 17:23:29 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 6 2018-07-04 17:25:36 PDT Comment hidden (obsolete)
Darin Adler
Comment 7 2018-07-04 18:15:05 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 8 2018-07-04 18:16:40 PDT Comment hidden (obsolete)
Darin Adler
Comment 9 2018-07-04 19:18:56 PDT
Created attachment 344307 [details] Patch
EWS Watchlist
Comment 10 2018-07-04 19:20:47 PDT Comment hidden (obsolete)
Darin Adler
Comment 11 2018-07-05 07:07:19 PDT
Comment on attachment 344307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344307&action=review > Source/WebCore/platform/graphics/cocoa/GPURenderCommandEncoderMetal.mm:85 > + // FIXME: Why can't we set the count to zero? Should probably say "Why do we need a special optimization for count of zero?"
Darin Adler
Comment 12 2018-07-05 14:55:09 PDT
That latest version of the patch passes all the tests on EWS so I hope someone can review now. Annoying that the style checker has lots of complaints.
Darin Adler
Comment 13 2018-07-06 09:10:00 PDT
Any takers for review?
Sam Weinig
Comment 14 2018-07-06 15:44:12 PDT
(In reply to Darin Adler from comment #13) > Any takers for review? I'm working my way through.
Sam Weinig
Comment 15 2018-07-06 18:03:31 PDT
Comment on attachment 344307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344307&action=review > Source/WebCore/ChangeLog:74 > + (WebCore::WebGPUBuffer::create): Return nullptr if we fail to create a GPU buffer. > + Also use more references rather than pointers for things that are never null. This is a change in behavior (we always used to return an object). What is motivating this change? Do we have a test for this new behavior? > Source/WebCore/ChangeLog:268 > + * platform/graphics/cocoa/GPUBufferMetal.mm: Instead of being in the platform/graphics/cocoa directory, should we move this (and all the other metal files, to a platform/graphics/metal directory? > Source/WTF/wtf/Compiler.h:303 > +#if !defined(OBJC_PROTOCOL) && defined(__OBJC__) > +#define OBJC_PROTOCOL(protocolName) @protocol protocolName; using protocolName = NSObject <protocolName> > +#endif I'm not clear on what the 'using...' part is doing. Might be worth a comment. > Source/WebCore/html/canvas/WebGPUCommandBuffer.cpp:75 > RefPtr<WebGPURenderCommandEncoder> WebGPUCommandBuffer::createRenderCommandEncoderWithDescriptor(WebGPURenderPassDescriptor& descriptor) > { > - RefPtr<WebGPURenderCommandEncoder> commandEncoder = WebGPURenderCommandEncoder::create(this->context(), this, &descriptor); > - return commandEncoder; > + return WebGPURenderCommandEncoder::create(*context(), m_buffer, descriptor.descriptor()); > } It looks like WebGPURenderCommandEncoder::create returns a Ref<WebGPURenderCommandEncoder>, so this function could as well. > Source/WebCore/html/canvas/WebGPUCommandBuffer.cpp:80 > RefPtr<WebGPUComputeCommandEncoder> WebGPUCommandBuffer::createComputeCommandEncoder() > { > - RefPtr<WebGPUComputeCommandEncoder> commandEncoder = WebGPUComputeCommandEncoder::create(this->context(), this); > - return commandEncoder; > + return WebGPUComputeCommandEncoder::create(*context(), m_buffer); > } Ditto. > Source/WebCore/html/canvas/WebGPUCommandQueue.cpp:52 > RefPtr<WebGPUCommandBuffer> WebGPUCommandQueue::createCommandBuffer() > { > - RefPtr<WebGPUCommandBuffer> buffer = WebGPUCommandBuffer::create(this->context(), this); > - return buffer; > + return WebGPUCommandBuffer::create(*context(), m_queue); > } WebGPUCommandBuffer::create returns a Ref<WebGPUCommandBuffer>, so this function can as well. > Source/WebCore/html/canvas/WebGPUDrawable.cpp:45 > + , m_texture { WebGPUTexture::createFromDrawableTexture(context, GPUTexture { m_drawable }) } WebGPUTexture::createFromDrawableTexture returns a Ref<WebGPUTexture>, so m_texture can be a Ref<WebGPUTexture>. > Source/WebCore/html/canvas/WebGPULibrary.cpp:58 > + auto function = WebGPUFunction::create(*context(), m_library, name); > if (!function->function()) > return nullptr; > - return function; > + return WTFMove(function); It seems like this could be done slightly more efficiently if the GPUFunction was created first, checked to see if the function was non-null, and then only if it was non-null, moved into a newly created WebGPUFunction. This would avoid the allocation of the WebGPUFunction in the case that the function doesn't exist. > Source/WebCore/html/canvas/WebGPURenderPassDescriptor.cpp:47 > -RefPtr<WebGPURenderPassDepthAttachmentDescriptor> WebGPURenderPassDescriptor::depthAttachment() > +WebGPURenderPassDepthAttachmentDescriptor* WebGPURenderPassDescriptor::depthAttachment() > { > - if (!m_renderPassDescriptor) > - return nullptr; > - > - if (!m_depthAttachmentDescriptor) { > - RefPtr<GPURenderPassDepthAttachmentDescriptor> platformDepthAttachment = m_renderPassDescriptor->depthAttachment(); > - m_depthAttachmentDescriptor = WebGPURenderPassDepthAttachmentDescriptor::create(this->context(), platformDepthAttachment.get()); > - } > - return m_depthAttachmentDescriptor; > + if (!m_depthAttachment) > + m_depthAttachment = WebGPURenderPassDepthAttachmentDescriptor::create(*context(), m_descriptor.depthAttachment()); > + return m_depthAttachment.get(); > } WebGPURenderPassDepthAttachmentDescriptor::create returns a Ref<WebGPURenderPassDepthAttachmentDescriptor>, so I think this function can also return a Ref<> (or a reference). > Source/WebCore/html/canvas/WebGPURenderingContext.cpp:82 > - auto renderingContext = std::unique_ptr<WebGPURenderingContext>(new WebGPURenderingContext(canvas, device.releaseNonNull())); > + auto renderingContext = std::unique_ptr<WebGPURenderingContext>(new WebGPURenderingContext(canvas, WTFMove(device))); Can std::make_unique be used here? > Source/WebCore/html/canvas/WebGPURenderingContext.cpp:162 > RefPtr<WebGPURenderPipelineState> WebGPURenderingContext::createRenderPipelineState(WebGPURenderPipelineDescriptor& descriptor) > { > - RefPtr<WebGPURenderPipelineState> state = WebGPURenderPipelineState::create(this, &descriptor); > - return state; > + return WebGPURenderPipelineState::create(*this, descriptor.descriptor()); > } This can return a Ref<WebGPURenderPipelineState>. > Source/WebCore/html/canvas/WebGPURenderingContext.cpp:167 > RefPtr<WebGPUDepthStencilState> WebGPURenderingContext::createDepthStencilState(WebGPUDepthStencilDescriptor& descriptor) > { > - RefPtr<WebGPUDepthStencilState> state = WebGPUDepthStencilState::create(this, &descriptor); > - return state; > + return WebGPUDepthStencilState::create(*this, descriptor.descriptor()); > } This can return a Ref<WebGPURenderPipelineState> > Source/WebCore/html/canvas/WebGPURenderingContext.cpp:172 > RefPtr<WebGPUComputePipelineState> WebGPURenderingContext::createComputePipelineState(WebGPUFunction& function) > { > - RefPtr<WebGPUComputePipelineState> state = WebGPUComputePipelineState::create(this, &function); > - return state; > + return WebGPUComputePipelineState::create(*this, function.function()); > } This can return a Ref<WebGPURenderPipelineState>. > Source/WebCore/html/canvas/WebGPURenderingContext.cpp:177 > RefPtr<WebGPUCommandQueue> WebGPURenderingContext::createCommandQueue() > { > - RefPtr<WebGPUCommandQueue> queue = WebGPUCommandQueue::create(this); > - return queue; > + return WebGPUCommandQueue::create(*this); > } This can return a Ref<WebGPUCommandQueue>. > Source/WebCore/html/canvas/WebGPURenderingContext.cpp:182 > RefPtr<WebGPUDrawable> WebGPURenderingContext::nextDrawable() > { > - RefPtr<WebGPUDrawable> drawable = WebGPUDrawable::create(this); > - return drawable; > + return WebGPUDrawable::create(*this); > } This can return a Ref<WebGPUDrawable> > Source/WebCore/html/canvas/WebGPURenderingContext.cpp:192 > RefPtr<WebGPUTexture> WebGPURenderingContext::createTexture(WebGPUTextureDescriptor& descriptor) > { > - RefPtr<WebGPUTexture> texture = WebGPUTexture::create(this, &descriptor); > - return texture; > + return WebGPUTexture::create(*this, descriptor.descriptor()); > } This can return a Ref<WebGPUTexture>. > Source/WebCore/platform/graphics/cocoa/GPURenderPipelineDescriptorMetal.mm:59 > + if (!newFunction.metal()) > + return; > + [m_metal setVertexFunction:newFunction.metal()]; This is changing behavior when newFunction.metal() is nil, it used to call setVertexFunction with nil. Why is that the right change? > Source/WebCore/platform/graphics/cocoa/GPURenderPipelineDescriptorMetal.mm:66 > + if (!newFunction.metal()) > + return; > + [m_metal setFragmentFunction:newFunction.metal()]; Ditto. > Source/WebCore/platform/graphics/cocoa/GPURenderPipelineStateMetal.mm:40 > +GPURenderPipelineState::GPURenderPipelineState(const GPUDevice& device, const GPURenderPipelineDescriptor& descriptor) > + : m_metal { adoptNS([device.metal() newRenderPipelineStateWithDescriptor:descriptor.metal() error:nil]) } Should we be checking this error value? > Source/WebCore/platform/graphics/gpu/GPUCommandBuffer.cpp:55 > +DOMPromiseProxy<IDLVoid>& GPUCommandBuffer::completed() > { > + if (!m_addedCompletedHandler) { > + addCompletedHandler(); > + m_addedCompletedHandler = true; > + } > return m_completedPromise; > } This feels like a layering violation. Classes in the platform layer should not be using DOMPromiseProxy. I believe what we have done traditionally is just use stored callback functions.
Dean Jackson
Comment 16 2018-07-07 11:52:44 PDT
Comment on attachment 344307 [details] Patch Thanks Darin!
Darin Adler
Comment 17 2018-07-12 08:11:28 PDT
Comment on attachment 344307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344307&action=review >> Source/WebCore/ChangeLog:74 >> + Also use more references rather than pointers for things that are never null. > > This is a change in behavior (we always used to return an object). What is motivating this change? Do we have a test for this new behavior? The only call site is WebGPURenderingContext::createBuffer, a web-exposed function that returns a WebGPUBuffer? and I presumed the reason for the nullable return value was to handle failure. The old behavior was to return a WebGPUBuffer with 0 length and null for contents, which does not seem as good to me, but I guess the real issue is what we want to specify. I did not add a test, nor is there an existing test that covers this; should add one. The failures are ones in GPUBuffer::GPUBuffer; I’m not sure how to manufacture one for the test. >> Source/WebCore/ChangeLog:268 >> + * platform/graphics/cocoa/GPUBufferMetal.mm: > > Instead of being in the platform/graphics/cocoa directory, should we move this (and all the other metal files, to a platform/graphics/metal directory? Yes, I’d like that slightly better. >> Source/WTF/wtf/Compiler.h:303 >> +#endif > > I'm not clear on what the 'using...' part is doing. Might be worth a comment. What the "using" part does is define a type with the same name as the protocol. That way we can write MTLBuffer * or RetainPtr<MTLBuffer> instead of NSObject<MTLBuffer> * or id<MTLBuffer>. This lets us use syntax that works in both C++ and Objective-C++ for types in headers. I’d be happy to write a comment about that. >> Source/WebCore/html/canvas/WebGPUCommandBuffer.cpp:75 >> } > > It looks like WebGPURenderCommandEncoder::create returns a Ref<WebGPURenderCommandEncoder>, so this function could as well. Which is good since the IDL file also doesn’t indicate that the return value is nullable. Done. >> Source/WebCore/html/canvas/WebGPUCommandBuffer.cpp:80 >> } > > Ditto. Done. >> Source/WebCore/html/canvas/WebGPUCommandQueue.cpp:52 >> } > > WebGPUCommandBuffer::create returns a Ref<WebGPUCommandBuffer>, so this function can as well. I will do this, but I also have to change the IDL to return a non-nullable. >> Source/WebCore/html/canvas/WebGPULibrary.cpp:58 >> + return WTFMove(function); > > It seems like this could be done slightly more efficiently if the GPUFunction was created first, checked to see if the function was non-null, and then only if it was non-null, moved into a newly created WebGPUFunction. This would avoid the allocation of the WebGPUFunction in the case that the function doesn't exist. It’s more efficient for the failure case, and possibly slightly less efficient for the success case, since we have to move the GPUFunction. But, sure, I will do it. >> Source/WebCore/html/canvas/WebGPURenderPassDescriptor.cpp:47 >> } > > WebGPURenderPassDepthAttachmentDescriptor::create returns a Ref<WebGPURenderPassDepthAttachmentDescriptor>, so I think this function can also return a Ref<> (or a reference). Done. >> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:82 >> + auto renderingContext = std::unique_ptr<WebGPURenderingContext>(new WebGPURenderingContext(canvas, WTFMove(device))); > > Can std::make_unique be used here? It could if we make the WebGPURenderingContext constructor public or make make_unique a friend. But in either case this has the bad effect of allowing others to make one of these without calling the create function. I think I will leave it alone for now. >> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:162 >> } > > This can return a Ref<WebGPURenderPipelineState>. Requires change to the IDL file. Done. >> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:167 >> } > > This can return a Ref<WebGPURenderPipelineState> Requires change to the IDL file. Done. >> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:172 >> } > > This can return a Ref<WebGPURenderPipelineState>. Requires change to the IDL file. Done. >> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:177 >> } > > This can return a Ref<WebGPUCommandQueue>. Requires change to the IDL file. Done. >> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:182 >> } > > This can return a Ref<WebGPUDrawable> Requires change to the IDL file. Done. >> Source/WebCore/html/canvas/WebGPURenderingContext.cpp:192 >> } > > This can return a Ref<WebGPUTexture>. Requires change to the IDL file. Done. >> Source/WebCore/platform/graphics/cocoa/GPURenderPipelineDescriptorMetal.mm:59 >> + [m_metal setVertexFunction:newFunction.metal()]; > > This is changing behavior when newFunction.metal() is nil, it used to call setVertexFunction with nil. Why is that the right change? Oops, it’s not. Changing back. >> Source/WebCore/platform/graphics/cocoa/GPURenderPipelineDescriptorMetal.mm:66 >> + [m_metal setFragmentFunction:newFunction.metal()]; > > Ditto. Changing back. >> Source/WebCore/platform/graphics/cocoa/GPURenderPipelineStateMetal.mm:40 >> + : m_metal { adoptNS([device.metal() newRenderPipelineStateWithDescriptor:descriptor.metal() error:nil]) } > > Should we be checking this error value? Well, we are checking it; it returns nil when it fails. I suppose your question amounts to: Should we use an NSError out argument so we can report error details? But I don’t know how we we would do that. Log in the console? >> Source/WebCore/platform/graphics/gpu/GPUCommandBuffer.cpp:55 >> } > > This feels like a layering violation. Classes in the platform layer should not be using DOMPromiseProxy. I believe what we have done traditionally is just use stored callback functions. As a bonus this will eliminate the need for GPUCommandBuffer to be a reference counted object. I’ll give it a try.
Darin Adler
Comment 18 2018-07-13 21:30:08 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 19 2018-07-13 21:32:00 PDT Comment hidden (obsolete)
Darin Adler
Comment 20 2018-07-13 21:50:48 PDT Comment hidden (obsolete)
Darin Adler
Comment 21 2018-07-13 21:51:34 PDT
Latest patch, much larger, includes the move of all the Metal files from the "cocoa" directory to the "metal" directory.
Darin Adler
Comment 22 2018-07-13 21:52:35 PDT
Comment on attachment 344307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344307&action=review >>> Source/WebCore/ChangeLog:74 >>> + Also use more references rather than pointers for things that are never null. >> >> This is a change in behavior (we always used to return an object). What is motivating this change? Do we have a test for this new behavior? > > The only call site is WebGPURenderingContext::createBuffer, a web-exposed function that returns a WebGPUBuffer? and I presumed the reason for the nullable return value was to handle failure. The old behavior was to return a WebGPUBuffer with 0 length and null for contents, which does not seem as good to me, but I guess the real issue is what we want to specify. > > I did not add a test, nor is there an existing test that covers this; should add one. The failures are ones in GPUBuffer::GPUBuffer; I’m not sure how to manufacture one for the test. My latest patch deals with all the comments except for this one. Sam, I could change the behavior back, land this without a test, or try to figure out how to write a test. I’m thinking “change the behavior back” maybe?
EWS Watchlist
Comment 23 2018-07-13 21:52:49 PDT Comment hidden (obsolete)
Darin Adler
Comment 24 2018-07-13 22:16:05 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 25 2018-07-13 22:18:50 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 26 2018-07-14 00:20:51 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 27 2018-07-14 00:21:04 PDT Comment hidden (obsolete)
Sam Weinig
Comment 28 2018-07-18 17:48:10 PDT
(In reply to Darin Adler from comment #22) > Comment on attachment 344307 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344307&action=review > > >>> Source/WebCore/ChangeLog:74 > >>> + Also use more references rather than pointers for things that are never null. > >> > >> This is a change in behavior (we always used to return an object). What is motivating this change? Do we have a test for this new behavior? > > > > The only call site is WebGPURenderingContext::createBuffer, a web-exposed function that returns a WebGPUBuffer? and I presumed the reason for the nullable return value was to handle failure. The old behavior was to return a WebGPUBuffer with 0 length and null for contents, which does not seem as good to me, but I guess the real issue is what we want to specify. > > > > I did not add a test, nor is there an existing test that covers this; should add one. The failures are ones in GPUBuffer::GPUBuffer; I’m not sure how to manufacture one for the test. > > My latest patch deals with all the comments except for this one. > > Sam, I could change the behavior back, land this without a test, or try to > figure out how to write a test. I’m thinking “change the behavior back” > maybe? I think "change the behavior back" sounds right. Given how big this is, separating out behavior change into a separate commit seems like a good idea.
Darin Adler
Comment 29 2018-07-24 09:15:45 PDT
Comment on attachment 344307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344307&action=review >>>>> Source/WebCore/ChangeLog:74 >>>>> + Also use more references rather than pointers for things that are never null. >>>> >>>> This is a change in behavior (we always used to return an object). What is motivating this change? Do we have a test for this new behavior? >>> >>> The only call site is WebGPURenderingContext::createBuffer, a web-exposed function that returns a WebGPUBuffer? and I presumed the reason for the nullable return value was to handle failure. The old behavior was to return a WebGPUBuffer with 0 length and null for contents, which does not seem as good to me, but I guess the real issue is what we want to specify. >>> >>> I did not add a test, nor is there an existing test that covers this; should add one. The failures are ones in GPUBuffer::GPUBuffer; I’m not sure how to manufacture one for the test. >> >> My latest patch deals with all the comments except for this one. >> >> Sam, I could change the behavior back, land this without a test, or try to figure out how to write a test. I’m thinking “change the behavior back” maybe? > > I think "change the behavior back" sounds right. Given how big this is, separating out behavior change into a separate commit seems like a good idea. Done. Will land once EWS and I are convinced the patch is ready.
Darin Adler
Comment 30 2018-07-24 21:38:07 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 31 2018-07-24 21:40:57 PDT Comment hidden (obsolete)
Darin Adler
Comment 32 2018-07-25 21:06:50 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 33 2018-07-25 21:09:53 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 34 2018-07-25 22:33:11 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 35 2018-07-25 22:33:13 PDT Comment hidden (obsolete)
Darin Adler
Comment 36 2018-07-25 22:39:55 PDT
Created attachment 345824 [details] Patch
EWS Watchlist
Comment 37 2018-07-25 22:40:09 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 38 2018-07-25 22:40:11 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 39 2018-07-25 22:42:30 PDT
Attachment 345824 [details] did not pass style-queue: ERROR: Source/WebCore/html/canvas/WebGPUTexture.cpp:47: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/html/canvas/WebGPUCommandBuffer.cpp:50: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/html/canvas/WebGPUCommandBuffer.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/html/canvas/WebGPUBuffer.cpp:43: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/metal/GPUCommandBufferMetal.mm:42: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/html/canvas/WebGPUCommandQueue.cpp:43: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/html/canvas/WebGPURenderCommandEncoder.cpp:49: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/metal/GPUTextureDescriptorMetal.mm:37: Missing spaces around : [whitespace/init] [4] ERROR: Source/WebCore/platform/graphics/metal/GPUTextureDescriptorMetal.mm:38: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/html/canvas/WebGPUDepthStencilState.cpp:43: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/html/canvas/WebGPURenderPipelineState.cpp:43: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/html/canvas/WebGPUDrawable.cpp:44: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/metal/GPUFunctionMetal.mm:39: Missing spaces around : [whitespace/init] [4] ERROR: Source/WebCore/html/canvas/WebGPUFunction.cpp:43: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/html/canvas/WebGPURenderPipelineColorAttachmentDescriptor.cpp:40: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/metal/GPUDrawableMetal.mm:40: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/html/canvas/WebGPULibrary.cpp:43: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/metal/GPUComputeCommandEncoderMetal.mm:47: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WebCore/html/canvas/WebGPUComputePipelineState.cpp:40: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/metal/GPUDepthStencilStateMetal.mm:40: Missing spaces around : [whitespace/init] [4] ERROR: Source/WebCore/html/canvas/WebGPUComputeCommandEncoder.cpp:53: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/metal/GPURenderPipelineStateMetal.mm:40: Missing spaces around : [whitespace/init] [4] Total errors found: 22 in 110 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 40 2018-07-26 08:43:04 PDT
Committed r234258: <https://trac.webkit.org/changeset/234258>
Radar WebKit Bug Importer
Comment 41 2018-07-26 08:45:19 PDT
<rdar://problem/42625316>
Note You need to log in before you can comment on or make changes to this bug.