cc/blimp: Add (de)-serialization for PictureLayer and ScrollbarLayer.

cc/blimp: Add (de)-serialization for PictureLayer and ScrollbarLayer.

This adds support for serializing these layers from the new code path
and brings us in parity with what the data the existing code serializes.

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

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

[Khushal, 2016-10-06 03:32:00]

Description was changed from

==========
cc/blimp: Add (de)-serialization for PictureLayer and ScrollbarLayer.

This adds support for serializing these layers from the new code path
and brings us in parity with what the data the existing code serializes.

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

to

==========
cc/blimp: Add (de)-serialization for PictureLayer and ScrollbarLayer.

This adds support for serializing these layers from the new code path
and brings us in parity with what the data the existing code serializes.

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

[Khushal, 2016-10-06 03:32:14]

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

[Khushal, 2016-10-06 03:45:23]

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

[commit-bot: I haz the power, 2016-10-06 03:45:42]

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-10-06 04:15:22]

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

[commit-bot: I haz the power, 2016-10-06 04:15:23]

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

[vmpstr, 2016-10-06 18:34:21]

Can we move as much as possible to be member objects instead of unique_ptrs? We
can always add move constructors where needed, but minimize heap allocations.

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
File cc/blimp/compositor_state_deserializer.cc (right):

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.cc:138:
scrollbar_layer->SetScrollLayer(GetClientIdFromEngineId(scrollbar.second));
Is this just the id of |scrollbar_layer|?

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.cc:339: (*new_layer_map)[layer_node.id()]
= base::MakeUnique<LayerData>();
These two lines are doing two lookups for the same thing... why not

auto& layer_data = (*new_layer_map)[layer_node.id()];
DCHECK(!layer_data);
layer_data = base::MakeUnique<LayerData>();
// use layer_data.get()

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
File cc/blimp/compositor_state_deserializer.h (right):

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:38: std::unique_ptr<ClientPictureCache>
client_picture_cache,
It looks a bit weird that the deserializer takes ownership of the cache. Can you
put a comment on the constructor explaining?

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:39: ScrollCallback scroll_callback,
const ref

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:47: int GetClientIdFromEngineId(int
engine_layer_id) const;
Is this just GetLayerForEngineId()->id()? (if one exists?). I wonder if we need
two public functions for getting pretty similar information.

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:65:
std::unique_ptr<DeserializedContentLayerClient> content_layer_client;
Can this just be a member and not a unique_ptr?

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:69: std::unordered_map<int,
std::unique_ptr<LayerData>>;
Can this be a map of int to LayerData? And maybe you can add a LayerData move
ctor?

[Khushal, 2016-10-06 20:05:40]

Thanks Vlad!

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
File cc/blimp/compositor_state_deserializer.cc (right):

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.cc:138:
scrollbar_layer->SetScrollLayer(GetClientIdFromEngineId(scrollbar.second));
On 2016/10/06 18:34:21, vmpstr wrote:
> Is this just the id of |scrollbar_layer|?

I think it would be the scroll layer id for the scrollbar layer.
https://cs.chromium.org/chromium/src/cc/layers/solid_color_scrollbar_layer.h?...

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.cc:339: (*new_layer_map)[layer_node.id()]
= base::MakeUnique<LayerData>();
On 2016/10/06 18:34:21, vmpstr wrote:
> These two lines are doing two lookups for the same thing... why not
> 
> auto& layer_data = (*new_layer_map)[layer_node.id()];
> DCHECK(!layer_data);
> layer_data = base::MakeUnique<LayerData>();
> // use layer_data.get()

Done.

DCHECK is unnecessary, we do it at the beginning of the function.

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
File cc/blimp/compositor_state_deserializer.h (right):

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:38: std::unique_ptr<ClientPictureCache>
client_picture_cache,
On 2016/10/06 18:34:21, vmpstr wrote:
> It looks a bit weird that the deserializer takes ownership of the cache. Can
you
> put a comment on the constructor explaining?

TBH I want to restructure the code to eliminate the cache abstraction, and do
this in a better way in this class itself. For now I was steering clear of
rocking the boat too much and keeping things the old way where LTH used to take
ownership of this. It should be easy to have the caller own it too. So I don't
really have a good explainination for why. :P

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:39: ScrollCallback scroll_callback,
On 2016/10/06 18:34:21, vmpstr wrote:
> const ref

