Bug 222593 – Adding new test conditions for WebGL should be simpler

• Component: WebGL
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

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.

[Kimmo Kinnunen]

Created attachment 421925 [details]
Patch

[Kenneth Russell]

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]

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 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]

Created attachment 422125 [details]
Patch

[Kimmo Kinnunen]

(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 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]

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]

<rdar://problem/75025136>