cc/blimp: Set up the framework for state serialization.

cc/blimp: Set up the framework for state serialization.

1) Split the inputs and rest of the fields in the protos so we only
serialize the inputs for the new serialization code.
2) Add a CompositorStateDeserializer to deserialize compositor protos.

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

Committed: https://crrev.com/0c4d7f1fac75e0d294b10f7ba465d99469b81d55
Cr-Commit-Position: refs/heads/master@{#423001}

[Khushal, 2016-09-29 06:55:04]

Description was changed from

==========
cc/blimp: Set up the framework for state serialization.

1) Split the inputs and rest of the fields in the protos so we only
serialize the inputs for the new serialization code.
2) Add a CompositorStateDeserializer to deserialize compositor protos.

BUG=648442
==========

to

==========
cc/blimp: Set up the framework for state serialization.

1) Split the inputs and rest of the fields in the protos so we only
serialize the inputs for the new serialization code.
2) Add a CompositorStateDeserializer to deserialize compositor protos.

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

[Khushal, 2016-09-29 17:21:17]

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

[Khushal, 2016-09-29 17:21:17]

Hey Ali, I still have to add tests for this but wanted to get a review from you
about the general setup. A few things that I was unsure about:

1) Where to put the deserialization code? It would be nicer if it lived next to
the serialization code but since it shouldn't have access to Layer internals I
did not want to put it into this class. I'll add a comment block above the
serialization code to point to the correct file once the transition is complete.

2) I thought about having client layer ids match the engine layer ids but the
blimp UI code can add layers to the tree after receiving an update from blink
which could lead to collisions. Its fine to have this mapping, will just make
tests a little iffy because they will add Layers to the LayerTree on the
"engine" and then expect the corresponding LayerImpls to have matching ids,
which will not be true because we'll have this (de-)serialization step in
between having client Layers. Let me know if you have any ideas about this.

For this change, I'm also going to update the TreeSynchronizer tests to run with
this (de-)serialization in between.

Thanks!

[ajuma, 2016-09-29 20:55:17]

On 2016/09/29 17:21:17, Khushal wrote:
> Hey Ali, I still have to add tests for this but wanted to get a review from
you
> about the general setup. A few things that I was unsure about:
> 
> 1) Where to put the deserialization code? It would be nicer if it lived next
to
> the serialization code but since it shouldn't have access to Layer internals I
> did not want to put it into this class. I'll add a comment block above the
> serialization code to point to the correct file once the transition is
complete.

It does seem a bit confusing to have the serialization and deserialization logic
in separate places. Given that we're able to deserialize using only
publicly-exposed methods of Layer, what are we missing in order to serialize
using only the public API of Layer?

> 2) I thought about having client layer ids match the engine layer ids but the
> blimp UI code can add layers to the tree after receiving an update from blink
> which could lead to collisions. Its fine to have this mapping, will just make
> tests a little iffy because they will add Layers to the LayerTree on the
> "engine" and then expect the corresponding LayerImpls to have matching ids,
> which will not be true because we'll have this (de-)serialization step in
> between having client Layers. Let me know if you have any ideas about this.

Could we change the way the ids are selected so that, say, blimp UI code always
uses ids with the high-order bit set to 1? Having a blimp-only mapping doesn't
sound bad on its own, but many tests  rely heavily on LayerImpls having the same
id as the corresponding Layer.

> For this change, I'm also going to update the TreeSynchronizer tests to run
with
> this (de-)serialization in between.

Any particular reason for putting this in TreeSynchronizer tests instead of
elsewhere? TreeSynchronizer seems a bit unusual since it's normally used after
we've built property trees (so by then, many of the "interesting" parts of
Layers have been moved to property tree nodes), but this serialization step is
supposed to happen before we build property trees.

[ajuma, 2016-09-29 20:55:34]

https://codereview.chromium.org/2375363002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2375363002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:426:
layer_tree_->ToProtobuf(layer_tree_host_proto->mutable_layer_tree(), true);
Instead of passing |true| here, please create a variable with a good name
(perhaps "inputs_only"), set that variable's value to true, and pass that
variable as the argument. That'll make this more readable. An alternative would
be to change the type of the second argument to an enum. (And same comment for
the other callsites of methods with boolean arguments added in this CL.)

[Khushal, 2016-09-29 21:43:50]

On 2016/09/29 20:55:17, ajuma wrote:
> On 2016/09/29 17:21:17, Khushal wrote:
> > Hey Ali, I still have to add tests for this but wanted to get a review from
> you
> > about the general setup. A few things that I was unsure about:
> > 
> > 1) Where to put the deserialization code? It would be nicer if it lived next
> to
> > the serialization code but since it shouldn't have access to Layer internals
I
> > did not want to put it into this class. I'll add a comment block above the
> > serialization code to point to the correct file once the transition is
> complete.
> 
> It does seem a bit confusing to have the serialization and deserialization
logic
> in separate places. Given that we're able to deserialize using only
> publicly-exposed methods of Layer, what are we missing in order to serialize
> using only the public API of Layer?

