CC Animations: Make LayerAnimationController creation optional

CC Animations: Make LayerAnimationController creation optional

Don't call SetAnimationRegistrar directly - make RegisterForAnimations explicit.

Previous episodes:
https://codereview.chromium.org/1122393003/
https://codereview.chromium.org/1159633002/

This is a prerequisite to implement compositor animation timelines. Next episode:
https://codereview.chromium.org/947033002/

BUG=394777
R=ajuma@chromium.org
R=vollick@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5719778b3aa0a3acbc8202f8c689b6a25d0372c0
Cr-Commit-Position: refs/heads/master@{#331739}

[loyso (OOO), 2015-04-23 07:48:10]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org

[loyso (OOO), 2015-04-23 07:48:46]

loyso@chromium.org changed reviewers:
+ piman@chromium.org

[loyso (OOO), 2015-04-23 08:26:34]

https://codereview.chromium.org/1101823002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/1/cc/layers/layer.cc#newcode1240
cc/layers/layer.cc:1240: DCHECK(layer_animation_controller_);
webkit_unit_tests are failing. I'm considering to accumulate all
observers/delegates and pass them to LAC object on it's creation in
RegisterForAnimations.

[ajuma, 2015-04-23 15:09:16]

https://codereview.chromium.org/1101823002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/1/cc/layers/layer.cc#newcode1240
cc/layers/layer.cc:1240: DCHECK(layer_animation_controller_);
On 2015/04/23 08:26:33, loyso wrote:
> webkit_unit_tests are failing. I'm considering to accumulate all
> observers/delegates and pass them to LAC object on it's creation in
> RegisterForAnimations.

Hmm, these failures suggest that WebViewImpl::registerForAnimation is getting
called while m_layerTreeView is null (so that registerForAnimation is earlying
out). If this is correct, and I'm reading WebViewImpl correctly, this can only
happen after WebViewImpl::willCloseLayerTreeView has been called, and there's no
way to later have a non-null m_layerTreeView. So another option might be to
change this DCHECK (and the one in RemoveLayerAnimationEventObserver below) to
an 'if', but this seems fragile. Your suggestion sounds like a better approach.

https://codereview.chromium.org/1101823002/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:147: void Layer::SetCompositor(Compositor* compositor) {
This only gets called on the root layer, and only gets called with a non-null
compositor by Compositor::SetRootLayer. But later in the same call, the root
cc_layer gets added as a child of root_web_layer, which will lead to
cc::Layer::SetLayerTreeHost getting called on all the cc layers, which will lead
to them being registered. So the call to RegisterForAnimations below isn't
necessary, if we move AttachObservers so that it happens at the end of
Compositor::SetRootLayer.

https://codereview.chromium.org/1101823002/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:180: child->RegisterForAnimations(compositor);
The call to cc_layer_->AddChild(child->cc_layer_) above should lead to a call to
Layer::SetLayerTreeHost on child->cc_layer_ and all of its descendants, which in
turn will register these layers for animation, so there shouldn't be a need to
do that again here.

[loyso (OOO), 2015-04-24 03:44:53]

https://codereview.chromium.org/1101823002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/1/cc/layers/layer.cc#newcode1240
cc/layers/layer.cc:1240: DCHECK(layer_animation_controller_);
On 2015/04/23 15:09:15, ajuma wrote:
> On 2015/04/23 08:26:33, loyso wrote:
> > webkit_unit_tests are failing. I'm considering to accumulate all
> > observers/delegates and pass them to LAC object on it's creation in
> > RegisterForAnimations.
> 
> Hmm, these failures suggest that WebViewImpl::registerForAnimation is getting
> called while m_layerTreeView is null (so that registerForAnimation is earlying
> out). If this is correct, and I'm reading WebViewImpl correctly, this can only
> happen after WebViewImpl::willCloseLayerTreeView has been called, and there's
no
> way to later have a non-null m_layerTreeView. So another option might be to
> change this DCHECK (and the one in RemoveLayerAnimationEventObserver below) to
> an 'if', but this seems fragile. Your suggestion sounds like a better
approach.

Well, not really. We had a bunch of problems here:
1) LinkHighlight creates it's own content layer w/o registering it for
animations. See my fix and further explanations here:
https://codereview.chromium.org/1089043003/

2) We use WebLayerTreeViewImplForTesting for m_layerTreeView.
registerForAnimations was not plumbed to LayerTreeHost in that implementations.
See my fix in the upcoming PatchSet.

3) Some times we destroy layers without calling registerForAnimations or
setAnimationDelegate at all. Also see my fix in the upcoming PatchSet.

Very interesting :) It's cool that we identified this sub-task as a separate CL
and we isolate risks.

https://codereview.chromium.org/1101823002/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:147: void Layer::SetCompositor(Compositor* compositor) {
On 2015/04/23 15:09:15, ajuma wrote:
> This only gets called on the root layer, and only gets called with a non-null
> compositor by Compositor::SetRootLayer. But later in the same call, the root
> cc_layer gets added as a child of root_web_layer, which will lead to
> cc::Layer::SetLayerTreeHost getting called on all the cc layers, which will
lead
> to them being registered. So the call to RegisterForAnimations below isn't
> necessary, if we move AttachObservers so that it happens at the end of
> Compositor::SetRootLayer.

Done.

https://codereview.chromium.org/1101823002/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:180: child->RegisterForAnimations(compositor);
On 2015/04/23 15:09:15, ajuma wrote:
> The call to cc_layer_->AddChild(child->cc_layer_) above should lead to a call
to
> Layer::SetLayerTreeHost on child->cc_layer_ and all of its descendants, which
in
> turn will register these layers for animation, so there shouldn't be a need to
> do that again here.

Done.

[ajuma, 2015-04-24 16:02:16]

