Bug 175223 – Web Inspector: tint all pixels drawn by shader program when hovering ShaderProgramTreeElement

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

This would help visually distinguish multiple shader programs for a single canvas.

[Devin Rousso]

Created attachment 317791 [details]
Patch

[Devin Rousso]

Created attachment 317792 [details]
[Image] After Patch is applied

[Matt Baker]

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.

[Devin Rousso]

Created attachment 318727 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

Created attachment 318735 [details]
Patch

[Devin Rousso]

Created attachment 318782 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 318796 [details]
Patch

[Devin Rousso]

Created attachment 318797 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

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.

[Joseph Pecoraro]

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.

[Radar WebKit Bug Importer]

<rdar://problem/34040691>

[Devin Rousso]

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!

[Matt Baker]

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!

[Devin Rousso]

Created attachment 319183 [details]
[Image] Alternative blue highlight

[Devin Rousso]

Created attachment 319307 [details]
Still need to make a test for if a depth/stencil buffer is attached

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

Created attachment 319322 [details]
Still need to make a test for if a depth/stencil buffer is attached

[Devin Rousso]

Created attachment 320097 [details]
Patch

Changed some repeated code to use a macro

[Blaze Burg]

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.

[Devin Rousso]

Created attachment 334331 [details]
Patch

Rebase now that shaders are visible again

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Devin Rousso]

Created attachment 334344 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Devin Rousso]

Created attachment 334356 [details]
Patch

[Matt Baker]

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.

[Devin Rousso]

Created attachment 336905 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336905 [details]
Patch

Clearing flags on attachment: 336905

Committed r230127: <https://trac.webkit.org/changeset/230127>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.