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.
<rdar://problem/20352149>
Created attachment 313306 [details] [Patch] WIP
Created attachment 313307 [details] [Patch] WIP Need r? for the bots to run the tests. Trying to find tests that fail :|
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.
Created attachment 313310 [details] Patch
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.
Created attachment 313381 [details] Patch
Comment on attachment 313381 [details] Patch Clearing flags on attachment: 313381 Committed r218603: <http://trac.webkit.org/changeset/218603>
All reviewed patches have been landed. Closing bug.