The code is in flux right now, the old serialization code needs to serialize
everything because the client consumes the result of a main frame update run on
the engine and gives it to the impl thread directly, so it needs to access a lot
more Layer internals. For the new code which is serializing only Inputs, each
field set has a getter already so we should be able to serialize using public
methods too. Though something like setting the Layer type which is overridden by
each Layer would still have to be on the Layer. I can take a look at doing that
once the old code path is killed, will be clearer then.
Btw, do you think there is any advantage in having the serialization code live
within the class, w.r.t easier for people to remember to update when something
new is added?

> 
> > 2) I thought about having client layer ids match the engine layer ids but
the
> > blimp UI code can add layers to the tree after receiving an update from
blink
> > which could lead to collisions. Its fine to have this mapping, will just
make
> > tests a little iffy because they will add Layers to the LayerTree on the
> > "engine" and then expect the corresponding LayerImpls to have matching ids,
> > which will not be true because we'll have this (de-)serialization step in
> > between having client Layers. Let me know if you have any ideas about this.
> 
> Could we change the way the ids are selected so that, say, blimp UI code
always
> uses ids with the high-order bit set to 1? Having a blimp-only mapping doesn't
> sound bad on its own, but many tests  rely heavily on LayerImpls having the
same
> id as the corresponding Layer.

Sure, that sounds better and will be much easier too. I'll keep the ids the same
in this change then and send a subsequent patch to you later when we get closer
to adding things for the blimp UI.

> 
> > For this change, I'm also going to update the TreeSynchronizer tests to run
> with
> > this (de-)serialization in between.
> 
> Any particular reason for putting this in TreeSynchronizer tests instead of
> elsewhere? TreeSynchronizer seems a bit unusual since it's normally used after
> we've built property trees (so by then, many of the "interesting" parts of
> Layers have been moved to property tree nodes), but this serialization step is
> supposed to happen before we build property trees.

The TreeSynchronizerTests would build a LayerTree, assume that the client does
not want to change anything so jump directly to what we do post UpdateLayers,
i.e., build PropertyTrees, then commit (synchronize the trees to create the
pending tree) and inspect that tree to make sure it was correct. I was thinking
about doing something like, having the tests update the LayerTree on the
"engine", serialize to a proto and deserialize to a LayerTree on the client, and
then perform the tree sync and let the test continue. I want to re-use and run
as much of the existing tests to cover this additional step between commits to
make sure all the existing functionality works and so that if something new is
added, it naturally gets covered in any new tests added with it.

[Khushal, 2016-09-29 21:44:51]

On 2016/09/29 20:55:34, ajuma wrote:
>
https://codereview.chromium.org/2375363002/diff/1/cc/blimp/layer_tree_host_re...
> File cc/blimp/layer_tree_host_remote.cc (right):
> 
>
https://codereview.chromium.org/2375363002/diff/1/cc/blimp/layer_tree_host_re...
> cc/blimp/layer_tree_host_remote.cc:426:
> layer_tree_->ToProtobuf(layer_tree_host_proto->mutable_layer_tree(), true);
> Instead of passing |true| here, please create a variable with a good name
> (perhaps "inputs_only"), set that variable's value to true, and pass that
> variable as the argument. That'll make this more readable. An alternative
would
> be to change the type of the second argument to an enum. (And same comment for
> the other callsites of methods with boolean arguments added in this CL.)

Using a temp variable sounds good. Its mostly staging to move over the code to
this new point.

[ajuma, 2016-09-29 22:23:43]

On 2016/09/29 21:43:50, Khushal wrote:
> On 2016/09/29 20:55:17, ajuma wrote:
> > On 2016/09/29 17:21:17, Khushal wrote:
> > > Hey Ali, I still have to add tests for this but wanted to get a review
from
> > you
> > > about the general setup. A few things that I was unsure about:
> > > 
> > > 1) Where to put the deserialization code? It would be nicer if it lived
next
> > to
> > > the serialization code but since it shouldn't have access to Layer
internals
> I
> > > did not want to put it into this class. I'll add a comment block above the
> > > serialization code to point to the correct file once the transition is
> > complete.
> > 
> > It does seem a bit confusing to have the serialization and deserialization
> logic
> > in separate places. Given that we're able to deserialize using only
> > publicly-exposed methods of Layer, what are we missing in order to serialize
> > using only the public API of Layer?
> 
> The code is in flux right now, the old serialization code needs to serialize
> everything because the client consumes the result of a main frame update run
on
> the engine and gives it to the impl thread directly, so it needs to access a
lot
> more Layer internals. For the new code which is serializing only Inputs, each
> field set has a getter already so we should be able to serialize using public
> methods too. Though something like setting the Layer type which is overridden
by
> each Layer would still have to be on the Layer. I can take a look at doing
that
> once the old code path is killed, will be clearer then.
> Btw, do you think there is any advantage in having the serialization code live
> within the class, w.r.t easier for people to remember to update when something
> new is added?

