[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


Adrienne Walker <enne at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #145316|review?                     |review-
               Flag|                            |




--- Comment #35 from Adrienne Walker <enne at google.com>  2012-06-01 15:13:18 PST ---
(From update of attachment 145316)
View in context: https://bugs.webkit.org/attachment.cgi?id=145316&action=review

Calculating scissor rects along with visibility rather than during drawing seems great.  (Although those two calculate functions really should eventually be combined...)  I like the code changes here a lot, but have some questions about your tests.

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:390
> +        /* empty list */

nit: Don't use C-style comments unless it's multiline.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1217
> +        // 0 is the hardcoded return value of getUniformLocation
> +        // 1 is count ???
> +        // Boolean cast to str??
> +        // float* values
> +        EXPECT_CALL(*m_context, uniformMatrix4fv(0, 1, _, _))
> +            .WillOnce(Return())
> +            .RetiresOnSaturation();
> +
> +        // 0 is the hardcoded return value of getUniformLocation
> +        // 3 floats are color / 255
> +        // 1 is Alpha
> +        EXPECT_CALL(*m_context, uniform4f(0, _, _, _, 1))
> +            .WillOnce(Return())
> +            .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?

> 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.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1270
> +    harness.mustSetScissor(0, 0, 10, 10);
> +    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?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1336
> +static PassOwnPtr<CCLayerTreeHostImpl> setupLayersForOpacity(bool partialSwap, CCLayerTreeHostImplClient* client)
> +{
> +    CCSettings settings;
> +    settings.partialSwapEnabled = true;

What's the partialSwap parameter supposed to be doing? Are the tests using this wrong?

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

What does this test do that viewportCovered doesn't do?

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

nit: useless whitespace

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