https://codereview.chromium.org/1101823002/diff/40001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/40001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:156: SendPendingThreadedAnimations();
This needs to happen after the cc layers are registered for animation (or else
the calls to AddAnimation won't actually add animations), so should be moved to
SetCompositorRootLayer (my fault for putting it in SetCompositor in the first
place).

https://codereview.chromium.org/1101823002/diff/40001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/1101823002/diff/40001/ui/compositor/layer.h#n...
ui/compositor/layer.h:91: void SetCompositorRootLayer(scoped_refptr<cc::Layer>
root_layer);
SetCompositorRootCCLayer maybe? (so the difference between this and
SetCompositor is clearer)

[loyso (OOO), 2015-04-27 06:34:09]

https://codereview.chromium.org/1101823002/diff/40001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/40001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:156: SendPendingThreadedAnimations();
On 2015/04/24 16:02:16, ajuma wrote:
> This needs to happen after the cc layers are registered for animation (or else
> the calls to AddAnimation won't actually add animations), so should be moved
to
> SetCompositorRootLayer (my fault for putting it in SetCompositor in the first
> place).

Done.

https://codereview.chromium.org/1101823002/diff/40001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/1101823002/diff/40001/ui/compositor/layer.h#n...
ui/compositor/layer.h:91: void SetCompositorRootLayer(scoped_refptr<cc::Layer>
root_layer);
On 2015/04/24 16:02:16, ajuma wrote:
> SetCompositorRootCCLayer maybe? (so the difference between this and
> SetCompositor is clearer)

Done.

[ajuma, 2015-04-27 14:51:56]

Some of the test failures look real. In particular, there are crashes involving
removing animations from non-existent controllers. I'd suggest looking at the
logic in ThreadedLayerAnimationElement to see if something is going wrong there
-- one thing that jumps out is that it only adds a threaded animation when the
duration is non-zero, but at the end of an animation it always tries to remove a
threaded animation. The logic in Layer::RemoveThreadedAnimation protects against
un-registered layers by checking if there are pending threaded animations, but
if no threaded animation was added in the first place then this will incorrectly
think that the animation was sent to cc.

Also noticed one more thing:

https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:1064: if (cc_layer_->layer_animation_controller())
It's probably better to move this null check to
cc::Layer::AddLayerAnimationEventObserver.

[loyso (OOO), 2015-04-28 05:10:36]

https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:1064: if (cc_layer_->layer_animation_controller())
On 2015/04/27 14:51:56, ajuma wrote:
> It's probably better to move this null check to
> cc::Layer::AddLayerAnimationEventObserver.

This is the only place in the project where
cc::Layer::AddLayerAnimationEventObserver is called.
And you mentioned that it would be fragile.
In that case a subscriber can subscribe and don't get any notification. That
would be a bug which is hard to track.

[loyso (OOO), 2015-04-28 05:23:13]

https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:1064: if (cc_layer_->layer_animation_controller())
On 2015/04/27 14:51:56, ajuma wrote:
> It's probably better to move this null check to
> cc::Layer::AddLayerAnimationEventObserver.
May we put a DCHECK here?

[ajuma, 2015-04-28 14:36:05]

lgtm. Thanks for the fixes to ThreadedLayerAnimationElement!

https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:1064: if (cc_layer_->layer_animation_controller())
On 2015/04/28 05:23:13, loyso wrote:
> On 2015/04/27 14:51:56, ajuma wrote:
> > It's probably better to move this null check to
> > cc::Layer::AddLayerAnimationEventObserver.
> May we put a DCHECK here?

If a DCHECK works, that'd be great. If not, perhaps add a TODO to fix this when
we switch over to the new system (e.g. so that either the call to
AddLayerAnimationEventObserver returns false when it fails, or it  always
succeeds).

[loyso (OOO), 2015-04-29 00:11:34]

loyso@chromium.org changed reviewers:
+ jbauman@chromium.org, kbr@chromium.org, sky@chromium.org

[Ian Vollick, 2015-04-29 00:37:53]

On 2015/04/28 14:36:05, ajuma wrote:
> lgtm. Thanks for the fixes to ThreadedLayerAnimationElement!
> 
> https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc
> File ui/compositor/layer.cc (right):
> 
>
https://codereview.chromium.org/1101823002/diff/60001/ui/compositor/layer.cc#...
> ui/compositor/layer.cc:1064: if (cc_layer_->layer_animation_controller())
> On 2015/04/28 05:23:13, loyso wrote:
> > On 2015/04/27 14:51:56, ajuma wrote:
> > > It's probably better to move this null check to
> > > cc::Layer::AddLayerAnimationEventObserver.
> > May we put a DCHECK here?
> 
> If a DCHECK works, that'd be great. If not, perhaps add a TODO to fix this
when
> we switch over to the new system (e.g. so that either the call to
> AddLayerAnimationEventObserver returns false when it fails, or it  always
> succeeds).

ui lgtm

[piman, 2015-04-29 00:46:11]

I'm concerned about adding a lot of recursion on tree operations.

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:161: // This function must only be called on the root ui
layer.
Can you add a DCHECK that it's the root (it doesn't have a parent)?

Why does it need to be separated from SetCompositor?

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:172: compositor_->layer_animator_collection());
We're doing 2 recursions (one in each of these functions). Is it really needed?
It seems inefficient.

Maybe a better way is to provide a single template RecurseTree function that
takes a functor, to which you can pass a lambda:

template<typename F> void RecurseTree(const F& f) {
  f(this);
  for (child : children_)
    child->RecurseTree(f);
}

Then here you can do something like:
LayerAnimatorCollection* collection = compositor_->layer_animator_collection();
RecurseTree([collection](Layer* layer) {
  layer->DetachAnimationObserver();
  layer->RemoveAnimators(collection);
});

And same in other places below where we're doing multiple recursions.

