[webkit-reviews] review granted: [Bug 87167] [chromium] Cleanup scissor rect computation/use with damage : [Attachment 145444] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has granted zlieber at chromium.org's request
for review:
Bug 87167: [chromium] Cleanup scissor rect computation/use with damage
https://bugs.webkit.org/show_bug.cgi?id=87167

Attachment 145444: Patch
https://bugs.webkit.org/attachment.cgi?id=145444&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=145444&action=review


Thanks for the changes.  R=me, with two small changes:

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1275
> +    // One scissor for clearing surface, another for drawing the quads
> +    harness.mustSetScissor(0, 8, 2, 2);
> +    harness.mustSetScissor(0, 8, 2, 2);

Hmm.  I still think mustSetScissor should allow any number of redundant scissor
settings.  If we start shadowing state locally in LRC and not repeatedly
setting scissors, then this test would needlessly fail because of that
implementation detail.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1421
> +TEST_F(CCLayerTreeHostImplTest, quadsToFillScreen)

I still don't think this really adds anything over the other test (or occlusion
tests that have multiple surfaces) and should probably just get removed.  If
you're going to add a gutter quad test related to partial swap, then it should
probably be something where the scissor is smaller than the viewport and then
you verify that the quads fill the scissor.