I'm not sure how much it helps people remember, since in large files (like
layer.{h, cc}) you might not notice the serialization code even if it happens to
be in that file. Maybe it's more obvious in smaller files. But having tests fail
is the best way to help people remember :)

> 
> > 
> > > 2) I thought about having client layer ids match the engine layer ids but
> the
> > > blimp UI code can add layers to the tree after receiving an update from
> blink
> > > which could lead to collisions. Its fine to have this mapping, will just
> make
> > > tests a little iffy because they will add Layers to the LayerTree on the
> > > "engine" and then expect the corresponding LayerImpls to have matching
ids,
> > > which will not be true because we'll have this (de-)serialization step in
> > > between having client Layers. Let me know if you have any ideas about
this.
> > 
> > Could we change the way the ids are selected so that, say, blimp UI code
> always
> > uses ids with the high-order bit set to 1? Having a blimp-only mapping
doesn't
> > sound bad on its own, but many tests  rely heavily on LayerImpls having the
> same
> > id as the corresponding Layer.
> 
> Sure, that sounds better and will be much easier too. I'll keep the ids the
same
> in this change then and send a subsequent patch to you later when we get
closer
> to adding things for the blimp UI.
> 
> > 
> > > For this change, I'm also going to update the TreeSynchronizer tests to
run
> > with
> > > this (de-)serialization in between.
> > 
> > Any particular reason for putting this in TreeSynchronizer tests instead of
> > elsewhere? TreeSynchronizer seems a bit unusual since it's normally used
after
> > we've built property trees (so by then, many of the "interesting" parts of
> > Layers have been moved to property tree nodes), but this serialization step
is
> > supposed to happen before we build property trees.
> 
> The TreeSynchronizerTests would build a LayerTree, assume that the client does
> not want to change anything so jump directly to what we do post UpdateLayers,
> i.e., build PropertyTrees, then commit (synchronize the trees to create the
> pending tree) and inspect that tree to make sure it was correct. I was
thinking
> about doing something like, having the tests update the LayerTree on the
> "engine", serialize to a proto and deserialize to a LayerTree on the client,
and
> then perform the tree sync and let the test continue. I want to re-use and run
> as much of the existing tests to cover this additional step between commits to
> make sure all the existing functionality works and so that if something new is
> added, it naturally gets covered in any new tests added with it.

Khushal pointed out to me offline that he plans to add
serialization/deserialization to other tests (including LayerTreeTests) too, so
TreeSynchronizerTests are just a starting point. That plan sgtm.

[danakj, 2016-09-29 22:28:58]

On Thu, Sep 29, 2016 at 3:23 PM, <ajuma@chromium.org> wrote:

