Bug 222593 - Adding new test conditions for WebGL should be simpler
Summary: Adding new test conditions for WebGL should be simpler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks: 222546
  Show dependency treegraph
 
Reported: 2021-03-02 04:50 PST by Kimmo Kinnunen
Modified: 2021-03-04 02:49 PST (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 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.
Comment 1 Kimmo Kinnunen 2021-03-02 05:45:18 PST
Created attachment 421925 [details]
Patch
Comment 2 Kenneth Russell 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?
Comment 3 Kimmo Kinnunen 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
?
Comment 4 Kenneth Russell 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.
Comment 5 Kimmo Kinnunen 2021-03-03 11:03:42 PST
Created attachment 422125 [details]
Patch
Comment 6 Kimmo Kinnunen 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?
Comment 7 Kenneth Russell 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.
Comment 8 EWS 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].
Comment 9 Radar WebKit Bug Importer 2021-03-04 01:01:15 PST
<rdar://problem/75025136>