Done.

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:47: int GetClientIdFromEngineId(int
engine_layer_id) const;
On 2016/10/06 18:34:21, vmpstr wrote:
> Is this just GetLayerForEngineId()->id()? (if one exists?). I wonder if we
need
> two public functions for getting pretty similar information.

Meh. Was just being lazy. Removed.

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:65:
std::unique_ptr<DeserializedContentLayerClient> content_layer_client;
On 2016/10/06 18:34:21, vmpstr wrote:
> Can this just be a member and not a unique_ptr?

Only needed for PictureLayers so I used a unique_ptr.

https://codereview.chromium.org/2397843003/diff/1/cc/blimp/compositor_state_d...
cc/blimp/compositor_state_deserializer.h:69: std::unordered_map<int,
std::unique_ptr<LayerData>>;
On 2016/10/06 18:34:21, vmpstr wrote:
> Can this be a map of int to LayerData? And maybe you can add a LayerData move
> ctor?

Good point. Done.

https://codereview.chromium.org/2397843003/diff/1/cc/layers/solid_color_scrol...
File cc/layers/solid_color_scrollbar_layer.cc (right):

https://codereview.chromium.org/2397843003/diff/1/cc/layers/solid_color_scrol...
cc/layers/solid_color_scrollbar_layer.cc:128: void
SolidColorScrollbarLayer::PushPropertiesTo(LayerImpl* layer) {
While we are it, how does this work? When I read through the code, changing the
scroll layer id doesn't PushProperties, it requests a tree sync?

[Khushal, 2016-10-07 17:40:59]

friendly ping. :)

[vmpstr, 2016-10-07 23:50:05]

https://codereview.chromium.org/2397843003/diff/40001/cc/blimp/deserialized_c...
File cc/blimp/deserialized_content_layer_client.cc (right):

https://codereview.chromium.org/2397843003/diff/40001/cc/blimp/deserialized_c...
cc/blimp/deserialized_content_layer_client.cc:16: display_list_ = display_list;
move

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

https://codereview.chromium.org/2397843003/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:499: explicit Layer(int layer_id);
I'd rather this be something like layer->SetLayerForTesting()

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

https://codereview.chromium.org/2397843003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.h:71: PictureLayer(int engine_layer_id,
ContentLayerClient* client);
This is... weird. Should this be "layer_id". I kind of don't like adding ctors
that are only used in tests. We usually use either ForTesting or overload for
this type of thing.

https://codereview.chromium.org/2397843003/diff/40001/cc/layers/solid_color_s...
File cc/layers/solid_color_scrollbar_layer.h (right):

https://codereview.chromium.org/2397843003/diff/40001/cc/layers/solid_color_s...
cc/layers/solid_color_scrollbar_layer.h:53: SolidColorScrollbarLayer(int
engine_layer_id,
This definitely needs a comment. I also don't understand the engline_layer_id
here. This code is primarily not blimp, so the engine reference here is kind of
obscure.

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

https://codereview.chromium.org/2397843003/diff/40001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:152: //
PictureLayer::PictureLayerInputs.------------------------
Style nit: do you really need -----, can we just separate blocks as

// This is blah.
one
two
three

four
five
six

is it clear that one two and three are blah but four five and six aren't?

[Khushal, 2016-10-08 00:41:33]

https://codereview.chromium.org/2397843003/diff/40001/cc/blimp/deserialized_c...
File cc/blimp/deserialized_content_layer_client.cc (right):

https://codereview.chromium.org/2397843003/diff/40001/cc/blimp/deserialized_c...
cc/blimp/deserialized_content_layer_client.cc:16: display_list_ = display_list;
On 2016/10/07 23:50:05, vmpstr wrote:
> move

Done.

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

https://codereview.chromium.org/2397843003/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:499: explicit Layer(int layer_id);
On 2016/10/07 23:50:05, vmpstr wrote:
> I'd rather this be something like layer->SetLayerForTesting()

That's actually much better and simplifies things a bunch. Thanks! Done.

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

https://codereview.chromium.org/2397843003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.h:71: PictureLayer(int engine_layer_id,
ContentLayerClient* client);
On 2016/10/07 23:50:05, vmpstr wrote:
> This is... weird. Should this be "layer_id". I kind of don't like adding ctors
> that are only used in tests. We usually use either ForTesting or overload for
> this type of thing.

Not needed anymore. Removed.

https://codereview.chromium.org/2397843003/diff/40001/cc/layers/solid_color_s...
File cc/layers/solid_color_scrollbar_layer.h (right):

