Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416273002:
Enable some remote LTH tests in layer_tree_host_unittests. (Closed)
DescriptionEnable some remote LTH tests in layer_tree_host_unittests.
For Blimp we added LayerTreeHostRemote on the engine side, and setup
the test structure to reuse the cc unittests for LTH remote.
This CL went through around 20 tests in layer_tree_host_unittests.
BUG=653371
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/0f6110c8db6415566429f9c99c9e3544e8be82e7
Cr-Commit-Position: refs/heads/master@{#426046}
Patch Set 1 #Patch Set 2 : Polish some comments. #
Total comments: 24
Patch Set 3 : Rebased. #Patch Set 4 : Fixes based on review. #Patch Set 5 : Fixed the compiling.. #
Total comments: 6
Patch Set 6 : Polish comments. #Patch Set 7 : Polish more comments. #Patch Set 8 : Minor comment polish again.. #
Messages
Total messages: 38 (27 generated)
Description was changed from ========== Enable some remote LTH tests in layer_tree_host_unittests. For Blimp we added LayerTreeHostRemote on the engine side, and setup the test structure to reuse the cc unittests for LTH remote. This CL went through around 20 tests in layer_tree_host_unittests. BUG=653371 ========== to ========== Enable some remote LTH tests in layer_tree_host_unittests. For Blimp we added LayerTreeHostRemote on the engine side, and setup the test structure to reuse the cc unittests for LTH remote. This CL went through around 20 tests in layer_tree_host_unittests. BUG=653371 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by xingliu@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...
xingliu@chromium.org changed reviewers: + khushalsagar@chromium.org
Please help take a look.
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... File cc/test/layer_tree_host_remote_for_testing.cc (right): https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:72: layer_tree_host_remote_->UpdateStateOnInProcessHost(); We should just kill this method now. I'm not sure why I deferred doing this till the Update call anyway. If a proto comes, we can deserialize right away, during the BeginMainFrame call itself. https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:297: std::unique_ptr<CompositorProtoState> compositor_proto_state) { Can you add a DCHECK at the beginning of this method for, DCHECK(layer_tree_host_in_process_->CommitRequested()); The name doesn't say it but that method returns true even if the main frame is in progress, and we want to make sure that the only time we get proto updates from the remote host is when the main frame on the InProcessHost is in progress. https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:298: std::unique_ptr<CompositorProtoState> pending_compositor_proto_state = Don't really need this temp var. https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:303: // We can patch test only layer data that are not serialized after the I would say do it in this patch itself. This was an oversight from my end. Just walk the entire tree after deserializing, and copy test only values like |force_render_surface_for_testing_| from Layer here. https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:311: void LayerTreeHostRemoteForTesting::UpdateStateOnInProcessHost() { Remove this method. https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:316: layer_tree_host_in_process_->SetNeedsCommit(); Move this to ProcessRemoteCompositorUpdate. The aim of this was to say that the remote host should go till the commit pipeline only if someone calls SetNeedsCommit on the LTH. Now this might not result in a commit on the InProcessHost because the state could all still be the same (nothing changed on any layer but we'll still end up serializing the LayerTree), so the commit will be optimized out. So we explicitly force a commit to prevent that. If you're getting a call to ProcessRemoteCompositorUpdate, you know that you want the InProcessHost to commit. https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:102: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestHasImplThreadTest); Looks like we didn't actually enable it for remote. :P https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:437: class LayerTreeHostContextCacheTest : public LayerTreeHostTest { Can you add a comment on this saying that "Since the LayerTreeHostContextCacheTests exclusively test the behavior of the LayerTreeHostImpl, they don't need to run for the remote mode." https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:913: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestPushNodeOwnerToNodeIdMap); Can you make the comment a bit more descriptive about what the problem is? "This test compares the value of tree indexes from Layers on the engine to the resulting PropertyTrees copied to the pending tree after the commit on the client. This will result in a failure since while the client side Layers would have the correct values for these indexes, the engine side Layers always hold uninitialized indexes since PropertyTrees are never built on the engine. See crbug.com/655795." https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:996: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestSurfaceDamage); Again, a little more descriptive comment helps in addressing this later. :) "This test changes properties on the Layer and ensures that the subtree damage is tracked correctly on the resulting RenderSurfaceImpl for the corresponding LayerImpl. Since the remote code path currently synchronizes the hierarchy between the engine and client for every frame, all Layers on the client end up being marked as damaged. Enable this when crbug.com/605170 is fixed." https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1097: SINGLE_THREAD_TEST_F(LayerTreeHostTestPropertyTreesChangedSync); +ajuma, is there a reason this is not enabled for mutli-thread mode? https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1209: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestEffectTreeSync); Update the comment, "This test verifies that correct values are retained on the impl thread in cases where they are animated on that thread. Since remote mode doesn't support threaded animations, we don't need to run this in remote mode." Same comment for TransformTreeSync. https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1362: // LTH remote test is not needed since we don't do animation. And here. https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1436: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestSwitchMaskLayer); "This test also verifies that the Layers updated in a main frame correspond only to the Layers that need updates computed using draw_property_utils::FindLayersThatNeedUpdates. Since LayerTreeHostRemote currently updates all Layers during the main frame, the test fails for remote mode. TODO(xingliu): Revisit enabling this when crbug.com/650885 is resolved."
The CQ bit was checked by xingliu@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by xingliu@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.
Thanks for updating the comments, please help take a look. https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... File cc/test/layer_tree_host_remote_for_testing.cc (right): https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:72: layer_tree_host_remote_->UpdateStateOnInProcessHost(); On 2016/10/14 20:12:31, Khushal wrote: > We should just kill this method now. I'm not sure why I deferred doing this till > the Update call anyway. If a proto comes, we can deserialize right away, during > the BeginMainFrame call itself. Done. https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:303: // We can patch test only layer data that are not serialized after the On 2016/10/14 20:12:31, Khushal wrote: > I would say do it in this patch itself. This was an oversight from my end. > Just walk the entire tree after deserializing, and copy test only values like > |force_render_surface_for_testing_| from Layer here. Done. Please help to take a look if the patch logic is correct, it looks for client layer with GetLayerForEngineId and the engine id from the layer update proto. https://codereview.chromium.org/2416273002/diff/20001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:311: void LayerTreeHostRemoteForTesting::UpdateStateOnInProcessHost() { On 2016/10/14 20:12:31, Khushal wrote: > Remove this method. Done. https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:102: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestHasImplThreadTest); On 2016/10/14 20:12:31, Khushal wrote: > Looks like we didn't actually enable it for remote. :P My bad. Done. https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:437: class LayerTreeHostContextCacheTest : public LayerTreeHostTest { On 2016/10/14 20:12:31, Khushal wrote: > Can you add a comment on this saying that > "Since the LayerTreeHostContextCacheTests exclusively test the behavior of the > LayerTreeHostImpl, they don't need to run for the remote mode." Done. https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:913: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestPushNodeOwnerToNodeIdMap); On 2016/10/14 20:12:31, Khushal wrote: > Can you make the comment a bit more descriptive about what the problem is? > > "This test compares the value of tree indexes from Layers on the engine to the > resulting PropertyTrees copied to the pending tree after the commit on the > client. This will result in a failure since while the client side Layers would > have the correct values for these indexes, the engine side Layers always hold > uninitialized indexes since PropertyTrees are never built on the engine. See > crbug.com/655795." Done. https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:996: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestSurfaceDamage); On 2016/10/14 20:12:31, Khushal wrote: > Again, a little more descriptive comment helps in addressing this later. :) > > "This test changes properties on the Layer and ensures that the subtree damage > is tracked correctly on the resulting RenderSurfaceImpl for the corresponding > LayerImpl. Since the remote code path currently synchronizes the hierarchy > between the engine and client for every frame, all Layers on the client end up > being marked as damaged. > Enable this when crbug.com/605170 is fixed." Done. https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1436: SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestSwitchMaskLayer); On 2016/10/14 20:12:32, Khushal wrote: > "This test also verifies that the Layers updated in a main frame correspond only > to the Layers that need updates computed using > draw_property_utils::FindLayersThatNeedUpdates. Since LayerTreeHostRemote > currently updates all Layers during the main frame, the test fails for remote > mode. > TODO(xingliu): Revisit enabling this when crbug.com/650885 is resolved." Done.
khushalsagar@chromium.org changed reviewers: + ajuma@chromium.org
Thanks. lgtm. +ajuma for cc review. https://codereview.chromium.org/2416273002/diff/80001/cc/test/layer_tree_host... File cc/test/layer_tree_host_remote_for_testing.cc (right): https://codereview.chromium.org/2416273002/diff/80001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:308: // Patch test only layer data that are not serialized into network nit: "Copy test only ..."? https://codereview.chromium.org/2416273002/diff/80001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:309: // messages. So in test cases, LTH in process will have the same test only nit: "So in test cases, Layers on the client have the same state as their corresponding layers on the engine." https://codereview.chromium.org/2416273002/diff/80001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:315: // After deserialization, ask for a new commit. We should keep the old comment here. The thing is, what we are asking for here is not a new commit. Just that the current main frame goes till the commit stage, to match the what happened on the engine where the frame went till the commit stage (because that's the only way we could have gotten a proto frame update).
https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1097: SINGLE_THREAD_TEST_F(LayerTreeHostTestPropertyTreesChangedSync); On 2016/10/14 20:12:32, Khushal wrote: > +ajuma, is there a reason this is not enabled for mutli-thread mode? Also, ping on this one to Ali.
lgtm https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2416273002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1097: SINGLE_THREAD_TEST_F(LayerTreeHostTestPropertyTreesChangedSync); On 2016/10/18 00:59:06, Khushal wrote: > On 2016/10/14 20:12:32, Khushal wrote: > > +ajuma, is there a reason this is not enabled for mutli-thread mode? > > Also, ping on this one to Ali. No good reason that I can see. To make it work with multi-thread mode, I think it should be enough to change CommitCompleteOnThread to use sync_tree rather than active_tree.
The CQ bit was checked by xingliu@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...
Updated the comments, prepare to commit this CL. https://codereview.chromium.org/2416273002/diff/80001/cc/test/layer_tree_host... File cc/test/layer_tree_host_remote_for_testing.cc (right): https://codereview.chromium.org/2416273002/diff/80001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:308: // Patch test only layer data that are not serialized into network On 2016/10/18 00:58:02, Khushal wrote: > nit: "Copy test only ..."? Done. https://codereview.chromium.org/2416273002/diff/80001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:309: // messages. So in test cases, LTH in process will have the same test only On 2016/10/18 00:58:02, Khushal wrote: > nit: "So in test cases, Layers on the client have the same state as their > corresponding layers on the engine." Done. https://codereview.chromium.org/2416273002/diff/80001/cc/test/layer_tree_host... cc/test/layer_tree_host_remote_for_testing.cc:315: // After deserialization, ask for a new commit. On 2016/10/18 00:58:01, Khushal wrote: > We should keep the old comment here. The thing is, what we are asking for here > is not a new commit. Just that the current main frame goes till the commit > stage, to match the what happened on the engine where the frame went till the > commit stage (because that's the only way we could have gotten a proto frame > update). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xingliu@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 xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org, khushalsagar@chromium.org Link to the patchset: https://codereview.chromium.org/2416273002/#ps140001 (title: "Minor comment polish again..")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Enable some remote LTH tests in layer_tree_host_unittests. For Blimp we added LayerTreeHostRemote on the engine side, and setup the test structure to reuse the cc unittests for LTH remote. This CL went through around 20 tests in layer_tree_host_unittests. BUG=653371 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Enable some remote LTH tests in layer_tree_host_unittests. For Blimp we added LayerTreeHostRemote on the engine side, and setup the test structure to reuse the cc unittests for LTH remote. This CL went through around 20 tests in layer_tree_host_unittests. BUG=653371 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/0f6110c8db6415566429f9c99c9e3544e8be82e7 Cr-Commit-Position: refs/heads/master@{#426046} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0f6110c8db6415566429f9c99c9e3544e8be82e7 Cr-Commit-Position: refs/heads/master@{#426046} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
