Bug 143236 - Web Inspector: add console messages for WebGL shader compile and program link errors/warnings
Summary: Web Inspector: add console messages for WebGL shader compile and program link...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
Keywords: InRadar
Depends on:
Reported: 2015-03-30 13:52 PDT by Matt Baker
Modified: 2017-06-20 11:35 PDT (History)
17 users (show)

See Also:

[Patch] WIP (20.26 KB, patch)
2017-06-19 09:42 PDT, Devin Rousso
hi: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (20.26 KB, patch)
2017-06-19 09:51 PDT, Devin Rousso
hi: commit-queue-
Details | Formatted Diff | Diff
Patch (24.03 KB, patch)
2017-06-19 11:00 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.71 KB, patch)
2017-06-20 01:05 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2015-03-30 13:52:49 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-03-30 13:53:06 PDT
Comment 2 Devin Rousso 2017-06-19 09:42:03 PDT
Created attachment 313306 [details]
[Patch] WIP
Comment 3 Devin Rousso 2017-06-19 09:51:19 PDT
Created attachment 313307 [details]
[Patch] WIP

Need r? for the bots to run the tests.  Trying to find tests that fail :|
Comment 4 Build Bot 2017-06-19 09:53:07 PDT
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.
Comment 5 Devin Rousso 2017-06-19 11:00:17 PDT
Created attachment 313310 [details]
Comment 6 Joseph Pecoraro 2017-06-19 12:30:59 PDT
Comment on attachment 313310 [details]

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.


> 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.
Comment 7 Devin Rousso 2017-06-20 01:05:51 PDT
Created attachment 313381 [details]
Comment 8 WebKit Commit Bot 2017-06-20 11:35:35 PDT
Comment on attachment 313381 [details]

Clearing flags on attachment: 313381

Committed r218603: <http://trac.webkit.org/changeset/218603>
Comment 9 WebKit Commit Bot 2017-06-20 11:35:37 PDT
All reviewed patches have been landed.  Closing bug.