https://codereview.chromium.org/2397843003/diff/40001/cc/layers/solid_color_s...
cc/layers/solid_color_scrollbar_layer.h:53: SolidColorScrollbarLayer(int
engine_layer_id,
On 2016/10/07 23:50:05, vmpstr wrote:
> This definitely needs a comment. I also don't understand the engline_layer_id
> here. This code is primarily not blimp, so the engine reference here is kind
of
> obscure. 

Not needed anymore. \o/

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

https://codereview.chromium.org/2397843003/diff/40001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:152: //
PictureLayer::PictureLayerInputs.------------------------
On 2016/10/07 23:50:05, vmpstr wrote:
> Style nit: do you really need -----, can we just separate blocks as
> 
> // This is blah.
> one
> two
> three
> 
> four
> five
> six
> 
> is it clear that one two and three are blah but four five and six aren't?

I did it in the layer proto so did it here just to be consistent. Wouldn't worry
about this too much, its going to go away soon anyway.

[Khushal, 2016-10-10 18:58:55]

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

[commit-bot: I haz the power, 2016-10-10 18:59:20]

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

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

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

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-10-10 21:02:46]

pingy. :)

[vmpstr, 2016-10-10 21:28:26]

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

https://codereview.chromium.org/2397843003/diff/80001/cc/blimp/compositor_sta...
cc/blimp/compositor_state_deserializer.cc:63:
CompositorStateDeserializer::LayerData::~LayerData() = default;
nit: move dtors to be next to ctors.

https://codereview.chromium.org/2397843003/diff/80001/cc/blimp/compositor_sta...
cc/blimp/compositor_state_deserializer.cc:143:
scrollbar_layer->SetScrollLayer(GetClientIdFromEngineId(scrollbar.second));
Can you explain the relationships between these ids please?

What is scrollbar.first and scrollbar.second here? Can you maybe put them in
well named variables?

https://codereview.chromium.org/2397843003/diff/80001/cc/proto/layer.proto
File cc/proto/layer.proto (right):

https://codereview.chromium.org/2397843003/diff/80001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:49: optional SolidColorScrollbarLayerProperties
solid_scrollbar = 7;
6?

https://codereview.chromium.org/2397843003/diff/80001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:153: optional bool nearest_neighbor = 5;
Are we renumbering these any time soon?

[Khushal, 2016-10-10 21:39:25]

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

https://codereview.chromium.org/2397843003/diff/80001/cc/blimp/compositor_sta...
cc/blimp/compositor_state_deserializer.cc:63:
CompositorStateDeserializer::LayerData::~LayerData() = default;
On 2016/10/10 21:28:25, vmpstr wrote:
> nit: move dtors to be next to ctors.

Done.

https://codereview.chromium.org/2397843003/diff/80001/cc/blimp/compositor_sta...
cc/blimp/compositor_state_deserializer.cc:143:
scrollbar_layer->SetScrollLayer(GetClientIdFromEngineId(scrollbar.second));
On 2016/10/10 21:28:25, vmpstr wrote:
> Can you explain the relationships between these ids please?
> 
> What is scrollbar.first and scrollbar.second here? Can you maybe put them in
> well named variables?

Done.

https://codereview.chromium.org/2397843003/diff/80001/cc/proto/layer.proto
File cc/proto/layer.proto (right):

https://codereview.chromium.org/2397843003/diff/80001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:49: optional SolidColorScrollbarLayerProperties
solid_scrollbar = 7;
On 2016/10/10 21:28:25, vmpstr wrote:
> 6?

Done.

https://codereview.chromium.org/2397843003/diff/80001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:153: optional bool nearest_neighbor = 5;
On 2016/10/10 21:28:25, vmpstr wrote:
> Are we renumbering these any time soon?

Not high priority, probably after the old path is removed.

[vmpstr, 2016-10-10 21:41:56]

lgtm

[Khushal, 2016-10-10 21:45:38]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-10-10 21:46:08]

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-10 22:55:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-10 22:57:59]

Description was changed from

==========
cc/blimp: Add (de)-serialization for PictureLayer and ScrollbarLayer.

This adds support for serializing these layers from the new code path
and brings us in parity with what the data the existing code serializes.

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

to

==========
cc/blimp: Add (de)-serialization for PictureLayer and ScrollbarLayer.

This adds support for serializing these layers from the new code path
and brings us in parity with what the data the existing code serializes.

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

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

[commit-bot: I haz the power, 2016-10-10 22:58:00]

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