Summary: | [chromium] CCDamageTrackerTest.verifyDamageForPerspectiveClippedLayer needs to be cleaner | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dana Jansens <danakj> | ||||||
Component: | New Bugs | Assignee: | Shawn Singh <shawnsingh> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | danakj, enne, jamesr, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 85241 | ||||||||
Attachments: |
|
Description
Dana Jansens
2012-04-30 16:29:06 PDT
(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. |