[webkit-reviews] review granted: [Bug 175223] Web Inspector: tint all pixels drawn by shader program when hovering ShaderProgramTreeElement : [Attachment 334356] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 175223: Web Inspector: tint all pixels drawn by shader program when
hovering ShaderProgramTreeElement
https://bugs.webkit.org/show_bug.cgi?id=175223

Attachment 334356: Patch

https://bugs.webkit.org/attachment.cgi?id=334356&action=review




--- Comment #49 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 334356
  --> https://bugs.webkit.org/attachment.cgi?id=334356
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.