Refactor MutatorHostClient from LayerTreeHost to LayerTree.

Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.

BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/95d9e6b63179642af8391906ccbe586bd3587b56
Cr-Commit-Position: refs/heads/master@{#412727}

[xingliu, 2016-08-05 02:09:03]

Description was changed from

==========
Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
==========

to

==========
Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[xingliu, 2016-08-05 02:10:40]

xingliu@chromium.org changed reviewers:
+ khushalsagar@chromium.org

[xingliu, 2016-08-05 02:10:41]

Refactor MutatorHostClient, send to team member for internal review before
sending to cc team.

[xingliu, 2016-08-05 02:11:10]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 02:11:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 03:06:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 03:06:05]

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-08-08 16:55:02]

khushalsagar@chromium.org changed reviewers:
+ ajuma@chromium.org

[Khushal, 2016-08-08 16:55:03]

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

https://codereview.chromium.org/2216203002/diff/1/cc/layers/layer.h#newcode561
cc/layers/layer.h:561: friend class LayerTree;
Did we need to friend LayerTree in this change?

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:371:
layer_tree_->animation_host()->SetMutatorHostClient(nullptr);
This should be done in the LayerTree dtor. But you need to make sure that you
destroy the LayerTree after you set the LTH to null on the root layer, since we
have a cached pointer to the LayerTree there which is always expected when we
have a reference to the host.
I would say move this to the LayerTree and destroy the tree after setting the
host to null on the root layer. Unless there is an ordering dependency here.
+ajuma, does this sound okay?

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1313: return layer_tree_->LayerById(id);
Callers can directly go to the LayerTree itself.

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1537: return layer_tree_->animation_host();
While we are at it, may be we can remove the methods that are simply delegating
to the LayerTree now and have callers use the LayerTree itself.

[ajuma, 2016-08-08 17:56:34]

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:371:
layer_tree_->animation_host()->SetMutatorHostClient(nullptr);
On 2016/08/08 16:55:03, Khushal wrote:
> This should be done in the LayerTree dtor. But you need to make sure that you
> destroy the LayerTree after you set the LTH to null on the root layer, since
we
> have a cached pointer to the LayerTree there which is always expected when we
> have a reference to the host.
> I would say move this to the LayerTree and destroy the tree after setting the
> host to null on the root layer. Unless there is an ordering dependency here.
> +ajuma, does this sound okay?

Sounds good.

[xingliu, 2016-08-09 20:57:05]

Fixes based on review, also refactored iterators, and remove animation_host()
from LayerTreeHost. Also fixed all call sites.

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

https://codereview.chromium.org/2216203002/diff/1/cc/layers/layer.h#newcode561
cc/layers/layer.h:561: friend class LayerTree;
On 2016/08/08 16:55:02, Khushal wrote:
> Did we need to friend LayerTree in this change?

In LayerTree MutatorHostClient override functions, it will directly call private
methods of Layer, so I guess we still need to friend LayerTree for Layer, same
way as what it does for LayerTreeHost.

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:371:
layer_tree_->animation_host()->SetMutatorHostClient(nullptr);
On 2016/08/08 16:55:03, Khushal wrote:
> This should be done in the LayerTree dtor. But you need to make sure that you
> destroy the LayerTree after you set the LTH to null on the root layer, since
we
> have a cached pointer to the LayerTree there which is always expected when we
> have a reference to the host.
> I would say move this to the LayerTree and destroy the tree after setting the
> host to null on the root layer. Unless there is an ordering dependency here.
> +ajuma, does this sound okay?

Done.

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1313: return layer_tree_->LayerById(id);
On 2016/08/08 16:55:03, Khushal wrote:
> Callers can directly go to the LayerTree itself.

Done, also move the iterators and all call sites for that. Also the templates in
tree_synchronizor

https://codereview.chromium.org/2216203002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1537: return layer_tree_->animation_host();
On 2016/08/08 16:55:02, Khushal wrote:
> While we are at it, may be we can remove the methods that are simply
delegating
> to the LayerTree now and have callers use the LayerTree itself.

Done, also fix the call sites.

[xingliu, 2016-08-09 21:20:01]

