cc: Set up the framework to restrict access to Layer internals.

cc: Clean up Layer and the Layer public API.

To create a distinction between the Layer data that is received from
the embedder and values that are internally cached or set during
PropertyTree building, move the embedder data, callbacks and interfaces
into a seperate struct.

Also move public methods on Layer which are meant to be used internally
in cc but shouldn't be made available to the embedder to private and
restrict access using an explicit friend list.

The patch only moves the variables and method declarations and should
be a no-op.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b51998fe37830e22bd770de5f5ad08b663b465fd
Cr-Commit-Position: refs/heads/master@{#404286}

[khushalsagar, 2016-07-06 18:26:58]

Description was changed from

==========
cc: Set up the framework to restrict access to Layer internals.

Move public methods from Layer which are used internally in cc but
should not be available to the embedder to an inner class in Layer
with a friend list for limited access.

BUG=625284
==========

to

==========
cc: Set up the framework to restrict access to Layer internals.

Move public methods from Layer which are used internally in cc but
should not be available to the embedder to an inner class in Layer
with a friend list for limited access.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Khushal, 2016-07-06 18:31:42]

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

[Khushal, 2016-07-06 18:31:43]

If we are using friend classes then this looks like the cleanest way to restrict
access to internally used methods from the embedder and selectively expose the
private details of Layer to the internal cc classes as well.

Does this look close to what you had in mind?

[enne (OOO), 2016-07-06 19:57:42]

I think this would work but just feels a little bit more overengineered than
maybe needs be.  I think it would be equivalent to not have Layer::Internals,
make those functions private, and just have Layer friend LayerTreeHost and
LayerInternalsForTest directly.  I can see the concern that maybe this means
that LayerTreeHost would poke at Layer internals that it shouldn't, but I'm not
significantly worried about that.

[Khushal, 2016-07-06 20:36:42]

On 2016/07/06 19:57:42, enne wrote:
> I think this would work but just feels a little bit more overengineered than
> maybe needs be.  I think it would be equivalent to not have Layer::Internals,
> make those functions private, and just have Layer friend LayerTreeHost and
> LayerInternalsForTest directly.  I can see the concern that maybe this means
> that LayerTreeHost would poke at Layer internals that it shouldn't, but I'm
not
> significantly worried about that.

Okay, made the methods private there. I was just trying to avoid a tighter
coupling between these classes but if that won't be a problem in practice then
this is simpler. Will start putting the elements that come from the embedder in
separate structs in follow up patches.

[Khushal, 2016-07-06 20:36:59]

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

[commit-bot: I haz the power, 2016-07-06 20:37:50]

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

[enne (OOO), 2016-07-06 20:49:01]

Thanks! lgtm

[Khushal, 2016-07-06 21:20:52]

Description was changed from

==========
cc: Set up the framework to restrict access to Layer internals.

Move public methods from Layer which are used internally in cc but
should not be available to the embedder to an inner class in Layer
with a friend list for limited access.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Set up the framework to restrict access to Layer internals.

Move public methods from Layer which are used internally in cc but
should not be available to the embedder to private in Layer
with a friend list for limited access.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-06 22:08:45]

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

[commit-bot: I haz the power, 2016-07-06 22:08:46]

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

[khushalsagar, 2016-07-07 02:37:45]

Description was changed from

==========
cc: Set up the framework to restrict access to Layer internals.

Move public methods from Layer which are used internally in cc but
should not be available to the embedder to private in Layer
with a friend list for limited access.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move Layer variables received from the embedder into Input struct.

To create a distinction between the Layer data that is received from
the embedder and values that are internally cached or set during
PropertyTree building, move the embedder data, callbacks and interfaces
into a seperate struct. The patch only moves the variables and should
be a no-op.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Khushal, 2016-07-07 02:38:00]

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

[commit-bot: I haz the power, 2016-07-07 02:38:25]

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-07-07 03:54:09]

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

[commit-bot: I haz the power, 2016-07-07 03:54:10]

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-07-07 16:43:05]

khushalsagar@chromium.org changed reviewers:
- khushalsagar@chromium.org

[Khushal, 2016-07-07 16:43:06]

I'll just move the data that comes from the embedder into a separate struct. We
can maintain this distinction within the class going forward.

The aim of making these methods private was to restrict access, I'm not sure if
friending them here is the best way or really necessary at this point. We can
revisit this later if it turns out to be a problem.

[enne (OOO), 2016-07-07 17:32:01]

