Bug 82071 - [chromium] Improved composited debug borders
Summary: [chromium] Improved composited debug borders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-23 11:20 PDT by Dana Jansens
Modified: 2012-03-23 14:46 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.62 KB, patch)
2012-03-23 11:31 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (9.39 KB, patch)
2012-03-23 12:47 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (9.75 KB, patch)
2012-03-23 13:11 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-03-23 11:20:47 PDT
[chromium] Improved composited debug borders
Comment 1 Dana Jansens 2012-03-23 11:31:32 PDT
Created attachment 133521 [details]
Patch
Comment 2 Dana Jansens 2012-03-23 11:35:01 PDT
http://i.imgur.com/DT3gX.png
Comment 3 Adrienne Walker 2012-03-23 12:01:37 PDT
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 4 Vangelis Kokkevis 2012-03-23 12:14:12 PDT
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 ?
Comment 5 Dana Jansens 2012-03-23 12:46:02 PDT
(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.
Comment 6 Dana Jansens 2012-03-23 12:47:40 PDT
Created attachment 133539 [details]
Patch
Comment 7 James Robinson 2012-03-23 12:48:53 PDT
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)
Comment 8 Dana Jansens 2012-03-23 13:00:58 PDT
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
Comment 9 Dana Jansens 2012-03-23 13:11:12 PDT
Created attachment 133545 [details]
Patch

Without static Color objects.
Comment 10 Dana Jansens 2012-03-23 13:27:00 PDT
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 11 Adrienne Walker 2012-03-23 13:53:39 PDT
Comment on attachment 133545 [details]
Patch

Excellent.  Thanks for fixing this!
Comment 12 WebKit Review Bot 2012-03-23 14:46:15 PDT
Comment on attachment 133545 [details]
Patch

Clearing flags on attachment: 133545

Committed r111909: <http://trac.webkit.org/changeset/111909>
Comment 13 WebKit Review Bot 2012-03-23 14:46:20 PDT
All reviewed patches have been landed.  Closing bug.