Mark layers that can use LCD text based on layer transform and opacity.

Mark layers that can use LCD text based on layer transform and opacity.
- Text AA settings are not adjusted during animation to avoid repaints.
- Renamed Layer::useLCDText to Layer::canUseLCDText.


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

[alokp, 2012-11-07 17:32:30]

Adrienne/Dana: This is ready for review.

[danakj, 2012-11-07 18:15:00]

Thanks Alok, looks pretty good! I'd like to make this a bit more general, and
let chromium choose to use LCD text rather than hard-coding it in the
compositor.

Can you list the unit test coverage in the commit description as well? Thanks.

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:558: bool layerCanUseLCDText = canAdjustTextAA ?
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation() :
false;
This statement is weird to me. It is essentially:

layerCanUseLCDText = canAdjustTextAA && canUseLCDText && (drawOpacity == 1.0) &&
drawTransform.isIntegerTranslation()

If that is what you mean, can you write it that way instead of with a
conditional? But it seems a little odd.

If a surface-owning layer is animating opacity, then canUseLCDText will become
false.

Within the surface, opacity won't be animating, but canUseLCDText is false,
because of the surface "cant adjust AA".

In the end, I think the result would be right, but it would be the same with or
without the canAdjustTextAA included in this statement here.

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common.h
File cc/layer_tree_host_common.h (right):

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common.h...
cc/layer_tree_host_common.h:23: // Returns the default LCD text setting.
Can we make this a LayerTreeSetting instead? Other platforms (eg highdpi ones)
will like to use this as well. That would allow chromium to set it
appropriately.

I guess leave the ifdef on the LayerTreeSetting for now, but then file a bug for
android to set the value from outside the compositor, and remove the ifdef?

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
File cc/layer_tree_host_common_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4095: root->setOpacity(0.5);
The root layer owns a render surface, so this isn't testing what you are
expecting it to test. Can you make a child layer of the root, and do this entire
test on that child instead?

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4106: TEST(LayerTreeHostCommonTest,
verifyCanUseLCDTextForLayerTree)
Seems like this is testing "...ForRenderSurface" more than "...ForLayerTree"?

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4108: scoped_refptr<Layer> root =
Layer::create();
Can you make a child of the root, that owns a render surface
(setForceRenderSurface(true)), and test with that instead of testing the root
layer? That will more directly mimic real life.

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4151: scoped_refptr<Layer> root =
Layer::create();
Again, please use a child of the root to be more robust.

http://codereview.chromium.org/11360093/diff/1014/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

http://codereview.chromium.org/11360093/diff/1014/cc/tiled_layer.cc#newcode285
cc/tiled_layer.cc:285: DCHECK(!useLCDText);
This won't work well with making it a LayerTreeSetting.

Maybe DCHECK(layerTreeHost()->settings().canUseLCDText || !useLCDText) ?

http://codereview.chromium.org/11360093/diff/1014/cc/tiled_layer_unittest.cc
File cc/tiled_layer_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/1014/cc/tiled_layer_unittest.cc#...
cc/tiled_layer_unittest.cc:1430: EXPECT_EQ(4,
child->fakeLayerUpdater()->updateCount());
Why did this happen?

[alokp, 2012-11-08 22:34:07]

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:558: bool layerCanUseLCDText = canAdjustTextAA ?
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation() :
false;
You are right. The logic to NOT mess with text AA setting while animation is
active was not correct. To fix this I had to add a
Layer::screenSpaceOpacityisAnimating.

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common.h
File cc/layer_tree_host_common.h (right):

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common.h...
cc/layer_tree_host_common.h:23: // Returns the default LCD text setting.
On 2012/11/07 18:15:00, danakj wrote:
> Can we make this a LayerTreeSetting instead? Other platforms (eg highdpi ones)
> will like to use this as well. That would allow chromium to set it
> appropriately.
> 
> I guess leave the ifdef on the LayerTreeSetting for now, but then file a bug
for
> android to set the value from outside the compositor, and remove the ifdef?

Done.

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
File cc/layer_tree_host_common_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4095: root->setOpacity(0.5);
On 2012/11/07 18:15:00, danakj wrote:
> The root layer owns a render surface, so this isn't testing what you are
> expecting it to test. Can you make a child layer of the root, and do this
entire
> test on that child instead?

Done.

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4106: TEST(LayerTreeHostCommonTest,
verifyCanUseLCDTextForLayerTree)
On 2012/11/07 18:15:00, danakj wrote:
> Seems like this is testing "...ForRenderSurface" more than "...ForLayerTree"?

Done.

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4108: scoped_refptr<Layer> root =
Layer::create();
On 2012/11/07 18:15:00, danakj wrote:
> Can you make a child of the root, that owns a render surface
> (setForceRenderSurface(true)), and test with that instead of testing the root
> layer? That will more directly mimic real life.

Done.

http://codereview.chromium.org/11360093/diff/1014/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4151: scoped_refptr<Layer> root =
Layer::create();
On 2012/11/07 18:15:00, danakj wrote:
> Again, please use a child of the root to be more robust.

Done.

http://codereview.chromium.org/11360093/diff/1014/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