> On 2016/09/29 21:43:50, Khushal wrote:
> > On 2016/09/29 20:55:17, ajuma wrote:
> > > On 2016/09/29 17:21:17, Khushal wrote:
> > > > Hey Ali, I still have to add tests for this but wanted to get a
> review
> from
> > > you
> > > > about the general setup. A few things that I was unsure about:
> > > >
> > > > 1) Where to put the deserialization code? It would be nicer if it
> lived
> next
> > > to
> > > > the serialization code but since it shouldn't have access to Layer
> internals
> > I
> > > > did not want to put it into this class. I'll add a comment block
> above the
> > > > serialization code to point to the correct file once the transition
> is
> > > complete.
> > >
> > > It does seem a bit confusing to have the serialization and
> deserialization
> > logic
> > > in separate places. Given that we're able to deserialize using only
> > > publicly-exposed methods of Layer, what are we missing in order to
> serialize
> > > using only the public API of Layer?
> >
> > The code is in flux right now, the old serialization code needs to
> serialize
> > everything because the client consumes the result of a main frame update
> run
> on
> > the engine and gives it to the impl thread directly, so it needs to
> access a
> lot
> > more Layer internals. For the new code which is serializing only Inputs,
> each
> > field set has a getter already so we should be able to serialize using
> public
> > methods too. Though something like setting the Layer type which is
> overridden
> by
> > each Layer would still have to be on the Layer. I can take a look at
> doing
> that
> > once the old code path is killed, will be clearer then.
> > Btw, do you think there is any advantage in having the serialization
> code live
> > within the class, w.r.t easier for people to remember to update when
> something
> > new is added?
>
> I'm not sure how much it helps people remember, since in large files (like
> layer.{h, cc}) you might not notice the serialization code even if it
> happens to
> be in that file. Maybe it's more obvious in smaller files. But having
> tests fail
> is the best way to help people remember :)
>
>
> >
> > >
> > > > 2) I thought about having client layer ids match the engine layer
> ids but
> > the
> > > > blimp UI code can add layers to the tree after receiving an update
> from
> > blink
> > > > which could lead to collisions. Its fine to have this mapping, will
> just
> > make
> > > > tests a little iffy because they will add Layers to the LayerTree on
> the
> > > > "engine" and then expect the corresponding LayerImpls to have
> matching
> ids,
> > > > which will not be true because we'll have this (de-)serialization
> step in
> > > > between having client Layers. Let me know if you have any ideas about
> this.
> > >
> > > Could we change the way the ids are selected so that, say, blimp UI
> code
> > always
> > > uses ids with the high-order bit set to 1? Having a blimp-only mapping
> doesn't
> > > sound bad on its own, but many tests rely heavily on LayerImpls having
> the
> > same
> > > id as the corresponding Layer.
> >
> > Sure, that sounds better and will be much easier too. I'll keep the ids
> the
> same
> > in this change then and send a subsequent patch to you later when we get
> closer
> > to adding things for the blimp UI.
> >
> > >
> > > > For this change, I'm also going to update the TreeSynchronizer tests
> to
> run
> > > with
> > > > this (de-)serialization in between.
> > >
> > > Any particular reason for putting this in TreeSynchronizer tests
> instead of
> > > elsewhere? TreeSynchronizer seems a bit unusual since it's normally
> used
> after
> > > we've built property trees (so by then, many of the "interesting"
> parts of
> > > Layers have been moved to property tree nodes), but this serialization
> step
> is
> > > supposed to happen before we build property trees.
> >
> > The TreeSynchronizerTests would build a LayerTree, assume that the
> client does
> > not want to change anything so jump directly to what we do post
> UpdateLayers,
> > i.e., build PropertyTrees, then commit (synchronize the trees to create
> the
> > pending tree) and inspect that tree to make sure it was correct. I was
> thinking
> > about doing something like, having the tests update the LayerTree on the
> > "engine", serialize to a proto and deserialize to a LayerTree on the
> client,
> and
> > then perform the tree sync and let the test continue. I want to re-use
> and run
> > as much of the existing tests to cover this additional step between
> commits to
> > make sure all the existing functionality works and so that if something
> new is
> > added, it naturally gets covered in any new tests added with it.
>
> Khushal pointed out to me offline that he plans to add
> serialization/deserialization to other tests (including LayerTreeTests)
> too, so
> TreeSynchronizerTests are just a starting point. That plan sgtm.
>

I'd like to be sold on this idea. To me layer tree tests are testing the
behaviour of cc, esp LTHI, TileManager, the proxies and such, all together.
They are not testing embedders of cc.

Ya we have a new implementation of LayerTreeHost, but it's actually a
replacement for cc not a part of it, in the blimp engine. IMO we should
have tests of the blink side up to a Test/Mock/Fake LayerTreeHost, tests of
the implementation of the LTHRemote that are pretty independent, or that
test its interactions with the cc embedder on the client (the stuff before
cc::LTHInProcess) and call it there more or less.

But I'm open to hearing more thoughts.


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

-- 
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-09-30 00:12:31]