I think most recursive functions should be removed or changed to use
RecurseTree.

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:188: child->SendPendingThreadedAnimations();
2 recursions here too. Is that needed?

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:192: child->AddAnimatorsInTreeToCollection(collection);
And a third one here.

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:205: child->DetachAnimationObservers();
2 recursions

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:513: DetachAnimationObservers();
Is this right? Is it done at the right time?
We removed all children in the cc tree on l.507, and we'll reattach them on
l.531.
Is it right to recurse over the children when the tree is in a transient state?
Seems scary.
Do you need to recurse at all? The children will be the same before/after, only
this one layer is changing.

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer_an...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer_an...
ui/compositor/layer_animation_element.cc:343: if (Started() && IsThreaded()) {
We don't use the thread in the UI compositor any more. Do we need support for
all this?

[loyso (OOO), 2015-04-29 01:30:39]

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer_an...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer_an...
ui/compositor/layer_animation_element.cc:343: if (Started() && IsThreaded()) {
On 2015/04/29 00:46:11, piman (Very slow to review) wrote:
> We don't use the thread in the UI compositor any more. Do we need support for
> all this?

AFAIK, despite the fact that we don't use the thread in the UI compositor, we
still use the whole compositor infrastructure (LayerTreeHost/LayerTreeHostImpl
objects with SingleThreadProxy between them).

Apart from that, IsThreaded is defined as:
  bool IsThreaded() const override { return (duration() != base::TimeDelta()); }

Simply speaking, it doesn't pass 0-time anymations to the CC-compositor code
(which is started in SingleThreadProxy mode).

That's just a poor naming.
  // Whether this element animates on the compositor thread.
  virtual bool IsThreaded() const;

[Ken Russell (switch to Gerrit), 2015-04-29 01:48:32]

I defer review of content/renderer/gpu/ to piman.

[piman, 2015-04-29 02:08:38]

On Tue, Apr 28, 2015 at 6:30 PM, <loyso@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer_an...
> File ui/compositor/layer_animation_element.cc (right):
>
>
>
https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer_an...
> ui/compositor/layer_animation_element.cc:343: if (Started() &&
> IsThreaded()) {
> On 2015/04/29 00:46:11, piman (Very slow to review) wrote:
>
>> We don't use the thread in the UI compositor any more. Do we need
>>
> support for
>
>> all this?
>>
>
> AFAIK, despite the fact that we don't use the thread in the UI
> compositor, we still use the whole compositor infrastructure
> (LayerTreeHost/LayerTreeHostImpl objects with SingleThreadProxy between
> them).
>
> Apart from that, IsThreaded is defined as:
>   bool IsThreaded() const override { return (duration() !=
> base::TimeDelta()); }
>
> Simply speaking, it doesn't pass 0-time anymations to the CC-compositor
> code (which is started in SingleThreadProxy mode).
>
> That's just a poor naming.
>

Can we fix the naming?


>   // Whether this element animates on the compositor thread.
>   virtual bool IsThreaded() const;
>
> https://codereview.chromium.org/1101823002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[loyso (OOO), 2015-04-29 02:53:49]

On 2015/04/29 02:08:38, piman (Very slow to review) wrote:
> > Apart from that, IsThreaded is defined as:
> >   bool IsThreaded() const override { return (duration() !=
> > base::TimeDelta()); }
> Can we fix the naming?

That would be the domino effect! We should rename the interface method. We
should rename LayerAnimationSequence::IsFirstElementThreaded. This CL supposed
to be very small :)

My primary task is to move out the animation engine. The cc::Layers system is
deprecated in favor of DisplayLists. ui::Layers system is also questionable.
It definitely needs a good support, but the scope of that support must be
reasonable.

I can do the renaming here, np.

[loyso (OOO), 2015-04-29 03:25:29]

On 2015/04/29 02:08:38, piman (Very slow to review) wrote:
> > Apart from that, IsThreaded is defined as:
> >   bool IsThreaded() const override { return (duration() !=
> > base::TimeDelta()); }
> > That's just a poor naming.
> Can we fix the naming?
The 'threaded' term:
https://code.google.com/p/chromium/codesearch#search/&q=Threaded%20file:src/u...

[loyso (OOO), 2015-04-29 03:49:33]

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:161: // This function must only be called on the root ui
layer.
On 2015/04/29 00:46:11, piman (Very slow to review) wrote:
> Can you add a DCHECK that it's the root (it doesn't have a parent)?
Ack'ed.

> Why does it need to be separated from SetCompositor?
I thought on merging them into just one call. But
Compositor::SetRootLayer code allows to call SetCompositorRootCCLayer w/o
setting the compositor (calling SetCompositor). I'm not sure, whether we use
this configuration in runtime on all our platforms. That's a change in behavior.
It would be better to have such a transform as a separate small CL to isolate
risks.

[piman, 2015-04-29 03:53:40]

On Tue, Apr 28, 2015 at 7:53 PM, <loyso@chromium.org> wrote:

> On 2015/04/29 02:08:38, piman (Very slow to review) wrote:
>
>> > Apart from that, IsThreaded is defined as:
>> >   bool IsThreaded() const override { return (duration() !=
>> > base::TimeDelta()); }
>> Can we fix the naming?
>>
>
> That would be the domino effect! We should rename the interface method. We
> should rename LayerAnimationSequence::IsFirstElementThreaded. This CL
> supposed
> to be very small :)
>
> My primary task is to move out the animation engine. The cc::Layers system
> is
> deprecated in favor of DisplayLists. ui::Layers system is also
> questionable.
> It definitely needs a good support, but the scope of that support must be
> reasonable.
>
> I can do the renaming here, np.
>

It's ok to leave to a separate patch.


>
> https://codereview.chromium.org/1101823002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-04-29 04:08:15]

On Tue, Apr 28, 2015 at 8:49 PM, <loyso@chromium.org> wrote:

>
>
> https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc
> File ui/compositor/layer.cc (right):
>
>
>
https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
> ui/compositor/layer.cc:161: // This function must only be called on the
> root ui layer.
> On 2015/04/29 00:46:11, piman (Very slow to review) wrote:
>
>> Can you add a DCHECK that it's the root (it doesn't have a parent)?
>>
> Ack'ed.
>
>  Why does it need to be separated from SetCompositor?
>>
> I thought on merging them into just one call. But
> Compositor::SetRootLayer code allows to call SetCompositorRootCCLayer
> w/o setting the compositor (calling SetCompositor).


That seems bogus. A layer's GetCompositor() must be the compositor that
owns the layer tree in which that layer is. The self-assignment case is
handled with an early out.

I'd replace it with a DCHECK.


> I'm not sure,
> whether we use this configuration in runtime on all our platforms.
> That's a change in behavior. It would be better to have such a transform
> as a separate small CL to isolate risks.
>
> https://codereview.chromium.org/1101823002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[loyso (OOO), 2015-04-29 08:15:02]

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:513: DetachAnimationObservers();
On 2015/04/29 00:46:11, piman (Very slow to review) wrote:
> Is this right? Is it done at the right time?
> We removed all children in the cc tree on l.507, and we'll reattach them on
> l.531.
> Is it right to recurse over the children when the tree is in a transient
state?
> Seems scary.
> Do you need to recurse at all? The children will be the same before/after,
only
> this one layer is changing.

