Summary: | WebGPU: Backend - Library and Functions | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||
Component: | WebGL | Assignee: | Dean Jackson <dino> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, dino, graouts, kondapallykalyan, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 167952 | ||||||
Attachments: |
|
Description
Dean Jackson
2017-03-08 13:27:10 PST
Created attachment 303884 [details]
Patch
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.
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? 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. Committed r213650: <http://trac.webkit.org/changeset/213650> |