On 2016/09/29 22:28:58, danakj wrote:
> On Thu, Sep 29, 2016 at 3:23 PM, <mailto:ajuma@chromium.org> wrote:
> 
> > On 2016/09/29 21:43:50, Khushal wrote:
> > > On 2016/09/29 20:55:17, ajuma wrote:
> > > > On 2016/09/29 17:21:17, Khushal wrote:
> > > > > Hey Ali, I still have to add tests for this but wanted to get a
> > review
> > from
> > > > you
> > > > > about the general setup. A few things that I was unsure about:
> > > > >
> > > > > 1) Where to put the deserialization code? It would be nicer if it
> > lived
> > next
> > > > to
> > > > > the serialization code but since it shouldn't have access to Layer
> > internals
> > > I
> > > > > did not want to put it into this class. I'll add a comment block
> > above the
> > > > > serialization code to point to the correct file once the transition
> > is
> > > > complete.
> > > >
> > > > It does seem a bit confusing to have the serialization and
> > deserialization
> > > logic
> > > > in separate places. Given that we're able to deserialize using only
> > > > publicly-exposed methods of Layer, what are we missing in order to
> > serialize
> > > > using only the public API of Layer?
> > >
> > > The code is in flux right now, the old serialization code needs to
> > serialize
> > > everything because the client consumes the result of a main frame update
> > run
> > on
> > > the engine and gives it to the impl thread directly, so it needs to
> > access a
> > lot
> > > more Layer internals. For the new code which is serializing only Inputs,
> > each
> > > field set has a getter already so we should be able to serialize using
> > public
> > > methods too. Though something like setting the Layer type which is
> > overridden
> > by
> > > each Layer would still have to be on the Layer. I can take a look at
> > doing
> > that
> > > once the old code path is killed, will be clearer then.
> > > Btw, do you think there is any advantage in having the serialization
> > code live
> > > within the class, w.r.t easier for people to remember to update when
> > something
> > > new is added?
> >
> > I'm not sure how much it helps people remember, since in large files (like
> > layer.{h, cc}) you might not notice the serialization code even if it
> > happens to
> > be in that file. Maybe it's more obvious in smaller files. But having
> > tests fail
> > is the best way to help people remember :)
> >
> >
> > >
> > > >
> > > > > 2) I thought about having client layer ids match the engine layer
> > ids but
> > > the
> > > > > blimp UI code can add layers to the tree after receiving an update
> > from
> > > blink
> > > > > which could lead to collisions. Its fine to have this mapping, will
> > just
> > > make
> > > > > tests a little iffy because they will add Layers to the LayerTree on
> > the
> > > > > "engine" and then expect the corresponding LayerImpls to have
> > matching
> > ids,
> > > > > which will not be true because we'll have this (de-)serialization
> > step in
> > > > > between having client Layers. Let me know if you have any ideas about
> > this.
> > > >
> > > > Could we change the way the ids are selected so that, say, blimp UI
> > code
> > > always
> > > > uses ids with the high-order bit set to 1? Having a blimp-only mapping
> > doesn't
> > > > sound bad on its own, but many tests rely heavily on LayerImpls having
> > the
> > > same
> > > > id as the corresponding Layer.
> > >
> > > Sure, that sounds better and will be much easier too. I'll keep the ids
> > the
> > same
> > > in this change then and send a subsequent patch to you later when we get
> > closer
> > > to adding things for the blimp UI.
> > >
> > > >
> > > > > For this change, I'm also going to update the TreeSynchronizer tests
> > to
> > run
> > > > with
> > > > > this (de-)serialization in between.
> > > >
> > > > Any particular reason for putting this in TreeSynchronizer tests
> > instead of
> > > > elsewhere? TreeSynchronizer seems a bit unusual since it's normally
> > used
> > after
> > > > we've built property trees (so by then, many of the "interesting"
> > parts of
> > > > Layers have been moved to property tree nodes), but this serialization
> > step
> > is
> > > > supposed to happen before we build property trees.
> > >
> > > The TreeSynchronizerTests would build a LayerTree, assume that the
> > client does
> > > not want to change anything so jump directly to what we do post
> > UpdateLayers,
> > > i.e., build PropertyTrees, then commit (synchronize the trees to create
> > the
> > > pending tree) and inspect that tree to make sure it was correct. I was
> > thinking
> > > about doing something like, having the tests update the LayerTree on the
> > > "engine", serialize to a proto and deserialize to a LayerTree on the
> > client,
> > and
> > > then perform the tree sync and let the test continue. I want to re-use
> > and run
> > > as much of the existing tests to cover this additional step between
> > commits to
> > > make sure all the existing functionality works and so that if something
> > new is
> > > added, it naturally gets covered in any new tests added with it.
> >
> > Khushal pointed out to me offline that he plans to add
> > serialization/deserialization to other tests (including LayerTreeTests)
> > too, so
> > TreeSynchronizerTests are just a starting point. That plan sgtm.
> >
> 
> I'd like to be sold on this idea. To me layer tree tests are testing the
> behaviour of cc, esp LTHI, TileManager, the proxies and such, all together.
> They are not testing embedders of cc.
> 
> Ya we have a new implementation of LayerTreeHost, but it's actually a
> replacement for cc not a part of it, in the blimp engine. IMO we should
> have tests of the blink side up to a Test/Mock/Fake LayerTreeHost, tests of
> the implementation of the LTHRemote that are pretty independent, or that
> test its interactions with the cc embedder on the client (the stuff before
> cc::LTHInProcess) and call it there more or less.
> 
> But I'm open to hearing more thoughts.

I would say one way of looking at it is that LayerTreeHost and main thread code
is already almost just a staging ground where the embedder gets to store data
and the tree sync and push properties is the internal details of that data being
moved to the impl side where its actually used, though there is some additional
code for property trees or draw property calculations on the main thread side.
But the embedder doesn't need to be aware of these details so we already have
test cases where we mutate state on the main thread and verify the complete data
flow up till its use on the impl thread.
(https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_unittest.cc?l=6505
for example). Now with LayerTreeHostRemote, there is an additional layer of
indirection, the embedder still talks to the LayerTreeHost API, but you first
serialize data to be transported across a process boundary and then use the
InProcess API on the client to transport across a thread boundary.

We will certainly have the blink tests and Layout tests verify this as well, but
cc already has a lot of test cases to verify the data flow across threads, and
having additional coverage with smaller scoped unit-tests would surely do more
good than harm. :)

> 
> 
> >
> >
> >
> > https://codereview.chromium.org/2375363002/
> >
> 
> -- 
> 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.

[Khushal, 2016-09-30 20:48:03]