Acknowledged.

[piman, 2015-04-30 00:40:38]

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc...
ui/compositor/layer.cc:1023: DCHECK(cc_layer_->layer_animation_controller());
Is this DCHECK valid? SendPendingThreadedAnimations is called any time the layer
is added to the tree, regardless of animation controllers.

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc...
ui/compositor/layer.cc:1024: cc_layer_->AddLayerAnimationEventObserver(this);
Is this idempotent? SendPendingThreadedAnimations is called when we add the
layer to the tree, but we don't call RemoveLayerAnimationEventObserver when we
remove from the tree.

What is the invariant wrt the AnimationEventObserver?

Can we add unit tests to verify that?

[loyso (OOO), 2015-04-30 01:03:04]

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc...
ui/compositor/layer.cc:1023: DCHECK(cc_layer_->layer_animation_controller());
On 2015/04/30 00:40:38, piman (Very slow to review) wrote:
> Is this DCHECK valid? SendPendingThreadedAnimations is called any time the
layer
> is added to the tree, regardless of animation controllers.

SendPendingThreadedAnimations is only called after AddChild so LayerTreeHost /
RegisterForAnimations are set up.

SendPendingThreadedAnimations requires LAC anyway. It adds animations into LAC.

In fact, we can remove it because we have the same one in
cc_layer_->AddLayerAnimationEventObserver.

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc...
ui/compositor/layer.cc:1024: cc_layer_->AddLayerAnimationEventObserver(this);
On 2015/04/30 00:40:38, piman (Very slow to review) wrote:
> Is this idempotent? SendPendingThreadedAnimations is called when we add the
> layer to the tree, but we don't call RemoveLayerAnimationEventObserver when we
> remove from the tree.
> 
> What is the invariant wrt the AnimationEventObserver?
> 
> Can we add unit tests to verify that?

Yes, it's idempotent. Apart from SwithToLayer case, we detach ui::Layer from
cc::Layer in ui::Layer destructor only.

In general, the idea is simple. We defer AddLayerAnimationEventObserver from
CreateCCLayer call until SetCompositor OR Add cases. That's it.

"What is the invariant wrt the AnimationEventObserver?"
Sorry, I haven't caught the question.

[loyso (OOO), 2015-04-30 01:04:03]

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc...
ui/compositor/layer.cc:1023: DCHECK(cc_layer_->layer_animation_controller());
On 2015/04/30 01:03:04, loyso wrote:
> On 2015/04/30 00:40:38, piman (Very slow to review) wrote:
> > Is this DCHECK valid? SendPendingThreadedAnimations is called any time the
> layer
> > is added to the tree, regardless of animation controllers.
> 
> SendPendingThreadedAnimations is only called after AddChild so LayerTreeHost /
> RegisterForAnimations are set up.
> 
> SendPendingThreadedAnimations requires LAC anyway. It adds animations into
LAC.
> 
> In fact, we can remove it because we have the same one in
> cc_layer_->AddLayerAnimationEventObserver.

* Remove - I meant duplicate DCHECK.

[loyso (OOO), 2015-05-01 03:46:21]

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/120001/ui/compositor/layer.cc...
ui/compositor/layer.cc:1024: cc_layer_->AddLayerAnimationEventObserver(this);
On 2015/04/30 00:40:38, piman (Very slow to review) wrote:
> Is this idempotent? SendPendingThreadedAnimations is called when we add the
> layer to the tree, but we don't call RemoveLayerAnimationEventObserver when we
> remove from the tree.
> 
> What is the invariant wrt the AnimationEventObserver?
> 
> Can we add unit tests to verify that?

We call SendPendingThreadedAnimations at two places: Layer::SetCompositor and
Layer::Add.
1) In this CL it's clearly seen that in Layer::SetCompositor we always have
LTHs/LACs for the whole subtree prepared (root_web_layer always has a host set
and we add root ui layer into root_webLayer in the line right before the
SendPendingThreadedAnimations call).
2) In Layer::Add we skip SendPendingThreadedAnimations if Layer is orphaned.
Once we add the orphaned sub-tree into a layer with Compositor+LTH+LAC set, we
call SendPendingThreadedAnimations recursively for the whole previously orphaned
subtree.

[piman, 2015-05-01 19:25:13]

I think I'm starting to get my head around this, but, my main question is why
we're deferring creating the LayerAnimationController in cc::Layer until the
first time it's attached to the tree. It seems to add a lot of complexity but I
don't understand the value, because it doesn't use any state that requires it to
be in the tree, and we keep the same LAC as we move from tree to tree. So why
not create it at construction time?

The next patch doesn't really give any more insight... The LAC doesn't change in
there.

[loyso (OOO), 2015-05-04 01:02:14]

On 2015/05/01 19:25:13, piman (Very slow to review) wrote:
> I think I'm starting to get my head around this, but, my main question is why
> we're deferring creating the LayerAnimationController in cc::Layer until the
> first time it's attached to the tree. It seems to add a lot of complexity but
I
> don't understand the value, because it doesn't use any state that requires it
to
> be in the tree, and we keep the same LAC as we move from tree to tree. So why
> not create it at construction time?
> 
> The next patch doesn't really give any more insight... The LAC doesn't change
in
> there.

That's in the next-after-next patch =)
See Layer::RegisterForAnimations. We use LayerTreeSettings there.
https://codereview.chromium.org/1010663002/patch/160001/170005

In general, we want two animation systems to co-exist. In a new mode
Layer::layer_animation_controller_ must be null. We can enable the new mode
separately in cc Layers for ui and blink.
And we want to get rid of LAC controller eventually.

[piman, 2015-05-04 23:58:44]

On Sun, May 3, 2015 at 6:02 PM, <loyso@chromium.org> wrote:

> On 2015/05/01 19:25:13, piman (Very slow to review) wrote:
>
>> I think I'm starting to get my head around this, but, my main question is
>> why
>> we're deferring creating the LayerAnimationController in cc::Layer until
>> the
>> first time it's attached to the tree. It seems to add a lot of complexity
>> but
>>
> I
>
>> don't understand the value, because it doesn't use any state that
>> requires it
>>
> to
>
>> be in the tree, and we keep the same LAC as we move from tree to tree. So
>> why
>> not create it at construction time?
>>
>
>  The next patch doesn't really give any more insight... The LAC doesn't
>> change
>>
> in
>
>> there.
>>
>
> That's in the next-after-next patch =)
> See Layer::RegisterForAnimations. We use LayerTreeSettings there.
> https://codereview.chromium.org/1010663002/patch/160001/170005


