Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
330003003:
cc: Pass ideal contents scale to PictureLayerTiling UpdateTilePriorities. (Closed)
Descriptioncc: Pass ideal contents scale to PictureLayerTiling UpdateTilePriorities.
Avoid passing MaximumContentsScale and pass ideal contents scale while
updating tile priorities from PictureLayerImpl, and fix related test.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280409
Patch Set 1 #Patch Set 2 : updated test #
Total comments: 12
Patch Set 3 : review comments addressed. #
Messages
Total messages: 14 (1 generated)
Can you please take a look. https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (left): https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2067: EXPECT_TRUE(reached_required); As mentioned above, this check is not required. https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:1998: } I think we shouldnt be iterating over all the tiles, instead only in the visible content rect for activating tiles here, hence the CoverageIterator. https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2046: This check becomes redundant now, because the tiles activated are not iterated by the LayerEvictionTileIterator, not sure whether LayerEvictionTileIterator should have some visible rect concept ? I can remove it, if you want.
On 2014/06/20 13:35:20, sohanjg wrote: > Can you please take a look. > > https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... > File cc/layers/picture_layer_impl_unittest.cc (left): > > https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... > cc/layers/picture_layer_impl_unittest.cc:2067: EXPECT_TRUE(reached_required); > As mentioned above, this check is not required. > > https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... > File cc/layers/picture_layer_impl_unittest.cc (right): > > https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... > cc/layers/picture_layer_impl_unittest.cc:1998: } > I think we shouldnt be iterating over all the tiles, instead only in the visible > content rect for activating tiles here, hence the CoverageIterator. > > https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... > cc/layers/picture_layer_impl_unittest.cc:2046: > This check becomes redundant now, because the tiles activated are not iterated > by the LayerEvictionTileIterator, not sure whether LayerEvictionTileIterator > should have some visible rect concept ? > I can remove it, if you want. enne@ can you please have a look at this one too? Thanks.
https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (left): https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2067: EXPECT_TRUE(reached_required); On 2014/06/20 13:35:20, sohanjg wrote: > As mentioned above, this check is not required. This still seems valid to me. I don't understand. https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:1998: } On 2014/06/20 13:35:20, sohanjg wrote: > I think we shouldnt be iterating over all the tiles, instead only in the visible > content rect for activating tiles here, hence the CoverageIterator. I think that's an ok change. This is just a test, so... Can you count how many things are marked and how many things are not marked and DCHECK that there's at least one thing in each category? (i.e. test that the test is testing something?) https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2046: On 2014/06/20 13:35:20, sohanjg wrote: > This check becomes redundant now, because the tiles activated are not iterated > by the LayerEvictionTileIterator, not sure whether LayerEvictionTileIterator > should have some visible rect concept ? > I can remove it, if you want. I don't understand why this is redundant.
https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2046: On 2014/06/25 18:33:07, enne wrote: > On 2014/06/20 13:35:20, sohanjg wrote: > > This check becomes redundant now, because the tiles activated are not iterated > > by the LayerEvictionTileIterator, not sure whether LayerEvictionTileIterator > > should have some visible rect concept ? > > I can remove it, if you want. > > I don't understand why this is redundant. Because none of the tiles iterated over by layerevictioniterator here are activated by the CoverageIterator above, so neither of the if-else block is executed, and consequently the reached_required dcheck becomes redundant. So i was thinking whether eviction iterator shud have a visible/content rect concept ?
https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2046: On 2014/06/25 20:17:53, sohanjg wrote: > On 2014/06/25 18:33:07, enne wrote: > > On 2014/06/20 13:35:20, sohanjg wrote: > > > This check becomes redundant now, because the tiles activated are not > iterated > > > by the LayerEvictionTileIterator, not sure whether LayerEvictionTileIterator > > > should have some visible rect concept ? > > > I can remove it, if you want. > > > > I don't understand why this is redundant. > > Because none of the tiles iterated over by layerevictioniterator here are > activated by the CoverageIterator above, so neither of the if-else block is > executed, and consequently the reached_required dcheck becomes redundant. > So i was thinking whether eviction iterator shud have a visible/content rect > concept ? The iterator above is marking some of the tiles as required. The eviction iterator needs to take *all* tiles into account. Both of those if-else statements inside the eviction iterator should be hit in order for this test to properly test what it's supposed to be doing?
https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:1998: } On 2014/06/25 18:33:07, enne wrote: > On 2014/06/20 13:35:20, sohanjg wrote: > > I think we shouldnt be iterating over all the tiles, instead only in the > visible > > content rect for activating tiles here, hence the CoverageIterator. > > I think that's an ok change. This is just a test, so... > > Can you count how many things are marked and how many things are not marked and > DCHECK that there's at least one thing in each category? (i.e. test that the > test is testing something?) I agree with this change. I was considering making that myself, since required tiles might be handled differently later on, and I would prefer to have an assurance that those tiles would only be in the visible rect. https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2046: On 2014/06/25 20:20:05, enne wrote: > On 2014/06/25 20:17:53, sohanjg wrote: > > On 2014/06/25 18:33:07, enne wrote: > > > On 2014/06/20 13:35:20, sohanjg wrote: > > > > This check becomes redundant now, because the tiles activated are not > > iterated > > > > by the LayerEvictionTileIterator, not sure whether > LayerEvictionTileIterator > > > > should have some visible rect concept ? > > > > I can remove it, if you want. > > > > > > I don't understand why this is redundant. > > > > Because none of the tiles iterated over by layerevictioniterator here are > > activated by the CoverageIterator above, so neither of the if-else block is > > executed, and consequently the reached_required dcheck becomes redundant. > > So i was thinking whether eviction iterator shud have a visible/content rect > > concept ? > > The iterator above is marking some of the tiles as required. The eviction > iterator needs to take *all* tiles into account. Both of those if-else > statements inside the eviction iterator should be hit in order for this test to > properly test what it's supposed to be doing? This part of the loop break out as soon as a now bin is visited, and now bin tiles are visited in the loop below. I'd like to see the if-else block replaced with an EXPECT_FALSE(tile->required_for_activation()) instead.
Please take a look, Thanks. https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:1998: } On 2014/06/25 21:08:52, vmpstr wrote: > On 2014/06/25 18:33:07, enne wrote: > > On 2014/06/20 13:35:20, sohanjg wrote: > > > I think we shouldnt be iterating over all the tiles, instead only in the > > visible > > > content rect for activating tiles here, hence the CoverageIterator. > > > > I think that's an ok change. This is just a test, so... > > > > Can you count how many things are marked and how many things are not marked > and > > DCHECK that there's at least one thing in each category? (i.e. test that the > > test is testing something?) > > I agree with this change. I was considering making that myself, since required > tiles might be handled differently later on, and I would prefer to have an > assurance that those tiles would only be in the visible rect. Done. https://codereview.chromium.org/330003003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2046: On 2014/06/25 21:08:52, vmpstr wrote: > On 2014/06/25 20:20:05, enne wrote: > > On 2014/06/25 20:17:53, sohanjg wrote: > > > On 2014/06/25 18:33:07, enne wrote: > > > > On 2014/06/20 13:35:20, sohanjg wrote: > > > > > This check becomes redundant now, because the tiles activated are not > > > iterated > > > > > by the LayerEvictionTileIterator, not sure whether > > LayerEvictionTileIterator > > > > > should have some visible rect concept ? > > > > > I can remove it, if you want. > > > > > > > > I don't understand why this is redundant. > > > > > > Because none of the tiles iterated over by layerevictioniterator here are > > > activated by the CoverageIterator above, so neither of the if-else block is > > > executed, and consequently the reached_required dcheck becomes redundant. > > > So i was thinking whether eviction iterator shud have a visible/content rect > > > concept ? > > > > The iterator above is marking some of the tiles as required. The eviction > > iterator needs to take *all* tiles into account. Both of those if-else > > statements inside the eviction iterator should be hit in order for this test > to > > properly test what it's supposed to be doing? > > This part of the loop break out as soon as a now bin is visited, and now bin > tiles are visited in the loop below. I'd like to see the if-else block replaced > with an EXPECT_FALSE(tile->required_for_activation()) instead. Done.
lgtm if it looks good to vmpstr
On 2014/06/26 17:42:58, enne wrote: > lgtm if it looks good to vmpstr Vlad ? Are the changes ok with you ?
On 2014/06/27 03:05:21, sohanjg wrote: > On 2014/06/26 17:42:58, enne wrote: > > lgtm if it looks good to vmpstr > > Vlad ? Are the changes ok with you ? Oh sorry, yes this lgtm.
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/330003003/40001
Message was sent while issue was closed.
Change committed as 280409
Message was sent while issue was closed.
Description was changed from ========== cc: Pass ideal contents scale to PictureLayerTiling UpdateTilePriorities. Avoid passing MaximumContentsScale and pass ideal contents scale while updating tile priorities from PictureLayerImpl, and fix related test. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280409 ========== to ========== cc: Pass ideal contents scale to PictureLayerTiling UpdateTilePriorities. Avoid passing MaximumContentsScale and pass ideal contents scale while updating tile priorities from PictureLayerImpl, and fix related test. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280409 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