http://codereview.chromium.org/11360093/diff/1014/cc/tiled_layer.cc#newcode285
cc/tiled_layer.cc:285: DCHECK(!useLCDText);
I realized that if-else block is not needed anymore. For Android useLCDText is
always false, so it will always get border texels.

http://codereview.chromium.org/11360093/diff/1014/cc/tiled_layer_unittest.cc
File cc/tiled_layer_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/1014/cc/tiled_layer_unittest.cc#...
cc/tiled_layer_unittest.cc:1430: EXPECT_EQ(4,
child->fakeLayerUpdater()->updateCount());
On 2012/11/07 18:15:00, danakj wrote:
> Why did this happen?

Because now we call setCanUseLCDText during updateLayers, which in turn might
reset tile textures and hence change the update count. I also noticed that there
were more unit-test failures on Android because many tests assume
NoBorderTexels. In the new patch, I workaround this problem by overriding
setCanUseLCDText for FakeTiledLayer.

[danakj, 2012-11-10 01:12:35]

Thanks for the changes, I like it as a LayerTreeSetting a lot more. A couple
more questions.

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:553: bool layerCanUseLCDText = adjustTextAA &&
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation();
I still don't understand why adjustTextAA is part of this variable though.

It says "the layer cannot use LCD text if it's unable to change it's lcd text
setting". And it propagates the adjustTextAA down to the surface's subtree
inside here.

Shouldn't these 2 variables be independent?

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:604: layer->setDrawTransformIsAnimating(false);
Can you undo this unrelated change?

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common_u...
File cc/layer_tree_host_common_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4092: bool canUseLCDText = true;
Can you add a simple test or parameterize this to verify layer's don't
canUseLCDText() when this is false, for our android friends?

http://codereview.chromium.org/11360093/diff/3028/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/tiled_layer.cc#newcode282
cc/tiled_layer.cc:282: setBorderTexelOption(useLCDText ?
LayerTilingData::NoBorderTexels : LayerTilingData::HasBorderTexels);
Is this change needed here, since useLCDText should always be false anyways, due
to the other ifdef? This seems like it better belongs in the CL to untangle
these two things?

[alokp, 2012-11-12 01:04:45]

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:553: bool layerCanUseLCDText = adjustTextAA &&
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation();
Sorry for the confusion. adjustTextAA and canUseLCDText are in fact independent.
adjustTextAA is part of the expression just to avoid evaluating the whole
expression, especially drawTransform.isIntegerTranslation, which might be
expensive. It does not matter what the value of canUseLCDText is when
adjustTextAA is true.

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:604: layer->setDrawTransformIsAnimating(false);
It is not entirely unrelated. It makes transform handling consistent with
opacity. But yeah, not necessary. I can undo it if you insist.

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common_u...
File cc/layer_tree_host_common_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4092: bool canUseLCDText = true;
On 2012/11/10 01:12:35, danakj wrote:
> Can you add a simple test or parameterize this to verify layer's don't
> canUseLCDText() when this is false, for our android friends?

Done.

http://codereview.chromium.org/11360093/diff/3028/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/tiled_layer.cc#newcode282
cc/tiled_layer.cc:282: setBorderTexelOption(useLCDText ?
LayerTilingData::NoBorderTexels : LayerTilingData::HasBorderTexels);
Not necessary, but now there is no need for this code anymore. Why have
unnecessary code?

[danakj, 2012-11-12 01:25:47]

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:553: bool layerCanUseLCDText = adjustTextAA &&
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation();
On 2012/11/12 01:04:45, Alok Priyadarshi wrote:
> Sorry for the confusion. adjustTextAA and canUseLCDText are in fact
independent.
> adjustTextAA is part of the expression just to avoid evaluating the whole
> expression, especially drawTransform.isIntegerTranslation, which might be
> expensive. It does not matter what the value of canUseLCDText is when
> adjustTextAA is true.

Ya, I think having the layerCanUseLCDText variable is great. But why isn't it:
bool layerCanUseLCDText = canUseLCDText && (drawOpacity == 1.0) &&
drawTransform.isIntegerTranslation(); ?

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:604: layer->setDrawTransformIsAnimating(false);
On 2012/11/12 01:04:45, Alok Priyadarshi wrote:
> It is not entirely unrelated. It makes transform handling consistent with
> opacity. But yeah, not necessary. I can undo it if you insist.

If any code later in the method wants to check animatingTransformToTarget, then
it will be wrong if we don't change the local variablt here as well. That's why
I think we should still set animatingTransformToTarget = false here.

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common_u...
File cc/layer_tree_host_common_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common_u...
cc/layer_tree_host_common_unittest.cc:4092: bool canUseLCDText = true;
On 2012/11/12 01:04:45, Alok Priyadarshi wrote:
> On 2012/11/10 01:12:35, danakj wrote:
> > Can you add a simple test or parameterize this to verify layer's don't
> > canUseLCDText() when this is false, for our android friends?
> 
> Done.

Awesome, thanks.

http://codereview.chromium.org/11360093/diff/3028/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/tiled_layer.cc#newcode282
cc/tiled_layer.cc:282: setBorderTexelOption(useLCDText ?
LayerTilingData::NoBorderTexels : LayerTilingData::HasBorderTexels);
On 2012/11/12 01:04:45, Alok Priyadarshi wrote:
> Not necessary, but now there is no need for this code anymore. Why have
> unnecessary code?

