[webkit-reviews] review denied: [Bug 85414] [chromium] Show borders for partial-draw-culled quads to visualize culling behaviour : [Attachment 139872] 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 85414: [chromium] Show borders for partial-draw-culled quads to visualize
culling behaviour
https://bugs.webkit.org/show_bug.cgi?id=85414

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

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


> Source/WebCore/ChangeLog:8
> +	   The borders are brown, and are only shown when the quad's visible
rect

(This is somewhat of an aside, but do you have any thoughts on how we can make
the meanings of our rainbow salad of border colors more clear? Maybe we could
put a link to a legend somewhere? Or even display a legend on the screen
somehow?)

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:68
> +    if (keepQuad) {

...so this only applies for partially culled quads and not fully culled ones?
Is that just to prevent too many debug borders on screen?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:68
> +    if (layer->hasDebugBorders())

(I keep thinking this should be a setting somewhere and not a per-layer flag. 
Or, better yet, a bag of settings so you could turn on the things you cared
about, since we seem to keep adding more and more border types.  Not in this
patch, I guess.)

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:91
> +	  
quadCuller.createCulledDebugBordersWithSharedQuadState(sharedQuadState.get());
>     
quadCuller.appendSurface(CCRenderSurfaceDrawQuad::create(sharedQuadState.get(),
surface->contentRect(), layer, surfaceDamageRect(), isReplica));
> +    quadCuller.createCulledDebugBordersWithSharedQuadState(0);

Whoa there, spooky action at a distance.  Also, this has bad ownership
issues--quads have naked pointers to shared quad state and the owner of the
shared quad state is CCRenderPass::m_sharedQuadStateList.  You can't just stick
any old pointer to an shared object state into a quad that you then delete.

Why can't the partially culled border quad re-use the same shared quad state
and just get that from the quad itself? It looks like you're using the border
shared quad state as a boolean.  Maybe CCQuadCuller::appendFoo should just take
an enum about whether or not to append culling quads, rather than depending on
the nullness of a member variable set elsewhere?