Do we have a guarantee that the LayerTreeSettings is consistent between
compositors in the same process?
If not, then this doesn't work as you move layers between trees (animation
state needs to move between the LAC and whatever replaces the LAC when
layers are switched between trees with or
without use_compositor_animation_timelines)
If it is, maybe it then belongs to a global value, independent of trees, at
which point you might as well decide whether or not to create the LAC at
construction time. For example you can pass a bool to the cc::Layer
constructor.

The current logic is a strange and complicated contract: a cc::Layer has a
LAC if and only if it was ever attached to a tree that was attached to a
LayerTreeHost, but some cc::Layer functions can only be called if one does
exist, so the client has to track that.
This complex contract is also not tested, and that bothers me.

In general, we want two animation systems to co-exist. In a new mode
> Layer::layer_animation_controller_ must be null. We can enable the new mode
> separately in cc Layers for ui and blink.
> And we want to get rid of LAC controller eventually.
>

What I would like to avoid is the complexity due to having to traverse the
trees when doing tree manipulations, just because we delayed the LAC
construction. Or if we /really/ need it, it should be hidden inside of
cc::Layer. The current code makes a bunch of assumptions of consistency
between the cc::Layer tree and the ui::Layer tree (e.g. the contract above,
the fact that AddLayerAnimationEventObserver is idempotent), which may be
mostly true, but adds a bunch of complexity and isn't tested. The Law of
Demeter is broken in several places. And I assume you have to replicate
some of that complexity on the blink side.


>
> https://codereview.chromium.org/1101823002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[loyso (OOO), 2015-05-05 04:16:58]

On 2015/05/04 23:58:44, piman (Very slow to review) wrote:

> Do we have a guarantee that the LayerTreeSettings is consistent between
> compositors in the same process?
> If not, then this doesn't work as you move layers between trees (animation
> state needs to move between the LAC and whatever replaces the LAC when
> layers are switched between trees with or
> without use_compositor_animation_timelines)
Yes, it's consistent - it comes from per-process command line switch.
I considered it to be a global variable. But I don't like that approach.
It was discussed here.
https://codereview.chromium.org/986173002
That's a cc-pervasive bool. I've found that it's idiomatic to put all such flags
to LayerTreeSettings.

> If it is, maybe it then belongs to a global value, independent of trees, at
> which point you might as well decide whether or not to create the LAC at
> construction time. For example you can pass a bool to the cc::Layer
> constructor.
> The current logic is a strange and complicated contract: a cc::Layer has a
> LAC if and only if it was ever attached to a tree that was attached to a
> LayerTreeHost, but some cc::Layer functions can only be called if one does
> exist, so the client has to track that.
It was always like that. We were unable to add animations until layer is
RegisteredForAnimations/SetLayerTreeHost.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc...
Alsoe, see my 'blink' comment below.

> This complex contract is also not tested, and that bothers me.
It had many implicit/explicit tests (see surrounding changes). And it had
crashes.

Any practical suggestions, what particular tests need to be added?

> In general, we want two animation systems to co-exist. In a new mode
> > Layer::layer_animation_controller_ must be null. We can enable the new mode
> > separately in cc Layers for ui and blink.
> > And we want to get rid of LAC controller eventually.
> What I would like to avoid is the complexity due to having to traverse the
> trees when doing tree manipulations, just because we delayed the LAC
> construction.
Not sure if I understand this point. This CL doesn't have or add any tree
traversals.

> Or if we /really/ need it, it should be hidden inside of
> cc::Layer. The current code makes a bunch of assumptions of consistency
> between the cc::Layer tree and the ui::Layer tree (e.g. the contract above,
> the fact that AddLayerAnimationEventObserver is idempotent), which may be
> mostly true, but adds a bunch of complexity and isn't tested. The Law of
> Demeter is broken in several places. 
As for AddLayerAnimationEventObserver, I think we CAN put a DCHECK there to
prove it's non-idempotent. So it is run only once with a paired Remove.

As for high-coupling/low-cohesion between ui and cc trees, it's a long standing
story. For example, we already had some efforts implemented (to avoid addition
of animations to orphaned layers - ui::Layer::pending_threaded_animations_).
It's not perfect and needs improvements. But most likely will be deleted with
Slimming Paint.

> And I assume you have to replicate
> some of that complexity on the blink side.
It's very easy and centralized for blink - we call registerForAnimations for
every layer.
That was my primary reason to make it the same in ui::. 

Simply speaking: this contract was always there and it comes from blink. I want
it to be unified.

[loyso (OOO), 2015-05-05 04:32:39]

p.s. I could accumulate all observers/delegates in cc::Layer and pass them to
LAC on its creation. But that would introduce some extra-memory overhead. Which
is, in turn, might be eased with move semantics/extra complexity for
src/base/observer_list.h container.

What do you think?

[piman, 2015-05-05 18:10:28]

On Mon, May 4, 2015 at 9:16 PM, <loyso@chromium.org> wrote:

> On 2015/05/04 23:58:44, piman (Very slow to review) wrote:
>
>  Do we have a guarantee that the LayerTreeSettings is consistent between
>> compositors in the same process?
>> If not, then this doesn't work as you move layers between trees (animation
>> state needs to move between the LAC and whatever replaces the LAC when
>> layers are switched between trees with or
>> without use_compositor_animation_timelines)
>>
> Yes, it's consistent - it comes from per-process command line switch.
> I considered it to be a global variable. But I don't like that approach.
> It was discussed here.
> https://codereview.chromium.org/986173002
> That's a cc-pervasive bool. I've found that it's idiomatic to put all such
> flags
> to LayerTreeSettings.


In all honesty, that's craziness to add this giant complex mess of delayed
creation just because you can't get to this flag until you're in a tree,
simply because it's not in the right place. Please take a step back and
look at it again.

 If it is, maybe it then belongs to a global value, independent of trees, at
