[webkit-reviews] review denied: [Bug 71920] [chromium] CCThreadProxy::finishAllRendering hangs if !visible : [Attachment 114481] Renamed to setNeedsForcedRedraw, cleaned up test

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Iain Merrick
<husky at google.com>'s request for review:
Bug 71920: [chromium] CCThreadProxy::finishAllRendering hangs if !visible
https://bugs.webkit.org/show_bug.cgi?id=71920

Attachment 114481: Renamed to setNeedsForcedRedraw, cleaned up test
https://bugs.webkit.org/attachment.cgi?id=114481&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114481&action=review


I don't see any tests that verify that if visible = false and
setNeedsForcedRedraw = true then we actually draw, which seems like the most
valuable thing to test since that's the new functionality for this patch. 
Other than the tests, though, this looks great.

> Source/WebKit/chromium/tests/CCSchedulerStateMachineTest.cpp:129
> +	   // We should get exactly the same results in three scenarios:
> +	   //  0. visible=true, setNeedsRedraw=true
> +	   //  1. visible=true, setNeedsForcedRedraw=true
> +	   //  2. visible=false, setNeedsForcedRedraw=true
> +	   // The final case is covered by
TestNoCommitStatesRedrawWhenInvisible.
> +	   for (size_t j = 0; j < 3; ++j) {
> +	       state.setNeedsRedraw(true);
> +	       state.setNeedsForcedRedraw(j >= 1);
> +	       state.setVisible(j <= 1);

I like trying to hit all the states but this is pretty hard to read.  A better
way might be to have an array of structs each of which has the desired value
for visible/setNeedsRedraw/setNeedsForcedRedraw and then iterate through that
array.