xingliu@chromium.org changed reviewers:
+ sievers@chromium.org, vollick@chromium.org

[xingliu, 2016-08-09 21:20:02]

Hi, Blimp client team is currently refactoring part of LayerTreeHost to
LayerTree, please help to review for the following files. Thank you.

vollick@chromium.org: Please review changes in ui/compositor/compositor.cc.

sievers@chromium.org: Please review changes in 
content/renderer/gpu/render_widget_compositor.cc,
content/renderer/gpu/gpu_benchmarking_extension.cc

[xingliu, 2016-08-09 21:25:47]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 21:26:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 21:38:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 21:38:49]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[xingliu, 2016-08-09 21:53:00]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 21:53:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 22:44:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 22:44:35]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[xingliu, 2016-08-10 00:31:13]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 00:32:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 04:29:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 04:29:28]

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-08-10 18:28:16]

khushalsagar@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org, loyso@chromium.org

[Khushal, 2016-08-10 18:28:17]

+loyso. You mentioned that it would be preferable to leave the AnimationHost and
MutatorHostClient with the LayerTreeHost, for symmetry with the
LayerTreeHostImpl. That sounds reasonable.

One option is to have a LayerTreeHostBase class which could retain this, and
have all implementations of LayerTreeHost interface inherit from that. Later on
it would have a weak reference to the MutatorHost instead.

One suggestion you had was to have the LayerTreeHost interface be a
cc::Compositor interface instead, and have the LayerTreeHostBase be a concrete
LayerTreeHost instead which is owned by the cc::Compositor. This was more about
naming I guess since the LayerTreeHost is perhaps a bit overloaded. The pattern
in chromium right now is to embed cc using a LayerTreeHost and the embedding
class is called the Compositor.  That would also change then? I think cc folks
would be the best to comment here for what is preferable.

[danakj, 2016-08-10 18:36:00]

On Wed, Aug 10, 2016 at 11:28 AM, <khushalsagar@chromium.org> wrote:

> +loyso. You mentioned that it would be preferable to leave the
> AnimationHost and
> MutatorHostClient with the LayerTreeHost, for symmetry with the
> LayerTreeHostImpl. That sounds reasonable.
>
> One option is to have a LayerTreeHostBase class which could retain this,
> and
> have all implementations of LayerTreeHost interface inherit from that.
> Later on
> it would have a weak reference to the MutatorHost instead.
>

Or just make both implementations of LayerTreeHost have an AnimationHost
(does blimp actually want one?). Sharing code is good but at some point
also just having 2 implementations of the interface is fine.


>
> One suggestion you had was to have the LayerTreeHost interface be a
> cc::Compositor interface instead, and have the LayerTreeHostBase be a
> concrete
> LayerTreeHost instead which is owned by the cc::Compositor. This was more
> about
> naming I guess since the LayerTreeHost is perhaps a bit overloaded. The
> pattern
> in chromium right now is to embed cc using a LayerTreeHost and the
> embedding
> class is called the Compositor. That would also change then? I think cc
> folks
> would be the best to comment here for what is preferable.
>

It really not even a compositor anymore heh, it just does some math,
handles input and such, and delegates compositing to the Display.

I don't wanna fuss about naming too much here, but renaming things will
have a high cognitive load across a lot of teams, since this is the public
API every team uses to get to cc. So I'd rather the interface was
LayerTreeHost. I don't really care what the cc implementation of it is
called, too bad it can't be LayerTreeHostImpl, lol. InProcessLayerTreeHost
or something legit describes it.



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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Khushal, 2016-08-10 18:55:40]

On 2016/08/10 18:36:00, danakj wrote:
> On Wed, Aug 10, 2016 at 11:28 AM, <mailto:khushalsagar@chromium.org> wrote:
> 
> > +loyso. You mentioned that it would be preferable to leave the
> > AnimationHost and
> > MutatorHostClient with the LayerTreeHost, for symmetry with the
> > LayerTreeHostImpl. That sounds reasonable.
> >
> > One option is to have a LayerTreeHostBase class which could retain this,
> > and
> > have all implementations of LayerTreeHost interface inherit from that.
> > Later on
> > it would have a weak reference to the MutatorHost instead.
> >
> 
> Or just make both implementations of LayerTreeHost have an AnimationHost
> (does blimp actually want one?). Sharing code is good but at some point
> also just having 2 implementations of the interface is fine.

