[webkit-reviews] review denied: [Bug 82071] [chromium] Improved composited debug borders : [Attachment 133521] 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 82071: [chromium] Improved composited debug borders
https://bugs.webkit.org/show_bug.cgi?id=82071

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

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


> Source/WebCore/ChangeLog:13
> +	   of its debug quad. Reverse the ordering when appening to fix this
> +	   .

s/appening to fix this\n./appending to fix it./

Nice comment justification.  ;)

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:67
> -    layer->appendQuads(quadCuller, sharedQuadState.get(), usedCheckerboard);

>      layer->appendDebugBorderQuad(quadCuller, sharedQuadState.get());
> +    layer->appendQuads(quadCuller, sharedQuadState.get(), usedCheckerboard);


See! Things implicitly being in reverse order is confusing.  ;)

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:159
> +		   if (hasDebugBorders()) {
> +		       Color color(debugTileBorderMissingTileRed,
debugTileBorderMissingTileGreen, debugTileBorderMissingTileBlue,
debugTileBorderAlpha);
> +		      
quadList.append(CCDebugBorderDrawQuad::create(sharedQuadState, tileRect, color,
debugTileBorderWidth));
> +		   }

This doesn't seem right.  This will interleave missing texture debug quads with
normal quads, drawing over some of them.

Change line 154 from "if (!tile || !tile->textureId()) {" to "if (!tile ||
!tile->textureId() || ((i + j) % 2) {" or "if (true) {" and you'll see what I
mean.