Bug 91823 – [chromium] Clean up LayerRendererChromium::drawTexturedQuad

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] Clean up LayerRendererChromium::drawTexturedQuad

[Adrienne Walker]

Created attachment 153427 [details]
Patch

[Adrienne Walker]

Created attachment 154515 [details]
Rebased

[Adrienne Walker]

Anybody feel like reviewing this? Officially? Unofficially? *crickets*

[Dana Jansens]

Comment on attachment 154515 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=154515&action=review

A few little comments, looks good overall!

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:749
> +    if (useAA)
> +        setShaderFloatQuad(surfaceQuad, shaderQuadLocation);

if useAA is false, then shaderQuadLocation should be -1? Can we do the if inside setShaderFloatQuad, the same as we do with setShaderOpacity?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:944
> +    setShaderFloatQuad(localQuad, uniforms.pointLocation);

The drawTexturedQuad would handle pointLocation == -1 but setShaderFloatQuad doesn't, more reason to have setShaderFloatQuad do that check/early-out?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:947
> +    FloatRect centeredRect(FloatPoint(-0.5 * tileRect.width(), -0.5 * tileRect.height()), tileRect.size());
> +    drawQuadGeometry(frame, quad->quadTransform(), centeredRect, uniforms.matrixLocation);

FIXME to make quadTransform() consistent with other quad types?

[Adrienne Walker]

Comment on attachment 154515 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=154515&action=review

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:749
>> +        setShaderFloatQuad(surfaceQuad, shaderQuadLocation);
> 
> if useAA is false, then shaderQuadLocation should be -1? Can we do the if inside setShaderFloatQuad, the same as we do with setShaderOpacity?

Sure.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:944
>> +    setShaderFloatQuad(localQuad, uniforms.pointLocation);
> 
> The drawTexturedQuad would handle pointLocation == -1 but setShaderFloatQuad doesn't, more reason to have setShaderFloatQuad do that check/early-out?

It's always used for the tile shader, for what it's worth.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:947
>> +    drawQuadGeometry(frame, quad->quadTransform(), centeredRect, uniforms.matrixLocation);
> 
> FIXME to make quadTransform() consistent with other quad types?

This is only because of the way the shader works.  It uses these bounds as a way to figure out how big the texture is supposed to be before applying AA to it.  I don't know if there's really anything to fix? I could add a comment to make it more clear that this is shader is special and behaves unexpectedly with respect to its geometry/transfom inputs.

[Dana Jansens]

Comment on attachment 154515 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=154515&action=review

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:947
>>> +    drawQuadGeometry(frame, quad->quadTransform(), centeredRect, uniforms.matrixLocation);
>> 
>> FIXME to make quadTransform() consistent with other quad types?
> 
> This is only because of the way the shader works.  It uses these bounds as a way to figure out how big the texture is supposed to be before applying AA to it.  I don't know if there's really anything to fix? I could add a comment to make it more clear that this is shader is special and behaves unexpectedly with respect to its geometry/transfom inputs.

Oh interesting.. thanks for the explanation. A comment might be nice for when I forget this again in a couple months :)

[Adrienne Walker]

Created attachment 155024 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 155024 [details]
Patch for landing

Clearing flags on attachment: 155024

Committed r123926: <http://trac.webkit.org/changeset/123926>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.