Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
322793002:
cc: Use LayerImpl instead of TiledLayerImpl in LTHI test. (Closed)
|
Created:
6 years, 6 months ago by sohanjg Modified:
5 years, 1 month ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptioncc: Use LayerImpl instead of TiledLayerImpl in LTHI test.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277075
Patch Set 1 #Patch Set 2 : remove unused includes #
Total comments: 2
Patch Set 3 : Updated tests for incomplete tiles. #
Total comments: 13
Patch Set 4 : review comments updated #
Total comments: 6
Patch Set 5 : review comments updated #
Total comments: 1
Patch Set 6 : Incomplete test name #Messages
Total messages: 12 (1 generated)
https://codereview.chromium.org/322793002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/322793002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:1888: append_quads_data->num_missing_tiles = true; num_missing_tiles is not a bool it's a count. it seems like we need two variables like tile_missing_, to set these 2 independently, so we can test each of them, rather than conflating the two.
Updated the tests with incomplete tile tests too. Please take a look. Thanks. https://codereview.chromium.org/322793002/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/322793002/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:1888: append_quads_data->num_missing_tiles = true; On 2014/06/09 17:03:40, danakj wrote: > num_missing_tiles is not a bool it's a count. > > it seems like we need two variables like tile_missing_, to set these 2 > independently, so we can test each of them, rather than conflating the two. Done.
https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2156: EXPECT_EQ(DRAW_SUCCESS, host_impl_->PrepareToDraw(&frame)); The comment above does not agree with this assertion.
https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:1899: ResourceProvider::ResourceId resource = we don't use this resource anymore, it can go away https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:1940: PrepareToDrawSucceedsWhenNoTexturesMissingAndIncompleteTile) { This is the same test as PrepareToDrawSucceedsWithNonAnimatedIncompleteTile, so you can remove it. https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2045: bool had_incomplete_tile = true; add a case for this being false please https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2063: PrepareToDrawSucceedsWithAnimationAndNoMissingTextureAndNoIncompleteTile) { This is the same as PrepareToDrawSucceedsWithAnimatedLayer, so you can remove this case https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2087: PrepareToDrawFailsWhenHighResRequiredButNoMissingTexturesAndIncompleteTile) { This says fails, but the draw is succeeding. And this says IncompleteTile, but you're setting it to false. I think this test name should stay the same as it was before, and then the rest of this test case is okay. https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2156: EXPECT_EQ(DRAW_SUCCESS, host_impl_->PrepareToDraw(&frame)); On 2014/06/10 18:35:09, enne wrote: > The comment above does not agree with this assertion. Ya, we do draw when we're missing a tile entirely, the test name and comment need fixing here.
I have updated the tests, and have removed the comments and updated the test descriptions to be cleaner. Please take a look. Thanks. https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:1899: ResourceProvider::ResourceId resource = On 2014/06/10 19:41:51, danakj wrote: > we don't use this resource anymore, it can go away Done. https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:1940: PrepareToDrawSucceedsWhenNoTexturesMissingAndIncompleteTile) { On 2014/06/10 19:41:51, danakj wrote: > This is the same test as PrepareToDrawSucceedsWithNonAnimatedIncompleteTile, so > you can remove it. Done. https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2045: bool had_incomplete_tile = true; On 2014/06/10 19:41:51, danakj wrote: > add a case for this being false please Done. https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2063: PrepareToDrawSucceedsWithAnimationAndNoMissingTextureAndNoIncompleteTile) { On 2014/06/10 19:41:51, danakj wrote: > This is the same as PrepareToDrawSucceedsWithAnimatedLayer, so you can remove > this case Done. https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2087: PrepareToDrawFailsWhenHighResRequiredButNoMissingTexturesAndIncompleteTile) { On 2014/06/10 19:41:51, danakj wrote: > This says fails, but the draw is succeeding. And this says IncompleteTile, but > you're setting it to false. > > I think this test name should stay the same as it was before, and then the rest > of this test case is okay. Done. https://codereview.chromium.org/322793002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2156: EXPECT_EQ(DRAW_SUCCESS, host_impl_->PrepareToDraw(&frame)); On 2014/06/10 19:41:51, danakj wrote: > On 2014/06/10 18:35:09, enne wrote: > > The comment above does not agree with this assertion. > > Ya, we do draw when we're missing a tile entirely, the test name and comment > need fixing here. Done.
Cool, thanks. A couple tiny things in the last patch set. https://codereview.chromium.org/322793002/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/322793002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2003: bool had_incomplete_tile = true; false here https://codereview.chromium.org/322793002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2024: bool tile_missing = true; false here https://codereview.chromium.org/322793002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2025: bool had_incomplete_tile = false; true here
Please take a look. Thanks. https://codereview.chromium.org/322793002/diff/60001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/322793002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2003: bool had_incomplete_tile = true; On 2014/06/11 17:51:03, danakj wrote: > false here Done. https://codereview.chromium.org/322793002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2024: bool tile_missing = true; On 2014/06/11 17:51:03, danakj wrote: > false here Done. https://codereview.chromium.org/322793002/diff/60001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2025: bool had_incomplete_tile = false; On 2014/06/11 17:51:03, danakj wrote: > true here Done.
LGTM thanks, make sure you use the full PrepareToDraw name in the test name though https://codereview.chromium.org/322793002/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/322793002/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:2021: PrepareToSucceedsWithAnimationAndIncompleteTiles) { PrepareToDrawSucceeds...
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/322793002/100001
Message was sent while issue was closed.
Change committed as 277075
Message was sent while issue was closed.
Description was changed from ========== cc: Use LayerImpl instead of TiledLayerImpl in LTHI test. BUG=None Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277075 ========== to ========== cc: Use LayerImpl instead of TiledLayerImpl in LTHI test. BUG=None Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277075 ========== |
