Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724383002:
TestCompositorFrameSink should use CompositorFrameSinkSupport (Closed)
|
Created:
3 years, 9 months ago by Saman Sami Modified:
3 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org Project:
chromium Visibility:
Public. |
DescriptionTestCompositorFrameSink should use CompositorFrameSinkSupport
Test should help us expand the test coverage of CompositorFrameSinkSupport.
TextureLayerChangeInvisibleMailboxTest needed to be changed because
after switching to CompositorFrameSinkSupport returning resources
is delayed until an ack is received unlike SurfaceFactory
which returns the resources immediately, so mailboxes can potentially
be released later than before and this messes with the checks.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2724383002
Cr-Original-Commit-Position: refs/heads/master@{#454403}
Committed: https://chromium.googlesource.com/chromium/src/+/146d1bb3fa62905c98950fbe510f7520935cc07b
Review-Url: https://codereview.chromium.org/2724383002
Cr-Commit-Position: refs/heads/master@{#454617}
Committed: https://chromium.googlesource.com/chromium/src/+/5dbd77c73cd8a5f42762c5036a9860bc729dc359
Patch Set 1 #Patch Set 2 : c #
Total comments: 8
Patch Set 3 : remove bound_ #Patch Set 4 : null display check #Patch Set 5 : bring back PostTask #
Total comments: 2
Messages
Total messages: 54 (38 generated)
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport BUG=696203 ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport BUG=696203 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
samans@chromium.org changed reviewers: + fsamuel@chromium.org
PTAL
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport BUG=696203 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us broaden the test coverage of CompositorFrameSinkSupport. BUG=696203 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us broaden the test coverage of CompositorFrameSinkSupport. BUG=696203 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. BUG=696203 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Sweet! Thanks! LGTM
samans@chromium.org changed reviewers: + danakj@chromium.org
Thanks for the lgtm! danakj@ PTAL
Can you explain the test changes in the CL description? https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... File cc/test/test_compositor_frame_sink.cc (right): https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:117: if (bound_) { doesn't look like there's any reason for this branch now? or for the bound_ variable? https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:122: support_.reset(); nit: = nullptr like the others https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:151: client_->DidReceiveCompositorFrameAck(); This needs to be in a new stack frame. Note the comment you deleted. Or does it not for some reason? https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:165: if (!display_ || !display_->has_scheduler()) how do we get here with a null display?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. BUG=696203 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. ==========
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... File cc/test/test_compositor_frame_sink.cc (right): https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:117: if (bound_) { On 2017/03/02 19:55:19, danakj wrote: > doesn't look like there's any reason for this branch now? or for the bound_ > variable? Done. https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:122: support_.reset(); On 2017/03/02 19:55:19, danakj wrote: > nit: = nullptr like the others Done. https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:151: client_->DidReceiveCompositorFrameAck(); On 2017/03/02 19:55:19, danakj wrote: > This needs to be in a new stack frame. Note the comment you deleted. Or does it > not for some reason? It works fine without it, but as we discussed I'll bring it back. https://codereview.chromium.org/2724383002/diff/20001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:165: if (!display_ || !display_->has_scheduler()) On 2017/03/02 19:55:19, danakj wrote: > how do we get here with a null display? Sorry this is a mistake.
LGTM https://codereview.chromium.org/2724383002/diff/80001/cc/test/test_compositor... File cc/test/test_compositor_frame_sink.cc (right): https://codereview.chromium.org/2724383002/diff/80001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:165: if (!display_->has_scheduler()) The other way to do this, since you have have branch on has_scheduler() here now anyways, would be to optionally PostTask here to SendCompositorFrameAckToClient instead of doing it after DrawAndSwap. It seems neither here nor there, so up to you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2724383002/diff/80001/cc/test/test_compositor... File cc/test/test_compositor_frame_sink.cc (right): https://codereview.chromium.org/2724383002/diff/80001/cc/test/test_compositor... cc/test/test_compositor_frame_sink.cc:165: if (!display_->has_scheduler()) On 2017/03/02 21:58:54, danakj wrote: > The other way to do this, since you have have branch on has_scheduler() here now > anyways, would be to optionally PostTask here to SendCompositorFrameAckToClient > instead of doing it after DrawAndSwap. It seems neither here nor there, so up to > you. Right. I think both approaches are equally good. I'm keeping things the way they are.
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2724383002/#ps80001 (title: "bring back PostTask")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488492607330740,
"parent_rev": "785bf143e2dd152e2a7deaa86f7ca39a46eb624a", "commit_rev":
"146d1bb3fa62905c98950fbe510f7520935cc07b"}
Message was sent while issue was closed.
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2724383002 Cr-Commit-Position: refs/heads/master@{#454403} Committed: https://chromium.googlesource.com/chromium/src/+/146d1bb3fa62905c98950fbe510f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/146d1bb3fa62905c98950fbe510f...
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 454403 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
Looks like this maybe introduces some flake by holding the mailboxes longer. Tests need to wait for the mailboxes to be returned before exiting when this is the case.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2726293002/ by danakj@chromium.org. The reason for reverting is: TextureLayerMailboxIsActivatedDuringCommit.RunMultiThread_DelegatingRenderer failing (I think flakily).
Message was sent while issue was closed.
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2724383002 Cr-Commit-Position: refs/heads/master@{#454403} Committed: https://chromium.googlesource.com/chromium/src/+/146d1bb3fa62905c98950fbe510f... ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2724383002 Cr-Commit-Position: refs/heads/master@{#454403} Committed: https://chromium.googlesource.com/chromium/src/+/146d1bb3fa62905c98950fbe510f... ==========
On 2017/03/02 23:28:41, danakj wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/2726293002/ by mailto:danakj@chromium.org. > > The reason for reverting is: > TextureLayerMailboxIsActivatedDuringCommit.RunMultiThread_DelegatingRenderer > failing (I think flakily). I fixed the flake in https://codereview.chromium.org/2729183002/. I rebased on top of it and I can confirm TextureLayerMailboxIsActivatedDuringCommit does not flake on my workstation. I'll try relanding this.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On Fri, Mar 3, 2017 at 12:29 PM, <samans@chromium.org> wrote: > On 2017/03/02 23:28:41, danakj wrote: > > A revert of this CL (patchset #5 id:80001) has been created in > > https://codereview.chromium.org/2726293002/ by mailto: > danakj@chromium.org. > > > > The reason for reverting is: > > TextureLayerMailboxIsActivatedDuringCommit.RunMultiThread_ > DelegatingRenderer > > failing (I think flakily). > > I fixed the flake in https://codereview.chromium.org/2729183002/. I > rebased on > top of it and I can confirm TextureLayerMailboxIsActivatedDuringCommit > does not > flake on my workstation. I'll try relanding this. > Thanks! > https://codereview.chromium.org/2724383002/ > -- 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488562556433060,
"parent_rev": "741bad136249946504139ca35420d78a312762f7", "commit_rev":
"5dbd77c73cd8a5f42762c5036a9860bc729dc359"}
Message was sent while issue was closed.
Description was changed from ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2724383002 Cr-Commit-Position: refs/heads/master@{#454403} Committed: https://chromium.googlesource.com/chromium/src/+/146d1bb3fa62905c98950fbe510f... ========== to ========== TestCompositorFrameSink should use CompositorFrameSinkSupport Test should help us expand the test coverage of CompositorFrameSinkSupport. TextureLayerChangeInvisibleMailboxTest needed to be changed because after switching to CompositorFrameSinkSupport returning resources is delayed until an ack is received unlike SurfaceFactory which returns the resources immediately, so mailboxes can potentially be released later than before and this messes with the checks. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2724383002 Cr-Original-Commit-Position: refs/heads/master@{#454403} Committed: https://chromium.googlesource.com/chromium/src/+/146d1bb3fa62905c98950fbe510f... Review-Url: https://codereview.chromium.org/2724383002 Cr-Commit-Position: refs/heads/master@{#454617} Committed: https://chromium.googlesource.com/chromium/src/+/5dbd77c73cd8a5f42762c5036a98... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/5dbd77c73cd8a5f42762c5036a98... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