On 2016/09/30 00:12:31, Khushal wrote:
> On 2016/09/29 22:28:58, danakj wrote:
> > On Thu, Sep 29, 2016 at 3:23 PM, <mailto:ajuma@chromium.org> wrote:
> > 
> > > On 2016/09/29 21:43:50, Khushal wrote:
> > > > On 2016/09/29 20:55:17, ajuma wrote:
> > > > > On 2016/09/29 17:21:17, Khushal wrote:
> > > > > > Hey Ali, I still have to add tests for this but wanted to get a
> > > review
> > > from
> > > > > you
> > > > > > about the general setup. A few things that I was unsure about:
> > > > > >
> > > > > > 1) Where to put the deserialization code? It would be nicer if it
> > > lived
> > > next
> > > > > to
> > > > > > the serialization code but since it shouldn't have access to Layer
> > > internals
> > > > I
> > > > > > did not want to put it into this class. I'll add a comment block
> > > above the
> > > > > > serialization code to point to the correct file once the transition
> > > is
> > > > > complete.
> > > > >
> > > > > It does seem a bit confusing to have the serialization and
> > > deserialization
> > > > logic
> > > > > in separate places. Given that we're able to deserialize using only
> > > > > publicly-exposed methods of Layer, what are we missing in order to
> > > serialize
> > > > > using only the public API of Layer?
> > > >
> > > > The code is in flux right now, the old serialization code needs to
> > > serialize
> > > > everything because the client consumes the result of a main frame update
> > > run
> > > on
> > > > the engine and gives it to the impl thread directly, so it needs to
> > > access a
> > > lot
> > > > more Layer internals. For the new code which is serializing only Inputs,
> > > each
> > > > field set has a getter already so we should be able to serialize using
> > > public
> > > > methods too. Though something like setting the Layer type which is
> > > overridden
> > > by
> > > > each Layer would still have to be on the Layer. I can take a look at
> > > doing
> > > that
> > > > once the old code path is killed, will be clearer then.
> > > > Btw, do you think there is any advantage in having the serialization
> > > code live
> > > > within the class, w.r.t easier for people to remember to update when
> > > something
> > > > new is added?
> > >
> > > I'm not sure how much it helps people remember, since in large files (like
> > > layer.{h, cc}) you might not notice the serialization code even if it
> > > happens to
> > > be in that file. Maybe it's more obvious in smaller files. But having
> > > tests fail
> > > is the best way to help people remember :)
> > >
> > >
> > > >
> > > > >
> > > > > > 2) I thought about having client layer ids match the engine layer
> > > ids but
> > > > the
> > > > > > blimp UI code can add layers to the tree after receiving an update
> > > from
> > > > blink
> > > > > > which could lead to collisions. Its fine to have this mapping, will
> > > just
> > > > make
> > > > > > tests a little iffy because they will add Layers to the LayerTree on
> > > the
> > > > > > "engine" and then expect the corresponding LayerImpls to have
> > > matching
> > > ids,
> > > > > > which will not be true because we'll have this (de-)serialization
> > > step in
> > > > > > between having client Layers. Let me know if you have any ideas
about
> > > this.
> > > > >
> > > > > Could we change the way the ids are selected so that, say, blimp UI
> > > code
> > > > always
> > > > > uses ids with the high-order bit set to 1? Having a blimp-only mapping
> > > doesn't
> > > > > sound bad on its own, but many tests rely heavily on LayerImpls having
> > > the
> > > > same
> > > > > id as the corresponding Layer.
> > > >
> > > > Sure, that sounds better and will be much easier too. I'll keep the ids
> > > the
> > > same
> > > > in this change then and send a subsequent patch to you later when we get
> > > closer
> > > > to adding things for the blimp UI.
> > > >
> > > > >
> > > > > > For this change, I'm also going to update the TreeSynchronizer tests
> > > to
> > > run
> > > > > with
> > > > > > this (de-)serialization in between.
> > > > >
> > > > > Any particular reason for putting this in TreeSynchronizer tests
> > > instead of
> > > > > elsewhere? TreeSynchronizer seems a bit unusual since it's normally
> > > used
> > > after
> > > > > we've built property trees (so by then, many of the "interesting"
> > > parts of
> > > > > Layers have been moved to property tree nodes), but this serialization
> > > step
> > > is
> > > > > supposed to happen before we build property trees.
> > > >
> > > > The TreeSynchronizerTests would build a LayerTree, assume that the
> > > client does
> > > > not want to change anything so jump directly to what we do post
> > > UpdateLayers,
> > > > i.e., build PropertyTrees, then commit (synchronize the trees to create
> > > the
> > > > pending tree) and inspect that tree to make sure it was correct. I was
> > > thinking
> > > > about doing something like, having the tests update the LayerTree on the
> > > > "engine", serialize to a proto and deserialize to a LayerTree on the
> > > client,
> > > and
> > > > then perform the tree sync and let the test continue. I want to re-use
> > > and run
> > > > as much of the existing tests to cover this additional step between
> > > commits to
> > > > make sure all the existing functionality works and so that if something
> > > new is
> > > > added, it naturally gets covered in any new tests added with it.
> > >
> > > Khushal pointed out to me offline that he plans to add
> > > serialization/deserialization to other tests (including LayerTreeTests)
> > > too, so
> > > TreeSynchronizerTests are just a starting point. That plan sgtm.
> > >
> > 
> > I'd like to be sold on this idea. To me layer tree tests are testing the
> > behaviour of cc, esp LTHI, TileManager, the proxies and such, all together.
> > They are not testing embedders of cc.
> > 
> > Ya we have a new implementation of LayerTreeHost, but it's actually a
> > replacement for cc not a part of it, in the blimp engine. IMO we should
> > have tests of the blink side up to a Test/Mock/Fake LayerTreeHost, tests of
> > the implementation of the LTHRemote that are pretty independent, or that
> > test its interactions with the cc embedder on the client (the stuff before
> > cc::LTHInProcess) and call it there more or less.
> > 
> > But I'm open to hearing more thoughts.
> 
> I would say one way of looking at it is that LayerTreeHost and main thread
code
> is already almost just a staging ground where the embedder gets to store data
> and the tree sync and push properties is the internal details of that data
being
> moved to the impl side where its actually used, though there is some
additional
> code for property trees or draw property calculations on the main thread side.
> But the embedder doesn't need to be aware of these details so we already have
> test cases where we mutate state on the main thread and verify the complete
data
> flow up till its use on the impl thread.
>
(https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_unittest.cc?l=6505
> for example). Now with LayerTreeHostRemote, there is an additional layer of
> indirection, the embedder still talks to the LayerTreeHost API, but you first
> serialize data to be transported across a process boundary and then use the
> InProcess API on the client to transport across a thread boundary.
> 
> We will certainly have the blink tests and Layout tests verify this as well,
but
> cc already has a lot of test cases to verify the data flow across threads, and
> having additional coverage with smaller scoped unit-tests would surely do more
> good than harm. :)

