cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in
CommitComplete after we have drawn twice. Its been flaky
because of the mismatch in the number of times we commit and draw.
This CL fixes it.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/10a2c8a57326345ebe92c2805a85e1ba943f9852
Cr-Commit-Position: refs/heads/master@{#421995}

[jaydasika, 2016-09-27 22:44:32]

Description was changed from

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

BUG=598491
==========

to

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-09-27 22:45:13]

The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 22:45:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jaydasika, 2016-09-27 22:46:43]

jaydasika@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org

[jaydasika, 2016-09-27 22:46:43]

Was able to repro the flakiness locally and adding a
PostSetNeedsCommitToMainThread fixed it.

[commit-bot: I haz the power, 2016-09-27 23:02:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 23:02:42]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-09-27 23:18:04]

On 2016/09/27 22:46:43, jaydasika wrote:
> adding a PostSetNeedsCommitToMainThread fixed it.

Mention this in the CL description, that's the important part rather than the
re-enable IMO.

[danakj, 2016-09-27 23:18:15]

On 2016/09/27 23:18:04, danakj wrote:
> On 2016/09/27 22:46:43, jaydasika wrote:
> > adding a PostSetNeedsCommitToMainThread fixed it.
> 
> Mention this in the CL description, that's the important part rather than the
> re-enable IMO.

And maybe explain why it fixed it in the description too

[jaydasika, 2016-09-27 23:28:24]

Description was changed from

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in CommitComplete
after we have drawn twice. Its been flaky because we don't force a commit after
draw and so, may never reach the EndTest().
This CL forces the commit.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-09-27 23:28:47]

Description was changed from

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in CommitComplete
after we have drawn twice. Its been flaky because we don't force a commit after
draw and so, may never reach the EndTest().
This CL forces the commit.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in 
CommitComplete after we have drawn twice. Its been flaky because we 
don't force a commit after draw and so, may never reach the EndTest().
This CL forces the commit.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-09-27 23:29:07]

On 2016/09/27 23:18:15, danakj wrote:
> On 2016/09/27 23:18:04, danakj wrote:
> > On 2016/09/27 22:46:43, jaydasika wrote:
> > > adding a PostSetNeedsCommitToMainThread fixed it.
> > 
> > Mention this in the CL description, that's the important part rather than
the
> > re-enable IMO.
> 
> And maybe explain why it fixed it in the description too

Done

[danakj, 2016-09-27 23:32:18]

https://codereview.chromium.org/2376623004/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2376623004/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:1517: PostSetNeedsCommitToMainThread();
Oh thanks for the investigation! I have a suggested fix, as it sounds like this
test is flaky because it might draw a diff number of times than commits, and in
those cases I don't think it ends up testing the right things, tho at least it
won't time out now.

What if we do:
- Increment num_draws_ in DidCommitAndDrawFrame.
- Move EndTest() to DidCommitAndDrawFrame.
- Move the PostSetNeedsRedrawRect to DidCommitAndDrawFrame.
- Remove DrawLayersOnThread.

Would that fix and also eliminate weird racing?

[jaydasika, 2016-09-27 23:42:58]

https://codereview.chromium.org/2376623004/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2376623004/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:1517: PostSetNeedsCommitToMainThread();
On 2016/09/27 23:32:17, danakj wrote:
> Oh thanks for the investigation! I have a suggested fix, as it sounds like
this
> test is flaky because it might draw a diff number of times than commits, and
in
> those cases I don't think it ends up testing the right things, tho at least it
> won't time out now.
> 
> What if we do:
> - Increment num_draws_ in DidCommitAndDrawFrame.
> - Move EndTest() to DidCommitAndDrawFrame.
> - Move the PostSetNeedsRedrawRect to DidCommitAndDrawFrame.
> - Remove DrawLayersOnThread.
> 
> Would that fix and also eliminate weird racing?

That + Adding PostSetNeedsCommitToMainThread() to CommitComplete() will work.

[danakj, 2016-09-28 00:36:13]

On Tue, Sep 27, 2016 at 4:42 PM, <jaydasika@chromium.org> wrote:

>
> https://codereview.chromium.org/2376623004/diff/1/cc/
> trees/layer_tree_host_unittest.cc
> File cc/trees/layer_tree_host_unittest.cc (right):
>
> https://codereview.chromium.org/2376623004/diff/1/cc/
> trees/layer_tree_host_unittest.cc#newcode1517
> cc/trees/layer_tree_host_unittest.cc:1517:
> PostSetNeedsCommitToMainThread();
> On 2016/09/27 23:32:17, danakj wrote:
> > Oh thanks for the investigation! I have a suggested fix, as it sounds
> like this
> > test is flaky because it might draw a diff number of times than
> commits, and in
> > those cases I don't think it ends up testing the right things, tho at
> least it
> > won't time out now.
> >
> > What if we do:
> > - Increment num_draws_ in DidCommitAndDrawFrame.
> > - Move EndTest() to DidCommitAndDrawFrame.
> > - Move the PostSetNeedsRedrawRect to DidCommitAndDrawFrame.
> > - Remove DrawLayersOnThread.
> >
> > Would that fix and also eliminate weird racing?
>
> That + Adding PostSetNeedsCommitToMainThread() to CommitComplete() will
> work.
>

Oh right ofcourse, that SGTM


>
> https://codereview.chromium.org/2376623004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jaydasika, 2016-09-28 19:16:20]

Description was changed from

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in 
CommitComplete after we have drawn twice. Its been flaky because we 
don't force a commit after draw and so, may never reach the EndTest().
This CL forces the commit.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in 
CommitComplete after we have drawn twice. Its been flaky because of the mismatch
in the number of times we commit and draw.
This CL fixes it.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-09-28 19:16:29]

Description was changed from

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in 
CommitComplete after we have drawn twice. Its been flaky because of the mismatch
in the number of times we commit and draw.
This CL fixes it.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in 
CommitComplete after we have drawn twice. Its been flaky
because of the mismatch in the number of times we commit and draw.
This CL fixes it.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jaydasika, 2016-09-28 19:17:15]

Addressed comments

[jaydasika, 2016-09-28 19:17:24]

The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 19:17:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 19:29:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 19:29:55]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-09-29 21:35:54]

LGTM

https://codereview.chromium.org/2376623004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2376623004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:1534: PostSetNeedsCommitToMainThread();
one nit: you could just put this as layer_tree_host()->SetNeedsCommit() in
DidCommitAndDrawFrame. Less posting = less chances for confusion with ordering.

https://codereview.chromium.org/2376623004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:1544:
PostSetNeedsRedrawRectToMainThread(invalid_rect_);
this doesn't need to post, it can just set the redraw rect on layer_tree_host()
directly since this is the main thread.

Also, don't do this if we're ending/ended test. So like

if (num_draws_ < 2) {
  lth->SetNeedsRedrawRect
  lth->SetNeedsCommit
} else {
  EndTest();
}

?

[jaydasika, 2016-09-30 00:20:52]

https://codereview.chromium.org/2376623004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2376623004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:1534: PostSetNeedsCommitToMainThread();
On 2016/09/29 21:35:53, danakj wrote:
> one nit: you could just put this as layer_tree_host()->SetNeedsCommit() in
> DidCommitAndDrawFrame. Less posting = less chances for confusion with
ordering.

Done.

https://codereview.chromium.org/2376623004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:1544:
PostSetNeedsRedrawRectToMainThread(invalid_rect_);
On 2016/09/29 21:35:53, danakj wrote:
> this doesn't need to post, it can just set the redraw rect on
layer_tree_host()
> directly since this is the main thread.
> 
> Also, don't do this if we're ending/ended test. So like
> 
> if (num_draws_ < 2) {
>   lth->SetNeedsRedrawRect
>   lth->SetNeedsCommit
> } else {
>   EndTest();
> }
> 
> ?

Done.

[jaydasika, 2016-09-30 00:21:00]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-09-30 00:21:01]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2376623004/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2016-09-30 00:21:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 00:39:30]

Description was changed from

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in 
CommitComplete after we have drawn twice. Its been flaky
because of the mismatch in the number of times we commit and draw.
This CL fixes it.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in 
CommitComplete after we have drawn twice. Its been flaky
because of the mismatch in the number of times we commit and draw.
This CL fixes it.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-09-30 00:39:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-30 00:42:01]

Description was changed from

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in 
CommitComplete after we have drawn twice. Its been flaky
because of the mismatch in the number of times we commit and draw.
This CL fixes it.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc : Enable LayerTreeHostTestGpuRasterDeviceSizeChanged test

LayerTreeHostTestGpuRasterDeviceSizeChanged calls EndTest() in
CommitComplete after we have drawn twice. Its been flaky
because of the mismatch in the number of times we commit and draw.
This CL fixes it.

BUG=598491
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/10a2c8a57326345ebe92c2805a85e1ba943f9852
Cr-Commit-Position: refs/heads/master@{#421995}
==========

[commit-bot: I haz the power, 2016-09-30 00:42:04]

Patchset 4 (id:??) landed as
https://crrev.com/10a2c8a57326345ebe92c2805a85e1ba943f9852
Cr-Commit-Position: refs/heads/master@{#421995}