Serialize PictureLayer properties.

Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Don't query LayerImpl::bounds() in PictureLayer::PushPropertiesTo().
  Instead rely on either Layer::bounds() or
  Layer::paint_properties().bounds depending on the
  source_frame_number.  Since LayerImpl's bounds are set by
  Layer::PushPropertiesTo() right before this check, this should
  result in the same behavior.
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

BUG=538710
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8e9da48fa5086b27e076b21f729254cc8451c923
Cr-Commit-Position: refs/heads/master@{#367300}

[David Trainor- moved to gerrit, 2015-12-15 00:14:08]

Description was changed from

==========
Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Added update_bounds_ to track Layer::bounds() at time of update so
  that PushPropertiesTo() and LayerSpecificPropertiesToProto() don't
  have to query LayerImpl::bounds().
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

BUG=538710
==========

to

==========
Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Added update_bounds_ to track Layer::bounds() at time of update so
  that PushPropertiesTo() and LayerSpecificPropertiesToProto() don't
  have to query LayerImpl::bounds().
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

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

[David Trainor- moved to gerrit, 2015-12-15 17:23:02]

Description was changed from

==========
Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Added update_bounds_ to track Layer::bounds() at time of update so
  that PushPropertiesTo() and LayerSpecificPropertiesToProto() don't
  have to query LayerImpl::bounds().
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

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

to

==========
Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Don't query LayerImpl::bounds() in PictureLayer::PushPropertiesTo().
  Instead rely on either Layer::bounds() or
  Layer::paint_properties().bounds depending on the
  source_frame_number.  Since LayerImpl's bounds are set by
  Layer::PushPropertiesTo() right before this check, this should
  result in the same behavior.
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

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

[David Trainor- moved to gerrit, 2015-12-15 17:54:30]

dtrainor@chromium.org changed reviewers:
+ danakj@chromium.org, nyquist@chromium.org, vmpstr@chromium.org

[David Trainor- moved to gerrit, 2015-12-15 17:54:30]

ptal thanks!

danakj@: It still says you're behind on reviews, so feel free to just look at
picture_layer.cc if you'd like, since we discussed it over chat.  I ended up not
using a new 'update_bounds_' in PictureLayer because it was causing a valid test
failure (and update_bounds_ was inherently always the same thing as
recording_source_->GetSize()).  Instead I grabbed the correct bounds from Layer
which should be the same behavior as we have now.

The only difference now is that the bounds I use don't automatically include
LayerImpl::bounds_delta().  But I'm not quite sure if I have to... it looks like
if we had a bounds_delta our dchecks would have failed whenever we got an update
to the recording source.  wdyt?

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (left):

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:55: gfx::Size impl_bounds = layer_impl->bounds();
So, it looks like LayerImpl::bounds() is set a few lines above at
Layer::PushPropertiesTo().  So I think if I can access the right bounds
(Layer::bounds() vs Layer::paint_properties().bounds) we should be okay just
using those instead of LayerImpl.

My only concern is that LayerImpl applies a bounds_delta to the bounds it
returns, so this change doesn't result in the same behavior.  Do we need to
worry about that delta here?  If we do, I could account for it on the
PushPropertiesTo method and just use the correct Layer bounds on the
serialization method.

[vmpstr, 2015-12-16 04:21:08]

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/empty_content...
File cc/layers/empty_content_layer_client.cc (right):

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/empty_content...
cc/layers/empty_content_layer_client.cc:42: return 0U;
nit: 0u (lower case)

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

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/layer_proto_c...
cc/layers/layer_proto_converter.cc:110: case proto::Picture:
I think this should be PictureLayer (and maybe base can be renamed to Layer)

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:238: if (update_source_frame_number_ !=
source_frame_number &&
This if block should not be in this function. Validate sounds very much line
just a dcheck function, not something that can mutate the class.

Also, when you move this out, you can wrap the insides of this function in #if
DCHECK_IS_ON(), kinda like:

void PictureLayer::ValidateRecordingSource() {
#if DCHECK_IS_ON()
  ...
#endif  // DCHECK_IS_ON()
}

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

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer.h:18: class TestSerializationPictureLayer;
Does the friend declaration below act as a forward declare as well? If not, can
you put a blank line between this and the rest of the declares and a comment
about where this is used?

https://codereview.chromium.org/1527863002/diff/20001/cc/playback/display_lis...
File cc/playback/display_list_recording_source.cc (right):

https://codereview.chromium.org/1527863002/diff/20001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:79: if (proto.has_display_list()) {
When would it not? Can you leave a comment?

https://codereview.chromium.org/1527863002/diff/20001/cc/proto/layer.proto
File cc/proto/layer.proto (right):

https://codereview.chromium.org/1527863002/diff/20001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:20: Picture = 2;
As I mentined elsewhere, maybe these should match the cc class name Layer,
PictureLayer, etc

https://codereview.chromium.org/1527863002/diff/20001/cc/proto/layer.proto#ne...
cc/proto/layer.proto:58: // TODO(dtrainor): use a 'oneof' union when it's
supported in Chromium.
File a bug and reference it here please

[David Trainor- moved to gerrit, 2015-12-16 06:51:52]

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:238: if (update_source_frame_number_ !=
source_frame_number &&
On 2015/12/16 04:21:08, vmpstr wrote:
> This if block should not be in this function. Validate sounds very much line
> just a dcheck function, not something that can mutate the class.
> 
> Also, when you move this out, you can wrap the insides of this function in #if
> DCHECK_IS_ON(), kinda like:
> 
> void PictureLayer::ValidateRecordingSource() {
> #if DCHECK_IS_ON()
>   ...
> #endif  // DCHECK_IS_ON()
> }

I'll have to rename the function then.  We need this behavior in two places,
which is why I pulled it out from PushPropertiesTo (where the logic used to be).
 It's basically checking whether or not the recording source is valid and, if
not, destroying it before the commit.

[vmpstr, 2015-12-16 18:55:39]

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1527863002/diff/20001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:238: if (update_source_frame_number_ !=
source_frame_number &&
On 2015/12/16 06:51:52, David Trainor wrote:
> On 2015/12/16 04:21:08, vmpstr wrote:
> > This if block should not be in this function. Validate sounds very much line
> > just a dcheck function, not something that can mutate the class.
> > 
> > Also, when you move this out, you can wrap the insides of this function in
#if
> > DCHECK_IS_ON(), kinda like:
> > 
> > void PictureLayer::ValidateRecordingSource() {
> > #if DCHECK_IS_ON()
> >   ...
> > #endif  // DCHECK_IS_ON()
> > }
> 
> I'll have to rename the function then.  We need this behavior in two places,
> which is why I pulled it out from PushPropertiesTo (where the logic used to
be).
>  It's basically checking whether or not the recording source is valid and, if
> not, destroying it before the commit.

Renaming the function is fine as well. You can also make this two functions: one
that verifies the thing and one that changes the recording_source. My main
concern is that Verify* type of functions should be const.

[David Trainor- moved to gerrit, 2015-12-16 19:10:59]

ptal thanks!

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/layers/empty_...
File cc/layers/empty_content_layer_client.cc (right):

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/layers/empty_...
cc/layers/empty_content_layer_client.cc:42: return 0U;
On 2015/12/16 04:21:08, vmpstr wrote:
> nit: 0u (lower case)

Done.

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/layers/layer_...
File cc/layers/layer_proto_converter.cc (right):

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/layers/layer_...
cc/layers/layer_proto_converter.cc:110: case proto::Picture:
On 2015/12/16 04:21:08, vmpstr wrote:
> I think this should be PictureLayer (and maybe base can be renamed to Layer)

Done.

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/layers/pictur...
File cc/layers/picture_layer.cc (right):

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/layers/pictur...
cc/layers/picture_layer.cc:238: if (update_source_frame_number_ !=
source_frame_number &&
On 2015/12/16 06:51:52, David Trainor wrote:
> On 2015/12/16 04:21:08, vmpstr wrote:
> > This if block should not be in this function. Validate sounds very much line
> > just a dcheck function, not something that can mutate the class.
> > 
> > Also, when you move this out, you can wrap the insides of this function in
#if
> > DCHECK_IS_ON(), kinda like:
> > 
> > void PictureLayer::ValidateRecordingSource() {
> > #if DCHECK_IS_ON()
> >   ...
> > #endif  // DCHECK_IS_ON()
> > }
> 
> I'll have to rename the function then.  We need this behavior in two places,
> which is why I pulled it out from PushPropertiesTo (where the logic used to
be).
>  It's basically checking whether or not the recording source is valid and, if
> not, destroying it before the commit.

Changed it to DropRecordingSourceContentIfInvalid().  Is that more along the
lines of what you want?

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/layers/pictur...
File cc/layers/picture_layer.h (right):

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/layers/pictur...
cc/layers/picture_layer.h:18: class TestSerializationPictureLayer;
On 2015/12/16 04:21:08, vmpstr wrote:
> Does the friend declaration below act as a forward declare as well? If not,
can
> you put a blank line between this and the rest of the declares and a comment
> about where this is used?

Done.

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/playback/disp...
File cc/playback/display_list_recording_source.cc (right):

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/playback/disp...
cc/playback/display_list_recording_source.cc:79: if (proto.has_display_list()) {
On 2015/12/16 04:21:08, vmpstr wrote:
> When would it not? Can you leave a comment?

If we serialized an empty RecordingSource (which happens if the Layer bounds
were updated while the Layer wasn't in the viewport).  I added a unit test that
simulates this situation in display_list_recording_source_unittest.cc.

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/proto/layer.p...
File cc/proto/layer.proto (right):

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/proto/layer.p...
cc/proto/layer.proto:20: Picture = 2;
On 2015/12/16 04:21:08, vmpstr wrote:
> As I mentined elsewhere, maybe these should match the cc class name Layer,
> PictureLayer, etc

Done.  Also moved to caps to fit chromium style guide and added UNKNOWN = 0,
which lets us inherently handle receiving layer types that the client doesn't
support.

https://chromiumcodereview.appspot.com/1527863002/diff/20001/cc/proto/layer.p...
cc/proto/layer.proto:58: // TODO(dtrainor): use a 'oneof' union when it's
supported in Chromium.
On 2015/12/16 04:21:08, vmpstr wrote:
> File a bug and reference it here please

Done.

https://chromiumcodereview.appspot.com/1527863002/diff/40001/cc/layers/pictur...
File cc/layers/picture_layer.cc (right):

https://chromiumcodereview.appspot.com/1527863002/diff/40001/cc/layers/pictur...
cc/layers/picture_layer.cc:56: DropRecordingSourceContentIfInvalid();
Moved this up to keep it in the same place as the original logic.

https://chromiumcodereview.appspot.com/1527863002/diff/40001/cc/layers/pictur...
File cc/layers/picture_layer.h (right):

https://chromiumcodereview.appspot.com/1527863002/diff/40001/cc/layers/pictur...
cc/layers/picture_layer.h:55: void
SetTypeForProtoSerialization(proto::LayerNode* proto) const override;
Forgot this... added unit tests to check this too.

[vmpstr, 2015-12-18 07:42:34]

lgtm

https://codereview.chromium.org/1527863002/diff/40001/cc/layers/empty_content...
File cc/layers/empty_content_layer_client.h (right):

https://codereview.chromium.org/1527863002/diff/40001/cc/layers/empty_content...
cc/layers/empty_content_layer_client.h:19: class CC_EXPORT
EmptyContentLayerClient : public ContentLayerClient {
Can you add a class comment here to explain what this does / how it's used?

[David Trainor- moved to gerrit, 2016-01-04 06:50:15]

https://chromiumcodereview.appspot.com/1527863002/diff/40001/cc/layers/empty_...
File cc/layers/empty_content_layer_client.h (right):

https://chromiumcodereview.appspot.com/1527863002/diff/40001/cc/layers/empty_...
cc/layers/empty_content_layer_client.h:19: class CC_EXPORT
EmptyContentLayerClient : public ContentLayerClient {
On 2015/12/18 07:42:33, vmpstr wrote:
> Can you add a class comment here to explain what this does / how it's used?

Done.

[David Trainor- moved to gerrit, 2016-01-04 06:50:19]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2016-01-04 06:50:19]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1527863002/#ps60001
(title: "Rebased and added comment")

[commit-bot: I haz the power, 2016-01-04 06:50:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1527863002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1527863002/60001

[commit-bot: I haz the power, 2016-01-04 09:53:13]

Description was changed from

==========
Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Don't query LayerImpl::bounds() in PictureLayer::PushPropertiesTo().
  Instead rely on either Layer::bounds() or
  Layer::paint_properties().bounds depending on the
  source_frame_number.  Since LayerImpl's bounds are set by
  Layer::PushPropertiesTo() right before this check, this should
  result in the same behavior.
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

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

to

==========
Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Don't query LayerImpl::bounds() in PictureLayer::PushPropertiesTo().
  Instead rely on either Layer::bounds() or
  Layer::paint_properties().bounds depending on the
  source_frame_number.  Since LayerImpl's bounds are set by
  Layer::PushPropertiesTo() right before this check, this should
  result in the same behavior.
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

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

[commit-bot: I haz the power, 2016-01-04 09:53:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-04 09:54:32]

Description was changed from

==========
Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Don't query LayerImpl::bounds() in PictureLayer::PushPropertiesTo().
  Instead rely on either Layer::bounds() or
  Layer::paint_properties().bounds depending on the
  source_frame_number.  Since LayerImpl's bounds are set by
  Layer::PushPropertiesTo() right before this check, this should
  result in the same behavior.
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

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

to

==========
Serialize PictureLayer properties.

Serialize the properties specific to the PictureLayer subclass.  This
required a few slight changes:
- Don't query LayerImpl::bounds() in PictureLayer::PushPropertiesTo().
  Instead rely on either Layer::bounds() or
  Layer::paint_properties().bounds depending on the
  source_frame_number.  Since LayerImpl's bounds are set by
  Layer::PushPropertiesTo() right before this check, this should
  result in the same behavior.
- Create an EmptyContentLayerClient to be used by all PictureLayer
  instances created by
  LayerProtoConverter::FindOrAllocateAndConstruct().

BUG=538710
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8e9da48fa5086b27e076b21f729254cc8451c923
Cr-Commit-Position: refs/heads/master@{#367300}
==========

[commit-bot: I haz the power, 2016-01-04 09:54:33]

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