Hey Dana, Could I get your blessing on this, before I go ahead with
re-structuring the test?

> 
> > 
> > 
> > >
> > >
> > >
> > > https://codereview.chromium.org/2375363002/
> > >
> > 
> > -- 
> > 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.

[Khushal, 2016-10-04 09:11:50]

As I tried re-structuring the TreeSync tests, it was getting much more intrusive
on the setup there and didn't really appear to be a good fit. I think
LayerTreeTests would be better, I'll take up those in a follow up patch. For now
I've added a unittest for CompositorStateDeserializer and added some
verification using PropertyTrees there itself.

For the layer id setup, I just injected a LayerFactory into the deserializer so
we can have matching ids for tests but the rest of the code can be the same.
That seemed like a cleaner solution.

PTAL.

https://codereview.chromium.org/2375363002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2375363002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:426:
layer_tree_->ToProtobuf(layer_tree_host_proto->mutable_layer_tree(), true);
On 2016/09/29 20:55:33, ajuma wrote:
> Instead of passing |true| here, please create a variable with a good name
> (perhaps "inputs_only"), set that variable's value to true, and pass that
> variable as the argument. That'll make this more readable. An alternative
would
> be to change the type of the second argument to an enum. (And same comment for
> the other callsites of methods with boolean arguments added in this CL.)

Done.