Yah, Ok :)

[alokp, 2012-11-12 03:58:42]

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:553: bool layerCanUseLCDText = adjustTextAA &&
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation();
> But why isn't it:
> bool layerCanUseLCDText = canUseLCDText && (drawOpacity == 1.0) &&
> drawTransform.isIntegerTranslation(); ?

It is essentially that. adjustTextAA is part of the expression just to avoid
evaluating "(drawOpacity == 1.0) && drawTransform.isIntegerTranslation()" when
adjustTextAA is false.

It is the short form of:
bool layerCanUseLCDText = false;
if (adjustTextAA) {
  layerCanUseLCDText = canUseLCDText && (drawOpacity == 1.0) &&
drawTransform.isIntegerTranslation();
} else {
  // No need to compute layerCanUseLCDText because it will not be set on layers
anyway.
}

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:604: layer->setDrawTransformIsAnimating(false);
reverted.

[danakj, 2012-11-12 05:55:03]

LGTM

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:553: bool layerCanUseLCDText = adjustTextAA &&
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation();
On 2012/11/12 03:58:42, Alok Priyadarshi wrote:
> > But why isn't it:
> > bool layerCanUseLCDText = canUseLCDText && (drawOpacity == 1.0) &&
> > drawTransform.isIntegerTranslation(); ?
> 
> It is essentially that. adjustTextAA is part of the expression just to avoid
> evaluating "(drawOpacity == 1.0) && drawTransform.isIntegerTranslation()" when
> adjustTextAA is false.
> 
> It is the short form of:
> bool layerCanUseLCDText = false;
> if (adjustTextAA) {
>   layerCanUseLCDText = canUseLCDText && (drawOpacity == 1.0) &&
> drawTransform.isIntegerTranslation();
> } else {
>   // No need to compute layerCanUseLCDText because it will not be set on
layers
> anyway.
> }

Ohh okay, I see. 

Can you please add a comment explaining that then for posterity?

[alokp, 2012-11-12 16:50:10]

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/3028/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:553: bool layerCanUseLCDText = adjustTextAA &&
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation();
On 2012/11/12 05:55:03, danakj wrote:
> On 2012/11/12 03:58:42, Alok Priyadarshi wrote:
> > > But why isn't it:
> > > bool layerCanUseLCDText = canUseLCDText && (drawOpacity == 1.0) &&
> > > drawTransform.isIntegerTranslation(); ?
> > 
> > It is essentially that. adjustTextAA is part of the expression just to avoid
> > evaluating "(drawOpacity == 1.0) && drawTransform.isIntegerTranslation()"
when
> > adjustTextAA is false.
> > 
> > It is the short form of:
> > bool layerCanUseLCDText = false;
> > if (adjustTextAA) {
> >   layerCanUseLCDText = canUseLCDText && (drawOpacity == 1.0) &&
> > drawTransform.isIntegerTranslation();
> > } else {
> >   // No need to compute layerCanUseLCDText because it will not be set on
> layers
> > anyway.
> > }
> 
> Ohh okay, I see. 
> 
> Can you please add a comment explaining that then for posterity?

Done.

[alokp, 2012-11-12 16:54:22]

enne: Do you have any comments?

[enne (OOO), 2012-11-14 17:42:15]

https://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:552: // Adjusting text AA method during animation
may cause repaints, which in-turn jank.
Grammar.  "which in turn causes jank." maybe?

https://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:554: // To avoid color fringing, LCD text should
only be used on opaque layers with just integral translation.
If this is true, where is the check for opaque()?

https://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common...
cc/layer_tree_host_common.cc:555: bool layerCanUseLCDText = adjustTextAA &&
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation();
Can you rename canUseLCDText to something more like "subtreeCanUseLCDText" for
clarity?

I agree with danakj that the performance to confusion tradeoff of having
adjustTextAA in this expression is not worth having it there.

Have you done any investigation into whether or not it's valid to alpha blend an
opaque layer with LCD text on it? That's something I'm still not clear on.

https://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common...
File cc/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common...
cc/layer_tree_host_common_unittest.cc:4128: case LAYER_PRESERVES_3D:
How does preserves 3d enter into this?

https://codereview.chromium.org/11360093/diff/15001/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

https://codereview.chromium.org/11360093/diff/15001/cc/tiled_layer.cc#newcode275
cc/tiled_layer.cc:275: void TiledLayer::setCanUseLCDText(bool useLCDText)
Where's the invalidation here if this setting changes?

[alokp, 2012-11-16 04:41:11]

Uploading the new patch shortly.

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:552: // Adjusting text AA method during animation
may cause repaints, which in-turn jank.
On 2012/11/14 17:42:15, enne wrote:
> Grammar.  "which in turn causes jank." maybe?

Done.

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:554: // To avoid color fringing, LCD text should
only be used on opaque layers with just integral translation.
On 2012/11/14 17:42:15, enne wrote:
> If this is true, where is the check for opaque()?