>> which point you might as well decide whether or not to create the LAC at
>> construction time. For example you can pass a bool to the cc::Layer
>> constructor.
>> The current logic is a strange and complicated contract: a cc::Layer has a
>> LAC if and only if it was ever attached to a tree that was attached to a
>> LayerTreeHost, but some cc::Layer functions can only be called if one does
>> exist, so the client has to track that.
>>
> It was always like that. We were unable to add animations until layer is
> RegisteredForAnimations/SetLayerTreeHost.
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc...
> Alsoe, see my 'blink' comment below.
>
>  This complex contract is also not tested, and that bothers me.
>>
> It had many implicit/explicit tests (see surrounding changes). And it had
> crashes.
>
> Any practical suggestions, what particular tests need to be added?
>
>  In general, we want two animation systems to co-exist. In a new mode
>> > Layer::layer_animation_controller_ must be null. We can enable the new
>> mode
>> > separately in cc Layers for ui and blink.
>> > And we want to get rid of LAC controller eventually.
>> What I would like to avoid is the complexity due to having to traverse the
>> trees when doing tree manipulations, just because we delayed the LAC
>> construction.
>>
> Not sure if I understand this point. This CL doesn't have or add any tree
> traversals.
>
>  Or if we /really/ need it, it should be hidden inside of
>> cc::Layer. The current code makes a bunch of assumptions of consistency
>> between the cc::Layer tree and the ui::Layer tree (e.g. the contract
>> above,
>> the fact that AddLayerAnimationEventObserver is idempotent), which may be
>> mostly true, but adds a bunch of complexity and isn't tested. The Law of
>> Demeter is broken in several places.
>>
> As for AddLayerAnimationEventObserver, I think we CAN put a DCHECK there to
> prove it's non-idempotent. So it is run only once with a paired Remove.
>
> As for high-coupling/low-cohesion between ui and cc trees, it's a long
> standing
> story. For example, we already had some efforts implemented (to avoid
> addition
> of animations to orphaned layers -
> ui::Layer::pending_threaded_animations_).
> It's not perfect and needs improvements. But most likely will be deleted
> with
> Slimming Paint.
>
>  And I assume you have to replicate
>> some of that complexity on the blink side.
>>
> It's very easy and centralized for blink - we call registerForAnimations
> for
> every layer.
> That was my primary reason to make it the same in ui::.
>
> Simply speaking: this contract was always there and it comes from blink. I
> want
> it to be unified.
>
> https://codereview.chromium.org/1101823002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2015-05-05 18:28:45]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2015-05-05 18:28:46]

The blink registration mechanism is an extremely ugly (I can say that since I
created it) way to associate particular cc::Layers with a particular tree even
when they aren't in that tree in order to work around cases where the blink-side
state associated with different trees is different.  It's not a model to follow
for anything else that does not have this requirement.

[loyso (OOO), 2015-05-06 03:04:39]

On 2015/05/05 18:28:46, jamesr wrote:
> The blink registration mechanism is an extremely ugly
Yeah, sure. I just wanted to make it explicit in cc. This is a short-term
intermediate CL. We want to clear out LayerAnimationController and
AnimationRegistrar in our new animation engine.

[loyso (OOO), 2015-05-06 03:26:25]

On 2015/05/05 18:10:28, piman (Very slow to review) wrote:
> In all honesty, that's craziness to add this giant complex mess of delayed
> creation just because you can't get to this flag until you're in a tree,
> simply because it's not in the right place. Please take a step back and
> look at it again.
PTAL! If it's ok to go with per-process global variable then I will clean-up the
LayerTreeSettings flag usage.

[piman, 2015-05-06 18:58:15]

Overall, I think this is much much simpler and clearer.

I suggested passing the bool to the Layer constructor, instead of having the
global inside of cc, because in single-process mode (for tests, or WebView?) we
may want a different value for browser vs renderer compositors for a staged
rollout? If this is not a concern (we will enable it on both at the same time),
and this is a transient state only, then I'm ok either way.

Lastly, I think the changes in ui/compositor are noop in functionality, now,
correct? They clean up a little bit so I'm not opposed to having them though,
but maybe in a separate CL?

[loyso (OOO), 2015-05-07 02:06:51]

On 2015/05/06 18:58:15, piman (Very slow to review) wrote:

> I suggested passing the bool to the Layer constructor
We have many sub-classes for cc::Layer. And we create objects of those
sub-classes at so many places/subsystems and
at different moments in time (initialization stages). Do you propose to query
CommandLine singleton with content:: switch everywhere?

Alternatively, we could make a special case (constructors) for ui:: (we have
ui::Layer::CreateCcLayer plus five creations/calls for ui::Layer::SwitchToLayer)
and go with a global variable for the rest of subsystems. That would be not that
clean.

That's why I still prefer RegisterForAnimations approach.

So, please - elaborate on passing bool on construction.

> instead of having the
> global inside of cc, because in single-process mode (for tests, or WebView?)
we
> may want a different value for browser vs renderer compositors for a staged
> rollout? If this is not a concern (we will enable it on both at the same
time),
> and this is a transient state only, then I'm ok either way.
Thanks for mentioning WebView, that might be concern.

> Lastly, I think the changes in ui/compositor are noop in functionality, now,
> correct? They clean up a little bit so I'm not opposed to having them though,
> but maybe in a separate CL?
Sure, np. I'll make all the unrelated ui:: changes as a separate CL.

[loyso (OOO), 2015-05-07 02:13:15]

Unrelated: it is maybe sad that we have ::Create factory methods everywhere but
we don't use any IoC idioms (passing the context/environment argument etc).

[piman, 2015-05-07 03:03:00]

On Wed, May 6, 2015 at 7:06 PM, <loyso@chromium.org> wrote:

> On 2015/05/06 18:58:15, piman (Very slow to review) wrote:
>
>  I suggested passing the bool to the Layer constructor
>>
> We have many sub-classes for cc::Layer. And we create objects of those
> sub-classes at so many places/subsystems and
> at different moments in time (initialization stages). Do you propose to
> query
> CommandLine singleton with content:: switch everywhere?
>

We mostly create cc::Layers in 4 places. cc/blink for the renderer,
ui/compositor for the aura browser compositor,
chrome/browser/android/compositor for the android browser compositor, and
cc unit tests.
It's easy to have one bool in each (no need to parse strings on every layer
creation). See also e.g. WebContentLayerImpl or ui::Layer::CreateCcLayer
where we choose between ContentLayer and PictureLayer based on a static.
What you're doing seems similar in spirit.

