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

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> 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 318782: Patch

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




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 318782
  --> https://bugs.webkit.org/attachment.cgi?id=318782
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.