[Webkit-unassigned] [Bug 87167] [chromium] Cleanup scissor rect computation/use with damage

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=87167





--- Comment #36 from zlieber at chromium.org  2012-06-01 19:27:39 PST ---
(From update of attachment 145316)
View in context: https://bugs.webkit.org/attachment.cgi?id=145316&action=review

Thanks; fixed all except where indicated

>> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:390
>> +        /* empty list */
> 
> nit: Don't use C-style comments unless it's multiline.

Actually didn't need this comment at all; removed.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1217
>> +            .RetiresOnSaturation();
> 
> Do you really need to expect all of these uniforms? This seems a bit overtested. Is drawElements and useProgram(1) enough to catch this?

Yes that would be enough; Moved them to be ignored.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1262
>> +    // Set up mock graphics context
> 
> I love all the comments you have in headers in this patch, but I feel like a lot of these tests are a bit over-commented.  You don't need to say what you're doing unless it's not obvious.

Removed more obvious comments

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1270
>> +    harness.mustSetScissor(0, 0, 10, 10);
> 
> Why is the scissor set twice? Can we not do that or at worst make the test not have to care about that implementation detail?

First is for clearing the surface, second for drawing the quads; added comment to that effect. It seems important to distinguish this in the tests, in the sense that either 3 calls or 1 would be wrong.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1336
>> +    settings.partialSwapEnabled = true;
> 
> What's the partialSwap parameter supposed to be doing? Are the tests using this wrong?

Good catch thanks. The parameter was not used, but it only affected test coverage, not correctness. This is because the expectations are identical in both cases.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1440
>> +TEST_F(CCLayerTreeHostImplTest, quadsToFillScreen)
> 
> What does this test do that viewportCovered doesn't do?

I see two differences - one that this one is using partial swap, and two that we have more surfaces. I also remember needing this to catch something, though cannot remember what right now.

>> Source/WebKit/chromium/tests/CCRenderSurfaceTest.cpp:116
>> +    
> 
> nit: useless whitespace

Fixed

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.