cc: remove flaky LayerTreeHostTestFinishAllRendering

cc: removeLayerTreeHostTestFinishAllRendering

This flaky test can be removed because it 
doesn't force any draw operation and it returns 
regardless of scheduler state.


R=danakj@chromium.org
BUG=373371
TEST=covered by existing test

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272048

[simonhong, 2014-05-19 08:11:16]

Dear dana,
I assumed that this test want to check whether FinishAllRendering() is called or
not.
So I removed SetNeedsRedraw() which causes race condition based on my
assumption.
Please correct me if the second SetNeedsRedraw() must be called.
Thanks!

[danakj, 2014-05-20 18:40:30]

https://codereview.chromium.org/290853003/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/290853003/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest.cc:1550: EXPECT_EQ(0, draw_count_);
Hm, before this test was verifying that FinishAllRendering() (when needs-redraw
is true) does not end up doing a draw.

After this change, the test doesn't really verify anything except that
DidCommitAndDrawFrame happens after DrawLayersOnThread, and that
FinishAllRendering doesn't crash.

This test was added in https://bugs.webkit.org/show_bug.cgi?id=86919 to verify
that we don't draw during FinishAllRendering to not have bad interaction with
acquire layer textures. The latter no longer exists, and I'm not sure how to
test this well without it being flaky, and the finish all rendering code path no
longer goes anywhere near drawing.

I propose we remove this test.

[danakj, 2014-05-20 18:40:41]

+piman who wrote the test

[piman, 2014-05-20 20:55:33]

On Tue, May 20, 2014 at 11:40 AM, <danakj@chromium.org> wrote:

> +piman who wrote the test
>
> https://codereview.chromium.org/290853003/
>

I think we can remove the test. We don't really care whether
FinishAllRendering interacts with Draw or not, per se. We care about 2
things:
1- FinishAllRendering doesn't force a draw if we can't draw (missing
resource or whatever). I don't think this can happen any more.
2- FinishAllRendering eventually returns, regardless of the current
scheduler state. This may be better served by scheduler unit tests.

Antoine

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-05-20 20:58:52]

On Tue, May 20, 2014 at 4:55 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Tue, May 20, 2014 at 11:40 AM, <danakj@chromium.org> wrote:
>
>> +piman who wrote the test
>>
>> https://codereview.chromium.org/290853003/
>>
>
> I think we can remove the test. We don't really care whether
> FinishAllRendering interacts with Draw or not, per se. We care about 2
> things:
> 1- FinishAllRendering doesn't force a draw if we can't draw (missing
> resource or whatever). I don't think this can happen any more.
> 2- FinishAllRendering eventually returns, regardless of the current
> scheduler state. This may be better served by scheduler unit tests.
>

Cool. It doesn't even interact with the scheduler, it just goes thru the
proxy to get to LTHI::FinishAllRendering() which calls the renderer
directly.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2014-05-20 22:17:37]

Thanks for sharing infos!
I removed this test and updated cl description.
PTAL again!

[danakj, 2014-05-20 22:18:09]

LGTM

[danakj, 2014-05-20 22:18:20]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-20 22:18:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonhong@chromium.org/290853003/20001

[commit-bot: I haz the power, 2014-05-21 20:02:29]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-22 02:49:46]

Change committed as 272048