Summary: | [chromium] Clean up LayerRendererChromium::drawTexturedQuad | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrienne Walker <enne> | ||||||||
Component: | New Bugs | Assignee: | Adrienne Walker <enne> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cc-bugs, danakj, enne, jamesr, shawnsingh, vangelis, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 91815 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Adrienne Walker
2012-07-19 23:30:33 PDT
Created attachment 153427 [details]
Patch
Created attachment 154515 [details]
Rebased
Anybody feel like reviewing this? Officially? Unofficially? *crickets* 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? 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. 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 :) Created attachment 155024 [details]
Patch for landing
Comment on attachment 155024 [details] Patch for landing Clearing flags on attachment: 155024 Committed r123926: <http://trac.webkit.org/changeset/123926> All reviewed patches have been landed. Closing bug. |