Bug 187333 - Improve WebGPU implementation, including using Metal Objective-C protocols more simply and correctly
Summary: Improve WebGPU implementation, including using Metal Objective-C protocols mo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-04 15:58 PDT by Darin Adler
Modified: 2018-07-26 08:45 PDT (History)
15 users (show)

See Also:

Attachments
Patch (274.61 KB, patch)
2018-07-04 17:03 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (274.86 KB, patch)
2018-07-04 17:23 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (274.94 KB, patch)
2018-07-04 18:15 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (283.80 KB, patch)
2018-07-04 19:18 PDT, Darin Adler 		sam: review+
Details | Formatted Diff | Diff
Patch (285.53 KB, patch)
2018-07-13 21:30 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (368.09 KB, patch)
2018-07-13 21:50 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (374.67 KB, patch)
2018-07-13 22:16 PDT, Darin Adler 		ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (375.54 KB, patch)
2018-07-25 21:06 PDT, Darin Adler 		ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2018-07-04 15:58:00 PDT 
Improve WebGPU implementation, including using Metal Objective-C protocols more simply and correctly
Comment 1 Darin Adler 2018-07-04 17:03:52 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 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.
Comment 3 Darin Adler 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.
Comment 4 EWS Watchlist 2018-07-04 17:05:32 PDT Comment hidden (obsolete)
Comment 5 Darin Adler 2018-07-04 17:23:29 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-07-04 17:25:36 PDT Comment hidden (obsolete)
Comment 7 Darin Adler 2018-07-04 18:15:05 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-07-04 18:16:40 PDT Comment hidden (obsolete)
Comment 9 Darin Adler 2018-07-04 19:18:56 PDT 
Created attachment 344307 [details]
Patch
Comment 10 EWS Watchlist 2018-07-04 19:20:47 PDT Comment hidden (obsolete)
Comment 11 Darin Adler 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?"
Comment 12 Darin Adler 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.
Comment 13 Darin Adler 2018-07-06 09:10:00 PDT 
Any takers for review?
Comment 14 Sam Weinig 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.
Comment 15 Sam Weinig 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.
Comment 16 Dean Jackson 2018-07-07 11:52:44 PDT 
Comment on attachment 344307 [details]
Patch

Thanks Darin!
Comment 17 Darin Adler 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.
Comment 18 Darin Adler 2018-07-13 21:30:08 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-07-13 21:32:00 PDT Comment hidden (obsolete)
Comment 20 Darin Adler 2018-07-13 21:50:48 PDT Comment hidden (obsolete)
Comment 21 Darin Adler 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.
Comment 22 Darin Adler 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?
Comment 23 EWS Watchlist 2018-07-13 21:52:49 PDT Comment hidden (obsolete)
Comment 24 Darin Adler 2018-07-13 22:16:05 PDT Comment hidden (obsolete)
Comment 25 EWS Watchlist 2018-07-13 22:18:50 PDT Comment hidden (obsolete)
Comment 26 EWS Watchlist 2018-07-14 00:20:51 PDT Comment hidden (obsolete)
Comment 27 EWS Watchlist 2018-07-14 00:21:04 PDT Comment hidden (obsolete)
Comment 28 Sam Weinig 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.
Comment 29 Darin Adler 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.
Comment 30 Darin Adler 2018-07-24 21:38:07 PDT Comment hidden (obsolete)
Comment 31 EWS Watchlist 2018-07-24 21:40:57 PDT Comment hidden (obsolete)
Comment 32 Darin Adler 2018-07-25 21:06:50 PDT Comment hidden (obsolete)
Comment 33 EWS Watchlist 2018-07-25 21:09:53 PDT Comment hidden (obsolete)
Comment 34 EWS Watchlist 2018-07-25 22:33:11 PDT Comment hidden (obsolete)
Comment 35 EWS Watchlist 2018-07-25 22:33:13 PDT Comment hidden (obsolete)
Comment 36 Darin Adler 2018-07-25 22:39:55 PDT 
Created attachment 345824 [details]
Patch
Comment 37 EWS Watchlist 2018-07-25 22:40:09 PDT Comment hidden (obsolete)
Comment 38 EWS Watchlist 2018-07-25 22:40:11 PDT Comment hidden (obsolete)
Comment 39 EWS Watchlist 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.
Comment 40 Darin Adler 2018-07-26 08:43:04 PDT 
Committed r234258: <https://trac.webkit.org/changeset/234258>
Comment 41 Radar WebKit Bug Importer 2018-07-26 08:45:19 PDT 
<rdar://problem/42625316>