Efficiently handle image layer scaling

Efficiently handle image layer scaling

1. Convert image scale caused by change of width/height to transformation scale in WebImageLayerImpl.
2. Don't scale up/down images when rastering image layers except for minimum contents scale.

BUG=172563
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184464

[Xianzhu, 2013-02-20 23:18:41]

To fix maps pinch-zoom regression.

The first version of patch is for preview.
TODOs:
1. tests
2. origin anchor handling.
and other missed things not seen by me yet.

[Xianzhu, 2013-02-20 23:27:25]

https://codereview.chromium.org/12326022/diff/1/cc/picture_image_layer.cc
File cc/picture_image_layer.cc (right):

https://codereview.chromium.org/12326022/diff/1/cc/picture_image_layer.cc#new...
cc/picture_image_layer.cc:7: #include "cc/math_util.h"
Will remove this line.

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

https://codereview.chromium.org/12326022/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:662: *raster_contents_scale = ideal_contents_scale_;
Will add a blank line.

[Xianzhu, 2013-02-21 01:30:27]

Does the added logic in compositor_bindings layer look good? I'm also preparing
another change that moves the logic into PictureImageLayer.

[enne (OOO), 2013-02-21 02:15:30]

+shawnsingh

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

https://codereview.chromium.org/12326022/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:658: void
PictureLayerImpl::CalculateRasterContentsScale(
Is this supposed to be overridden somewhere? Why did this move?

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:28: // Impl-side paining
can't handle change of image layer bounds effectively.
Typo.

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:31: class
WebLayerImplForPictureImageLayer : public WebLayerImpl {
bikeshed: Can you name this class for its purpose rather than who its callers
are, e.g. WebFixedBoundsLayerImpl?

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:46: // TODO(wangxianzhu):
Transform anchorPoint.
I don't know that you need to transform the anchor point. I thought the anchor
point was in the space of the unit square, so should be unchanged under scaling.
 Maybe shawnsingh can confirm that.

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:111:
m_layer->setTransform(m_scaleTransform * m_originalTransform);
This will not work as written.  Transforms are applied hierarchically and will
also apply (incorrectly) to this layer's children.  You could try to undo it
with the sublayer transform, perhaps?  At any rate, any futzing with the
transform hierarchy needs some unit tests.

[enne (OOO), 2013-02-21 02:18:36]

I also wanted to add that I like the idea of fixing an image layer's bounds to
the bitmap's bounds, since that's really the closest to how content layers
behave.

Where does #2 from the CL description happen in the code?

[Xianzhu, 2013-02-21 02:47:46]

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

https://codereview.chromium.org/12326022/diff/1/cc/picture_layer_impl.cc#newc...
cc/picture_layer_impl.cc:658: void
PictureLayerImpl::CalculateRasterContentsScale(
On 2013/02/21 02:15:30, enne wrote:
> Is this supposed to be overridden somewhere? Why did this move?

Sorry, I forgot to include the new files (picture_image_layer_impl.h/cc) in this
change. Will include in the next patch set.

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:31: class
WebLayerImplForPictureImageLayer : public WebLayerImpl {
On 2013/02/21 02:15:30, enne wrote:
> bikeshed: Can you name this class for its purpose rather than who its callers
> are, e.g. WebFixedBoundsLayerImpl?

I'll create a standalone file for it so that it could be reused by other types
of layers that need the feature.

A bit concern about the naming. WebFixedBoundsLayerImpl looks like an
implementation for a WebKit interface WebFixedBoundsLayer. Would
WebLayerImplFixedBounds be better?

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:46: // TODO(wangxianzhu):
Transform anchorPoint.
On 2013/02/21 02:15:30, enne wrote:
> I don't know that you need to transform the anchor point. I thought the anchor
> point was in the space of the unit square, so should be unchanged under
scaling.
>  Maybe shawnsingh can confirm that.

I knew little about it when wrote the TODO :)
Read GraphicsLayerChromium.cpp and it seems to never set a non-zero anchor or
non-identity transform onto a content layer (including image layer). Is this
right?

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:111:
m_layer->setTransform(m_scaleTransform * m_originalTransform);
On 2013/02/21 02:15:30, enne wrote:
> This will not work as written.  Transforms are applied hierarchically and will
> also apply (incorrectly) to this layer's children.  You could try to undo it
> with the sublayer transform, perhaps?  At any rate, any futzing with the
> transform hierarchy needs some unit tests.

I'd like to keep the situation simple by limiting the feature to layers that
don't have children, like image layers. Does this make sense?

Will write unit tests.

[shawnsingh, 2013-02-21 04:16:42]

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:46: // TODO(wangxianzhu):
Transform anchorPoint.
I can confirm Enne's expectation - anchor point has x and y values in the
interval [0, 1], where the unit square is mapped to the layer's bounds.   also,
in my opinion, looking at where this code exists, perhaps we shouldn't worry
that GraphicsLayerChromium would never set a non-zero anchor or non-identity
transform; we should go ahead and do what seems like a correct general layer API
anyway.  The fact that GraphicsLayerChromium only makes limited use of the
layer's features may change as the codebase evolves.

[Xianzhu, 2013-02-21 17:44:38]

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:46: // TODO(wangxianzhu):
Transform anchorPoint.
On 2013/02/21 04:16:42, shawnsingh wrote:
Thanks for the confirmation.

Two questions:
1. Does it make sense to limit such layers (auto scaling on bounds change and
fixed compositor layer bounds, such as image layers) not to have children? This
would make things simpler.

2. Do we need to care about mask and replica layers when converting bounds scale
to transform scale for an image layer?

[enne (OOO), 2013-02-21 18:35:55]

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:31: class
WebLayerImplForPictureImageLayer : public WebLayerImpl {
On 2013/02/21 02:47:47, Xianzhu wrote:
> On 2013/02/21 02:15:30, enne wrote:
> > bikeshed: Can you name this class for its purpose rather than who its
callers
> > are, e.g. WebFixedBoundsLayerImpl?
> 
> I'll create a standalone file for it so that it could be reused by other types
> of layers that need the feature.
> 
> A bit concern about the naming. WebFixedBoundsLayerImpl looks like an
> implementation for a WebKit interface WebFixedBoundsLayer. Would
> WebLayerImplFixedBounds be better?
> 

WebLayerImplFixedBounds sounds fine to me, thanks.

https://codereview.chromium.org/12326022/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_image_layer_impl.cc:46: // TODO(wangxianzhu):
Transform anchorPoint.
On 2013/02/21 17:44:38, Xianzhu wrote:
> On 2013/02/21 04:16:42, shawnsingh wrote:
> Thanks for the confirmation.
> 
> Two questions:
> 1. Does it make sense to limit such layers (auto scaling on bounds change and
> fixed compositor layer bounds, such as image layers) not to have children?
This
> would make things simpler.

I don't think it would.  I think having to undo the transform when an image
layer had children appended to it would be really awkward.  More awkward than
just applying the inverse transform to the sublayer transform.  You'd have to
write just about the same tests anyway.

> 2. Do we need to care about mask and replica layers when converting bounds
scale
> to transform scale for an image layer?

I don't think so.  I think both replica and mask should still be correct after
these changes to move the scale from bounds to draw transform.  shawnsingh, do
you have thoughts here?

> Read GraphicsLayerChromium.cpp and it seems to never set a non-zero anchor or
> non-identity transform onto a content layer (including image layer). Is this
> right?

I agree with shawnsingh that you cannot count on this behavior.

[shawnsingh, 2013-02-21 19:17:06]

> > 2. Do we need to care about mask and replica layers when converting bounds
> scale
> > to transform scale for an image layer?
> 
> I don't think so.  I think both replica and mask should still be correct after
> these changes to move the scale from bounds to draw transform.  shawnsingh, do
> you have thoughts here?
> 

I think it will be correct, too.   The mask doesn't use any of its own
transforms;  the replica seems to use its owner's transform in order to
determine its own transform, too, which should already have the scale you placed
in there.

[Xianzhu, 2013-02-21 23:03:15]

PTAL

1. Added unit test
2. Undoes bounds scale for sublayerTransform
3. Handles non-zero anchor point when calculating bounds scale (as bounds scale
should always scale from the top-left corner of the layer).

[enne (OOO), 2013-02-22 00:10:49]

https://codereview.chromium.org/12326022/diff/11001/cc/picture_image_layer_im...
File cc/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/11001/cc/picture_image_layer_im...
cc/picture_image_layer_impl.cc:40: if (ideal_contents_scale_ >
I don't understand this math.  If the ideal contents scale is less than minimum
contents scale, then you should not use it for either raster or low_res_raster. 
It's a minimum.

It seems like raster_contents_scale should be ideal_contents_scale clamped
between the minimum and 1.

https://codereview.chromium.org/12326022/diff/11001/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc (right):

https://codereview.chromium.org/12326022/diff/11001/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:16:
TEST(WebLayerImplFixedBoundsTest, IdentityBounds)
Thanks for these tests.  It's hard to tell from looking at numbers what the
expected results are supposed to be.

I think I was looking for tests that verified that changing the fixed bounds of
WebLayerImplFixedBounds did *not* affect the final draw transform of that layer
or one of its children.

In other words, have a WebLayerImplFixedBounds with a WebLayer child.  This will
give you a drawTransform and contentBounds on the cc::Layer backing each of
those WebLayers.  If you transform the gfx::Rect(contentBounds) by the draw
transform, the resulting quad (often a rect) is the layer's rect transformed
into its target surface space.

For both the fixed bounds layers and its child, this result should be unchanged
under modification of the fixed bounds.  Additionally, this result for parent
and child should also be the same regardless of whether you're using a WebLayer
or a WebLayerImplFixedBounds.

Sorry to be picky about tests, but transforms are always tricky to get right,
and I want to be extra confident that this isn't going to regress something
unexpected.

[Xianzhu, 2013-02-22 00:53:30]

https://codereview.chromium.org/12326022/diff/11001/cc/picture_image_layer_im...
File cc/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/11001/cc/picture_image_layer_im...
cc/picture_image_layer_impl.cc:40: if (ideal_contents_scale_ >
On 2013/02/22 00:10:49, enne wrote:
> I don't understand this math.  If the ideal contents scale is less than
minimum
> contents scale, then you should not use it for either raster or
low_res_raster. 
> It's a minimum.
> 
> It seems like raster_contents_scale should be ideal_contents_scale clamped
> between the minimum and 1.

Actually I meant 'ideal_contents_scale_ != minimumContentsScale', i.e. unless
the original bounds is too small (when ideal_contents_scale_ is minimum or is
clamped to minimum), we use the fixed raster_contents_scale=1.

Using ideal_contents_scale_ as raster_contents_scale when it<1 will cause
low-quality image because of the imperfect scaling method in Skia (bug 174628).
In addition as we don't support non-uniform x/y contents scale, rastering will
cause another loss of image quality. Keeping contentsScale=1 will keep all
original information in images, then gpu will do better scaling.

In addition, some page may use a timer to animate the scaling of image which
would be still slow if the scale<1 and we change tiling every time the scale
changes.

If we don't have low-quality image issue, perhaps we can snap the scale to
several predefined levels, e.g. 1, 1/4, 1/16, etc.

https://codereview.chromium.org/12326022/diff/11001/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc (right):

https://codereview.chromium.org/12326022/diff/11001/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:16:
TEST(WebLayerImplFixedBoundsTest, IdentityBounds)
On 2013/02/22 00:10:49, enne wrote:
> Thanks for these tests.  It's hard to tell from looking at numbers what the
> expected results are supposed to be.
> 
> I think I was looking for tests that verified that changing the fixed bounds
of
> WebLayerImplFixedBounds did *not* affect the final draw transform of that
layer
> or one of its children.
> 
> In other words, have a WebLayerImplFixedBounds with a WebLayer child.  This
will
> give you a drawTransform and contentBounds on the cc::Layer backing each of
> those WebLayers.  If you transform the gfx::Rect(contentBounds) by the draw
> transform, the resulting quad (often a rect) is the layer's rect transformed
> into its target surface space.
> 
> For both the fixed bounds layers and its child, this result should be
unchanged
> under modification of the fixed bounds.  Additionally, this result for parent
> and child should also be the same regardless of whether you're using a
WebLayer
> or a WebLayerImplFixedBounds.
> 
> Sorry to be picky about tests, but transforms are always tricky to get right,
> and I want to be extra confident that this isn't going to regress something
> unexpected.

Agreed. Will change the tests.

[enne (OOO), 2013-02-22 01:18:17]

https://codereview.chromium.org/12326022/diff/11001/cc/picture_image_layer_im...
File cc/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/11001/cc/picture_image_layer_im...
cc/picture_image_layer_impl.cc:40: if (ideal_contents_scale_ >
Ok.  Maybe there's more work to be done here before we can have low-res images. 
In that case, shouldn't the math just be:

*raster_contents_scale =
std::max(layerTreeImpl()->settings().minimumContentsScale(), 1.f);

(In practice the minimum won't ever be more than 1, but might as well take it
into account.)

[Xianzhu, 2013-02-22 23:24:38]

PTAL.

1. Added black box tests to ensure WebLayerImplFixedBounds doesn't change the
final geometries.

2. Removed support of non-zero anchor point because I didn't find a way to
correctly support it. Now fall back to non-fixed bounds for non-zero anchor
point.

https://codereview.chromium.org/12326022/diff/11001/cc/picture_image_layer_im...
File cc/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/11001/cc/picture_image_layer_im...
cc/picture_image_layer_impl.cc:40: if (ideal_contents_scale_ >
On 2013/02/22 01:18:18, enne wrote:
> Ok.  Maybe there's more work to be done here before we can have low-res
images. 
> In that case, shouldn't the math just be:
> 
> *raster_contents_scale =
> std::max(layerTreeImpl()->settings().minimumContentsScale(), 1.f);
> 
> (In practice the minimum won't ever be more than 1, but might as well take it
> into account.)

Added std::max in the 'if' block, but still kept the else block for very small
contents scale case.

[enne (OOO), 2013-02-22 23:53:32]

https://codereview.chromium.org/12326022/diff/10002/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/12326022/diff/10002/cc/layer.h#newcode217
cc/layer.h:217: std::string debugName() const { return m_debugName; }
Not needed.

https://codereview.chromium.org/12326022/diff/10002/cc/picture_image_layer_im...
File cc/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/10002/cc/picture_image_layer_im...
cc/picture_image_layer_impl.cc:41: if (ideal_contents_scale_ !=
min_contents_scale) {
No, I think this is still wrong.

min_contents_scale comes from settings and does not change.  If your ideal
contents scale is 2 and your min contents scale is 0.125, you still want to use
1 as the raster scale and not 2 as this code indicates.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds.cc (right):

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds.cc:122: // Fall back to
non-fixed bounds for non-zero anchor point.
Can you make this a TODO, so that it's clear that this is not a limitation of
this approach, just something that isn't handled here yet?

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc (right):

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:28: #define
EXPECT_POINT3F_EQ(p1, p2) \
Can you put these in geometry_test_utils.h?

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:39:
gfx::Point3F transformPoint(const gfx::Transform& transform, const gfx::Point3F&
point)
style nits:

80 columns everywhere in this file, please.
Curly braces don't go on their own lines.
Local variables are named hacker_style.
Function names are StartCaps or hacker_style, depending on their complexity.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:80: void
compareToWebLayerImpl(const WebFloatPoint& anchorPoint,
This function is great.  That's exactly what I was hoping to see, thanks.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:93: gfx::Size
fixedBounds(200, 200); // (160, 70);
Are these debug comments that should be removed?

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:129:
gfx::RectF
fixedBoundsLayerContentRect(fixedBoundsLayer->layer()->contentBounds());
Content rects are not guaranteed to be the same.  If you pick different contents
scales for different layers, then they may differ.

However, the result of transforming the content rect by the transform should be
the unchanging.  You can do this by turning contentBounds into a gfx::Rect and
then calling mapQuad with the draw transform.  The resulting quad should be the
same between the different trees.

[enne (OOO), 2013-02-22 23:55:44]

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc (right):

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:132:
normalLayer->layer()->drawTransform().TransformRect(&normalLayerContentRect);
Oh, scratch my previous comment.  Can you not name this variable
normalLayerContentRect and instead name it something like
normal_layer_rect_in_target? Content rect has a pretty specific meaning, which
is why I misread this code before.

[Xianzhu, 2013-02-23 00:05:20]

https://codereview.chromium.org/12326022/diff/10002/cc/picture_image_layer_im...
File cc/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/10002/cc/picture_image_layer_im...
cc/picture_image_layer_impl.cc:41: if (ideal_contents_scale_ !=
min_contents_scale) {
On 2013/02/22 23:53:32, enne wrote:
> No, I think this is still wrong.
> 
> min_contents_scale comes from settings and does not change.  If your ideal
> contents scale is 2 and your min contents scale is 0.125, you still want to
use
> 1 as the raster scale and not 2 as this code indicates.

Yes, this is the case in the 'if' block (normally std::max(1,
min_contents_scale) == 1)

However, when ideal_contents_scale = 0.125, 0.125 instead of 1.0 is used to save
memory (the 'else' block)

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc (right):

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:39:
gfx::Point3F transformPoint(const gfx::Transform& transform, const gfx::Point3F&
point)
On 2013/02/22 23:53:32, enne wrote:
> style nits:
> 
> 80 columns everywhere in this file, please.
> Curly braces don't go on their own lines.
> Local variables are named hacker_style.
> Function names are StartCaps or hacker_style, depending on their complexity.

I thought all files under the directory should follow WebKit style. Is this an
intermediate state after they were moved out of WebKit? If yes, I will change
all the added files into chromium style.

[jamesr, 2013-02-23 00:54:32]

On 2013/02/23 00:05:20, Xianzhu wrote:
> webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:39:
> gfx::Point3F transformPoint(const gfx::Transform& transform, const
gfx::Point3F&
> point)
> On 2013/02/22 23:53:32, enne wrote:
> > style nits:
> > 
> > 80 columns everywhere in this file, please.
> > Curly braces don't go on their own lines.
> > Local variables are named hacker_style.
> > Function names are StartCaps or hacker_style, depending on their complexity.
> 
> I thought all files under the directory should follow WebKit style. Is this an
> intermediate state after they were moved out of WebKit? If yes, I will change
> all the added files into chromium style.

We're in an inconsistent state here.  New code should be Chromium style, edits
to existing files should follow the prevailing style of that file.

[Xianzhu, 2013-02-23 00:56:19]

https://codereview.chromium.org/12326022/diff/10002/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/12326022/diff/10002/cc/layer.h#newcode217
cc/layer.h:217: std::string debugName() const { return m_debugName; }
On 2013/02/22 23:53:32, enne wrote:
> Not needed.

Done.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds.cc (right):

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds.cc:122: // Fall back to
non-fixed bounds for non-zero anchor point.
On 2013/02/22 23:53:32, enne wrote:
> Can you make this a TODO, so that it's clear that this is not a limitation of
> this approach, just something that isn't handled here yet?

Done.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc (right):

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:28: #define
EXPECT_POINT3F_EQ(p1, p2) \
On 2013/02/22 23:53:32, enne wrote:
> Can you put these in geometry_test_utils.h?

Done.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:39:
gfx::Point3F transformPoint(const gfx::Transform& transform, const gfx::Point3F&
point)
Converted this file into Chromium style.

Still keep web_layer_impl_fixed_bounds.h and cc in WebKit style as other
existing files are in the style and they are in WebKit namespace.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:93: gfx::Size
fixedBounds(200, 200); // (160, 70);
On 2013/02/22 23:53:32, enne wrote:
> Are these debug comments that should be removed?

Done.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:132:
normalLayer->layer()->drawTransform().TransformRect(&normalLayerContentRect);
On 2013/02/22 23:55:44, enne wrote:
> Oh, scratch my previous comment.  Can you not name this variable
> normalLayerContentRect and instead name it something like
> normal_layer_rect_in_target? Content rect has a pretty specific meaning, which
> is why I misread this code before.

Done.

[enne (OOO), 2013-02-23 01:10:55]

https://codereview.chromium.org/12326022/diff/10002/cc/picture_image_layer_im...
File cc/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/10002/cc/picture_image_layer_im...
cc/picture_image_layer_impl.cc:41: if (ideal_contents_scale_ !=
min_contents_scale) {
On 2013/02/23 00:05:20, Xianzhu wrote:
> On 2013/02/22 23:53:32, enne wrote:
> > No, I think this is still wrong.
> > 
> > min_contents_scale comes from settings and does not change.  If your ideal
> > contents scale is 2 and your min contents scale is 0.125, you still want to
> use
> > 1 as the raster scale and not 2 as this code indicates.
> 
> Yes, this is the case in the 'if' block (normally std::max(1,
> min_contents_scale) == 1)
> 
> However, when ideal_contents_scale = 0.125, 0.125 instead of 1.0 is used to
save
> memory (the 'else' block)

You convinced me in comment #13 that having low res for image layers was not a
good idea at this time until some bugs were fixed.  Therefore, I think you
should remove the if/else and always do the code in the 'if' block.

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
File webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc (right):

https://codereview.chromium.org/12326022/diff/10002/webkit/compositor_binding...
webkit/compositor_bindings/web_layer_impl_fixed_bounds_unittest.cc:39:
gfx::Point3F transformPoint(const gfx::Transform& transform, const gfx::Point3F&
point)
On 2013/02/23 00:56:19, Xianzhu wrote:
> Converted this file into Chromium style.
> 
> Still keep web_layer_impl_fixed_bounds.h and cc in WebKit style as other
> existing files are in the style and they are in WebKit namespace.

As jamesr says, new code should be Chromium style, edits
to existing files should follow the prevailing style of that file.

Please don't add new files in WebKit style.

[Xianzhu, 2013-02-23 01:11:12]

PTAL. All new files have been converted into Chromium style.

[Xianzhu, 2013-02-23 01:17:19]

https://codereview.chromium.org/12326022/diff/10002/cc/picture_image_layer_im...
File cc/picture_image_layer_impl.cc (right):

https://codereview.chromium.org/12326022/diff/10002/cc/picture_image_layer_im...
cc/picture_image_layer_impl.cc:41: if (ideal_contents_scale_ !=
min_contents_scale) {
Done :)

[enne (OOO), 2013-02-23 04:24:18]

lgtm

Thanks again for taking the time to write those tests.  This makes me feel a lot
better about this change not being risky.

[commit-bot: I haz the power, 2013-02-23 05:48:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12326022/17019

[commit-bot: I haz the power, 2013-02-23 06:34:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-23 06:57:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12326022/17019

[commit-bot: I haz the power, 2013-02-23 07:01:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-25 16:48:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12326022/4008

[commit-bot: I haz the power, 2013-02-25 19:59:22]

Change committed as 184464