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

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied 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 318797: Patch

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




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