This would help visually distinguish multiple shader programs for a single canvas.
Created attachment 317791 [details] Patch
Created attachment 317792 [details] [Image] After Patch is applied
Comment on attachment 317791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317791&action=review > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2006 > + highlightForInspector(); This could be replaced by an RAII object which saves the blend state and performs the highlight on construction. On destruction it would restore the previous blend mode. This would be safer, and remove Inspector code from WebGLRenderingContextBase: namespace Inspector { class ScopedShaderProgramHighlight { public: ScopedShaderProgramHighlight(WebGLRenderingContextBase& context, WebGLProgram* currentProgram) : m_context(context) , m_currentProgram(currentProgram) { highlightProgram(); } ~ScopedShaderProgramHighlight() { restoreContextState(); } private: void highlight() { if (!m_currentProgram) return; if (LIKELY(!m_currentProgram->highlightForInspector()) return; // Other frame buffer binding checks... saveContextState(); m_context.enable(GraphicsContext3D::BLEND); m_context.blendColor(1, 0, 0.25, 1); m_context.blendEquation(GraphicsContext3D::FUNC_ADD); m_context.blendFunc(GraphicsContext3D::CONSTANT_COLOR, GraphicsContext3D::ONE_MINUS_SRC_ALPHA); } void saveContextState() { m_blendBeforeHighlightForInspector.color = m_context.getWebGLFloatArrayParameter(GraphicsContext3D::BLEND_COLOR); // ... } void restoreContextState() { // Restore context if highlighting happened. } struct { RefPtr<Float32Array> color; unsigned equationRGB { 0 }; unsigned equationAlpha { 0 }; unsigned srcRGB { 0 }; unsigned srcAlpha { 0 }; unsigned dstRGB { 0 }; unsigned dstAlpha { 0 }; bool enabled { false }; } m_blendBeforeHighlightForInspector; WebGLRenderingContextBase& m_context; WebGLProgram* m_program { nullptr }; }; // namespace Inspector This would need to go in a nested scope, so that the blend mode is restored before `markContextChangedAndNotifyCanvasObserver()` is called: void WebGLRenderingContextBase::drawArrays(GC3Denum mode, GC3Dint first, GC3Dsizei count) { if (!validateDrawArrays("drawArrays", mode, first, count, 0)) return; { Inspector::ScopedShaderProgramHighlight scopedHighlight(*this, m_currentProgram); clearIfComposited(); bool vertexAttrib0Simulated = false; if (!isGLES2Compliant()) vertexAttrib0Simulated = simulateVertexAttrib0(first + count - 1); bool usesFallbackTexture = false; if (!isGLES2NPOTStrict()) usesFallbackTexture = checkTextureCompleteness("drawArrays", true); m_context->drawArrays(mode, first, count); if (!isGLES2Compliant() && vertexAttrib0Simulated) restoreStatesAfterVertexAttrib0Simulation(); if (usesFallbackTexture) checkTextureCompleteness("drawArrays", false); } markContextChangedAndNotifyCanvasObserver(); } The only thing that would make this inconvenient would be if it wasn't possible to make the other render buffer checks (stencil, etc) without accessing the context's private data. The RAII object could always be a friend of WebGLRenderingContextBase, but that's pretty gross.
Created attachment 318727 [details] Patch
Comment on attachment 318727 [details] Patch Attachment 318727 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4357992 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 318731 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 318727 [details] Patch Attachment 318727 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4358003 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 318732 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 318727 [details] Patch Attachment 318727 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4358002 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 318734 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 318735 [details] Patch
Created attachment 318782 [details] Patch
Comment on attachment 318782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318782&action=review r- because the build thing. I scanned the rest of the patch and it looked good to me. I think Matt / Dean would be the best reviewers. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:512 > + void clearHighlight() > + { > + if (!m_program || LIKELY(!InspectorInstrumentation::isShaderProgramHighlighted(m_context, *m_program))) > + return; Instead of doing this, which may be another hash lookup in InspectorCanvasAgent for a canvas we already highlighted, I think it would be best to just have a member `m_didApply` and you can bail here if `!m_didApply`. > Source/WebCore/inspector/InspectorInstrumentation.cpp:1057 > +<<<<<<< HEAD > bool InspectorInstrumentation::isShaderProgramDisabledImpl(InstrumentingAgents& instrumentingAgents, WebGLProgram& program) > { > if (InspectorCanvasAgent* canvasAgent = instrumentingAgents.inspectorCanvasAgent()) > return canvasAgent->isShaderProgramDisabled(program); > +======= Oops.
Created attachment 318796 [details] Patch
Created attachment 318797 [details] Patch
Comment on attachment 318797 [details] Patch Attachment 318797 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4365060 New failing tests: media/track/track-element-load-event.html
Created attachment 318839 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 318797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318797&action=review This looks great, however r- for now. We need tests that prove this won't highlight shaders that draw to depth/stencil buffers. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:467 > + Extra space. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:497 > + if (!WTF::holds_alternative<unsigned>(stencilAttachment) || WTF::get<unsigned>(depthAttachment) != static_cast<unsigned>(GraphicsContext3D::RENDERBUFFER)) Shouldn't this be WTF::get<unsigned>(stencilAttachment) != static_cast<unsigned>(GraphicsContext3D::RENDERBUFFER)? > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:499 > + } WebGL2 has an additional attachment type, DEPTH_STENCIL_ATTACHMENT. We should probably check that as well. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:82 > + highlight() For consistency with `hideHighlight`, this should be `showHighlight`. Actually, I'd just drop the underscore from `_setShaderProgramHighlighted` and make it public. These two methods add a level of indirection that isn't needed.
Comment on attachment 318797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318797&action=review >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:497 >> + if (!WTF::holds_alternative<unsigned>(stencilAttachment) || WTF::get<unsigned>(depthAttachment) != static_cast<unsigned>(GraphicsContext3D::RENDERBUFFER)) > > Shouldn't this be WTF::get<unsigned>(stencilAttachment) != static_cast<unsigned>(GraphicsContext3D::RENDERBUFFER)? Matt Baker is awarded 1 `Hawk Eye` award.
<rdar://problem/34040691>
Comment on attachment 318797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318797&action=review > This looks great, however r- for now. We need tests that prove this won't > highlight shaders that draw to depth/stencil buffers. I'm not sure I know how to do this. Do you know of any examples I could use? I'll try searching later, but I figured it can't hurt to ask :P >>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:497 >>> + if (!WTF::holds_alternative<unsigned>(stencilAttachment) || WTF::get<unsigned>(depthAttachment) != static_cast<unsigned>(GraphicsContext3D::RENDERBUFFER)) >> >> Shouldn't this be WTF::get<unsigned>(stencilAttachment) != static_cast<unsigned>(GraphicsContext3D::RENDERBUFFER)? > > Matt Baker is awarded 1 `Hawk Eye` award. Hot damn! >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:499 >> + } > > WebGL2 has an additional attachment type, DEPTH_STENCIL_ATTACHMENT. We should probably check that as well. I didn't know that! Good catch!
Comment on attachment 318797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318797&action=review >>>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:497 >>>> + if (!WTF::holds_alternative<unsigned>(stencilAttachment) || WTF::get<unsigned>(depthAttachment) != static_cast<unsigned>(GraphicsContext3D::RENDERBUFFER)) >>> >>> Shouldn't this be WTF::get<unsigned>(stencilAttachment) != static_cast<unsigned>(GraphicsContext3D::RENDERBUFFER)? >> >> Matt Baker is awarded 1 `Hawk Eye` award. > > Hot damn! Achievement unlocked!
Created attachment 319183 [details] [Image] Alternative blue highlight
Created attachment 319307 [details] Still need to make a test for if a depth/stencil buffer is attached
Comment on attachment 319307 [details] Still need to make a test for if a depth/stencil buffer is attached Attachment 319307 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4406925 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 319318 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 319307 [details] Still need to make a test for if a depth/stencil buffer is attached Attachment 319307 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4406882 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 319319 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 319307 [details] Still need to make a test for if a depth/stencil buffer is attached Attachment 319307 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4406923 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 319320 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 319322 [details] Still need to make a test for if a depth/stencil buffer is attached
Created attachment 320097 [details] Patch Changed some repeated code to use a macro
Comment on attachment 320097 [details] Patch I cannot review this as-is because shader programs are apparently not shown anywhere. Please reset r? flag when this is resolved.
Created attachment 334331 [details] Patch Rebase now that shaders are visible again
Comment on attachment 334331 [details] Patch Attachment 334331 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6598274 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 334333 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 334331 [details] Patch Attachment 334331 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6598278 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 334334 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 334331 [details] Patch Attachment 334331 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6598293 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 334336 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 334344 [details] Patch
Comment on attachment 334344 [details] Patch Attachment 334344 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6601193 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 334347 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 334344 [details] Patch Attachment 334344 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6601292 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 334348 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 334344 [details] Patch Attachment 334344 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6601317 New failing tests: inspector/canvas/setShaderProgramHighlighted.html
Created attachment 334350 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 334356 [details] Patch
Comment on attachment 334356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334356&action=review r=me, with one style comment. Really nice! > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:463 > +#define SAVE_BLEND_VALUE(name, attachment, type) WebGLAny name = m_context.getParameter(GraphicsContext3D::attachment); \ Macros should be avoided unless you really need to do text preprocessing. A private member function template has none of the pitfalls/dangers and is about as concise: template <typename T> void saveBlendValue(GC3Denum attachment, T& destination) { WebGLAny param = m_context.getParameter(attachment); if (WTF::holds_alternative<T>(param)) destination = WTF::get<T>(param); } void saveBlend() { saveBlendValue(GraphicsContext3D::BLEND_COLOR, m_savedBlend.color); ... } Or you could just drop the struct, I don't think you really need it: saveBlendValue(GraphicsContext3D::BLEND_COLOR, m_blendColor); saveBlendValue(GraphicsContext3D::BLEND_EQUATION_RGB, m_blendEquationAlpha); etc.
Created attachment 336905 [details] Patch
Comment on attachment 336905 [details] Patch Clearing flags on attachment: 336905 Committed r230127: <https://trac.webkit.org/changeset/230127>
All reviewed patches have been landed. Closing bug.