[webkit-reviews] review denied: [Bug 81227] [chromium] Cull occluded surface quads : [Attachment 135131] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 2 18:40:50 PDT 2012


Adrienne Walker <enne at google.com> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 81227: [chromium] Cull occluded surface quads
https://bugs.webkit.org/show_bug.cgi?id=81227

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

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


> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:359
> +	   const RenderSurfaceType* targetSurface = m_stack.last().surface;

Maybe I am misunderstanding, but should this be secondLast.surface?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:393
> +    return unionRect(unoccludedSurfaceRect, unoccludedReplicaRect);

I see what you're doing here, but this is super weird.	It's weird to need to
draw part of either the surface or the replica because the other is unoccluded
and it's weird to have tests that this union is the correct result.

Could you maybe do one of:

(a) shave this yak and separate replicas into separate layers or even just
separate quads (in some separate CL)

(b) put the replica unoccluded rect somewhere separate so that when we do
complete (a) your tests don't have to be rewritten?

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:54
> +    if (drawQuad->material() == CCDrawQuad::RenderSurface)

Ew.  If we created different materials for render surfaces, I wouldn't want
occlusion to start failing.  The material property is how it should be drawn,
not what it is.

Can the caller pass this information in explicitly, e.g. refactor into
appendSurface/append/appendInternal?

> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:1041
>	   occlusion.leaveToTargetRenderSurface(parent->renderSurface());
> +
> +	   // There is nothing above child2's surface in the z-order.
> +	   EXPECT_EQ_RECT(IntRect(-10, 420, 70, 80),
occlusion.unoccludedContributingSurfaceContentRect(child2, IntRect(-10, 420,
70, 80)));

Should this expect call occur before the leaveToTargetRenderSurface? Why does
this not trigger the asserts in this function?


More information about the webkit-reviews mailing list