Bug 222593

Summary:

Adding new test conditions for WebGL should be simpler

Product:

WebKit

Reporter:

Kimmo Kinnunen <kkinnunen>

Component:

WebGL

Assignee:

Kimmo Kinnunen <kkinnunen>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, changseok, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, kbr, kondapallykalyan, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

222546

Attachments:

Description	Flags
Patch	none
Patch	ews-feeder: commit-queue-

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.