Bug 143236 – Web Inspector: add console messages for WebGL shader compile and program link errors/warnings

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

InspectorCanvasAgent should use ConsoleAgent for WebGL shader and program errors/warnings. Create a new message source (MessageSource::WebGL) for logging compile and link operations, including source code location to allow jumping to the relevant code in a ShaderProgram content view (needed for https://bugs.webkit.org/show_bug.cgi?id=138593).

Going through the ConsoleAgent guarantees users are informed of compile and link information even if they aren't currently inspecting the canvas resource.

[Radar WebKit Bug Importer]

<rdar://problem/20352149>

[Devin Rousso]

Created attachment 313306 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 313307 [details]
[Patch] WIP

Need r? for the bots to run the tests.  Trying to find tests that fail :|

[Build Bot]

Attachment 313307 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PageConsoleClient.h:35:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5741:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5742:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:5:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 5 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 313310 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 313310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313310&action=review

Looks good to me but we should probably have a WebGL reviewer take a look.

> LayoutTests/fast/canvas/webgl/shader-compile-logging-expected.txt:2
> +CONSOLE MESSAGE: line 19: WebGL: ERROR: 0:2: 'attribute' :  supported in vertex shaders only 
> +CONSOLE MESSAGE: line 19: WebGL: ERROR: 0:2: '' : No precision specified for (float)

Its a bit unfortunate that we have the 0 part of "0:#". 0 here is the "file" which I think will always be zero for us. However we match other browsers...

> Source/WebCore/ChangeLog:26
> +        Unify console logging helper functions to all follow the same path.  Additionally, errors
> +        now generate stack traces.  Shader compilation errors are now logged as well.

Nit: One space after periods.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:-104
> -const int maxGLErrorsAllowedToConsole = 256;

Lets keep this given what we talked about earlier.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1309
> +        Ref<Inspector::ScriptCallStack> stackTrace = Inspector::createScriptCallStack(JSMainThreadExecState::currentState(), Inspector::ScriptCallStack::maxCallStackSizeToCapture);

I wonder if there is a better ExecState we can use than JSMainThreadExecState::currentState(). I think its fine for now, as long as WebGL is only usable from the main thread. But if this were to change in the future then this could cause mysterious issues.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:745
> +    // Helper function to print errors ang  warnings to console.

Typos.

> Source/WebCore/page/PageConsoleClient.h:37
> +class ScriptCallStack;

By forward declaring this were you going to try to remove the include? Otherwise remove the forward declaration.

> Source/WebCore/page/PageConsoleClient.h:61
> +    void addMessage(std::unique_ptr<Inspector::ConsoleMessage>&&);

Lets add a comment that the others are deprecated. Callers should try to create the ConsoleMessage themselves. Perhaps then we can slowly phase out the super specific versions below.

[Devin Rousso]

Created attachment 313381 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 313381 [details]
Patch

Clearing flags on attachment: 313381

Committed r218603: <http://trac.webkit.org/changeset/218603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.