Bug 175223 - Web Inspector: tint all pixels drawn by shader program when hovering ShaderProgramTreeElement
Summary: Web Inspector: tint all pixels drawn by shader program when hovering ShaderPr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 138593 178744
Blocks: WebInspectorCanvasTab
  Show dependency treegraph
 
Reported: 2017-08-04 15:52 PDT by Devin Rousso
Modified: 2018-03-30 22:14 PDT (History)
19 users (show)

See Also:


Attachments
Patch (31.68 KB, patch)
2017-08-10 00:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (1.59 MB, image/png)
2017-08-10 00:44 PDT, Devin Rousso
no flags Details
Patch (36.89 KB, patch)
2017-08-21 20:36 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.17 MB, application/zip)
2017-08-21 21:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.28 MB, application/zip)
2017-08-21 21:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.98 MB, application/zip)
2017-08-21 21:53 PDT, Build Bot
no flags Details
Patch (36.95 KB, patch)
2017-08-21 21:55 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.90 KB, patch)
2017-08-22 12:25 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.56 KB, patch)
2017-08-22 14:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.49 KB, patch)
2017-08-22 14:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (2.08 MB, application/zip)
2017-08-22 18:06 PDT, Build Bot
no flags Details
[Image] Alternative blue highlight (1.56 MB, image/png)
2017-08-28 11:05 PDT, Devin Rousso
no flags Details
Still need to make a test for if a depth/stencil buffer is attached (36.56 KB, patch)
2017-08-29 16:48 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.55 MB, application/zip)
2017-08-29 18:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (2.28 MB, application/zip)
2017-08-29 18:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.69 MB, application/zip)
2017-08-29 18:42 PDT, Build Bot
no flags Details
Still need to make a test for if a depth/stencil buffer is attached (36.87 KB, patch)
2017-08-29 18:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.14 KB, patch)
2017-09-06 23:34 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.30 KB, patch)
2018-02-20 18:37 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.38 MB, application/zip)
2018-02-20 19:24 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.88 MB, application/zip)
2018-02-20 19:28 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.13 MB, application/zip)
2018-02-20 19:54 PST, EWS Watchlist
no flags Details
Patch (36.64 KB, patch)
2018-02-21 00:35 PST, Devin Rousso
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.21 MB, application/zip)
2018-02-21 01:38 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.57 MB, application/zip)
2018-02-21 01:44 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (2.97 MB, application/zip)
2018-02-21 02:02 PST, EWS Watchlist
no flags Details
Patch (36.64 KB, patch)
2018-02-21 03:06 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.69 KB, patch)
2018-03-30 20:38 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 Devin Rousso 2017-08-04 15:52:29 PDT
This would help visually distinguish multiple shader programs for a single canvas.
Comment 1 Devin Rousso 2017-08-10 00:43:43 PDT
Created attachment 317791 [details]
Patch
Comment 2 Devin Rousso 2017-08-10 00:44:01 PDT
Created attachment 317792 [details]
[Image] After Patch is applied
Comment 3 Matt Baker 2017-08-11 19:39:59 PDT
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.
Comment 4 Devin Rousso 2017-08-21 20:36:03 PDT
Created attachment 318727 [details]
Patch
Comment 5 Build Bot 2017-08-21 21:42:58 PDT
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
Comment 6 Build Bot 2017-08-21 21:42:59 PDT
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 7 Build Bot 2017-08-21 21:47:55 PDT
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
Comment 8 Build Bot 2017-08-21 21:47:56 PDT
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 9 Build Bot 2017-08-21 21:53:01 PDT
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
Comment 10 Build Bot 2017-08-21 21:53:03 PDT
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
Comment 11 Devin Rousso 2017-08-21 21:55:49 PDT
Created attachment 318735 [details]
Patch
Comment 12 Devin Rousso 2017-08-22 12:25:25 PDT
Created attachment 318782 [details]
Patch
Comment 13 Joseph Pecoraro 2017-08-22 13:20:29 PDT
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.
Comment 14 Devin Rousso 2017-08-22 14:02:16 PDT
Created attachment 318796 [details]
Patch
Comment 15 Devin Rousso 2017-08-22 14:08:57 PDT
Created attachment 318797 [details]
Patch
Comment 16 Build Bot 2017-08-22 18:06:13 PDT
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
Comment 17 Build Bot 2017-08-22 18:06:14 PDT
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 18 Matt Baker 2017-08-23 09:57:32 PDT
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 19 Joseph Pecoraro 2017-08-23 10:50:33 PDT
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.
Comment 20 Radar WebKit Bug Importer 2017-08-23 12:09:09 PDT
<rdar://problem/34040691>
Comment 21 Devin Rousso 2017-08-23 12:16:02 PDT
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 22 Matt Baker 2017-08-23 12:57:51 PDT
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!
Comment 23 Devin Rousso 2017-08-28 11:05:06 PDT
Created attachment 319183 [details]
[Image] Alternative blue highlight
Comment 24 Devin Rousso 2017-08-29 16:48:14 PDT
Created attachment 319307 [details]
Still need to make a test for if a depth/stencil buffer is attached
Comment 25 Build Bot 2017-08-29 18:41:23 PDT
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
Comment 26 Build Bot 2017-08-29 18:41:25 PDT
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 27 Build Bot 2017-08-29 18:41:41 PDT
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
Comment 28 Build Bot 2017-08-29 18:41:42 PDT
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 29 Build Bot 2017-08-29 18:42:52 PDT
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
Comment 30 Build Bot 2017-08-29 18:42:54 PDT
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
Comment 31 Devin Rousso 2017-08-29 18:46:04 PDT
Created attachment 319322 [details]
Still need to make a test for if a depth/stencil buffer is attached
Comment 32 Devin Rousso 2017-09-06 23:34:52 PDT
Created attachment 320097 [details]
Patch

