Bug 85245 – [chromium] CCDamageTrackerTest.verifyDamageForPerspectiveClippedLayer needs to be cleaner

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

[Dana Jansens]

Claim:
    // The expected damage should cover the entire root surface (500x500), but we don't
    // care whether the damage rect was clamped or is larger than the surface for this test.
    FloatRect rootDamageRect = root->renderSurface()->damageTracker()->currentDamageRect();

But it tests the rootDamageRect's width/height only. The following fails:

    EXPECT_LE(rootDamageRect.x(), 0);
    EXPECT_LE(rootDamageRect.y(), 0);
    EXPECT_GE(rootDamageRect.maxX(), 500);
    EXPECT_GE(rootDamageRect.maxY(), 500);

It seems to actually end up with a damage rect that is very large and with its bottom right corner at 0,0. I am not sure if this is wrong, though it smells a bit weird to me. But the test does not seem to be testing what it actually claims - that the damage covers the root surface.

[Shawn Singh]

(In reply to comment #0)
> Claim:
>     // The expected damage should cover the entire root surface (500x500), but we don't
>     // care whether the damage rect was clamped or is larger than the surface for this test.
>     FloatRect rootDamageRect = root->renderSurface()->damageTracker()->currentDamageRect();
> 
> But it tests the rootDamageRect's width/height only. The following fails:
> 
>     EXPECT_LE(rootDamageRect.x(), 0);
>     EXPECT_LE(rootDamageRect.y(), 0);
>     EXPECT_GE(rootDamageRect.maxX(), 500);
>     EXPECT_GE(rootDamageRect.maxY(), 500);
> 
> It seems to actually end up with a damage rect that is very large and with its bottom right corner at 0,0. I am not sure if this is wrong, though it smells a bit weird to me. But the test does not seem to be testing what it actually claims - that the damage covers the root surface.


So I added those two lines, and it seems to work correctly.  Can you please double-check what it is you felt might be wrong?

In the mean-time, I'll use this bug to submit a quick and trivial patch to add those two lines, because I agree they should be there.

[Dana Jansens]

I added a printf and get this

[ RUN      ] CCDamageTrackerTest.verifyDamageForPerspectiveClippedLayer
rootDamageRect -15099999.000000 -10000000.000000 15099996.000000 10000000.000000

The damage rect is not intersecting with the root at all, as its maxY is 0.

[Shawn Singh]

OK, I looked at it in detail and found the following:

The test was testing what it needed to, so nothing was really wrong.  However, it was certainly confusing and misleading, and I don't understand why I had written it that way when I originally wrote it.

So I'm about to submit a patch that fixes it to be a little bit more intuitive (as much as this perspective stuff can be...)  and changed the bug title to reflect the actual problem.

Thanks Dana!

[Shawn Singh]

Created attachment 145342 [details]
Patch

[Dana Jansens]

Comment on attachment 145342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145342&action=review

Good stuff!

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:366
> +    FloatRect damageWeCareAbout = FloatRect(FloatPoint::zero(), FloatSize(500, 500));
> +    rootDamageRect.intersect(damageWeCareAbout);
> +    EXPECT_FLOAT_RECT_EQ(damageWeCareAbout, rootDamageRect);

What about EXPECT_TRUE(rootdamageRect.contains(rootRect))? I think that's what we're doing in 2 lines here?

[Shawn Singh]

(In reply to comment #5)
> (From update of attachment 145342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145342&action=review
> 
> Good stuff!
> 
> > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:366
> > +    FloatRect damageWeCareAbout = FloatRect(FloatPoint::zero(), FloatSize(500, 500));
> > +    rootDamageRect.intersect(damageWeCareAbout);
> > +    EXPECT_FLOAT_RECT_EQ(damageWeCareAbout, rootDamageRect);
> 
> What about EXPECT_TRUE(rootdamageRect.contains(rootRect))? I think that's what we're doing in 2 lines here?

sure, sounds even better.

[Shawn Singh]

Created attachment 145344 [details]
Patch

[Shawn Singh]

James could you please review this?  Thanks in advance.

[James Robinson]

Comment on attachment 145344 [details]
Patch

R=me

[Shawn Singh]

Comment on attachment 145344 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 145344 [details]
Patch

Clearing flags on attachment: 145344

Committed r119432: <http://trac.webkit.org/changeset/119432>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.