[ajuma, 2016-10-04 15:45:10]

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/compositor_sta...
File cc/blimp/compositor_state_deserializer.cc (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/compositor_sta...
cc/blimp/compositor_state_deserializer.cc:83:
SynchronizeLayerHeirarchyRecursive(
Is this happening every frame? Are there plans to serialize only when the
hierarchy changes? (e.g. if a page is just repainting a composited layer or
changing its transform, there's no need to re-serialize the layer hierarchy)

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/compositor_sta...
cc/blimp/compositor_state_deserializer.cc:199: void
CompositorStateDeserializer::SynchronizeLayerHeirarchyRecursive(
typo: "Hierarchy"

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/layer_factory.h
File cc/blimp/layer_factory.h (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/layer_factory....
cc/blimp/layer_factory.h:20: virtual scoped_refptr<Layer> CreateLayer(int
engine_layer_id) = 0;
When you start to expand test coverage, you'll need to add support for other
subclasses of Layer (like FakePictureLayer).

https://codereview.chromium.org/2375363002/diff/40001/cc/layers/layer_proto_c...
File cc/layers/layer_proto_converter.cc (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/layers/layer_proto_c...
cc/layers/layer_proto_converter.cc:53:
layer->ToLayerPropertiesProto(layer_update, false);
Please create a variable for the bool argument.

https://codereview.chromium.org/2375363002/diff/40001/cc/proto/layer.proto
File cc/proto/layer.proto (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:102: // back to the engine.
Curious about this, is the plan to not support touch event handlers at all?

https://codereview.chromium.org/2375363002/diff/40001/cc/proto/layer_tree_hos...
File cc/proto/layer_tree_host.proto (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/proto/layer_tree_hos...
cc/proto/layer_tree_host.proto:33: // TODO(khushalsagar): Remove these when
crbug.com/648442
nit: missing end of sentence :)

[Khushal, 2016-10-04 18:34:48]

Thanks Ali.

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/compositor_sta...
File cc/blimp/compositor_state_deserializer.cc (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/compositor_sta...
cc/blimp/compositor_state_deserializer.cc:83:
SynchronizeLayerHeirarchyRecursive(
On 2016/10/04 15:45:10, ajuma wrote:
> Is this happening every frame? Are there plans to serialize only when the
> hierarchy changes? (e.g. if a page is just repainting a composited layer or
> changing its transform, there's no need to re-serialize the layer hierarchy)

Yeah, the existing code was doing this so I didn't change. Add a bug about it.
Thanks!

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/compositor_sta...
cc/blimp/compositor_state_deserializer.cc:199: void
CompositorStateDeserializer::SynchronizeLayerHeirarchyRecursive(
On 2016/10/04 15:45:10, ajuma wrote:
> typo: "Hierarchy"

Done.

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/layer_factory.h
File cc/blimp/layer_factory.h (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/blimp/layer_factory....
cc/blimp/layer_factory.h:20: virtual scoped_refptr<Layer> CreateLayer(int
engine_layer_id) = 0;
On 2016/10/04 15:45:10, ajuma wrote:
> When you start to expand test coverage, you'll need to add support for other
> subclasses of Layer (like FakePictureLayer).

Sounds good. For each of them we can add methods to create layers with the id
specified.

https://codereview.chromium.org/2375363002/diff/40001/cc/layers/layer_proto_c...
File cc/layers/layer_proto_converter.cc (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/layers/layer_proto_c...
cc/layers/layer_proto_converter.cc:53:
layer->ToLayerPropertiesProto(layer_update, false);
On 2016/10/04 15:45:10, ajuma wrote:
> Please create a variable for the bool argument.

Done.

https://codereview.chromium.org/2375363002/diff/40001/cc/proto/layer.proto
File cc/proto/layer.proto (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:102: // back to the engine.
On 2016/10/04 15:45:10, ajuma wrote:
> Curious about this, is the plan to not support touch event handlers at all?

Sending any continuous gestures to the engine will end up being a horrible
experience, touch handlers in particular end up blocking scrolling even if they
don't consume the event. We might send them only for passive listeners though.

https://codereview.chromium.org/2375363002/diff/40001/cc/proto/layer_tree_hos...
File cc/proto/layer_tree_host.proto (right):

https://codereview.chromium.org/2375363002/diff/40001/cc/proto/layer_tree_hos...
cc/proto/layer_tree_host.proto:33: // TODO(khushalsagar): Remove these when
crbug.com/648442
On 2016/10/04 15:45:10, ajuma wrote:
> nit: missing end of sentence :)

Done.

[ajuma, 2016-10-04 18:42:05]

Thanks, lgtm

[Khushal, 2016-10-04 20:10:20]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-10-04 20:10:20]

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/2375363002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-04 20:10:47]

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-10-04 20:32:28]

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

[commit-bot: I haz the power, 2016-10-04 20:32:29]

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...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Khushal, 2016-10-04 20:39:04]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-10-04 20:39:04]

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/2375363002/#ps100001
(title: "comment update")

[commit-bot: I haz the power, 2016-10-04 20:39:33]

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-10-04 21:41:35]

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

[commit-bot: I haz the power, 2016-10-04 21:41:37]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Khushal, 2016-10-04 22:16:34]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-10-04 22:16:35]

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/2375363002/#ps120001
(title: "test update")

[commit-bot: I haz the power, 2016-10-04 22:17:23]

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-10-04 23:41:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-04 23:43:13]

Description was changed from

==========
cc/blimp: Set up the framework for state serialization.

1) Split the inputs and rest of the fields in the protos so we only
serialize the inputs for the new serialization code.
2) Add a CompositorStateDeserializer to deserialize compositor protos.

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

to

==========
cc/blimp: Set up the framework for state serialization.

1) Split the inputs and rest of the fields in the protos so we only
serialize the inputs for the new serialization code.
2) Add a CompositorStateDeserializer to deserialize compositor protos.

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

Committed: https://crrev.com/0c4d7f1fac75e0d294b10f7ba465d99469b81d55
Cr-Commit-Position: refs/heads/master@{#423001}
==========

[commit-bot: I haz the power, 2016-10-04 23:43:14]

Patchset 7 (id:??) landed as
https://crrev.com/0c4d7f1fac75e0d294b10f7ba465d99469b81d55
Cr-Commit-Position: refs/heads/master@{#423001}