Bug 82071 – [chromium] Improved composited debug borders

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Improved composited debug borders

[Dana Jansens]

Created attachment 133521 [details]
Patch

[Dana Jansens]

http://i.imgur.com/DT3gX.png

[Adrienne Walker]

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.

[Vangelis Kokkevis]

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 ?

[Dana Jansens]

(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.

[Dana Jansens]

Created attachment 133539 [details]
Patch

[James Robinson]

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)

[Dana Jansens]

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

[Dana Jansens]

Created attachment 133545 [details]
Patch

Without static Color objects.

[Dana Jansens]

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.

[Adrienne Walker]

Comment on attachment 133545 [details]
Patch

Excellent.  Thanks for fixing this!

[WebKit Review Bot]

Comment on attachment 133545 [details]
Patch

Clearing flags on attachment: 133545

Committed r111909: <http://trac.webkit.org/changeset/111909>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.