We want one right now because the embedder wants it, and also because eventually
we would add support for threaded animations.
That interface is used just to query stuff from layers so its not much to
duplicate, but I figured better to just put parts that can be shared in a base
class.

> 
> 
> >
> > One suggestion you had was to have the LayerTreeHost interface be a
> > cc::Compositor interface instead, and have the LayerTreeHostBase be a
> > concrete
> > LayerTreeHost instead which is owned by the cc::Compositor. This was more
> > about
> > naming I guess since the LayerTreeHost is perhaps a bit overloaded. The
> > pattern
> > in chromium right now is to embed cc using a LayerTreeHost and the
> > embedding
> > class is called the Compositor. That would also change then? I think cc
> > folks
> > would be the best to comment here for what is preferable.
> >
> 
> It really not even a compositor anymore heh, it just does some math,
> handles input and such, and delegates compositing to the Display.
> 
> I don't wanna fuss about naming too much here, but renaming things will
> have a high cognitive load across a lot of teams, since this is the public
> API every team uses to get to cc. So I'd rather the interface was

Sounds good. I'd want to avoid taking that unnecessary churn too.

> LayerTreeHost. I don't really care what the cc implementation of it is
> called, too bad it can't be LayerTreeHostImpl, lol. InProcessLayerTreeHost

Lol, yeah, LTHI is the first name that I had thought of too. :P

> or something legit describes it.
> 
> 
> 
> >
> > https://codereview.chromium.org/2216203002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-08-10 20:10:42]

On Wed, Aug 10, 2016 at 11:55 AM, <khushalsagar@chromium.org> wrote:

> On 2016/08/10 18:36:00, danakj wrote:
> > On Wed, Aug 10, 2016 at 11:28 AM, <mailto:khushalsagar@chromium.org>
> wrote:
> >
> > > +loyso. You mentioned that it would be preferable to leave the
> > > AnimationHost and
> > > MutatorHostClient with the LayerTreeHost, for symmetry with the
> > > LayerTreeHostImpl. That sounds reasonable.
> > >
> > > One option is to have a LayerTreeHostBase class which could retain
> this,
> > > and
> > > have all implementations of LayerTreeHost interface inherit from that.
> > > Later on
> > > it would have a weak reference to the MutatorHost instead.
> > >
> >
> > Or just make both implementations of LayerTreeHost have an AnimationHost
> > (does blimp actually want one?). Sharing code is good but at some point
> > also just having 2 implementations of the interface is fine.
>
> We want one right now because the embedder wants it, and also because
> eventually
> we would add support for threaded animations.
> That interface is used just to query stuff from layers so its not much to
> duplicate, but I figured better to just put parts that can be shared in a
> base
> class.
>

Yeh, I'm just a bit skeptical in general about the
Interface->InterfaceBase->SubclassOfInterfaceBase pattern. It feels like an
abuse of inheritance. And the InterfaceBase should either be in a class
that is owned by (composed into) the implementation child class, or it
should just be duplicated into each implementation child class.




>
>
> >
> >
> > >
> > > One suggestion you had was to have the LayerTreeHost interface be a
> > > cc::Compositor interface instead, and have the LayerTreeHostBase be a
> > > concrete
> > > LayerTreeHost instead which is owned by the cc::Compositor. This was
> more
> > > about
> > > naming I guess since the LayerTreeHost is perhaps a bit overloaded. The
> > > pattern
> > > in chromium right now is to embed cc using a LayerTreeHost and the
> > > embedding
> > > class is called the Compositor. That would also change then? I think cc
> > > folks
> > > would be the best to comment here for what is preferable.
> > >
> >
> > It really not even a compositor anymore heh, it just does some math,
> > handles input and such, and delegates compositing to the Display.
> >
> > I don't wanna fuss about naming too much here, but renaming things will
> > have a high cognitive load across a lot of teams, since this is the
> public
> > API every team uses to get to cc. So I'd rather the interface was
>
> Sounds good. I'd want to avoid taking that unnecessary churn too.
>
> > LayerTreeHost. I don't really care what the cc implementation of it is
> > called, too bad it can't be LayerTreeHostImpl, lol.
> InProcessLayerTreeHost
>
> Lol, yeah, LTHI is the first name that I had thought of too. :P
>
> > or something legit describes it.
> >
> >
> >
> > >
> > > https://codereview.chromium.org/2216203002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2216203002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Khushal, 2016-08-10 21:41:02]

