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

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied 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 145316: Patch
https://bugs.webkit.org/attachment.cgi?id=145316&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
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