Bug 169371 – WebGPU: Backend - Library and Functions

• Component: WebGL
• Status: RESOLVED FIXED

[Dean Jackson]

Implement the backend part of loading a shader library and exposing functions.

[Dean Jackson]

<rdar://problem/30928792>

[Dean Jackson]

Created attachment 303884 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:37:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:40:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:43:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:46:  Missing space after ,  [whitespace/comma] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:46:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:47:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:51:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUTest.h:54:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUFunction.mm:57:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPUFunction.mm:61:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPULibrary.mm:65:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPULibrary.mm:79:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/mac/GPULibrary.mm:80:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
Total errors found: 13 in 16 files


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

[Tim Horton]

Comment on attachment 303884 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/GPUFunctionMetal.mm:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

2015?

> Source/WebCore/platform/graphics/cocoa/GPULibraryMetal.mm:45
> +    NSError* error = [NSError errorWithDomain:@"com.apple.WebKit.GPU" code:1 userInfo:nil];

All of the objc stars are on the wrong side everywhere :P

Also, what is this odd error domain with a weird name and that isn't in a header. And the codes should be an enum, probably

> Source/WebCore/platform/graphics/gpu/GPUFunction.cpp:52
> +GPUFunction::GPUFunction(GPULibrary* library, const String& name)

Maybe the Metal thing should be a subclass that you make on the platforms where you can, instead? Instead of polluting the cross-platform class (esp. the header) with all sort of metaly things. And then you could pure-virtualize some of these things and not provide base implementations that will never be useful to anyone.

> Tools/TestWebKitAPI/Tests/WebCore/mac/GPUFunction.mm:26
> +#ifdef ENABLE_WEBGPU

Can we not use proper #if ENABLE(WEBGPU) here, if you put it below config.h?

[Dean Jackson]

Comment on attachment 303884 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/GPULibraryMetal.mm:45
>> +    NSError* error = [NSError errorWithDomain:@"com.apple.WebKit.GPU" code:1 userInfo:nil];
> 
> All of the objc stars are on the wrong side everywhere :P
> 
> Also, what is this odd error domain with a weird name and that isn't in a header. And the codes should be an enum, probably

I couldn't see any consistency with what we do with NSErrors. And I don't actually use it.

>> Source/WebCore/platform/graphics/gpu/GPUFunction.cpp:52
>> +GPUFunction::GPUFunction(GPULibrary* library, const String& name)
> 
> Maybe the Metal thing should be a subclass that you make on the platforms where you can, instead? Instead of polluting the cross-platform class (esp. the header) with all sort of metaly things. And then you could pure-virtualize some of these things and not provide base implementations that will never be useful to anyone.

I generally agree, but decided not to do it.... at least for now. Mostly because I didn't want to virtualize everything. But I think it makes more sense, so I'll slowly convert them.

>> Tools/TestWebKitAPI/Tests/WebCore/mac/GPUFunction.mm:26
>> +#ifdef ENABLE_WEBGPU
> 
> Can we not use proper #if ENABLE(WEBGPU) here, if you put it below config.h?

We can. Thanks.

[Dean Jackson]

Committed r213650: <http://trac.webkit.org/changeset/213650>