Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
458153002:
cc: Use impl-side painting code in LTH scroll layer test. (Closed)
Descriptioncc: Use impl-side painting code in LTH scroll layer test.
Use PictureLayer instead of ContentLayer, and stop explicitly setting
impl-side painting as false for tests.
BUG=401492
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288968
Patch Set 1 #
Total comments: 2
Patch Set 2 : use PictureLayer instead of Fake. #
Total comments: 2
Patch Set 3 : updated more tests. #
Total comments: 10
Patch Set 4 : remove redundant tests. #Messages
Total messages: 21 (0 generated)
Can you please take a look. Thanks.
LGTM but I think prefer PictureLayer over FakePictureLayer if we don't use anything from the Fake subclass. https://codereview.chromium.org/458153002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest_scroll.cc (right): https://codereview.chromium.org/458153002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest_scroll.cc:452: root_scroll_layer_ = FakePictureLayer::Create(&fake_content_layer_client_); Looks like we don't use anything that FakePictureLayer provides, so you can just use PictureLayer directly for this?
updated. https://codereview.chromium.org/458153002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest_scroll.cc (right): https://codereview.chromium.org/458153002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest_scroll.cc:452: root_scroll_layer_ = FakePictureLayer::Create(&fake_content_layer_client_); On 2014/08/11 14:12:46, danakj wrote: > Looks like we don't use anything that FakePictureLayer provides, so you can just > use PictureLayer directly for this? Done.
Cool LGTM
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/458153002/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Looks like the tests don't pass with this change tho, did you try locally?
The CQ bit was unchecked by danakj@chromium.org
On 2014/08/11 15:58:40, danakj wrote: > The CQ bit was unchecked by mailto:danakj@chromium.org Strange.. i build debug cc_unittests on linux and verified the LayerTreeHostScrollTestLayerStructureChange test. also the 2 tests we had changed here (ScrollDestroyLayer, ScrollDestroyWholeTree), are passing and it fails for lot of other tests, which ideally should not (flakiness?)
We changed the LayerTreeHostScrollTestCaseWithChild in this patch, and I see that failing on the bots. On Mon, Aug 11, 2014 at 12:10 PM, <sohan.jyoti@samsung.com> wrote: > On 2014/08/11 15:58:40, danakj wrote: > >> The CQ bit was unchecked by mailto:danakj@chromium.org >> > > Strange.. > i build debug cc_unittests on linux and verified the > LayerTreeHostScrollTestLayerStructureChange test. > also the 2 tests we had changed here (ScrollDestroyLayer, > ScrollDestroyWholeTree), are passing and it fails > for lot of other tests, which ideally should not (flakiness?) > > https://codereview.chromium.org/458153002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/458153002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_scroll.cc (right): https://codereview.chromium.org/458153002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:564: root_scroll_layer_->content_bounds()); Picture layer's content bounds don't change like Tiled layer's did. You can use FakePictureLayer to make this a FakePictureLayerImpl which allows you to query the HighResTiling()->contents_scale().
On 2014/08/11 16:22:34, danakj wrote: > https://codereview.chromium.org/458153002/diff/20001/cc/trees/layer_tree_host... > File cc/trees/layer_tree_host_unittest_scroll.cc (right): > > https://codereview.chromium.org/458153002/diff/20001/cc/trees/layer_tree_host... > cc/trees/layer_tree_host_unittest_scroll.cc:564: > root_scroll_layer_->content_bounds()); > Picture layer's content bounds don't change like Tiled layer's did. You can use > FakePictureLayer to make this a FakePictureLayerImpl which allows you to query > the HighResTiling()->contents_scale(). Ahh..ok, i was only thinking of the 2 tests, were we changed impl-side painting setting. Sorry, i will fix and update this.
On Mon, Aug 11, 2014 at 12:27 PM, <sohan.jyoti@samsung.com> wrote: > On 2014/08/11 16:22:34, danakj wrote: > > https://codereview.chromium.org/458153002/diff/20001/cc/ > trees/layer_tree_host_unittest_scroll.cc > >> File cc/trees/layer_tree_host_unittest_scroll.cc (right): >> > > > https://codereview.chromium.org/458153002/diff/20001/cc/ > trees/layer_tree_host_unittest_scroll.cc#newcode564 > >> cc/trees/layer_tree_host_unittest_scroll.cc:564: >> root_scroll_layer_->content_bounds()); >> Picture layer's content bounds don't change like Tiled layer's did. You >> can >> > use > >> FakePictureLayer to make this a FakePictureLayerImpl which allows you to >> query >> the HighResTiling()->contents_scale(). >> > > Ahh..ok, i was only thinking of the 2 tests, were we changed impl-side > painting > setting. > Sorry, i will fix and update this. > BTW you can quickly run all cc tests with the --test-launcher-jobs=32 flag, which I almost always do unless debugging a single test, in case you've been avoiding running them all cuz some of them (pixel tests) are a bit slow. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
thanks for your tips about test launcher jobs, they really expedite the test run. i have updated the test, please take a look, hope i understood your comment correct. thanks. https://codereview.chromium.org/458153002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_scroll.cc (right): https://codereview.chromium.org/458153002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:564: root_scroll_layer_->content_bounds()); On 2014/08/11 16:22:34, danakj wrote: > Picture layer's content bounds don't change like Tiled layer's did. You can use > FakePictureLayer to make this a FakePictureLayerImpl which allows you to query > the HighResTiling()->contents_scale(). I hope u mean to make root_scroll_layer_impl a FakePictureLayerImpl, and compare its HighResTiling()->contents_scale with device scale factor ?
Thanks, yep! https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_scroll.cc (right): https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:564: // Ensure device scale factor is affecting the contents scale. I think the comment was fine as is. https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:651: RunTest(true, false, true); This test case is the same as the _ImplSidePaint below now. Let's remove it and remove the _ImplSidePaint qualifier from the twin below. https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:665: RunTest(true, true, true); same https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:735: RunTest(true, false, true); same https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:1238: scroll_destroy_whole_tree_ = true; This should be a 2space indent.. mind fixing it while you're here?
please take a look, thanks! https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_scroll.cc (right): https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:564: // Ensure device scale factor is affecting the contents scale. On 2014/08/12 13:26:46, danakj wrote: > I think the comment was fine as is. Done. https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:651: RunTest(true, false, true); On 2014/08/12 13:26:46, danakj wrote: > This test case is the same as the _ImplSidePaint below now. Let's remove it and > remove the _ImplSidePaint qualifier from the twin below. Done. https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:665: RunTest(true, true, true); On 2014/08/12 13:26:46, danakj wrote: > same Done. https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:735: RunTest(true, false, true); On 2014/08/12 13:26:46, danakj wrote: > same Done. https://codereview.chromium.org/458153002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_scroll.cc:1238: scroll_destroy_whole_tree_ = true; On 2014/08/12 13:26:46, danakj wrote: > This should be a 2space indent.. mind fixing it while you're here? Done. strange why the cl format, didnt take care of this :\
LGTM
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/458153002/60001
Message was sent while issue was closed.
Change committed as 288968 |