We check for draw-opacity here. Check for content-opaqueness is being done in
WebKit. I am working on relaxing the check for content-opaqueness by tracking
opaque region.

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:555: bool layerCanUseLCDText = adjustTextAA &&
canUseLCDText && (drawOpacity == 1.0) && drawTransform.isIntegerTranslation();
On 2012/11/14 17:42:15, enne wrote:
> Can you rename canUseLCDText to something more like "subtreeCanUseLCDText" for
> clarity?

Done.

> I agree with danakj that the performance to confusion tradeoff of having
> adjustTextAA in this expression is not worth having it there.

Done.

> Have you done any investigation into whether or not it's valid to alpha blend
an
> opaque layer with LCD text on it? That's something I'm still not clear on.

Math does not add up exactly, but the initial experiments suggest that it does
not look bad. I was mostly concerned about fringes when text rendered on white
background is blended on a dark background. But I did not notice that in the
test page I shared with you.

Anyways, this patch does not allow LCD text on layers with drawOpacity < 1. I
think we need to do more experiments which will be easier to do once this patch
is landed.

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common_...
File cc/layer_tree_host_common_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common_...
cc/layer_tree_host_common_unittest.cc:4128: case LAYER_PRESERVES_3D:
drawOpacity and drawOpacityAnimating is inherited only when the parent
preserves-3d. See calculateDrawTransformsInternal for details. Since LCD text
depends on draw-opacity, I wanted to test this parameter as well.

http://codereview.chromium.org/11360093/diff/15001/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

http://codereview.chromium.org/11360093/diff/15001/cc/tiled_layer.cc#newcode275
cc/tiled_layer.cc:275: void TiledLayer::setCanUseLCDText(bool useLCDText)
On 2012/11/14 17:42:15, enne wrote:
> Where's the invalidation here if this setting changes?

It was indirectly happening when tile layers were reset due to
setBorderTexelOption. But now that http://codereview.chromium.org/11360205/ has
landed, I have added an explicit invalidation in
ContentLayer::canUseLCDTextDidChange. I also added a unit-test to verify the
invalidation.

[enne (OOO), 2012-11-16 17:36:38]

What's the strategy for LCD text on platforms where we can pinch zoom?

I am worried about full layer invalidation every time the LCD text setting
changes.  What about the heuristic of "once a layer is unable to have LCD text,
it will never be able to have LCD text again" to prevent flip-flopping with full
invalidations?

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:554: // To avoid color fringing, LCD text should
only be used on opaque layers with just integral translation.
On 2012/11/16 04:41:11, Alok Priyadarshi wrote:
> On 2012/11/14 17:42:15, enne wrote:
> > If this is true, where is the check for opaque()?
> 
> We check for draw-opacity here. Check for content-opaqueness is being done in
> WebKit. I am working on relaxing the check for content-opaqueness by tracking
> opaque region.

Can you point me at where this check is currently being done?

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common_...
File cc/layer_tree_host_common_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common_...
cc/layer_tree_host_common_unittest.cc:4128: case LAYER_PRESERVES_3D:
On 2012/11/16 04:41:11, Alok Priyadarshi wrote:
> drawOpacity and drawOpacityAnimating is inherited only when the parent
> preserves-3d. See calculateDrawTransformsInternal for details. Since LCD text
> depends on draw-opacity, I wanted to test this parameter as well.

It still feels like overtesting to me.  We have other tests that make sure that
preserves3D affects draw opacity correctly, and it's ultimately only draw
opacity (regardless of how that value gets baked) that is being considered here.

http://codereview.chromium.org/11360093/diff/15001/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

http://codereview.chromium.org/11360093/diff/15001/cc/tiled_layer.cc#newcode275
cc/tiled_layer.cc:275: void TiledLayer::setCanUseLCDText(bool useLCDText)
On 2012/11/16 04:41:11, Alok Priyadarshi wrote:
> On 2012/11/14 17:42:15, enne wrote:
> > Where's the invalidation here if this setting changes?
> 
> It was indirectly happening when tile layers were reset due to
> setBorderTexelOption. But now that http://codereview.chromium.org/11360205/
has
> landed, I have added an explicit invalidation in
> ContentLayer::canUseLCDTextDidChange. I also added a unit-test to verify the
> invalidation.

Ah, great.  Thanks for the unit test.  :)

[alokp, 2012-11-16 21:33:12]

On 2012/11/16 17:36:38, enne wrote:
> What's the strategy for LCD text on platforms where we can pinch zoom?

AFAIK pinch-zoom is only supported on Android which disables LCD on all layers.
Even without this patch if we wanted to use LCD on platforms that support
pinch-zoom today, we would need to worry about the LCD text on the root layer.
So I guess this patch does not change anything in that respect.. unless I am
missing something.

> I am worried about full layer invalidation every time the LCD text setting
> changes.  What about the heuristic of "once a layer is unable to have LCD
text,
> it will never be able to have LCD text again" to prevent flip-flopping with
full
> invalidations?

Flip-flopping LCD text setting has two implications - performance and visual
popping. I am not too much concerned about visual popping because the parameters
(transform and opacity) that will trigger LCD text change will typically have a
bigger visual impact.

