cc: Use maximum tiling contents scales for picture layer scale

cc: Use maximum tiling contents scales for picture layer scale

Due to the way the math works out for calculating which tiles to use for a
given content rect, the content scale for a picture layer has to be the maximum
of all the tilings on a layer.

Currently, each layer only has one tiling, but this change also creates the
infrastructure for adding different tilings over time and clamping contents
scales to a minimum.

NOTRY=true
R=danakj@chromium.org
BUG=155209
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175420

[enne (OOO), 2013-01-06 03:39:59]

This pulls out part of https://codereview.chromium.org/11704002/ (generate more
tilings) so that it can land as a prerequisite to
https://codereview.chromium.org/11678003/ (fix low-res artifacts) and contents
scales can be clamped to a minimum more easily.

[danakj, 2013-01-07 15:57:44]

https://codereview.chromium.org/11777008/diff/1/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/11777008/diff/1/cc/layer_impl.h#newcode201
cc/layer_impl.h:201: virtual void calculateContentsScale(
So, this never actually sets the return values on the impl side right? The
float* and gfx::Size* will always be untouched in the current versions of this
method.

Should we add a comment saying that the draw properties are already set on impl
by push properties, and this is just to inform the layers about the ideal
contents scale?

Is it worth having these pointers here that could let a layer override the
contents scale/bounds on impl and not match the main thread (and also not match
any data from the main thread)?

What's your thinking long-term here?

https://codereview.chromium.org/11777008/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11777008/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:424: gfx::Vector2dF transformScale =
MathUtil::computeTransform2dScaleComponents(combinedTransform,
deviceScaleFactor);
Are you sure you don't want deviceScaleFactor * pageScaleFactor?

For things that are transformed in 3d, they will get fuzzy(er) as you zoom in
otherwise.

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

https://codereview.chromium.org/11777008/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:174: // The content scale and bounds for a
PictureLayerImpl is somewhat fictitous.
nit: "fictitious"

https://codereview.chromium.org/11777008/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:180: float max_contents_scale = 1;
If the largest contents scale is < 1, does this introduce floating point error
that we don't need to introduce if we had the integer space matching the largest
tiling?

1 seems reasonable to me anyhow, just asking.

nit: 1.f or 1.0f

[enne (OOO), 2013-01-07 17:00:01]

https://codereview.chromium.org/11777008/diff/1/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/11777008/diff/1/cc/layer_impl.h#newcode201
cc/layer_impl.h:201: virtual void calculateContentsScale(
On 2013/01/07 15:57:44, danakj wrote:
> So, this never actually sets the return values on the impl side right? The
> float* and gfx::Size* will always be untouched in the current versions of this
> method.
> 
> Should we add a comment saying that the draw properties are already set on
impl
> by push properties, and this is just to inform the layers about the ideal
> contents scale?
> 
> Is it worth having these pointers here that could let a layer override the
> contents scale/bounds on impl and not match the main thread (and also not
match
> any data from the main thread)?
> 
> What's your thinking long-term here?

The whole point of this function is so that a (Picture)LayerImpl can override
the values from the main thread.  This is my long-term thinking at the moment.

You are correct that most layers get their values from push properties and don't
have any need or ability to change their content bounds or content scale.

PTAL at the newest version.  I added a comment and set the out parameters to the
draw properties themselves to make it clearer.

https://codereview.chromium.org/11777008/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11777008/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:424: gfx::Vector2dF transformScale =
MathUtil::computeTransform2dScaleComponents(combinedTransform,
deviceScaleFactor);
On 2013/01/07 15:57:44, danakj wrote:
> Are you sure you don't want deviceScaleFactor * pageScaleFactor?
> 
> For things that are transformed in 3d, they will get fuzzy(er) as you zoom in
> otherwise.

Oh!  Good catch.  Done.

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

https://codereview.chromium.org/11777008/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:174: // The content scale and bounds for a
PictureLayerImpl is somewhat fictitous.
On 2013/01/07 15:57:44, danakj wrote:
> nit: "fictitious"

Done.

https://codereview.chromium.org/11777008/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:180: float max_contents_scale = 1;
On 2013/01/07 15:57:44, danakj wrote:
> If the largest contents scale is < 1, does this introduce floating point error
> that we don't need to introduce if we had the integer space matching the
largest
> tiling?
> 
> 1 seems reasonable to me anyhow, just asking.

Yes, yes.  I'm splitting things out into several patches to make them simpler to
review.  There's no minimum content scale yet; therefore, 1.

> nit: 1.f or 1.0f

Why? Where's that in the style guide? I see both 1.f and 1 used in Chromium.

[danakj, 2013-01-07 17:04:29]

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

https://codereview.chromium.org/11777008/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:180: float max_contents_scale = 1;
On 2013/01/07 17:00:01, enne wrote:
> Why? Where's that in the style guide? I see both 1.f and 1 used in Chromium.

I've been told this by jamesr when gtfo'ing stuff.

I'm not seeing it in the guide.. But I'm pretty sure you'd get a compiler error
(at least on some platforms) if you did this outside cc/ but we have reduced
errors for now. (Implicit conversion to float error.)

[enne (OOO), 2013-01-07 17:11:18]

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

https://codereview.chromium.org/11777008/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:180: float max_contents_scale = 1;
On 2013/01/07 17:04:29, danakj wrote:
> On 2013/01/07 17:00:01, enne wrote:
> > Why? Where's that in the style guide? I see both 1.f and 1 used in Chromium.
> 
> I've been told this by jamesr when gtfo'ing stuff.
> 
> I'm not seeing it in the guide.. But I'm pretty sure you'd get a compiler
error
> (at least on some platforms) if you did this outside cc/ but we have reduced
> errors for now. (Implicit conversion to float error.)

It is certainly the case that if you do something like "float x = 1.3" then you
get implicit conversion to float errors, since 1.3 is a double that can't be
converted losslessly to a float.

On the other hand, 1 can be converted from double to float without losing any
precision, so this shouldn't cause any errors.

git grep "float.*=\s*1;" shows this in ash and content, among other directories.

[danakj, 2013-01-07 17:41:00]

On 2013/01/07 17:11:18, enne wrote:
> https://codereview.chromium.org/11777008/diff/1/cc/picture_layer_impl.cc
> File cc/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/11777008/diff/1/cc/picture_layer_impl.cc#newc...
> cc/picture_layer_impl.cc:180: float max_contents_scale = 1;
> On 2013/01/07 17:04:29, danakj wrote:
> > On 2013/01/07 17:00:01, enne wrote:
> > > Why? Where's that in the style guide? I see both 1.f and 1 used in
Chromium.
> > 
> > I've been told this by jamesr when gtfo'ing stuff.
> > 
> > I'm not seeing it in the guide.. But I'm pretty sure you'd get a compiler
> error
> > (at least on some platforms) if you did this outside cc/ but we have reduced
> > errors for now. (Implicit conversion to float error.)
> 
> It is certainly the case that if you do something like "float x = 1.3" then
you
> get implicit conversion to float errors, since 1.3 is a double that can't be
> converted losslessly to a float.
> 
> On the other hand, 1 can be converted from double to float without losing any
> precision, so this shouldn't cause any errors.
> 
> git grep "float.*=\s*1;" shows this in ash and content, among other
directories.

Ah, here it is:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=0_and_...

[enne (OOO), 2013-01-07 17:46:13]

On 2013/01/07 17:41:00, danakj wrote:

> Ah, here it is:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=0_and_...

Hrm.  Ok, thanks.  Done.

[danakj, 2013-01-07 17:47:42]

Thanks, I like the base class method a lot better now. LGTM

[commit-bot: I haz the power, 2013-01-07 17:49:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11777008/10001

[commit-bot: I haz the power, 2013-01-07 19:06:43]

Retried try job too often on win_aura for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-07 19:16:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11777008/10001

[commit-bot: I haz the power, 2013-01-07 21:57:37]

Retried try job too often on win_aura for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-07 23:21:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/11777008/10001

[commit-bot: I haz the power, 2013-01-08 00:00:21]

Change committed as 175420