cc: Split out calcDrawEtc from drawLayers

cc: Split out calcDrawEtc from drawLayers

Previously, calcDrawEtc was coupled to drawLayers.  Immediately before drawing,
it would get called to update layer state based on any impl-side input that
came in after the commit.

Impl-side painting needs to update layer transforms / visible content rect
state prior to rasterizing contents.  And, if input comes in, it might also
need to re-update prior to drawing.  LayerTreeHostImpl then gets a
needsUpdateLayers() flag, and updateLayers() (which calls calcDraw) is a no-op
if nothing has changed on the tree.  Additionally, the calculate render surface
layer list is persisted since the last updateLayers() call.

The LayerTreeHostImpl now sets the needsUpdate flag after commit, scroll, pinch
zoom, and animation.  These are the easy ones.

Unfortunately, this creates an amazing foot gun where you can manipulate the
impl tree on the impl thread and updateLayers won't get called, because the
needsUpdate flag hasn't been called explicitly.  This mostly happens in tests,
but could happen elsewhere.  To fix this, setFoo calls on LayerImpl now calls
setNeedsUpdate() on its host.

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

[enne (OOO), 2012-12-06 02:51:32]

This change really needs some tests like the verify set needs commit tests for
cc::Layer, but I wanted to first get buy-in on the general direction.

Are there objections to making setNeedsUpdate happen implicitly? Part of me
wonders if  maybe that's overkill because mostly only tests need it.

[danakj, 2012-12-06 03:04:47]

Before reading the code:

- What if a test calls prepareToDraw but doesn't drawLayers. willDraw feels a
bit late? How about #ifndef NDEBUG guarded walk through the layer tree in
updateLayers that (always) checks and asserts that every layer has its host set?

- "update" is super overloaded and I'd prefer a different name that doesn't
conflict with updating layer textures/resources.

[danakj, 2012-12-06 03:09:39]

On 2012/12/06 03:04:47, danakj wrote:
> Before reading the code:
> 
> - What if a test calls prepareToDraw but doesn't drawLayers. willDraw feels a
> bit late? How about #ifndef NDEBUG guarded walk through the layer tree in
> updateLayers that (always) checks and asserts that every layer has its host
set?

Oh right, willDraw is in prepareToDraw. La la la..

> - "update" is super overloaded and I'd prefer a different name that doesn't
> conflict with updating layer textures/resources.

[danakj, 2012-12-06 03:23:04]

What if we:
- make addChild and removeChild set the LTHImpl on the child
- make setLTHI recursively propogate
- make LTHI::setRootLayer set it on the root layer
- remove setLTHI from tree synchronizer.

Could we then get away with no unit tests having to worry about anything?

Is there any reasons to not do that? I stuck it in tree synchronizer originally
just because that seemed like it would be enough, but it's clearly not for
tests.

https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:380: setNeedsUpdateLayers();
The layer will do this.

https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:386: if (!needsUpdateLayers())
m_

https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:508: if (!m_rootLayerImpl)
This is DCHECKed in prepareToDraw, by asking canDraw()

[enne (OOO), 2012-12-06 17:20:22]

I can make setLayerTreeHostImpl more implicit.  It felt a little like overkill,
but that does make things clearer.

I picked LTHI::updateLayers because it corresponds exactly to the already
existing LTH::updateLayers.

https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:386: if (!needsUpdateLayers())
On 2012/12/06 03:23:04, danakj wrote:
> m_

Why?

[danakj, 2012-12-06 17:38:50]

https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:386: if (!needsUpdateLayers())
On 2012/12/06 17:20:23, enne wrote:
> On 2012/12/06 03:23:04, danakj wrote:
> > m_
> 
> Why?

Cuz we always generally do this. Less indirection/uncertainty about what's being
read.

[enne (OOO), 2012-12-06 18:01:05]

For what it's worth, the reason to not do it recursively is to avoid unnecessary
O(n^2) behavior in tree walks during layer sync.  If you want to set the host
correctly, both setLTHI and addChild need to recursively walk their entire
subtree to make sure that all of those children have the right host.