I share the concern about performance due to too many invalidations. I like your
heuristic. If we were to implement it, I think I would change the API that will
only allow disabling LCD text. Each layer by default will allow LCD text until
it is disabled:

bool Layer::canUseLCDText();
void Layer::setCannotUseLCDText();

If this is OK then I would also get rid of the check that tries to not change
LCD text setting during animation because a layer can be invalidated at most
once. And if a layer is being animated then it probably should not have LCD text
anyway.

[alokp, 2012-11-16 21:33:52]

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:554: // To avoid color fringing, LCD text should
only be used on opaque layers with just integral translation.
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromiu...

It was added in http://trac.webkit.org/changeset/133499

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common_...
File cc/layer_tree_host_common_unittest.cc (right):

http://codereview.chromium.org/11360093/diff/15001/cc/layer_tree_host_common_...
cc/layer_tree_host_common_unittest.cc:4128: case LAYER_PRESERVES_3D:
On 2012/11/16 17:36:38, enne wrote:
> On 2012/11/16 04:41:11, Alok Priyadarshi wrote:
> > drawOpacity and drawOpacityAnimating is inherited only when the parent
> > preserves-3d. See calculateDrawTransformsInternal for details. Since LCD
text
> > depends on draw-opacity, I wanted to test this parameter as well.
> 
> It still feels like overtesting to me.  We have other tests that make sure
that
> preserves3D affects draw opacity correctly, and it's ultimately only draw
> opacity (regardless of how that value gets baked) that is being considered
here.

Done.

[jamesr, 2012-11-16 21:37:40]

On 2012/11/16 21:33:12, Alok Priyadarshi wrote:
> On 2012/11/16 17:36:38, enne wrote:
> > What's the strategy for LCD text on platforms where we can pinch zoom?
> 
> AFAIK pinch-zoom is only supported on Android which disables LCD on all
layers.

This is a bad assumption.  Pinch-zoom is also a feature on win 8 and other
browsers support it on OS X.

[alokp, 2012-11-16 21:40:19]

On 2012/11/16 21:37:40, jamesr wrote:
> On 2012/11/16 21:33:12, Alok Priyadarshi wrote:
> > On 2012/11/16 17:36:38, enne wrote:
> > > What's the strategy for LCD text on platforms where we can pinch zoom?
> > 
> > AFAIK pinch-zoom is only supported on Android which disables LCD on all
> layers.
> 
> This is a bad assumption.  Pinch-zoom is also a feature on win 8 and other
> browsers support it on OS X.

Do we use LCD text on root layer on those platforms?

[jamesr, 2012-11-16 21:42:14]

On 2012/11/16 21:40:19, Alok Priyadarshi wrote:
> On 2012/11/16 21:37:40, jamesr wrote:
> > On 2012/11/16 21:33:12, Alok Priyadarshi wrote:
> > > On 2012/11/16 17:36:38, enne wrote:
> > > > What's the strategy for LCD text on platforms where we can pinch zoom?
> > > 
> > > AFAIK pinch-zoom is only supported on Android which disables LCD on all
> > layers.
> > 
> > This is a bad assumption.  Pinch-zoom is also a feature on win 8 and other
> > browsers support it on OS X.
> 
> Do we use LCD text on root layer on those platforms?

Yes.

[alokp, 2012-11-16 21:47:37]

On 2012/11/16 21:42:14, jamesr wrote:
> On 2012/11/16 21:40:19, Alok Priyadarshi wrote:
> > On 2012/11/16 21:37:40, jamesr wrote:
> > > On 2012/11/16 21:33:12, Alok Priyadarshi wrote:
> > > > On 2012/11/16 17:36:38, enne wrote:
> > > > > What's the strategy for LCD text on platforms where we can pinch zoom?
> > > > 
> > > > AFAIK pinch-zoom is only supported on Android which disables LCD on all
> > > layers.
> > > 
> > > This is a bad assumption.  Pinch-zoom is also a feature on win 8 and other
> > > browsers support it on OS X.
> > 
> > Do we use LCD text on root layer on those platforms?
> 
> Yes.

So don't we already have this problem on root layer. I do not understand how
this problem is related to this patch.

[enne (OOO), 2012-11-26 15:22:50]

Re: pinch-zoom.  You're right that we currently have this problem, so it's ok to
punt on this existing bug for this patch.

However, I am still concerned about performance and conservatively do not want
to add additional repaints as a result of this patch.  It seems like maybe it'd
be a dozen lines at most to prevent flip-flopping, so I'd like to see it added
before this lands.

Can you also add a trace event for an invalidation that's caused by an LCD text
switch?

On 2012/11/16 21:33:12, Alok Priyadarshi wrote:

> Flip-flopping LCD text setting has two implications - performance and visual
> popping. I am not too much concerned about visual popping because the
parameters
> (transform and opacity) that will trigger LCD text change will typically have
a
> bigger visual impact.
> 
> I share the concern about performance due to too many invalidations. I like
your
> heuristic. If we were to implement it, I think I would change the API that
will
> only allow disabling LCD text. Each layer by default will allow LCD text until
> it is disabled:
> 
> bool Layer::canUseLCDText();
> void Layer::setCannotUseLCDText();

What's wrong with Layer::setCanUseLCDText(bool)?