On 2016/08/10 20:10:42, danakj wrote:
> On Wed, Aug 10, 2016 at 11:55 AM, <mailto:khushalsagar@chromium.org> wrote:
> 
> > On 2016/08/10 18:36:00, danakj wrote:
> > > On Wed, Aug 10, 2016 at 11:28 AM, <mailto:khushalsagar@chromium.org>
> > wrote:
> > >
> > > > +loyso. You mentioned that it would be preferable to leave the
> > > > AnimationHost and
> > > > MutatorHostClient with the LayerTreeHost, for symmetry with the
> > > > LayerTreeHostImpl. That sounds reasonable.
> > > >
> > > > One option is to have a LayerTreeHostBase class which could retain
> > this,
> > > > and
> > > > have all implementations of LayerTreeHost interface inherit from that.
> > > > Later on
> > > > it would have a weak reference to the MutatorHost instead.
> > > >
> > >
> > > Or just make both implementations of LayerTreeHost have an AnimationHost
> > > (does blimp actually want one?). Sharing code is good but at some point
> > > also just having 2 implementations of the interface is fine.
> >
> > We want one right now because the embedder wants it, and also because
> > eventually
> > we would add support for threaded animations.
> > That interface is used just to query stuff from layers so its not much to
> > duplicate, but I figured better to just put parts that can be shared in a
> > base
> > class.
> >
> 
> Yeh, I'm just a bit skeptical in general about the
> Interface->InterfaceBase->SubclassOfInterfaceBase pattern. It feels like an
> abuse of inheritance. And the InterfaceBase should either be in a class
> that is owned by (composed into) the implementation child class, or it
> should just be duplicated into each implementation child class.

I guess it ends up being useful particularly in cases where the interface is
supposed to be a public API and the base class has the code that all the
implementations can share. Also so that the functionalities that are supposed to
be internal only go on the base class, which is what the internal components
see, rather than being a part of the public interface. RenderWidgetHostView
would be a good example of that. But then we would use DEPS to forbid includes
of internal classes elsewhere, and we don't really have that concept in cc in
general. So I'm okay with duplicating it across implementations if you think
that's better. :)

> 
> 
> 
> 
> >
> >
> > >
> > >
> > > >
> > > > One suggestion you had was to have the LayerTreeHost interface be a
> > > > cc::Compositor interface instead, and have the LayerTreeHostBase be a
> > > > concrete
> > > > LayerTreeHost instead which is owned by the cc::Compositor. This was
> > more
> > > > about
> > > > naming I guess since the LayerTreeHost is perhaps a bit overloaded. The
> > > > pattern
> > > > in chromium right now is to embed cc using a LayerTreeHost and the
> > > > embedding
> > > > class is called the Compositor. That would also change then? I think cc
> > > > folks
> > > > would be the best to comment here for what is preferable.
> > > >
> > >
> > > It really not even a compositor anymore heh, it just does some math,
> > > handles input and such, and delegates compositing to the Display.
> > >
> > > I don't wanna fuss about naming too much here, but renaming things will
> > > have a high cognitive load across a lot of teams, since this is the
> > public
> > > API every team uses to get to cc. So I'd rather the interface was
> >
> > Sounds good. I'd want to avoid taking that unnecessary churn too.
> >
> > > LayerTreeHost. I don't really care what the cc implementation of it is
> > > called, too bad it can't be LayerTreeHostImpl, lol.
> > InProcessLayerTreeHost
> >
> > Lol, yeah, LTHI is the first name that I had thought of too. :P
> >
> > > or something legit describes it.
> > >
> > >
> > >
> > > >
> > > > https://codereview.chromium.org/2216203002/
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> > https://codereview.chromium.org/2216203002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2016-08-11 17:39:44]

