cc: don't add low-res tiling to GPU rasterized layers.

cc: don't add low-res tiling to GPU rasterized layers.

R=enne@chromium.org,vmpstr@chromium.org
BUG=332593
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244227

[ernstm, 2014-01-10 00:35:27]

PTAL

[vmpstr, 2014-01-10 00:52:02]

https://codereview.chromium.org/132083007/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/132083007/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:948: if (ShouldHaveLowResTiling() &&
!is_pinching &&
We could technically have a case where after a super quick pinch zoom, we get
that the previous high res contents scale is now a low res contents scale, which
would avoid this check but still have a low res tiling? I mean that's probably
highly unlikely...

Would setting low_res_factor to 1 in GPU raster case around line 1058 in this
file also get rid of the low res tiling?

[enne (OOO), 2014-01-10 01:42:33]

https://codereview.chromium.org/132083007/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/132083007/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:948: if (ShouldHaveLowResTiling() &&
!is_pinching &&
On 2014/01/10 00:52:02, vmpstr wrote:
> We could technically have a case where after a super quick pinch zoom, we get
> that the previous high res contents scale is now a low res contents scale,
which
> would avoid this check but still have a low res tiling? I mean that's probably
> highly unlikely...
> 
> Would setting low_res_factor to 1 in GPU raster case around line 1058 in this
> file also get rid of the low res tiling?

I don't think so.  I think you'd maybe need to add an additional case to
PictureLayerImpl::CleanUpTilingsOnActiveLayer to clean up low res tilings on
layers that shouldn't have low res tilings.  Or remove it here?

[ernstm, 2014-01-10 17:37:42]

https://codereview.chromium.org/132083007/diff/70001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/132083007/diff/70001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1096: // Low resolution can't activate, so only
keep one around.
Is this comment not up to date? It looks like we were keeping all low-res
tilings before my change.

https://codereview.chromium.org/132083007/diff/70001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1097: if (tiling->resolution() == LOW_RESOLUTION
&& ShouldHaveLowResTiling())
This should work, right?

[enne (OOO), 2014-01-10 17:48:57]

lgtm

https://codereview.chromium.org/132083007/diff/70001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/132083007/diff/70001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1096: // Low resolution can't activate, so only
keep one around.
On 2014/01/10 17:37:43, Manfred Ernst wrote:
> Is this comment not up to date? It looks like we were keeping all low-res
> tilings before my change.

We can activate to low res now.  I think the comment is just wrong.

https://codereview.chromium.org/132083007/diff/70001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1097: if (tiling->resolution() == LOW_RESOLUTION
&& ShouldHaveLowResTiling())
On 2014/01/10 17:37:43, Manfred Ernst wrote:
> This should work, right?

Yup.

[ernstm, 2014-01-10 17:53:31]

https://codereview.chromium.org/132083007/diff/70001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/132083007/diff/70001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1096: // Low resolution can't activate, so only
keep one around.
On 2014/01/10 17:48:57, enne wrote:
> On 2014/01/10 17:37:43, Manfred Ernst wrote:
> > Is this comment not up to date? It looks like we were keeping all low-res
> > tilings before my change.
> 
> We can activate to low res now.  I think the comment is just wrong.

I updated the comment.

[commit-bot: I haz the power, 2014-01-10 18:04:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/132083007/110001

[commit-bot: I haz the power, 2014-01-10 20:13:33]

Change committed as 244227