Changed some repeated code to use a macro
Comment 33 Brian Burg 2017-10-25 10:51:07 PDT
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.
Comment 34 Devin Rousso 2018-02-20 18:37:13 PST
Created attachment 334331 [details]
Patch

Rebase now that shaders are visible again
Comment 35 EWS Watchlist 2018-02-20 19:24:23 PST
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
Comment 36 EWS Watchlist 2018-02-20 19:24:25 PST
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 37 EWS Watchlist 2018-02-20 19:28:02 PST
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
Comment 38 EWS Watchlist 2018-02-20 19:28:04 PST
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 39 EWS Watchlist 2018-02-20 19:54:26 PST
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
Comment 40 EWS Watchlist 2018-02-20 19:54:28 PST
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
Comment 41 Devin Rousso 2018-02-21 00:35:42 PST
Created attachment 334344 [details]
Patch
Comment 42 EWS Watchlist 2018-02-21 01:38:32 PST
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
Comment 43 EWS Watchlist 2018-02-21 01:38:34 PST
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 44 EWS Watchlist 2018-02-21 01:44:23 PST
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
Comment 45 EWS Watchlist 2018-02-21 01:44:25 PST
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 46 EWS Watchlist 2018-02-21 02:01:51 PST
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
Comment 47 EWS Watchlist 2018-02-21 02:02:03 PST
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
Comment 48 Devin Rousso 2018-02-21 03:06:10 PST
Created attachment 334356 [details]
Patch
Comment 49 Matt Baker 2018-03-30 18:29:44 PDT
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.
Comment 50 Devin Rousso 2018-03-30 20:38:29 PDT
Created attachment 336905 [details]
Patch
Comment 51 WebKit Commit Bot 2018-03-30 22:14:54 PDT
Comment on attachment 336905 [details]
Patch

Clearing flags on attachment: 336905

Committed r230127: <https://trac.webkit.org/changeset/230127>
Comment 52 WebKit Commit Bot 2018-03-30 22:14:56 PDT
All reviewed patches have been landed.  Closing bug.