Agreed that renaming things is a lot of confusion. I'd prefer LayerTreeHost to
be the top level interface with LayerTreeHostInProcess and LayerTreeHostBlimp or
whatever to differentiate concrete implementations.

Also agreed about disliking using inheritance to share code.  If there's code to
share, then use composition or code duplication to achieve that over some
complicated inheritance hierarchy.

[xingliu, 2016-08-11 18:43:46]

Description was changed from

==========
Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Based on the discussions, this CL will be closed, and we will move AnimationHost
ownership back to LayerTreeHost.


Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[loyso (OOO), 2016-08-12 00:57:34]

On 2016/08/10 18:28:17, Khushal wrote:
> One suggestion you had was to have the LayerTreeHost interface be a
> cc::Compositor interface instead, and have the LayerTreeHostBase be a concrete
> LayerTreeHost instead which is owned by the cc::Compositor. This was more
about
> naming I guess since the LayerTreeHost is perhaps a bit overloaded. The
pattern
> in chromium right now is to embed cc using a LayerTreeHost and the embedding
> class is called the Compositor.
Yeah, that's what I mean. We have ui::Layer and cc::Layer.
Would be reasonable for embedders like ui::Compositor to embed CC via
cc::Compositor class.

> That would also change then? I think cc folks
> would be the best to comment here for what is preferable.
Chromium is a large scale project, so it's ok to leave things 'as is' according
to The Principle of Least Astonishment :)
We can go without fundamental renaming.

[loyso (OOO), 2016-08-12 01:06:01]

This CL overall l-g-t-m.

[xingliu, 2016-08-12 01:14:44]

Description was changed from

==========
Based on the discussions, this CL will be closed, and we will move AnimationHost
ownership back to LayerTreeHost.


Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[xingliu, 2016-08-15 16:18:01]

Friendly ping.

[enne (OOO), 2016-08-15 17:11:34]

lgtm

[Khushal, 2016-08-15 17:46:39]