You're right that it would be helpful to pass some environment to
*Layer::Create - maybe someone just needs to bite the bullet.

Alternatively, we could make a special case (constructors) for ui:: (we have
> ui::Layer::CreateCcLayer plus five creations/calls for
> ui::Layer::SwitchToLayer)
> and go with a global variable for the rest of subsystems. That would be
> not that
> clean.
>
> That's why I still prefer RegisterForAnimations approach.
>
> So, please - elaborate on passing bool on construction.
>
>  instead of having the
>> global inside of cc, because in single-process mode (for tests, or
>> WebView?)
>>
> we
>
>> may want a different value for browser vs renderer compositors for a
>> staged
>> rollout? If this is not a concern (we will enable it on both at the same
>>
> time),
>
>> and this is a transient state only, then I'm ok either way.
>>
> Thanks for mentioning WebView, that might be concern.
>
>  Lastly, I think the changes in ui/compositor are noop in functionality,
>> now,
>> correct? They clean up a little bit so I'm not opposed to having them
>> though,
>> but maybe in a separate CL?
>>
> Sure, np. I'll make all the unrelated ui:: changes as a separate CL.
>
> https://codereview.chromium.org/1101823002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[loyso (OOO), 2015-05-07 06:38:48]

A 'big-picture' context:

What we want to achieve as a result for this series of CLs:
crrev.com/1131833002 Blink: Torpedo the old intrusive animation system. [DEMO,
not for review]
crrev.com/1130043003 CC: Torpedo the old intrusive animation system. [DEMO, not
for review]

We delete RegisterForAnimations there.

[loyso (OOO), 2015-05-07 06:39:19]

http://crrev.com/1131833002 Blink: Torpedo the old intrusive animation system.
[DEMO,
not for review]
http://crrev.com/1130043003 CC: Torpedo the old intrusive animation system.
[DEMO, not
for review]

[piman, 2015-05-07 22:35:25]

On Wed, May 6, 2015 at 11:38 PM, <loyso@chromium.org> wrote:

> A 'big-picture' context:
>
> What we want to achieve as a result for this series of CLs:
> crrev.com/1131833002 Blink: Torpedo the old intrusive animation system.
> [DEMO,
> not for review]
> crrev.com/1130043003 CC: Torpedo the old intrusive animation system.
> [DEMO, not
> for review]
>
> We delete RegisterForAnimations there.
>

I don't have a problem with RegisterForAnimations per se, I have a problem
with the confusing contracts and the resulting complexity.


>
> https://codereview.chromium.org/1101823002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-08 18:21:40]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-05-08 18:21:41]

Since this has to work for Orphan layers I can see that LayerTreeSettings isn't
going to work.

So I guess using a global will make sense for this. You can still use different
command line flags.
--enable-the-thing would set the global appropriately in RenderThreadImpl
directly to cc::Layer::SetFoo().
--ui-enable-the-thing would set the global appropriately in
GpuProcessTransportFactory (via a static method on ui::Compositor I think).

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:145: RegisterForAnimations(host->animation_registrar(),
host->settings());
if (layer_animation_controller_)?

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1408: const LayerTreeSettings& settings) {
why are settings coming here but not being used?

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.h#newc...
cc/layers/layer.h:430: DCHECK(layer_animation_controller_);
i don't think this DCHECK is doing much for you? you'd just null-deref on the
next line anyways.

[loyso (OOO), 2015-05-11 06:59:14]

On 2015/05/08 18:21:41, danakj wrote:

> Since this has to work for Orphan layers I can see that LayerTreeSettings
isn't
> going to work.
It hasn't to work for orphan layers. We are unable to add animations if
AnimationRegistrar isn't specified.

> So I guess using a global will make sense for this. You can still use
different
> command line flags.
> --enable-the-thing would set the global appropriately in RenderThreadImpl
> directly to cc::Layer::SetFoo().
> --ui-enable-the-thing would set the global appropriately in
> GpuProcessTransportFactory (via a static method on ui::Compositor I think).
Does the global flag work for Android WebView?

[loyso (OOO), 2015-05-11 06:59:43]

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:145: RegisterForAnimations(host->animation_registrar(),
host->settings());
On 2015/05/08 18:21:40, danakj wrote:
> if (layer_animation_controller_)?
Done in RegisterForAnimations.

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1408: const LayerTreeSettings& settings) {
On 2015/05/08 18:21:40, danakj wrote:
> why are settings coming here but not being used?
Sorry, the CL was in inconsistent state (while prototyping possible approaches).
I was going to use it in the next-after-next CL
https://codereview.chromium.org/1010663002/patch/160001/170005

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/1101823002/diff/140001/cc/layers/layer.h#newc...
cc/layers/layer.h:430: DCHECK(layer_animation_controller_);
On 2015/05/08 18:21:40, danakj wrote:
> i don't think this DCHECK is doing much for you? you'd just null-deref on the
> next line anyways.
It makes the problem clear. null-deref is undefined behavior.

[loyso (OOO), 2015-05-11 07:01:31]

On 2015/05/07 03:03:00, piman (Very slow to review) wrote:

> >  I suggested passing the bool to the Layer constructor
> We mostly create cc::Layers in 4 places. cc/blink for the renderer,
> ui/compositor for the aura browser compositor,
> chrome/browser/android/compositor for the android browser compositor, and
> cc unit tests.
> It's easy to have one bool in each (no need to parse strings on every layer
> creation). See also e.g. WebContentLayerImpl or ui::Layer::CreateCcLayer
> where we choose between ContentLayer and PictureLayer based on a static.
> What you're doing seems similar in spirit.
> 
> You're right that it would be helpful to pass some environment to
> *Layer::Create - maybe someone just needs to bite the bullet.
Done. Please, take a look.

[piman, 2015-05-11 21:31:24]

I would suggest moving the introduction of LayerSettings into a separate patch.
It's always better to separate big invasive changes (that don't introduce
functionality) from other changes, in case the patch has to be reverted.

