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.
(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.
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.
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!
Created attachment 145342 [details] Patch
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?
(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.
Created attachment 145344 [details] Patch
James could you please review this? Thanks in advance.
Comment on attachment 145344 [details] Patch R=me
Comment on attachment 145344 [details] Patch Thanks!
Comment on attachment 145344 [details] Patch Clearing flags on attachment: 145344 Committed r119432: <http://trac.webkit.org/changeset/119432>
All reviewed patches have been landed. Closing bug.