[chromium] Improved composited debug borders
Created attachment 133521 [details] Patch
http://i.imgur.com/DT3gX.png
Comment on attachment 133521 [details] Patch 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.
Comment on attachment 133521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133521&action=review It would also be nice if tile boundaries have different colors from layer borders. Some times it's hard to tell apart tiles from 256x256 layers (e.g. in maps). > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:39 > +static const int debugSurfaceBorderWidth = 2; Couldn't these be initialized directly as WebCore::Color's ?
(In reply to comment #3) > (From update of attachment 133521 [details]) > 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./ +1 > See! Things implicitly being in reverse order is confusing. ;) Indeed :/ > > 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. Yeh, you're right, the left edge would get erased by the tile beside it. I moved the debug borders stuff up to the top so all borders are appended, then content tiles. (In reply to comment #4) > (From update of attachment 133521 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133521&action=review > > It would also be nice if tile boundaries have different colors from layer borders. Some times it's hard to tell apart tiles from 256x256 layers (e.g. in maps). Cyan! :) > > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:39 > > +static const int debugSurfaceBorderWidth = 2; > > Couldn't these be initialized directly as WebCore::Color's ? Yeh, I was trying to avoid the ire of reviewers for using static structs. I'll switch em to Colors.
Created attachment 133539 [details] Patch
Comment on attachment 133539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133539&action=review > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:42 > +static const Color debugSurfaceBorderColor(0, 0, 255); > +static const Color debugReplicaBorderColor(160, 0, 255); I believe these generate static initializers, so I'm going to go ahead and yell at you not to do this (or alternately confirm with the C++ standard that no static initializer is generated for this code)
Interesting..It seems like the Color class can be staticly initialized? http://en.wikipedia.org/wiki/C%2B%2B11#Modification_to_the_definition_of_plain_old_data
Created attachment 133545 [details] Patch Without static Color objects.
For posterity, I spoke with James. Color seems to fit the definition of POD in C++11, but not in C++03, so no go on static instances. And it is safer anyhow to not treat it like POD, since anyone could change the Color class and break our assumptions.
Comment on attachment 133545 [details] Patch Excellent. Thanks for fixing this!
Comment on attachment 133545 [details] Patch Clearing flags on attachment: 133545 Committed r111909: <http://trac.webkit.org/changeset/111909>
All reviewed patches have been landed. Closing bug.