Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
563523002:
cc: Change LayerTreeHost unit tests to use impl painting. (Closed)
Descriptioncc: Change LayerTreeHost unit tests to use impl painting.
Make LTH Unittests use PictureLayers instead of ContentLayer.
BUG=401492
Committed: https://crrev.com/25780da3c293ff9550ebcc83215137334166ee60
Cr-Commit-Position: refs/heads/master@{#304178}
Patch Set 1 #
Total comments: 14
Patch Set 2 : update more tests #
Total comments: 30
Patch Set 3 : review comments addressed. #Patch Set 4 : break down into smaller CLs #
Total comments: 23
Patch Set 5 : review comments addressed. #Patch Set 6 : rebased and use set_fill_with_nonsolidcolor #Patch Set 7 : avoid changing layer bounds as new code in FakeContentLayerClient paint, will ensure non-solid colo… #Patch Set 8 : rebased to ToT to use new non-solid color paint. #
Total comments: 9
Patch Set 9 : review comments addressed. #
Total comments: 1
Patch Set 10 : remove unnecessary comment. #Messages
Total messages: 37 (5 generated)
https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:976: class ContentLayerWithUpdateTracking : public ContentLayer { Do we change this to use PictureLayer ? LayerTreeHostTestContinuousPainting later on tests with this class for non-impl side painting too. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1006: class LayerTreeHostTestOpacityChange : public LayerTreeHostTest { LayerTreeHostTestOpacityChange looks to be valid only for non-impl painting ? Should we change this ?
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:976: class ContentLayerWithUpdateTracking : public ContentLayer { On 2014/09/10 14:11:19, sohanjg wrote: > Do we change this to use PictureLayer ? LayerTreeHostTestContinuousPainting > later on tests with this class for non-impl side painting too. Ya, leave this class alone, and just use FakePictureLayer instead as that test does for impl-side. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1006: class LayerTreeHostTestOpacityChange : public LayerTreeHostTest { On 2014/09/10 14:11:18, sohanjg wrote: > LayerTreeHostTestOpacityChange looks to be valid only for non-impl painting ? > Should we change this ? Ya let's change this. Blink could change the opacity during paint in implside too. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1161: MULTI_THREAD_TEST_F(LayerTreeHostTestDeviceScaleFactorScalesViewportAndLayers); Does this test work? I'm a bit surprised you can activate a picture layer when impl-side painting is turned off XD. Anyhow can you change this to the IMPL version of the macro so it only runs with impl-side. PicturLayer + non-impl side doesn't mean much. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1164: class LayerTreeHostTestDirectRendererAtomicCommit : public LayerTreeHostTest { This test doesn't make sense for impl-side you can leave it NOIMPL and TODO() to remove it once impl-side ships everywhere (looks like m39 *cross fingers*)
https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1127: EXPECT_EQ(child_bounds_scaled, child->content_bounds()); when using picturelayer the content_bounds, are not scaled by the device scale factor here, is that a bug? https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1139: when using picturelayer the transforms, are not scaled by the device scale factor here, is that a bug?
https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1127: EXPECT_EQ(child_bounds_scaled, child->content_bounds()); On 2014/09/11 14:53:57, sohanjg wrote: > when using picturelayer the content_bounds, are not scaled by the device scale > factor here, is that a bug? No, that's the change you made to picture layer to make its contents scale always 1. You need to check the MaxTilingContentScale() instead.
can u please take a look, thanks. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:976: class ContentLayerWithUpdateTracking : public ContentLayer { On 2014/09/10 15:24:29, danakj wrote: > On 2014/09/10 14:11:19, sohanjg wrote: > > Do we change this to use PictureLayer ? LayerTreeHostTestContinuousPainting > > later on tests with this class for non-impl side painting too. > > Ya, leave this class alone, and just use FakePictureLayer instead as that test > does for impl-side. Acknowledged. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1006: class LayerTreeHostTestOpacityChange : public LayerTreeHostTest { On 2014/09/10 15:24:29, danakj wrote: > On 2014/09/10 14:11:18, sohanjg wrote: > > LayerTreeHostTestOpacityChange looks to be valid only for non-impl painting ? > > Should we change this ? > > Ya let's change this. Blink could change the opacity during paint in implside > too. Acknowledged. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1127: EXPECT_EQ(child_bounds_scaled, child->content_bounds()); On 2014/09/11 17:17:30, danakj wrote: > On 2014/09/11 14:53:57, sohanjg wrote: > > when using picturelayer the content_bounds, are not scaled by the device scale > > factor here, is that a bug? > > No, that's the change you made to picture layer to make its contents scale > always 1. You need to check the MaxTilingContentScale() instead. Acknowledged. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1161: MULTI_THREAD_TEST_F(LayerTreeHostTestDeviceScaleFactorScalesViewportAndLayers); On 2014/09/10 15:24:29, danakj wrote: > Does this test work? I'm a bit surprised you can activate a picture layer when > impl-side painting is turned off XD. Anyhow can you change this to the IMPL > version of the macro so it only runs with impl-side. PicturLayer + non-impl side > doesn't mean much. Acknowledged. https://codereview.chromium.org/563523002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:1164: class LayerTreeHostTestDirectRendererAtomicCommit : public LayerTreeHostTest { On 2014/09/10 15:24:29, danakj wrote: > This test doesn't make sense for impl-side you can leave it NOIMPL and TODO() to > remove it once impl-side ships everywhere (looks like m39 *cross fingers*) Acknowledged. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1531: class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit Do we change this for impl-side? i think this deals with PrioritizedResource mem management. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2089: class LayerTreeHostTestShutdownWithOnlySomeResourcesEvicted Same, Do we change this for impl-side?
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1531: class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit On 2014/09/15 10:38:39, sohanjg wrote: > Do we change this for impl-side? i think this deals with PrioritizedResource mem > management. This deals with surface memory management, which is done by the renderer not prioritized resource manager. I'd expect this test to still pass and test something valid with implside. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2089: class LayerTreeHostTestShutdownWithOnlySomeResourcesEvicted On 2014/09/15 10:38:39, sohanjg wrote: > Same, Do we change this for impl-side? This one I agree doesn't make sense with impl-side. It's testing the contents_texture_manager() directly.
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1531: class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit On 2014/09/15 15:23:43, danakj wrote: > On 2014/09/15 10:38:39, sohanjg wrote: > > Do we change this for impl-side? i think this deals with PrioritizedResource > mem > > management. > > This deals with surface memory management, which is done by the renderer not > prioritized resource manager. I'd expect this test to still pass and test > something valid with implside. Yes, right. But, to make sure that no surface is allocated, it sets memory policy by SetMemoryPolicy->SetManagedMemoryPolicy->EnforceManagedMemoryPolicy->ReduceContentsTextureMemoryOnImplThread. Should we find some way to enforce mem policy for impl-world ?
On 2014/09/17 07:27:43, sohanjg wrote: > https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... > File cc/trees/layer_tree_host_unittest.cc (right): > > https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... > cc/trees/layer_tree_host_unittest.cc:1531: class > LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit > On 2014/09/15 15:23:43, danakj wrote: > > On 2014/09/15 10:38:39, sohanjg wrote: > > > Do we change this for impl-side? i think this deals with PrioritizedResource > > mem > > > management. > > > > This deals with surface memory management, which is done by the renderer not > > prioritized resource manager. I'd expect this test to still pass and test > > something valid with implside. > > Yes, right. > But, to make sure that no surface is allocated, it sets memory policy by > SetMemoryPolicy->SetManagedMemoryPolicy->EnforceManagedMemoryPolicy->ReduceContentsTextureMemoryOnImplThread. > > Should we find some way to enforce mem policy for impl-world ? And any other test which requires changing that we are missing here ?
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (left): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1856: MULTI_THREAD_NOIMPL_TEST_F(LayerTreeHostTestContinuousInvalidate); you can just use IMPL instead of NOIMPL in this macro rather than splitting out multiple test functions? Also can you make this SINGLE_AND_MULTI_THREAD? https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:4407: // No output to copy for delegated renderers. this comment looks out of date, can you remove it? https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:4408: SINGLE_AND_MULTI_THREAD_TEST_F( just put IMPL in this macro. you're missing some cases like single thread in your TEST_Fs https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:344: scaled_layer_ = FakeContentLayer::Create(&client_); picture layer when impl side? https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:365: layer_tree_host()->SetDeviceScaleFactor(4.f); can you change the layer's bounds instead of changing device scale when using picture layer? https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1180: remove this blank line so the comment is attached to the test https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1531: class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit On 2014/09/17 07:27:43, sohanjg wrote: > On 2014/09/15 15:23:43, danakj wrote: > > On 2014/09/15 10:38:39, sohanjg wrote: > > > Do we change this for impl-side? i think this deals with PrioritizedResource > > mem > > > management. > > > > This deals with surface memory management, which is done by the renderer not > > prioritized resource manager. I'd expect this test to still pass and test > > something valid with implside. > > Yes, right. > But, to make sure that no surface is allocated, it sets memory policy by > SetMemoryPolicy->SetManagedMemoryPolicy->EnforceManagedMemoryPolicy->ReduceContentsTextureMemoryOnImplThread. > > Should we find some way to enforce mem policy for impl-world ? It's true, but that's a no-op when implside is on, and further down it also does UpdateTileManagerMemoryPolicy() which is for implside. So that path is used for both modes. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2089: class LayerTreeHostTestShutdownWithOnlySomeResourcesEvicted On 2014/09/15 15:23:43, danakj wrote: > On 2014/09/15 10:38:39, sohanjg wrote: > > Same, Do we change this for impl-side? > > This one I agree doesn't make sense with impl-side. It's testing the > contents_texture_manager() directly. TODO to remove it. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2450: scoped_refptr<ContentLayer> root_layer = ContentLayer::Create(&client_); missed this?
Please take a look, thanks. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (left): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1856: MULTI_THREAD_NOIMPL_TEST_F(LayerTreeHostTestContinuousInvalidate); On 2014/09/19 18:12:01, danakj wrote: > you can just use IMPL instead of NOIMPL in this macro rather than splitting out > multiple test functions? > > Also can you make this SINGLE_AND_MULTI_THREAD? Done. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:4408: SINGLE_AND_MULTI_THREAD_TEST_F( On 2014/09/19 18:12:01, danakj wrote: > just put IMPL in this macro. you're missing some cases like single thread in > your TEST_Fs Done. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:344: scaled_layer_ = FakeContentLayer::Create(&client_); On 2014/09/19 18:12:01, danakj wrote: > picture layer when impl side? Done. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:365: layer_tree_host()->SetDeviceScaleFactor(4.f); On 2014/09/19 18:12:00, danakj wrote: > can you change the layer's bounds instead of changing device scale when using > picture layer? Done. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1180: On 2014/09/19 18:12:01, danakj wrote: > remove this blank line so the comment is attached to the test Done. https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1590: renderer->HasAllocatedResourcesForTesting(surface1_render_pass_id)); If we test this for impl-side paint, this resource test fails, because some render passes are not removed in render_passes_in_draw_order when we do DecideRenderPassAllocationsForFrame. This is because RemoveRenderPasses, in CalculateRenderPasses skips quads which are not RENDER_PASS quads, and here we have SOLID_COLOR quads. Anything u suggest ? https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1605: EXPECT_LE(2u, root_layer_->update_count()); update_count() is only present in Fake layers. If we modify this test to use impl/non-impl layers based on setting, should we ignore this test ? https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2089: class LayerTreeHostTestShutdownWithOnlySomeResourcesEvicted On 2014/09/19 18:12:01, danakj wrote: > On 2014/09/15 15:23:43, danakj wrote: > > On 2014/09/15 10:38:39, sohanjg wrote: > > > Same, Do we change this for impl-side? > > > > This one I agree doesn't make sense with impl-side. It's testing the > > contents_texture_manager() directly. > > TODO to remove it. Done.
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1590: renderer->HasAllocatedResourcesForTesting(surface1_render_pass_id)); On 2014/09/22 09:31:18, sohanjg wrote: > If we test this for impl-side paint, this resource test fails, because some > render passes are not removed in render_passes_in_draw_order when we do > DecideRenderPassAllocationsForFrame. > This is because RemoveRenderPasses, in CalculateRenderPasses skips quads which > are not RENDER_PASS quads, and here we have SOLID_COLOR quads. > > Anything u suggest ? Change the layers to not draw solid color, you can use the new method FakeContentLayerClient::set_fill_with_nonsolid_color(). https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1605: EXPECT_LE(2u, root_layer_->update_count()); On 2014/09/22 09:31:19, sohanjg wrote: > update_count() is only present in Fake layers. If we modify this test to use > impl/non-impl layers based on setting, should we ignore this test ? I don't understand the question. FakePictureLayer can/does have a update_count() also.
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1605: EXPECT_LE(2u, root_layer_->update_count()); On 2014/09/22 14:48:21, danakj wrote: > On 2014/09/22 09:31:19, sohanjg wrote: > > update_count() is only present in Fake layers. If we modify this test to use > > impl/non-impl layers based on setting, should we ignore this test ? > > I don't understand the question. FakePictureLayer can/does have a update_count() > also. Let me rephrase it, if we make root/surface/replica layer to be either FakeContent or FakePicture, then we need to declare it as <Layer>, and <Layer> does not have update_count() member. If we test only for FakePictureLayer, we wouldnt have a SINGLE_THREAD option.
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1605: EXPECT_LE(2u, root_layer_->update_count()); On 2014/09/22 14:54:36, sohanjg wrote: > On 2014/09/22 14:48:21, danakj wrote: > > On 2014/09/22 09:31:19, sohanjg wrote: > > > update_count() is only present in Fake layers. If we modify this test to use > > > impl/non-impl layers based on setting, should we ignore this test ? > > > > I don't understand the question. FakePictureLayer can/does have a > update_count() > > also. > > Let me rephrase it, if we make root/surface/replica layer to be either > FakeContent or FakePicture, then we need to declare it as <Layer>, and <Layer> > does not have update_count() member. Oh, then have a FakeContentLayer and a FakePictureLayer as members of the class, and one of them will be null depending on the config? > If we test only for FakePictureLayer, we wouldnt have a SINGLE_THREAD option. > Not true anymore, we run implside single thread version of unit tests now.
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1590: renderer->HasAllocatedResourcesForTesting(surface1_render_pass_id)); On 2014/09/22 14:48:21, danakj wrote: > On 2014/09/22 09:31:18, sohanjg wrote: > > If we test this for impl-side paint, this resource test fails, because some > > render passes are not removed in render_passes_in_draw_order when we do > > DecideRenderPassAllocationsForFrame. > > This is because RemoveRenderPasses, in CalculateRenderPasses skips quads which > > are not RENDER_PASS quads, and here we have SOLID_COLOR quads. > > > > Anything u suggest ? > > Change the layers to not draw solid color, you can use the new method > FakeContentLayerClient::set_fill_with_nonsolid_color(). Even if we draw non solid color, the passes are still skipped, because it will skip for anything except for RENDER_PASS quads. When i checked the non-impl test, the quad list for RenderPassses are already empty or have only RENDER_PASS material (1 of the renderpass has PICTURE_CONTENT and TILED_CONTENT, but that is not tested here), when we check in RemoveRenderPasses, so it is removed and this test works fine. I wonder whether we are missing something for impl-side here ? Some differences in memory mgmt, is there, like max_memory_needed_bytes_ is never set for impl-side, hence it is always 0, and needs_commit will always be false in LTHI::SetManagedMemoryPolicy. But, thats not the root cause here. Can you help ? https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1605: EXPECT_LE(2u, root_layer_->update_count()); On 2014/09/22 14:58:45, danakj wrote: > On 2014/09/22 14:54:36, sohanjg wrote: > > On 2014/09/22 14:48:21, danakj wrote: > > > On 2014/09/22 09:31:19, sohanjg wrote: > > > > update_count() is only present in Fake layers. If we modify this test to > use > > > > impl/non-impl layers based on setting, should we ignore this test ? > > > > > > I don't understand the question. FakePictureLayer can/does have a > > update_count() > > > also. > > > > Let me rephrase it, if we make root/surface/replica layer to be either > > FakeContent or FakePicture, then we need to declare it as <Layer>, and <Layer> > > does not have update_count() member. > > Oh, then have a FakeContentLayer and a FakePictureLayer as members of the class, > and one of them will be null depending on the config? > > > If we test only for FakePictureLayer, we wouldnt have a SINGLE_THREAD option. > > > > Not true anymore, we run implside single thread version of unit tests now. Yea, if we make FakePictureLayer and FakeContentLayer as members, this will work.
sohan.jyoti@samsung.com changed reviewers: + enne@chromium.org
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1590: renderer->HasAllocatedResourcesForTesting(surface1_render_pass_id)); On 2014/09/23 11:46:45, sohanjg wrote: > On 2014/09/22 14:48:21, danakj wrote: > > On 2014/09/22 09:31:18, sohanjg wrote: > > > If we test this for impl-side paint, this resource test fails, because some > > > render passes are not removed in render_passes_in_draw_order when we do > > > DecideRenderPassAllocationsForFrame. > > > This is because RemoveRenderPasses, in CalculateRenderPasses skips quads > which > > > are not RENDER_PASS quads, and here we have SOLID_COLOR quads. > > > > > > Anything u suggest ? > > > > Change the layers to not draw solid color, you can use the new method > > FakeContentLayerClient::set_fill_with_nonsolid_color(). > > Even if we draw non solid color, the passes are still skipped, because it will > skip for anything except for RENDER_PASS quads. > > When i checked the non-impl test, the quad list for RenderPassses are already > empty or have only RENDER_PASS material (1 of the renderpass has PICTURE_CONTENT > and TILED_CONTENT, but that is not tested here), when we check in > RemoveRenderPasses, so it is removed and this test works fine. > > I wonder whether we are missing something for impl-side here ? > > Some differences in memory mgmt, is there, like max_memory_needed_bytes_ is > never set for impl-side, hence it is always 0, and needs_commit will always be > false in LTHI::SetManagedMemoryPolicy. But, thats not the root cause here. > Can you help ? Can you please give your feedback on this ?
https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1590: renderer->HasAllocatedResourcesForTesting(surface1_render_pass_id)); On 2014/10/13 09:09:19, sohanjg wrote: > On 2014/09/23 11:46:45, sohanjg wrote: > > On 2014/09/22 14:48:21, danakj wrote: > > > On 2014/09/22 09:31:18, sohanjg wrote: > > > > If we test this for impl-side paint, this resource test fails, because > some > > > > render passes are not removed in render_passes_in_draw_order when we do > > > > DecideRenderPassAllocationsForFrame. > > > > This is because RemoveRenderPasses, in CalculateRenderPasses skips quads > > which > > > > are not RENDER_PASS quads, and here we have SOLID_COLOR quads. > > > > > > > > Anything u suggest ? > > > > > > Change the layers to not draw solid color, you can use the new method > > > FakeContentLayerClient::set_fill_with_nonsolid_color(). > > > > Even if we draw non solid color, the passes are still skipped, because it will > > skip for anything except for RENDER_PASS quads. > > > > When i checked the non-impl test, the quad list for RenderPassses are already > > empty or have only RENDER_PASS material (1 of the renderpass has > PICTURE_CONTENT > > and TILED_CONTENT, but that is not tested here), when we check in > > RemoveRenderPasses, so it is removed and this test works fine. > > > > I wonder whether we are missing something for impl-side here ? > > > > Some differences in memory mgmt, is there, like max_memory_needed_bytes_ is > > never set for impl-side, hence it is always 0, and needs_commit will always be > > false in LTHI::SetManagedMemoryPolicy. But, thats not the root cause here. > > Can you help ? > > Can you please give your feedback on this ? OK I'm pretty confused what is different here in non-impl vs impl. The RemoveRenderPasses function removes empty render passes either way. Do you mean things are being culled differently with implside painting? The render pass is empty when it wasn't without or vice versa?
Patchset #4 (id:70001) has been deleted
Lets update LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit test, as a separate CL. Please take a look, thanks. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1520: class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit This test change i will update as a separate CL.
https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:365: // Changing the device scale factor causes a commit. It also changes move this comment into the else block https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:369: scaled_layer_->SetBounds(gfx::Size(4, 4)); comment saying SetBounds grows the layer and exposes new content https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1037: EXPECT_EQ(1, (int)update_check_layer_->update_count()); don't cast with c-style cast ever, you should use static cast. but here, you should just use EXPECT_EQ(1u, ...update_count()); https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1047: RunTest(true, false, true); Just make the test work with/without implside for now? Or override the settings.impl_side_painting=true if you can't. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. You can use this now https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... Just move this all to BeginTest? https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1125: EXPECT_EQ(1.5, child->MaximumTilingContentsScale()); MaxTilingContentsScale is a float isn't it? 1.5f? https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1141: drop whitespace https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1160: TEST_F(LayerTreeHostTestDeviceScaleFactorScalesViewportAndLayers, Keep using the macro please. You can just override settings.impl_side_painting=true Example: https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1520: class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit On 2014/10/16 16:06:52, sohanjg wrote: > This test change i will update as a separate CL. leave a TODO to make it work with implside painting?
Thanks for going through again, please take a look. Thanks. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:365: // Changing the device scale factor causes a commit. It also changes On 2014/10/18 19:39:26, danakj wrote: > move this comment into the else block Done. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:369: scaled_layer_->SetBounds(gfx::Size(4, 4)); On 2014/10/18 19:39:26, danakj wrote: > comment saying SetBounds grows the layer and exposes new content Done. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1037: EXPECT_EQ(1, (int)update_check_layer_->update_count()); On 2014/10/18 19:39:26, danakj wrote: > don't cast with c-style cast ever, you should use static cast. > > but here, you should just use EXPECT_EQ(1u, ...update_count()); Done. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1047: RunTest(true, false, true); On 2014/10/18 19:39:26, danakj wrote: > Just make the test work with/without implside for now? Or override the > settings.impl_side_painting=true if you can't. Done. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. On 2014/10/18 19:39:26, danakj wrote: > You can use this now > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > Just move this all to BeginTest? Moved to BeginTest, but set_fill_with_nonsolid_color, doesn't work too well here, i may re-look at it later if you want? https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1125: EXPECT_EQ(1.5, child->MaximumTilingContentsScale()); On 2014/10/18 19:39:26, danakj wrote: > MaxTilingContentsScale is a float isn't it? 1.5f? Acknowledged. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1141: On 2014/10/18 19:39:26, danakj wrote: > drop whitespace Done. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1160: TEST_F(LayerTreeHostTestDeviceScaleFactorScalesViewportAndLayers, On 2014/10/18 19:39:26, danakj wrote: > Keep using the macro please. You can just override > settings.impl_side_painting=true > > Example: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... Done. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1520: class LayerTreeHostTestSurfaceNotAllocatedForLayersOutsideMemoryLimit On 2014/10/18 19:39:26, danakj wrote: > On 2014/10/16 16:06:52, sohanjg wrote: > > This test change i will update as a separate CL. > > leave a TODO to make it work with implside painting? Done.
https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. On 2014/10/20 07:10:03, sohanjg wrote: > On 2014/10/18 19:39:26, danakj wrote: > > You can use this now > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > Just move this all to BeginTest? > > Moved to BeginTest, but set_fill_with_nonsolid_color, doesn't work too well > here, i may re-look at it later if you want? Why does it not work?
Sorry for being so so late on this, please let me know your opinion on using set_fill_with_nonsolid_color. thanks. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. On 2014/10/22 15:10:19, danakj wrote: > On 2014/10/20 07:10:03, sohanjg wrote: > > On 2014/10/18 19:39:26, danakj wrote: > > > You can use this now > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > > > Just move this all to BeginTest? > > > > Moved to BeginTest, but set_fill_with_nonsolid_color, doesn't work too well > > here, i may re-look at it later if you want? > > Why does it not work? Because CanHaveTilings fail for solid color check even if we use this function. That is because we do not perform a UpdateAndExpandInvalidation on the PicturePile here, which would have done a DetermineIfSolidColor analysis, and updated the PicturePile is_solid_color member.
https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. On 2014/10/29 08:31:00, sohanjg wrote: > On 2014/10/22 15:10:19, danakj wrote: > > On 2014/10/20 07:10:03, sohanjg wrote: > > > On 2014/10/18 19:39:26, danakj wrote: > > > > You can use this now > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > > > > > Just move this all to BeginTest? > > > > > > Moved to BeginTest, but set_fill_with_nonsolid_color, doesn't work too well > > > here, i may re-look at it later if you want? > > > > Why does it not work? > > Because CanHaveTilings fail for solid color check even if we use this function. > That is because we do not perform a UpdateAndExpandInvalidation on the > PicturePile here, which would have done a DetermineIfSolidColor analysis, and > updated the PicturePile is_solid_color member. Are you saying PictureLayer::Update never happens? I'd surely expect it to happen on the first commit. If add_draw_rect has any side effect, then set_fill_with_nonsolid_color would have side effects at the exact same point in time? BTW I changed how it works in https://codereview.chromium.org/690493002/
Please take a look, thanks. https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. On 2014/10/29 14:27:47, danakj wrote: > On 2014/10/29 08:31:00, sohanjg wrote: > > On 2014/10/22 15:10:19, danakj wrote: > > > On 2014/10/20 07:10:03, sohanjg wrote: > > > > On 2014/10/18 19:39:26, danakj wrote: > > > > > You can use this now > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > > > > > > > Just move this all to BeginTest? > > > > > > > > Moved to BeginTest, but set_fill_with_nonsolid_color, doesn't work too > well > > > > here, i may re-look at it later if you want? > > > > > > Why does it not work? > > > > Because CanHaveTilings fail for solid color check even if we use this > function. > > That is because we do not perform a UpdateAndExpandInvalidation on the > > PicturePile here, which would have done a DetermineIfSolidColor analysis, and > > updated the PicturePile is_solid_color member. > > Are you saying PictureLayer::Update never happens? I'd surely expect it to > happen on the first commit. If add_draw_rect has any side effect, then > set_fill_with_nonsolid_color would have side effects at the exact same point in > time? > > BTW I changed how it works in https://codereview.chromium.org/690493002/ No, commit is happening just the DetermineIfSolidColor fails because in AnalysisCanvas::drawRect, does_cover_canvas is TRUE (device rect and clip rect is same in IsFullQuad), hence its assumed to be solid color. Have updated the layer bounds and viewport in the test to account for it.
On 2014/10/31 10:58:52, sohanjg wrote: > Please take a look, thanks. > > https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... > File cc/trees/layer_tree_host_unittest.cc (right): > > https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... > cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. > On 2014/10/29 14:27:47, danakj wrote: > > On 2014/10/29 08:31:00, sohanjg wrote: > > > On 2014/10/22 15:10:19, danakj wrote: > > > > On 2014/10/20 07:10:03, sohanjg wrote: > > > > > On 2014/10/18 19:39:26, danakj wrote: > > > > > > You can use this now > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > > > > > > > > > Just move this all to BeginTest? > > > > > > > > > > Moved to BeginTest, but set_fill_with_nonsolid_color, doesn't work too > > well > > > > > here, i may re-look at it later if you want? > > > > > > > > Why does it not work? > > > > > > Because CanHaveTilings fail for solid color check even if we use this > > function. > > > That is because we do not perform a UpdateAndExpandInvalidation on the > > > PicturePile here, which would have done a DetermineIfSolidColor analysis, > and > > > updated the PicturePile is_solid_color member. > > > > Are you saying PictureLayer::Update never happens? I'd surely expect it to > > happen on the first commit. If add_draw_rect has any side effect, then > > set_fill_with_nonsolid_color would have side effects at the exact same point > in > > time? > > > > BTW I changed how it works in https://codereview.chromium.org/690493002/ > > No, commit is happening just the DetermineIfSolidColor fails because in > AnalysisCanvas::drawRect, does_cover_canvas is TRUE (device rect and clip rect > is same in IsFullQuad), hence its assumed to be solid color. > Have updated the layer bounds and viewport in the test to account for it. Does it work without those changes if you do this https://codereview.chromium.org/671653005/diff/410001/cc/test/fake_content_la... ?
On 2014/11/04 21:54:17, danakj wrote: > On 2014/10/31 10:58:52, sohanjg wrote: > > Please take a look, thanks. > > > > > https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... > > File cc/trees/layer_tree_host_unittest.cc (right): > > > > > https://codereview.chromium.org/563523002/diff/90001/cc/trees/layer_tree_host... > > cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. > > On 2014/10/29 14:27:47, danakj wrote: > > > On 2014/10/29 08:31:00, sohanjg wrote: > > > > On 2014/10/22 15:10:19, danakj wrote: > > > > > On 2014/10/20 07:10:03, sohanjg wrote: > > > > > > On 2014/10/18 19:39:26, danakj wrote: > > > > > > > You can use this now > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > > > > > > > > > > > Just move this all to BeginTest? > > > > > > > > > > > > Moved to BeginTest, but set_fill_with_nonsolid_color, doesn't work too > > > well > > > > > > here, i may re-look at it later if you want? > > > > > > > > > > Why does it not work? > > > > > > > > Because CanHaveTilings fail for solid color check even if we use this > > > function. > > > > That is because we do not perform a UpdateAndExpandInvalidation on the > > > > PicturePile here, which would have done a DetermineIfSolidColor analysis, > > and > > > > updated the PicturePile is_solid_color member. > > > > > > Are you saying PictureLayer::Update never happens? I'd surely expect it to > > > happen on the first commit. If add_draw_rect has any side effect, then > > > set_fill_with_nonsolid_color would have side effects at the exact same point > > in > > > time? > > > > > > BTW I changed how it works in https://codereview.chromium.org/690493002/ > > > > No, commit is happening just the DetermineIfSolidColor fails because in > > AnalysisCanvas::drawRect, does_cover_canvas is TRUE (device rect and clip rect > > is same in IsFullQuad), hence its assumed to be solid color. > > Have updated the layer bounds and viewport in the test to account for it. > > Does it work without those changes if you do this > https://codereview.chromium.org/671653005/diff/410001/cc/test/fake_content_la... > ? Yea! They work well with your changes in Paint, thanks! I have updated the code to use the viewport and layer bounds as before. Maybe we can land this once https://codereview.chromium.org/671653005/ lands.
I split it out into https://codereview.chromium.org/686113004/ On Wed, Nov 5, 2014 at 2:06 AM, <sohan.jyoti@samsung.com> wrote: > On 2014/11/04 21:54:17, danakj wrote: > >> On 2014/10/31 10:58:52, sohanjg wrote: >> > Please take a look, thanks. >> > >> > >> > > https://codereview.chromium.org/563523002/diff/90001/cc/ > trees/layer_tree_host_unittest.cc > >> > File cc/trees/layer_tree_host_unittest.cc (right): >> > >> > >> > > https://codereview.chromium.org/563523002/diff/90001/cc/ > trees/layer_tree_host_unittest.cc#newcode1058 > >> > cc/trees/layer_tree_host_unittest.cc:1058: // Paint non-solid color. >> > On 2014/10/29 14:27:47, danakj wrote: >> > > On 2014/10/29 08:31:00, sohanjg wrote: >> > > > On 2014/10/22 15:10:19, danakj wrote: >> > > > > On 2014/10/20 07:10:03, sohanjg wrote: >> > > > > > On 2014/10/18 19:39:26, danakj wrote: >> > > > > > > You can use this now >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/cc/test/fake_content_layer_client.h&l=37 > >> > > > > > > >> > > > > > > Just move this all to BeginTest? >> > > > > > >> > > > > > Moved to BeginTest, but set_fill_with_nonsolid_color, doesn't >> work >> > too > >> > > well >> > > > > > here, i may re-look at it later if you want? >> > > > > >> > > > > Why does it not work? >> > > > >> > > > Because CanHaveTilings fail for solid color check even if we use >> this >> > > function. >> > > > That is because we do not perform a UpdateAndExpandInvalidation on >> the >> > > > PicturePile here, which would have done a DetermineIfSolidColor >> > analysis, > >> > and >> > > > updated the PicturePile is_solid_color member. >> > > >> > > Are you saying PictureLayer::Update never happens? I'd surely expect >> it to >> > > happen on the first commit. If add_draw_rect has any side effect, then >> > > set_fill_with_nonsolid_color would have side effects at the exact same >> > point > >> > in >> > > time? >> > > >> > > BTW I changed how it works in https://codereview.chromium. >> org/690493002/ >> > >> > No, commit is happening just the DetermineIfSolidColor fails because in >> > AnalysisCanvas::drawRect, does_cover_canvas is TRUE (device rect and >> clip >> > rect > >> > is same in IsFullQuad), hence its assumed to be solid color. >> > Have updated the layer bounds and viewport in the test to account for >> it. >> > > Does it work without those changes if you do this >> > > https://codereview.chromium.org/671653005/diff/410001/cc/ > test/fake_content_layer_client.cc > >> ? >> > > Yea! They work well with your changes in Paint, thanks! > I have updated the code to use the viewport and layer bounds as before. > Maybe we can land this once https://codereview.chromium.org/671653005/ > lands. > > https://codereview.chromium.org/563523002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:277: virtual void BeginTest() override { remove virtual, override is enough https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:362: if (layer_tree_host()->settings().impl_side_painting) {} https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:364: else {} https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:453: virtual void BeginTest() override { ditto for all the virtuals https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1047: bool is_impl_paint; is_impl_paint_
Can you please take a look, thanks. https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:277: virtual void BeginTest() override { On 2014/11/12 16:44:27, danakj wrote: > remove virtual, override is enough Done. https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:362: if (layer_tree_host()->settings().impl_side_painting) On 2014/11/12 16:44:27, danakj wrote: > {} Done. https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:364: else On 2014/11/12 16:44:27, danakj wrote: > {} Done. https://codereview.chromium.org/563523002/diff/170001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:453: virtual void BeginTest() override { On 2014/11/12 16:44:27, danakj wrote: > ditto for all the virtuals Done.
LGTM https://codereview.chromium.org/563523002/diff/190001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/563523002/diff/190001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:1209: // Paint non-solid color. drop this comment
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/563523002/210001
Message was sent while issue was closed.
Committed patchset #10 (id:210001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/25780da3c293ff9550ebcc83215137334166ee60 Cr-Commit-Position: refs/heads/master@{#304178}
Message was sent while issue was closed.
Description was changed from ========== cc: Change LayerTreeHost unit tests to use impl painting. Make LTH Unittests use PictureLayers instead of ContentLayer. BUG=401492 Committed: https://crrev.com/25780da3c293ff9550ebcc83215137334166ee60 Cr-Commit-Position: refs/heads/master@{#304178} ========== to ========== cc: Change LayerTreeHost unit tests to use impl painting. Make LTH Unittests use PictureLayers instead of ContentLayer. BUG=401492 Committed: https://crrev.com/25780da3c293ff9550ebcc83215137334166ee60 Cr-Commit-Position: refs/heads/master@{#304178} ========== |
