[webkit-reviews] review denied: [Bug 87877] [chromium] Move drawing code for RenderSurfaces into LayerRendererChromium : [Attachment 144879] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 87877: [chromium] Move drawing code for RenderSurfaces into
LayerRendererChromium
https://bugs.webkit.org/show_bug.cgi?id=87877

Attachment 144879: Patch
https://bugs.webkit.org/attachment.cgi?id=144879&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=144879&action=review


> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:657
> +    // The replica is always drawn first, so free after drawing the
contents.
> +    bool shouldReleaseTextures = !quad->isReplica();

What if the contents surface quad is occluded/clipped but the replica is not?
How would this texture get unreserved? The previous behavior looks like we
always unreserved after the first render surface quad draws (possibly a
replica) and happily because nothing else reserved textures (causing potential
evictions), then those textures were still "valid" to use for the second render
surface quad even if those textures weren't reserved.  (Ack.) However, the
previous behavior seems mildly safer in that you couldn't ever have permanently
reserved surface textures.

How can this be better? Maybe you should unreserve all render surface textures
at the end of a frame? Do you need some sort of dummy quad that unreserves
textures? Can you check to see if the quads actually get added and set a
shouldReleaseTextures flag on them?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:680
> +    if (drawingSurface->hasValidContentsTexture()) {

Since you can't draw the background without a contents texture to draw into,
can you move this early out up above with the
contentsDeviceTransform.isInvertible check?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:67
> +    surface->appendQuads(quadCuller, sharedQuadState.get(), false,
surfaceDamageRect());

style nit: unnamed boolean function arg.  Can you name this isReplica still
like in the previous code or have two different appendQuads functions?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:47
> +static const int debugSurfaceBorderWidth = 2;

(We really need a CCColorConstants class... Not in this patch, though.)