https://codereview.chromium.org/1101823002/diff/160001/cc/blink/web_layer_imp...
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/1101823002/diff/160001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:51: cc::LayerSettings g_layer_settings;
Non-POD globals are forbidden by style guide. You can use a LazyInstance for
example, or have it in the RenderThreadImpl and pass it by pointer to
WebLayerImpl (being careful with tests).

https://codereview.chromium.org/1101823002/diff/160001/chrome/browser/android...
File chrome/browser/android/compositor/layer/content_layer.cc (right):

https://codereview.chromium.org/1101823002/diff/160001/chrome/browser/android...
chrome/browser/android/compositor/layer/content_layer.cc:16: cc::LayerSettings
g_content_layer_settings;
Same here wrt static non-POD.

https://codereview.chromium.org/1101823002/diff/160001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1101823002/diff/160001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:233:
settings.hud_layer_settings_.use_compositor_animation_timelines = true;
Could you use WebLayerImpl::LayerSettings() here for
settings.hud_layer_settings_? It would keep them consistent between each other.

https://codereview.chromium.org/1101823002/diff/160001/content/renderer/peppe...
File content/renderer/pepper/pepper_compositor_host.h (right):

https://codereview.chromium.org/1101823002/diff/160001/content/renderer/peppe...
content/renderer/pepper/pepper_compositor_host.h:45: static const
cc::LayerSettings& PepperLayerSettings();
Pepper should use the same settings as WebLayerImpl.

https://codereview.chromium.org/1101823002/diff/160001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101823002/diff/160001/ui/compositor/layer.cc...
ui/compositor/layer.cc:59: cc::LayerSettings g_ui_layer_settings;
Same here

[danakj, 2015-05-11 21:58:06]

On 2015/05/07 03:03:00, piman (Very slow to review) wrote:
> On Wed, May 6, 2015 at 7:06 PM, <mailto:loyso@chromium.org> wrote:
> 
> > On 2015/05/06 18:58:15, piman (Very slow to review) wrote:
> >
> >  I suggested passing the bool to the Layer constructor
> >>
> > We have many sub-classes for cc::Layer. And we create objects of those
> > sub-classes at so many places/subsystems and
> > at different moments in time (initialization stages). Do you propose to
> > query
> > CommandLine singleton with content:: switch everywhere?
> >
> 
> We mostly create cc::Layers in 4 places. cc/blink for the renderer,
> ui/compositor for the aura browser compositor,
> chrome/browser/android/compositor for the android browser compositor, and
> cc unit tests.
> It's easy to have one bool in each (no need to parse strings on every layer
> creation). See also e.g. WebContentLayerImpl or ui::Layer::CreateCcLayer
> where we choose between ContentLayer and PictureLayer based on a static.
> What you're doing seems similar in spirit.

Yes, I agree here. Changing Layer::Layer() constructor will probably be a bit
insane for unit tests, and the behaviour change is inside cc::Layer, whereas for
implside painting it's choosing between 2 Layer types so the behaviour change is
outside cc::Layer. So it's probably easiest to make the global inside cc::Layer
and just set it once for each embedder (ui::Compositor, cr/b/android/compositor,
RenderThreadImpl, and the webview thinger). You can trace back the calls to
LayerTreeHost constructor to find all possible embedders.

> You're right that it would be helpful to pass some environment to
> *Layer::Create - maybe someone just needs to bite the bullet.
> 
> Alternatively, we could make a special case (constructors) for ui:: (we have
> > ui::Layer::CreateCcLayer plus five creations/calls for
> > ui::Layer::SwitchToLayer)
> > and go with a global variable for the rest of subsystems. That would be
> > not that
> > clean.
> >
> > That's why I still prefer RegisterForAnimations approach.
> >
> > So, please - elaborate on passing bool on construction.
> >
> >  instead of having the
> >> global inside of cc, because in single-process mode (for tests, or
> >> WebView?)
> >>
> > we
> >
> >> may want a different value for browser vs renderer compositors for a
> >> staged
> >> rollout? If this is not a concern (we will enable it on both at the same
> >>
> > time),
> >
> >> and this is a transient state only, then I'm ok either way.
> >>
> > Thanks for mentioning WebView, that might be concern.
> >
> >  Lastly, I think the changes in ui/compositor are noop in functionality,
> >> now,
> >> correct? They clean up a little bit so I'm not opposed to having them
> >> though,
> >> but maybe in a separate CL?
> >>
> > Sure, np. I'll make all the unrelated ui:: changes as a separate CL.
> >
> > https://codereview.chromium.org/1101823002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[loyso (OOO), 2015-05-12 04:29:32]

On 2015/05/11 21:31:24, piman (Very slow to review) wrote:
> I would suggest moving the introduction of LayerSettings into a separate
patch.
Done. Please, review here:
https://codereview.chromium.org/1122393003

[loyso (OOO), 2015-05-12 07:31:42]

loyso@chromium.org changed reviewers:
- danakj@chromium.org, jamesr@chromium.org, jbauman@chromium.org,
kbr@chromium.org, sky@chromium.org

[loyso (OOO), 2015-05-25 06:42:51]

piman (Very slow to review) wrote:
> Lastly, I think the changes in ui/compositor are noop in functionality, now,
> correct? They clean up a little bit so I'm not opposed to having them though,
> but maybe in a separate CL?
Done. Please, review the separated CL here:
https://codereview.chromium.org/1159633002/

[loyso (OOO), 2015-05-25 06:58:52]

loyso@chromium.org changed reviewers:
+ sky@chromium.org

[loyso (OOO), 2015-05-25 06:58:53]

sky@chromium.org: Please review changes in

[piman, 2015-05-26 16:45:24]

LGTM for content/

[Ian Vollick, 2015-05-27 01:37:28]

On 2015/05/26 at 16:45:24, piman wrote:
> LGTM for content/

lgtm.

[sky, 2015-05-28 02:59:56]

LGTM

[loyso (OOO), 2015-05-28 03:03:04]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2015-05-28 03:03:05]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1101823002/#ps180001
(title: "Rebase on top of extracted changes.")

[commit-bot: I haz the power, 2015-05-28 03:03:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1101823002/180001

[commit-bot: I haz the power, 2015-05-28 04:10:34]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-05-28 04:11:13]

Patchset 10 (id:??) landed as
https://crrev.com/5719778b3aa0a3acbc8202f8c689b6a25d0372c0
Cr-Commit-Position: refs/heads/master@{#331739}