On 2012/12/06 17:38:50, danakj wrote:
> https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc
> File cc/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc#ne...
> cc/layer_tree_host_impl.cc:386: if (!needsUpdateLayers())
> On 2012/12/06 17:20:23, enne wrote:
> > On 2012/12/06 03:23:04, danakj wrote:
> > > m_
> > 
> > Why?
> 
> Cuz we always generally do this. Less indirection/uncertainty about what's
being
> read.

I don't really buy that argument.

[danakj, 2012-12-06 18:02:53]

On Thu, Dec 6, 2012 at 1:01 PM,  <enne@chromium.org> wrote:
> For what it's worth, the reason to not do it recursively is to avoid
> unnecessary
> O(n^2) behavior in tree walks during layer sync.  If you want to set the
> host
> correctly, both setLTHI and addChild need to recursively walk their entire
> subtree to make sure that all of those children have the right host.

Can't you stop recursing whenever you see a layer with the right
layertreehost? In order to have the wrong host below that, it would
have to be added and the parent not check it.

> On 2012/12/06 17:38:50, danakj wrote:
>>
>> https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc
>> File cc/layer_tree_host_impl.cc (right):
>
>
>
>
https://codereview.chromium.org/11447028/diff/1/cc/layer_tree_host_impl.cc#ne...
>>
>> cc/layer_tree_host_impl.cc:386: if (!needsUpdateLayers())
>> On 2012/12/06 17:20:23, enne wrote:
>> > On 2012/12/06 03:23:04, danakj wrote:
>> > > m_
>> >
>> > Why?
>
>
>> Cuz we always generally do this. Less indirection/uncertainty about what's
>
> being
>>
>> read.
>
>
> I don't really buy that argument.
>
> https://codereview.chromium.org/11447028/

[enne (OOO), 2012-12-06 18:06:18]

Yeah, ok, my mistake.  I think that would work and not be so bad.

[enne (OOO), 2012-12-06 18:50:34]

updateLayers => updateDrawProperties

PTAL

[danakj, 2012-12-06 19:12:10]