I think I'd like to have the layer itself decide whether or not it is going to
use LCD text, but have LayerTreeHostCommon be agnostic about the final decision
and just set values about whether the layer can use LCD text.

> If this is OK then I would also get rid of the check that tries to not change
> LCD text setting during animation because a layer can be invalidated at most
> once. And if a layer is being animated then it probably should not have LCD
text
> anyway.

I don't see how the animation check is related to these concerns.  If our
general sentiment is that LCD text doesn't bad during animation, then it seems
like this check should stay, which would keep LCD text before/during/after the
animation if the animating layer is already composited.

[alokp, 2012-12-10 23:25:24]

@enne: PTAL. I believe I addressed all the comments. Let me know if I missed
anything.

> However, I am still concerned about performance and conservatively do not want
> to add additional repaints as a result of this patch.  It seems like maybe
it'd
> be a dozen lines at most to prevent flip-flopping, so I'd like to see it added
> before this lands.

DONE.
 
> Can you also add a trace event for an invalidation that's caused by an LCD
text
> switch?

DONE.

[enne (OOO), 2012-12-11 02:47:15]

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h#newcode305
cc/layer.h:305: virtual bool canUseLCDTextWillChange();
Can you do this without adding more virtuals to Layer? I'm sorry to be picky,
but I feel like it's easy to dump things into the already overflowing base
class.

I'd really prefer a solution that encapsulated all of this inside of TiledLayer,
such as using some internal bool m_usingLCDText in TiledLayer (initialized to
true).  When updating a layer, you could check if canUseLCDText() is ever false
while m_usingLCDText is true, and then set it to false and invalidate.

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h#newcode378
cc/layer.h:378: bool m_canUseLCDText;
Can you move this to DrawProperties? I think it belongs there.

https://codereview.chromium.org/11360093/diff/36001/webkit/compositor_binding...
File webkit/compositor_bindings/web_content_layer_impl.cc (right):

https://codereview.chromium.org/11360093/diff/36001/webkit/compositor_binding...
webkit/compositor_bindings/web_content_layer_impl.cc:93: #if
WEBCONTENTLAYERCLIENT_HAS_CANPAINTLCDTEXT
Can this be removed?

[alokp, 2012-12-11 05:07:21]

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h#newcode305
cc/layer.h:305: virtual bool canUseLCDTextWillChange();
I do not understand your solution but the reason I want to make the decision at
the ContentLayer level and not TiledLayer because other layer types (image,
video, etc) should not be invalidated because of a change in LCD text settings.
I will talk to you offline.

On 2012/12/11 02:47:15, enne wrote:
> Can you do this without adding more virtuals to Layer? I'm sorry to be picky,
> but I feel like it's easy to dump things into the already overflowing base
> class.
> 
> I'd really prefer a solution that encapsulated all of this inside of
TiledLayer,
> such as using some internal bool m_usingLCDText in TiledLayer (initialized to
> true).  When updating a layer, you could check if canUseLCDText() is ever
false
> while m_usingLCDText is true, and then set it to false and invalidate.

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h#newcode378
cc/layer.h:378: bool m_canUseLCDText;
I considered doing that but I could think of a good way to invalidate layers
when this property changes. May be it will become clearer once I understand your
solution described above.

On 2012/12/11 02:47:15, enne wrote:
> Can you move this to DrawProperties? I think it belongs there.

https://codereview.chromium.org/11360093/diff/36001/webkit/compositor_binding...
File webkit/compositor_bindings/web_content_layer_impl.cc (right):

https://codereview.chromium.org/11360093/diff/36001/webkit/compositor_binding...
webkit/compositor_bindings/web_content_layer_impl.cc:93: #if
WEBCONTENTLAYERCLIENT_HAS_CANPAINTLCDTEXT
Yes. It is on my TODO list. I will do it in a separate patch.

On 2012/12/11 02:47:15, enne wrote:
> Can this be removed?

[alokp, 2012-12-12 17:42:24]

Moved m_canUseLCDText to DrawProperties.
The logic for avoiding flip-flopping of LCD text is now encapsulated inside
ContentLayer::update().

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h#newcode305
cc/layer.h:305: virtual bool canUseLCDTextWillChange();
On 2012/12/11 02:47:15, enne wrote:
> Can you do this without adding more virtuals to Layer? I'm sorry to be picky,
> but I feel like it's easy to dump things into the already overflowing base
> class.
> 
> I'd really prefer a solution that encapsulated all of this inside of
TiledLayer,
> such as using some internal bool m_usingLCDText in TiledLayer (initialized to
> true).  When updating a layer, you could check if canUseLCDText() is ever
false
> while m_usingLCDText is true, and then set it to false and invalidate.

Done.

https://codereview.chromium.org/11360093/diff/36001/cc/layer.h#newcode378
cc/layer.h:378: bool m_canUseLCDText;
On 2012/12/11 02:47:15, enne wrote:
> Can you move this to DrawProperties? I think it belongs there.

Done.

[enne (OOO), 2012-12-12 17:56:52]

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.cc
File cc/content_layer.cc (right):

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.cc#newco...
cc/content_layer.cc:118: drawProperties().can_use_lcd_text = m_canUseLCDText;
Draw properties should only be set during calcDrawProperties. 

Can you just expose m_canUseLCDText to wherever it needs to go to turn on LCD
text rather than going through draw properties? Maybe m_canUseLCDText could be
renamed to m_isUsingLCDText to make that more clear.

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.h
File cc/content_layer.h (right):

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.h#newcode66
cc/content_layer.h:66: bool m_canUseLCDText;
You need to initialize this in the constructor.

[alokp, 2012-12-12 19:03:04]

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.cc
File cc/content_layer.cc (right):

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.cc#newco...
cc/content_layer.cc:118: drawProperties().can_use_lcd_text = m_canUseLCDText;
On 2012/12/12 17:56:52, enne wrote:
> Draw properties should only be set during calcDrawProperties. 

> Can you just expose m_canUseLCDText to wherever it needs to go to turn on LCD
> text rather than going through draw properties? Maybe m_canUseLCDText could be
> renamed to m_isUsingLCDText to make that more clear.

Not sure if this is what you meant, but I moved m_canUseLCDText to the base
Layer class. The only derived class that cares about LCD text, ContentLayer,
syncs the member variable m_canUseLCDText with that in
DrawProperties::can_use_lcd_text during update.

The other option is make canUseLCDText virtual. The base Layer class will just
return the value in DrawProperties, while ContentLayer will return a different
value.

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.h
File cc/content_layer.h (right):

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.h#newcode66
cc/content_layer.h:66: bool m_canUseLCDText;
On 2012/12/12 17:56:52, enne wrote:
> You need to initialize this in the constructor.

Done.

[enne (OOO), 2012-12-12 19:14:46]

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.cc
File cc/content_layer.cc (right):

https://codereview.chromium.org/11360093/diff/41001/cc/content_layer.cc#newco...
cc/content_layer.cc:118: drawProperties().can_use_lcd_text = m_canUseLCDText;
Sorry that I wasn't clear enough.  Here's some more explanation:

The Layer class knows about "can use LCD text".  This is a draw property (not a
layer property), and it is only written to during calcDrawProperties.

The ContentLayer class knows about "is using LCD text" and handles invalidation
and a heuristic about switching too often.  This "is using LCD text" flag (what
you are calling ContentLayer::m_canUseLCDText here and I suggest should be
called something like m_usingLCDText instead) only exists on ContentLayer as a
member (and you'll want an accessor function).

I'm trying to say that this line that I'm commenting on here
("drawProperties().can_use_lcd_text = m_canUseLCDText") breaks the "draw
property" abstraction.  Draw properties should only get set during
calcDrawProperties.  Also, setting it to false seems wrong.  The ContentLayer
*can* use LCD text, it's just choosing not to use it.

If you want to trigger LCD text, you should not do it on Layer::canUseLCDText,
but instead you should modify WebContentLayerImpl::paintContents to get this
flag from ContentLayer::isUsingLCDText instead.

[alokp, 2012-12-12 19:40:46]

> The Layer class knows about "can use LCD text".  This is a draw property (not
a
> layer property), and it is only written to during calcDrawProperties.
> The ContentLayer class knows about "is using LCD text" and handles
invalidation
> and a heuristic about switching too often.  This "is using LCD text" flag
(what
> you are calling ContentLayer::m_canUseLCDText here and I suggest should be
> called something like m_usingLCDText instead) only exists on ContentLayer as a
> member (and you'll want an accessor function).
> 
> I'm trying to say that this line that I'm commenting on here
> ("drawProperties().can_use_lcd_text = m_canUseLCDText") breaks the "draw
> property" abstraction.  Draw properties should only get set during
> calcDrawProperties.  Also, setting it to false seems wrong.  The ContentLayer
> *can* use LCD text, it's just choosing not to use it.
> 
> If you want to trigger LCD text, you should not do it on Layer::canUseLCDText,
> but instead you should modify WebContentLayerImpl::paintContents to get this
> flag from ContentLayer::isUsingLCDText instead.

Trying to call a function that only exists on ContentLayer will need an ugly
static_cast in WebContentLayerImpl::paintContents. How about we have two
functions on the base Layer class:

Layer::allowsLCDText() {
  return m_drawProperties.allows_lcd_text;
}

Layer::useLCDText() {
  return m_useLCDText;
}

Layer::m_useLCDText will be false for all types of layers except ContentLayers.
ContentLayer::update() will set this value based on
DrawProperties::allows_lcd_text and the heuristic about switching too often.

What do you think?

[enne (OOO), 2012-12-12 19:44:22]

I'm not concerned about the static_cast.  The point of WebContentLayerImpl is to
be a bridge between ContentLayer and WebKit.  It already makes static_casts and
knows the exact type of its layer.

[jamesr, 2012-12-12 19:44:34]

On 2012/12/12 19:40:46, Alok Priyadarshi wrote:
> Trying to call a function that only exists on ContentLayer will need an ugly
> static_cast in WebContentLayerImpl::paintContents. How about we have two
> functions on the base Layer class:

WebContentLayerImpl knows the static type of its m_layer pointer and downcasts
in various places already.  That's fine.

[alokp, 2012-12-12 21:45:29]

Done.

[enne (OOO), 2012-12-12 21:49:48]

https://codereview.chromium.org/11360093/diff/51002/cc/content_layer.cc
File cc/content_layer.cc (right):

https://codereview.chromium.org/11360093/diff/51002/cc/content_layer.cc#newco...
cc/content_layer.cc:48: , m_useLCDText(false)
Am I reading this code right that m_useLCDText starts as false, but can only
change once.  So, it can get turned on, but then never turned off again?

https://codereview.chromium.org/11360093/diff/51002/webkit/compositor_binding...
File webkit/compositor_bindings/web_content_layer_impl.cc (right):

https://codereview.chromium.org/11360093/diff/51002/webkit/compositor_binding...
webkit/compositor_bindings/web_content_layer_impl.cc:92: false :
Oh, sad times.  I'll file a bug for this.

[alokp, 2012-12-12 21:53:22]

https://codereview.chromium.org/11360093/diff/51002/cc/content_layer.cc
File cc/content_layer.cc (right):

https://codereview.chromium.org/11360093/diff/51002/cc/content_layer.cc#newco...
cc/content_layer.cc:48: , m_useLCDText(false)
On 2012/12/12 21:49:48, enne wrote:
> Am I reading this code right that m_useLCDText starts as false, but can only
> change once.  So, it can get turned on, but then never turned off again?

See line 128: We always disable LCD text if needed. OTOH it can only be enabled
once. There is also a unit-test to verify that.

[enne (OOO), 2012-12-12 21:54:31]

lgtm

https://codereview.chromium.org/11360093/diff/51002/cc/content_layer.cc
File cc/content_layer.cc (right):

https://codereview.chromium.org/11360093/diff/51002/cc/content_layer.cc#newco...
cc/content_layer.cc:48: , m_useLCDText(false)
Oh, quite right.  My mistake.  :)

[commit-bot: I haz the power, 2012-12-12 21:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/11360093/51002

[commit-bot: I haz the power, 2012-12-12 21:59:08]

Failed to apply patch for cc/layer_tree_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layer_tree_host.cc
  Hunk #1 FAILED at 104.
  Hunk #2 FAILED at 117.
  Hunk #3 succeeded at 574 (offset -108 lines).
  2 out of 3 hunks FAILED -- saving rejects to file cc/layer_tree_host.cc.rej

Patch:       cc/layer_tree_host.cc
Index: cc/layer_tree_host.cc
diff --git a/cc/layer_tree_host.cc b/cc/layer_tree_host.cc
index
9194e1482797750bda1ec92c0045f3c5538373bc..7b93d78ed74d1edab3138d80205980488123e425
100644
--- a/cc/layer_tree_host.cc
+++ b/cc/layer_tree_host.cc
@@ -104,6 +104,7 @@ LayerTreeSettings::LayerTreeSettings()
     , pageScalePinchZoomEnabled(false)
     , backgroundColorInsteadOfCheckerboard(false)
     , showOverdrawInTracing(false)
+    , canUseLCDText(true)
     , refreshRate(0)
     , maxPartialTextureUpdates(std::numeric_limits<size_t>::max())
     , numRasterThreads(1)
@@ -117,6 +118,12 @@ LayerTreeSettings::LayerTreeSettings()
     backgroundColorInsteadOfCheckerboard =
CommandLine::ForCurrentProcess()->HasSwitch(switches::kBackgroundColorInsteadOfCheckerboard);
     showOverdrawInTracing =
CommandLine::ForCurrentProcess()->HasSwitch(switches::kTraceOverdraw);
 
+// TODO(alokp): Remove this hard-coded setting.
+// Platforms that need to disable LCD text must explicitly set this value.
+#if defined(OS_ANDROID)
+    canUseLCDText = false;
+#endif
+
     initialDebugState.showPropertyChangedRects =
CommandLine::ForCurrentProcess()->HasSwitch(cc::switches::kShowPropertyChangedRects);
     initialDebugState.showSurfaceDamageRects =
CommandLine::ForCurrentProcess()->HasSwitch(cc::switches::kShowSurfaceDamageRects);
     initialDebugState.showScreenSpaceRects =
CommandLine::ForCurrentProcess()->HasSwitch(cc::switches::kShowScreenSpaceRects);
@@ -682,7 +689,7 @@ void LayerTreeHost::updateLayers(Layer* rootLayer,
ResourceUpdateQueue& queue)
         }
 
         TRACE_EVENT0("cc", "LayerTreeHost::updateLayers::calcDrawEtc");
-        LayerTreeHostCommon::calculateDrawProperties(rootLayer,
deviceViewportSize(), m_deviceScaleFactor, m_pageScaleFactor,
rendererCapabilities().maxTextureSize, updateList);
+        LayerTreeHostCommon::calculateDrawProperties(rootLayer,
deviceViewportSize(), m_deviceScaleFactor, m_pageScaleFactor,
rendererCapabilities().maxTextureSize, m_settings.canUseLCDText, updateList);
     }
 
     // Reset partial texture update requests.

[commit-bot: I haz the power, 2012-12-13 06:29:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/11360093/56001

[commit-bot: I haz the power, 2012-12-13 09:19:01]

Change committed as 172842