I think making functions private and friend classes would also help clean up the
layer API to embedders and would prevent embedders from starting to use
functions that they hadn't used in the past that maybe should have been
considered internal.

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

https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:619: struct Data {
I'm not sure I understand the difference between Inputs and Inputs::Data.  Why
not just collapse everything into Inputs?

[Khushal, 2016-07-07 18:16:39]

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

[Khushal, 2016-07-07 18:16:40]

Okay, included the initial change for making methods private as well. I'll see
if there is a clean way to do this for the PropertyTreeBuilder too.

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

https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:619: struct Data {
On 2016/07/07 17:32:00, enne wrote:
> I'm not sure I understand the difference between Inputs and Inputs::Data.  Why
> not just collapse everything into Inputs?

Data gets serialized directly. Rest of the inputs are callbacks/interfaces that
have assumptions and should be correctly set up for the remote implementation as
well. For instance, Blimp's version of did_scroll_callback will be used to
trigger a state update to the server.

[khushalsagar, 2016-07-07 18:24:01]

Description was changed from

==========
cc: Move Layer variables received from the embedder into Input struct.

To create a distinction between the Layer data that is received from
the embedder and values that are internally cached or set during
PropertyTree building, move the embedder data, callbacks and interfaces
into a seperate struct. The patch only moves the variables and should
be a no-op.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Clean up Layer and the Layer public API.

To create a distinction between the Layer data that is received from
the embedder and values that are internally cached or set during
PropertyTree building, move the embedder data, callbacks and interfaces
into a seperate struct.

Also move public methods on Layer which are meant to be used internally
in cc but shouldn't be made available to the embedder to private and
restrict access using an explicit friend list.

The patch only moves the variables and method declarations and should
be a no-op.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Khushal, 2016-07-07 22:00:31]

On 2016/07/07 18:16:40, Khushal wrote:
> Okay, included the initial change for making methods private as well. I'll see
> if there is a clean way to do this for the PropertyTreeBuilder too.
> 
> https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h
> File cc/layers/layer.h (right):
> 
>
https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h#newco...
> cc/layers/layer.h:619: struct Data {
> On 2016/07/07 17:32:00, enne wrote:
> > I'm not sure I understand the difference between Inputs and Inputs::Data. 
Why
> > not just collapse everything into Inputs?
> 
> Data gets serialized directly. Rest of the inputs are callbacks/interfaces
that
> have assumptions and should be correctly set up for the remote implementation
as
> well. For instance, Blimp's version of did_scroll_callback will be used to
> trigger a state update to the server.

Does this look good?

[enne (OOO), 2016-07-07 22:10:10]

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

https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:619: struct Data {
On 2016/07/07 at 18:16:39, Khushal wrote:
> On 2016/07/07 17:32:00, enne wrote:
> > I'm not sure I understand the difference between Inputs and Inputs::Data. 
Why
> > not just collapse everything into Inputs?
> 
> Data gets serialized directly. Rest of the inputs are callbacks/interfaces
that have assumptions and should be correctly set up for the remote
implementation as well. For instance, Blimp's version of did_scroll_callback
will be used to trigger a state update to the server.

I still don't know that I see the difference.  It's not like scroll_parent or
mask_layer can be serialized directly?

[Khushal, 2016-07-07 22:23:07]

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

https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:619: struct Data {
On 2016/07/07 22:10:10, enne wrote:
> On 2016/07/07 at 18:16:39, Khushal wrote:
> > On 2016/07/07 17:32:00, enne wrote:
> > > I'm not sure I understand the difference between Inputs and Inputs::Data. 
> Why
> > > not just collapse everything into Inputs?
> > 
> > Data gets serialized directly. Rest of the inputs are callbacks/interfaces
> that have assumptions and should be correctly set up for the remote
> implementation as well. For instance, Blimp's version of did_scroll_callback
> will be used to trigger a state update to the server.
> 
> I still don't know that I see the difference.  It's not like scroll_parent or
> mask_layer can be serialized directly?

For cases where you store data as references you have to do a deep serialization
for sure. We store the hierarchy using ids for all the layers, including mask
and replica layer and send a list of layers in a separate proto.

But you can never serialize a callback. In such cases, if someone adds something
new, they have to make sure they replicate the expected behavior on the client
side too. The aim is just to demarcate that distinction. I don't mind putting it
in the same struct and just adding comments if that looks better.

[enne (OOO), 2016-07-07 22:24:54]

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

https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:619: struct Data {
On 2016/07/07 at 22:23:07, Khushal wrote:
> On 2016/07/07 22:10:10, enne wrote:
> > On 2016/07/07 at 18:16:39, Khushal wrote:
> > > On 2016/07/07 17:32:00, enne wrote:
> > > > I'm not sure I understand the difference between Inputs and
Inputs::Data. 
> > Why
> > > > not just collapse everything into Inputs?
> > > 
> > > Data gets serialized directly. Rest of the inputs are callbacks/interfaces
> > that have assumptions and should be correctly set up for the remote
> > implementation as well. For instance, Blimp's version of did_scroll_callback
> > will be used to trigger a state update to the server.
> > 
> > I still don't know that I see the difference.  It's not like scroll_parent
or
> > mask_layer can be serialized directly?
> 
> For cases where you store data as references you have to do a deep
serialization for sure. We store the hierarchy using ids for all the layers,
including mask and replica layer and send a list of layers in a separate proto.
> 
> But you can never serialize a callback. In such cases, if someone adds
something new, they have to make sure they replicate the expected behavior on
the client side too. The aim is just to demarcate that distinction. I don't mind
putting it in the same struct and just adding comments if that looks better.

I'd prefer comments rather than a separate struct if you don't mind.  :)

[Khushal, 2016-07-07 23:00:48]

On 2016/07/07 22:24:54, enne wrote:
> https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h
> File cc/layers/layer.h (right):
> 
>
https://codereview.chromium.org/2128633002/diff/40001/cc/layers/layer.h#newco...
> cc/layers/layer.h:619: struct Data {
> On 2016/07/07 at 22:23:07, Khushal wrote:
> > On 2016/07/07 22:10:10, enne wrote:
> > > On 2016/07/07 at 18:16:39, Khushal wrote:
> > > > On 2016/07/07 17:32:00, enne wrote:
> > > > > I'm not sure I understand the difference between Inputs and
> Inputs::Data. 
> > > Why
> > > > > not just collapse everything into Inputs?
> > > > 
> > > > Data gets serialized directly. Rest of the inputs are
callbacks/interfaces
> > > that have assumptions and should be correctly set up for the remote
> > > implementation as well. For instance, Blimp's version of
did_scroll_callback
> > > will be used to trigger a state update to the server.
> > > 
> > > I still don't know that I see the difference.  It's not like scroll_parent
> or
> > > mask_layer can be serialized directly?
> > 
> > For cases where you store data as references you have to do a deep
> serialization for sure. We store the hierarchy using ids for all the layers,
> including mask and replica layer and send a list of layers in a separate
proto.
> > 
> > But you can never serialize a callback. In such cases, if someone adds
> something new, they have to make sure they replicate the expected behavior on
> the client side too. The aim is just to demarcate that distinction. I don't
mind
> putting it in the same struct and just adding comments if that looks better.
> 
> I'd prefer comments rather than a separate struct if you don't mind.  :)

Done. :)

[enne (OOO), 2016-07-07 23:10:36]

lgtm

[Khushal, 2016-07-07 23:22:45]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-07-07 23:23:43]

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-07-08 01:04:38]

Committed patchset #5 (id:80001)

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

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 01:05:52]

Description was changed from

==========
cc: Clean up Layer and the Layer public API.

To create a distinction between the Layer data that is received from
the embedder and values that are internally cached or set during
PropertyTree building, move the embedder data, callbacks and interfaces
into a seperate struct.

Also move public methods on Layer which are meant to be used internally
in cc but shouldn't be made available to the embedder to private and
restrict access using an explicit friend list.

The patch only moves the variables and method declarations and should
be a no-op.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Clean up Layer and the Layer public API.

To create a distinction between the Layer data that is received from
the embedder and values that are internally cached or set during
PropertyTree building, move the embedder data, callbacks and interfaces
into a seperate struct.

Also move public methods on Layer which are meant to be used internally
in cc but shouldn't be made available to the embedder to private and
restrict access using an explicit friend list.

The patch only moves the variables and method declarations and should
be a no-op.

BUG=625284
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b51998fe37830e22bd770de5f5ad08b663b465fd
Cr-Commit-Position: refs/heads/master@{#404286}
==========

[commit-bot: I haz the power, 2016-07-08 01:05:53]

Patchset 5 (id:??) landed as
https://crrev.com/b51998fe37830e22bd770de5f5ad08b663b465fd
Cr-Commit-Position: refs/heads/master@{#404286}