https://codereview.chromium.org/11447028/diff/16001/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_impl.cc#newcode427
cc/layer_impl.cc:427: for (size_t i = 0; i < m_children.size(); ++i) {
extra {}s

https://codereview.chromium.org/11447028/diff/16001/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_impl.h#newcode164
cc/layer_impl.h:164: // Creating and destroying render surfaces should only be
performed by
I'm not sure about the destroying part here? Won't they be destroyed during tree
sync if the layer is destroyed, etc?

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:554: DCHECK(it->layerTreeHostImpl());
Should this DCHECK the layerTreeHostImpl() == this? Just in case..

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:763: updateDrawProperties();
I'm not clear why this moved out of calcRenderPasses. But then again I don't
think I care really either..

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1125: if (!m_settings.pageScalePinchZoomEnabled &&
pageScaleChange != 1) {
don't need the {} now

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1189: if (m_renderSurfaceLayerList.size() &&
m_rootLayerImpl->renderSurface())
should this line turn into if (!needsUpdateDrawProperties) ?

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1195: setNeedsUpdateDrawProperties();
why the set here?

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl_u...
File cc/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:1935: m_hostImpl->didDrawAllLayers(frame);
don't we need this to avoid "in between willdraw and diddraw"?

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:67: layerImpl->setLayerTreeHostImpl(hostImpl);
Now the cc::Layer will be able to reach through and poke the LTHImpl in
pushPropertiesTo() which I'm not sure I'm crazy about. What do you think about
that?

Maybe we could move the pushPropertiesTo() call up above this?

[enne (OOO), 2012-12-06 19:25:42]

https://codereview.chromium.org/11447028/diff/16001/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_impl.cc#newcode427
cc/layer_impl.cc:427: for (size_t i = 0; i < m_children.size(); ++i) {
On 2012/12/06 19:12:10, danakj wrote:
> extra {}s

Done.

https://codereview.chromium.org/11447028/diff/16001/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_impl.h#newcode164
cc/layer_impl.h:164: // Creating and destroying render surfaces should only be
performed by
On 2012/12/06 19:12:10, danakj wrote:
> I'm not sure about the destroying part here? Won't they be destroyed during
tree
> sync if the layer is destroyed, etc?

So, here's my thought process.  Setting a draw property should not
setNeedsUpdateDrawProperties(); only setting non-derived properties should cause
that.

More importantly, destroying/creating a render surface invalidates the RSLL
which is now held between frames, and really only the host can manage that. 
I'll update this comment to be more accurate that the caller is responsible for
calling setNeedsUpdateDrawProperties.

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:554: DCHECK(it->layerTreeHostImpl());
On 2012/12/06 19:12:10, danakj wrote:
> Should this DCHECK the layerTreeHostImpl() == this? Just in case..

Done.

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:763: updateDrawProperties();
On 2012/12/06 19:12:10, danakj wrote:
> I'm not clear why this moved out of calcRenderPasses. But then again I don't
> think I care really either..

Moved back.

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1189: if (m_renderSurfaceLayerList.size() &&
m_rootLayerImpl->renderSurface())
On 2012/12/06 19:12:10, danakj wrote:
> should this line turn into if (!needsUpdateDrawProperties) ?

See reply below.

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1195: setNeedsUpdateDrawProperties();
On 2012/12/06 19:12:10, danakj wrote:
> why the set here?

LayerTreeHostImplTest.clearRootRenderSurfaceAndScroll.  I'll take other
suggestions.

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl_u...
File cc/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:1935: m_hostImpl->didDrawAllLayers(frame);
On 2012/12/06 19:12:10, danakj wrote:
> don't we need this to avoid "in between willdraw and diddraw"?

You shouldn't call didDrawAllLayers twice.  See line 1944.

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:67: layerImpl->setLayerTreeHostImpl(hostImpl);
On 2012/12/06 19:12:10, danakj wrote:
> Now the cc::Layer will be able to reach through and poke the LTHImpl in
> pushPropertiesTo() which I'm not sure I'm crazy about. What do you think about
> that?
> 
> Maybe we could move the pushPropertiesTo() call up above this?

Is there something wrong with that?

[danakj, 2012-12-06 19:32:19]

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl_u...
File cc/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:1935: m_hostImpl->didDrawAllLayers(frame);
On 2012/12/06 19:25:42, enne wrote:
> On 2012/12/06 19:12:10, danakj wrote:
> > don't we need this to avoid "in between willdraw and diddraw"?
> 
> You shouldn't call didDrawAllLayers twice.  See line 1944.

Ah!

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:67: layerImpl->setLayerTreeHostImpl(hostImpl);
On 2012/12/06 19:25:42, enne wrote:
> On 2012/12/06 19:12:10, danakj wrote:
> > Now the cc::Layer will be able to reach through and poke the LTHImpl in
> > pushPropertiesTo() which I'm not sure I'm crazy about. What do you think
about
> > that?
> > 
> > Maybe we could move the pushPropertiesTo() call up above this?
> 
> Is there something wrong with that?

I'm not sure :) I know it's informed design choices in the past, and possibly
made for better (or worse) choices in the end. It enforces a bit of separation
in that the main thread layer can't really start poking easily at the whole impl
tree.

https://codereview.chromium.org/11447028/diff/19002/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/11447028/diff/19002/cc/layer_impl.h#newcode166
cc/layer_impl.h:166: // so that its list can be recreated.
Thanks, this comment is very clear.

[enne (OOO), 2012-12-06 19:34:09]

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:67: layerImpl->setLayerTreeHostImpl(hostImpl);
On 2012/12/06 19:32:20, danakj wrote:
> On 2012/12/06 19:25:42, enne wrote:
> > On 2012/12/06 19:12:10, danakj wrote:
> > > Now the cc::Layer will be able to reach through and poke the LTHImpl in
> > > pushPropertiesTo() which I'm not sure I'm crazy about. What do you think
> about
> > > that?
> > > 
> > > Maybe we could move the pushPropertiesTo() call up above this?
> > 
> > Is there something wrong with that?
> 
> I'm not sure :) I know it's informed design choices in the past, and possibly
> made for better (or worse) choices in the end. It enforces a bit of separation
> in that the main thread layer can't really start poking easily at the whole
impl
> tree.

It has?? I don't have any sense of what you're referring to here. Do you have a
specific example you're thinking of?

[danakj, 2012-12-06 19:36:20]

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:67: layerImpl->setLayerTreeHostImpl(hostImpl);
On 2012/12/06 19:34:09, enne wrote:
> On 2012/12/06 19:32:20, danakj wrote:
> > On 2012/12/06 19:25:42, enne wrote:
> > > On 2012/12/06 19:12:10, danakj wrote:
> > > > Now the cc::Layer will be able to reach through and poke the LTHImpl in
> > > > pushPropertiesTo() which I'm not sure I'm crazy about. What do you think
> > about
> > > > that?
> > > > 
> > > > Maybe we could move the pushPropertiesTo() call up above this?
> > > 
> > > Is there something wrong with that?
> > 
> > I'm not sure :) I know it's informed design choices in the past, and
possibly
> > made for better (or worse) choices in the end. It enforces a bit of
separation
> > in that the main thread layer can't really start poking easily at the whole
> impl
> > tree.
> 
> It has?? I don't have any sense of what you're referring to here. Do you have
a
> specific example you're thinking of?

I'm trying to remember what it was, but at least once I was going to do
something in there, I think it was for HudLayer, and this made me look for
another way.

[enne (OOO), 2012-12-06 19:49:02]

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:67: layerImpl->setLayerTreeHostImpl(hostImpl);
"I had to do something another way because I didn't want to change this" isn't a
really strong argument.  ;)

What could go wrong here? What would not doing this prevent?

[danakj, 2012-12-06 19:51:56]

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/tree_synchronizer.cc#n...
cc/tree_synchronizer.cc:67: layerImpl->setLayerTreeHostImpl(hostImpl);
On 2012/12/06 19:49:02, enne wrote:
> "I had to do something another way because I didn't want to change this" isn't
a
> really strong argument.  ;)
> 
> What could go wrong here? What would not doing this prevent?

I'm not really sure, I just wanted to make sure you'd thought about it before
doing it and it wasn't an accident.

I'm okay with it otherwise as I have no arguments really.

[danakj, 2012-12-06 19:56:22]

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1195: setNeedsUpdateDrawProperties();
On 2012/12/06 19:25:42, enne wrote:
> On 2012/12/06 19:12:10, danakj wrote:
> > why the set here?
> 
> LayerTreeHostImplTest.clearRootRenderSurfaceAndScroll.  I'll take other
> suggestions.

it's a bug to clear the render surface and not set needsUpdateDrawProps right?
Should the unit test be setting that then?

Or, if this is allowable use of the API, should clearRenderSurface() be setting
it?

https://codereview.chromium.org/11447028/diff/19002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/19002/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:555: DCHECK_EQ(it->layerTreeHostImpl(), this);
nit: expected value first

[enne (OOO), 2012-12-06 20:37:50]

After offline conversations with jamesr/danakj, I will change LayerImpl to take
its host in its ctor, since it should never change over the lifetime of the
layer.

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/16001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1195: setNeedsUpdateDrawProperties();
It kind of is now.  I could change the test to clear a surface and call
setNeedsUpdateDrawProps, and then I think everything would be clearer.

https://codereview.chromium.org/11447028/diff/19002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/19002/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:555: DCHECK_EQ(it->layerTreeHostImpl(), this);
On 2012/12/06 19:56:22, danakj wrote:
> nit: expected value first

Done.

[enne (OOO), 2012-12-07 07:13:39]

LayerImpl being changed to take the LTHI in its ctor:
https://codereview.chromium.org/11472021

[enne (OOO), 2012-12-07 20:59:08]

Rebased on top of that other patch.  PTAL.

Re: ensureRenderSurfaceLayerList.  This patch minimally changes that function to
behave exactly as it did before.  I tried changing it to check for
needsUpdateDrawProperties(), but some unit tests went haywire because it was
updating layers in a way that they weren't expecting.  I would rather leave this
code as-is and investigate in some other patch.

[enne (OOO), 2012-12-07 21:09:03]

Actually, scratch that.  I want to add a few unit tests in
layer_impl_unittests.cc to make sure setNeedsDrawProperties is called at the
right time.

[danakj, 2012-12-07 21:14:02]

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:939: setNeedsUpdateDrawProperties();
nit: 4 space indents

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1149: setNeedsUpdateDrawProperties();
Ok, throw a big TODO here please and file a bug. The above check can be a false
positive now.

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1532: if (copy.size() > 0)
nit: !copy.empty()

Or even better, save !m_activeAnimationControllers.empty() in nicely named temp
bool before the iteration.

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1533: m_client->setNeedsRedrawOnImplThread();
did you mean to setNeedsUpdateDrawProperties() here too?

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:308: void resetNeedsUpdateDrawProperties() {
m_needsUpdateDrawProperties = false; }
Should we get in the habit of adding ForTesting to the end of methods like this?

[enne (OOO), 2012-12-07 23:03:00]

Added testing, addressed comments.  PTAL.

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:939: setNeedsUpdateDrawProperties();
On 2012/12/07 21:14:02, danakj wrote:
> nit: 4 space indents

Done.

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1149: setNeedsUpdateDrawProperties();
On 2012/12/07 21:14:02, danakj wrote:
> Ok, throw a big TODO here please and file a bug. The above check can be a
false
> positive now.

Done.  https://code.google.com/p/chromium/issues/detail?id=164949

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1532: if (copy.size() > 0)
On 2012/12/07 21:14:02, danakj wrote:
> nit: !copy.empty()
> 
> Or even better, save !m_activeAnimationControllers.empty() in nicely named
temp
> bool before the iteration.

Actually, there's an early out for no active controllers earlier in the
function, so removed.

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1533: m_client->setNeedsRedrawOnImplThread();
On 2012/12/07 21:14:02, danakj wrote:
> did you mean to setNeedsUpdateDrawProperties() here too?

Done.

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11447028/diff/14027/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:308: void resetNeedsUpdateDrawProperties() {
m_needsUpdateDrawProperties = false; }
On 2012/12/07 21:14:02, danakj wrote:
> Should we get in the habit of adding ForTesting to the end of methods like
this?

Done.

[danakj, 2012-12-07 23:12:49]

LGTM and thanks for the tests! 2 small questions left.

https://codereview.chromium.org/11447028/diff/15008/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11447028/diff/15008/cc/layer_impl.cc#newcode99
cc/layer_impl.cc:99: {
should this early-out if m_children.empty() now?

https://codereview.chromium.org/11447028/diff/15008/cc/layer_impl_unittest.cc
File cc/layer_impl_unittest.cc (right):

https://codereview.chromium.org/11447028/diff/15008/cc/layer_impl_unittest.cc...
cc/layer_impl_unittest.cc:203: root->setScrollDelta(gfx::Vector2d(0, 0));
should we clear resetNeedsUpdateDrawProps after this?

https://codereview.chromium.org/11447028/diff/15008/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/15008/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1535: 
extra \n

[enne (OOO), 2012-12-07 23:25:04]

https://codereview.chromium.org/11447028/diff/15008/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11447028/diff/15008/cc/layer_impl.cc#newcode99
cc/layer_impl.cc:99: {
On 2012/12/07 23:12:49, danakj wrote:
> should this early-out if m_children.empty() now?

Done.

https://codereview.chromium.org/11447028/diff/15008/cc/layer_impl_unittest.cc
File cc/layer_impl_unittest.cc (right):

https://codereview.chromium.org/11447028/diff/15008/cc/layer_impl_unittest.cc...
cc/layer_impl_unittest.cc:203: root->setScrollDelta(gfx::Vector2d(0, 0));
On 2012/12/07 23:12:49, danakj wrote:
> should we clear resetNeedsUpdateDrawProps after this?

Done.

https://codereview.chromium.org/11447028/diff/15008/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11447028/diff/15008/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1535: 
On 2012/12/07 23:12:49, danakj wrote:
> extra \n

Done.

[commit-bot: I haz the power, 2012-12-09 00:47:18]

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

[commit-bot: I haz the power, 2012-12-09 09:14:41]

Change committed as 172005