https://codereview.chromium.org/2216203002/diff/60001/cc/debug/invalidation_b...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/debug/invalidation_b...
cc/debug/invalidation_benchmark.cc:66: void
InvalidationBenchmark::DidUpdateLayers(LayerTreeHost* host) {
How about updating the MicroBenchmark interface to take a LayerTree itself here?

https://codereview.chromium.org/2216203002/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:136:
layer_tree_->animation_host()->UnregisterElement(inputs_.element_id,
May be you can just add a RegisterElement method to the LayerTree, which can add
it to its element app and also register it with the AnimationHost, and the same
with unregistration.

https://codereview.chromium.org/2216203002/diff/60001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host.cc:95:
GetLayerTree()->animation_host()->PushPropertiesTo(
You can make the |layer_tree_| a protected member and directly access it.

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:707: void
FindLayersThatNeedUpdates(LayerTreeHost* layer_tree_host,
Pass the LayerTree here directly?

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree.h
File cc/trees/layer_tree.h (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree.h#n...
cc/trees/layer_tree.h:58: // MutatorHostClient implementation.
Can this be implemented privately instead?

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:683: void
LayerTreeHost::SetNeedsDisplayOnAllLayers() {
This method can move to the LayerTree as well.

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:967: void LayerTreeHost::SetElementIdsForTesting() {
Can move this to the LayerTree.

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:33: class
LayerTreeHostAnimationTest : public LayerTreeTest {
You can add a layer_tree() accessor to the LayerTreeTest, and use that instead
of the host->GetLayerTree thing in all the tests.

[xingliu, 2016-08-15 21:29:13]

Thanks for the review, just upload a patch to adjust the code based on review.

https://codereview.chromium.org/2216203002/diff/60001/cc/debug/invalidation_b...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/debug/invalidation_b...
cc/debug/invalidation_benchmark.cc:66: void
InvalidationBenchmark::DidUpdateLayers(LayerTreeHost* host) {
On 2016/08/15 17:46:39, Khushal wrote:
> How about updating the MicroBenchmark interface to take a LayerTree itself
here?

I think it makes sense, and there isn't too much work for this refactoring, but
maybe we can do it in a separate CL? since it's like modify an interface in
cc/debug.

https://codereview.chromium.org/2216203002/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:136:
layer_tree_->animation_host()->UnregisterElement(inputs_.element_id,
On 2016/08/15 17:46:39, Khushal wrote:
> May be you can just add a RegisterElement method to the LayerTree, which can
add
> it to its element app and also register it with the AnimationHost, and the
same
> with unregistration.

Done.

https://codereview.chromium.org/2216203002/diff/60001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host.cc:95:
GetLayerTree()->animation_host()->PushPropertiesTo(
On 2016/08/15 17:46:39, Khushal wrote:
> You can make the |layer_tree_| a protected member and directly access it.

Done.

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:707: void
FindLayersThatNeedUpdates(LayerTreeHost* layer_tree_host,
On 2016/08/15 17:46:39, Khushal wrote:
> Pass the LayerTree here directly?

Make sense, changed.

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree.h
File cc/trees/layer_tree.h (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree.h#n...
cc/trees/layer_tree.h:58: // MutatorHostClient implementation.
On 2016/08/15 17:46:39, Khushal wrote:
> Can this be implemented privately instead?
The interface functions are mainly called from cc/animation/. So maybe we can't.

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:683: void
LayerTreeHost::SetNeedsDisplayOnAllLayers() {
On 2016/08/15 17:46:39, Khushal wrote:
> This method can move to the LayerTree as well.

Done.

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:967: void LayerTreeHost::SetElementIdsForTesting() {
On 2016/08/15 17:46:39, Khushal wrote:
> Can move this to the LayerTree.

Done.

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:33: class
LayerTreeHostAnimationTest : public LayerTreeTest {
On 2016/08/15 17:46:39, Khushal wrote:
> You can add a layer_tree() accessor to the LayerTreeTest, and use that instead
> of the host->GetLayerTree thing in all the tests.

Done.

[xingliu, 2016-08-15 21:30:08]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 21:30:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 23:03:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 23:03:02]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-08-16 00:18:31]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 00:18:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[loyso (OOO), 2016-08-16 00:21:17]

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:364:
layer_tree_->animation_host()->SetSupportsScrollAnimations(
Notice that LayerTreeHost still deals with layer_tree_->animation_host() calls
so the logic is scattered. But I can't suggest anything (apart from creating
wrapper functions in LayerTree to hide animation_host() OR passing bool to
LayerTree ctor for this case.

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:558:
layer_tree_->animation_host()->PushPropertiesTo(
and here.

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:646:
layer_tree_->animation_host()->CreateImplInstance(
Same here. This is the logic how to pass AnimationHost impl to LTHI.

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:743:
layer_tree_->animation_host()->SetAnimationEvents(std::move(events));
.. and here.

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1115: AnimationHost* animation_host =
layer_tree_->animation_host();
Same here. Move AnimateLayers method to LayerTree?

[loyso (OOO), 2016-08-16 00:28:05]

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:558:
layer_tree_->animation_host()->PushPropertiesTo(
On 2016/08/16 00:21:17, loyso wrote:
> and here.
And if you add LayerTree::PushProperties (as in other entities) then there is q
question, what would the argument would be. Right now it must be LTHI (which
holds AnimationHost impl instance).
That's what I meant by symmetry.

[commit-bot: I haz the power, 2016-08-16 02:19:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 02:19:32]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-08-16 02:25:25]

Discussion on animation host related.

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:364:
layer_tree_->animation_host()->SetSupportsScrollAnimations(
On 2016/08/16 00:21:17, loyso wrote:
> Notice that LayerTreeHost still deals with layer_tree_->animation_host() calls
> so the logic is scattered. But I can't suggest anything (apart from creating
> wrapper functions in LayerTree to hide animation_host() OR passing bool to
> LayerTree ctor for this case.

I agree in general this breaks the symmetric, but since we don't want to copy
code between different implementations of LTH (base class are also not preferred
according to some previous discussion), it makes some sense to put animation
host inside LayerTree.

Also the impl side doesn't need to abstract out interface to control commit
behavior, so the general structure on the big pictures of two sides has
diverged.

@Khushal, please feel free to provide thoughts on this.

https://codereview.chromium.org/2216203002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:558:
layer_tree_->animation_host()->PushPropertiesTo(
On 2016/08/16 00:28:05, loyso wrote:
> On 2016/08/16 00:21:17, loyso wrote:
> > and here.
> And if you add LayerTree::PushProperties (as in other entities) then there is
q
> question, what would the argument would be. Right now it must be LTHI (which
> holds AnimationHost impl instance).
> That's what I meant by symmetry.

For PushPropertiesTo, there is another refactoring CL which will create wrapper
in LayerTree.(please refer to https://codereview.chromium.org/2183403002/ ).

[Khushal, 2016-08-16 05:37:18]

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree.h
File cc/trees/layer_tree.h (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree.h#n...
cc/trees/layer_tree.h:58: // MutatorHostClient implementation.
On 2016/08/15 21:29:12, xingliu wrote:
> On 2016/08/15 17:46:39, Khushal wrote:
> > Can this be implemented privately instead?
> The interface functions are mainly called from cc/animation/. So maybe we
can't.

You can still make the implementation here private. AnimationHost accesses it
through the MutatorHostClient interface.

[xingliu, 2016-08-16 16:45:15]

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree.h
File cc/trees/layer_tree.h (right):

https://codereview.chromium.org/2216203002/diff/60001/cc/trees/layer_tree.h#n...
cc/trees/layer_tree.h:58: // MutatorHostClient implementation.
On 2016/08/16 05:37:17, Khushal wrote:
> On 2016/08/15 21:29:12, xingliu wrote:
> > On 2016/08/15 17:46:39, Khushal wrote:
> > > Can this be implemented privately instead?
> > The interface functions are mainly called from cc/animation/. So maybe we
> can't.
> 
> You can still make the implementation here private. AnimationHost accesses it
> through the MutatorHostClient interface.

Oh, this is cool, fixed.

[xingliu, 2016-08-16 17:23:58]

@vollick, Friendly ping.

Please help review:

third_party/WebKit/Source/platform/testing/WebLayerTreeViewImplForTesting.cpp
 
ui/compositor/compositor.cc

[Ian Vollick, 2016-08-16 17:27:31]

On 2016/08/16 at 17:23:58, xingliu wrote:
> @vollick, Friendly ping.
> 
> Please help review:
> 
> third_party/WebKit/Source/platform/testing/WebLayerTreeViewImplForTesting.cpp
>  
> ui/compositor/compositor.cc

ui/compositor and platform/testing lgtm.

[xingliu, 2016-08-18 00:43:46]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 00:44:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 02:28:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 02:28:07]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-08-18 02:28:32]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2016-08-18 02:28:33]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
vollick@chromium.org
Link to the patchset: https://codereview.chromium.org/2216203002/#ps140001
(title: "Rebase on another LTH refactor CL.")

[commit-bot: I haz the power, 2016-08-18 02:29:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 03:53:59]

Description was changed from

==========
Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-18 03:54:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-18 03:56:06]

Description was changed from

==========
Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.



BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Refactor MutatorHostClient from LayerTreeHost to LayerTree.

This is another CL for refactoring part of LayerTreeHost to LayerTree.

Refactored MutatorHostClient, and also follow Khushal's other CL, to
cache raw pointer of LayerTreeHost in LayerTree, and use unique_ptr to
wrap LayerTree in LayerTreeHost.

BUG=628683
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/95d9e6b63179642af8391906ccbe586bd3587b56
Cr-Commit-Position: refs/heads/master@{#412727}
==========

[commit-bot: I haz the power, 2016-08-18 03:56:08]

Patchset 8 (id:??) landed as
https://crrev.com/95d9e6b63179642af8391906ccbe586bd3587b56
Cr-Commit-Position: refs/heads/master@{#412727}