WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
222593
Adding new test conditions for WebGL should be simpler
https://bugs.webkit.org/show_bug.cgi?id=222593
Summary
Adding new test conditions for WebGL should be simpler
Kimmo Kinnunen
Reported
2021-03-02 04:50:18 PST
Adding new test conditions for WebGL should be simpler It should be simpler to add test runner functions that change the environment so that some particular behaviour can be tested. Example:
bug 222546
needs to add a new flag telling the implementation to timeout calls to GPU Process. Current problems: - non-uniform names makes it hard to understand what part of the code is to support testing and what is not - many separate, non-uniform functions make it hard to catch all the functions which need to be disabled in non-test environments but enabled in normal operation - due to current WebGL graphics context implementation being separated in many hard-to-understand **GraphicsContextGL**.cpp/h files in various ports, it is hard to add a function correctly to all platforms.
Attachments
Patch
(25.00 KB, patch)
2021-03-02 05:45 PST
,
Kimmo Kinnunen
no flags
Details
Formatted Diff
Diff
Patch
(26.95 KB, patch)
2021-03-03 11:03 PST
,
Kimmo Kinnunen
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Kimmo Kinnunen
Comment 1
2021-03-02 05:45:18 PST
Created
attachment 421925
[details]
Patch
Kenneth Russell
Comment 2
2021-03-02 11:09:30 PST
Comment on
attachment 421925
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=421925&action=review
> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7563 > +void WebGLRenderingContextBase::simulateEventForTesting(const String& eventName)
Here and throughout - the use of strings and the fact that they're duplicated throughout the code base will make this error-prone to use and update. Would you consider defining either an enum or bitfield that can be referenced by all of the needed code, including in the GPU process?
Kimmo Kinnunen
Comment 3
2021-03-02 11:44:16 PST
I'd imagine enum would need to be duplicated in few different places spanning the software layer borders, so maybe it will then complicate more than simplify? E.g. it'd be hard to have the single enum declaration for JS/Internals.h to be #includable in the WebKit level. Should I just instead revert to method calls : simulateContextChangeForTesting simulateGPUStatusFailureForTesting simulateTimeoutForTesting ?
Kenneth Russell
Comment 4
2021-03-02 12:22:41 PST
Comment on
attachment 421925
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=421925&action=review
No, it's OK to move forward with this - I can see it'll significantly reduce boilerplate in the future. Couple of comments about documentation. r+
> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:381 > + WEBCORE_EXPORT void simulateEventForTesting(const String&);
Please add a comment documenting the valid inputs - or point to another header where they're documented.
> Source/WebCore/platform/graphics/GraphicsContextGL.h:1302 > + virtual void simulateEventForTesting(const String&) = 0;
Again, please point to documentation.
> Source/WebCore/testing/Internals.h:775 > + void simulateEventForWebGLContext(const String& eventName, WebGLRenderingContext&);
If this is the single place where these events should be documented then please add a comment here describing what they are.
> Source/WebCore/testing/Internals.idl:794 > + [Conditional=WEBGL] undefined simulateEventForWebGLContext(DOMString eventName, WebGLRenderingContext context);
Or here.
Kimmo Kinnunen
Comment 5
2021-03-03 11:03:42 PST
Created
attachment 422125
[details]
Patch
Kimmo Kinnunen
Comment 6
2021-03-03 11:04:48 PST
(In reply to Kenneth Russell from
comment #4
)
> Please add a comment documenting the valid inputs - or point to another > header where they're documented.
...
> Again, please point to documentation.
...
> If this is the single place where these events should be documented then > please add a comment here describing what they are.
...
> Or here.
Ok, sure, I see what you're doing! :) And you are right (of course). How about now?
Kenneth Russell
Comment 7
2021-03-03 11:11:34 PST
Comment on
attachment 422125
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=422125&action=review
Thanks for the update. Still looks good to me, one small continued comment. r+
> Source/WebCore/testing/Internals.idl:104 > + "GPUStatusFailure"
Will attempts to pass an invalid enum throw an exception? I think lowercase or hyphenated names would be better, and more inline with web standards conventions, because they're easier to remember.
EWS
Comment 8
2021-03-04 01:00:25 PST
Committed
r273877
: <
https://commits.webkit.org/r273877
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 422125
[details]
.
Radar WebKit Bug Importer
Comment 9
2021-03-04 01:01:15 PST
<
rdar://problem/75025136
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug