Serialize DisplayListRecordingSource to protos.

Serialize DisplayListRecordingSource to protos.

Add protobuf serialization to DisplayListRecordingSource.  Also refactor
the unittests to run all tests in both serialization and
non-serialization mode.

BUG=541321
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[David Trainor- moved to gerrit, 2015-12-09 03:37:08]

Description was changed from

==========
WIP

BUG=
==========

to

==========
WIP

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

[David Trainor- moved to gerrit, 2015-12-09 03:59:23]

Description was changed from

==========
WIP

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

to

==========
Serialize DisplayListRecordingSource to protos.

Add protobuf serialization to DisplayListRecordingSource.  Also refactor
the unittests to run all tests in both serialization and
non-serialization mode.

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

[David Trainor- moved to gerrit, 2015-12-09 04:03:34]

Description was changed from

==========
Serialize DisplayListRecordingSource to protos.

Add protobuf serialization to DisplayListRecordingSource.  Also refactor
the unittests to run all tests in both serialization and
non-serialization mode.

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

to

==========
Serialize DisplayListRecordingSource to protos.

Add protobuf serialization to DisplayListRecordingSource.  Also refactor
the unittests to run all tests in both serialization and
non-serialization mode.

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

[David Trainor- moved to gerrit, 2015-12-09 04:07:25]

Just an initial FYI.  I'm still looking into why two of these tests are failing.
 I'm trying to make it so all existing DisplayListRecordingSource tests run on
both serialization and non-serialization mode (like we're doing for other
components like the property tests).

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

https://codereview.chromium.org/1512823002/diff/20001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:51: RectToProto(recorded_viewport_,
proto->mutable_recorded_viewport());
Note to self: I need to write a unit test that validates that at least the
publicly exposed properties are the same after serialization/deserialization.

https://codereview.chromium.org/1512823002/diff/20001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:80: DetermineIfSolidColor();
Note to self: Combine this with the calls in UpdateAndExpandInvalidation() and
unify this to one call.

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

https://codereview.chromium.org/1512823002/diff/20001/cc/playback/display_lis...
cc/playback/display_list_recording_source_unittest.cc:42:
scoped_ptr<FakeDisplayListRecordingSource> CreateRecordingSource(
These are moved from the existing CreateRecordingSource,
CreateFilledRecordingSource, and CreateRasterSource helper methods that used to
exist outside the class.  I just added the if(test_serialization_) blocks.

https://codereview.chromium.org/1512823002/diff/20001/cc/playback/display_lis...
cc/playback/display_list_recording_source_unittest.cc:89: EXPECT_TRUE(image_a ==
image_b);
This is how we used to validate that images were equal.  I'm keeping this for
the non-serialized version, but added a deep copy for the serialized version as
new images are created.

https://codereview.chromium.org/1512823002/diff/20001/cc/playback/display_lis...
cc/playback/display_list_recording_source_unittest.cc:127: void StartTest()
override {
All of these tests should be roughly the same as the other counterparts.  I
changed how we validated images and moved all of the recording/raster source
creation calls to the helper methods.

https://codereview.chromium.org/1512823002/diff/20001/cc/playback/display_lis...
cc/playback/display_list_recording_source_unittest.cc:289: class
DisplayListRecordingSourceTestNoDiscardableImages
Failing in serialized mode.  Investigating.

https://codereview.chromium.org/1512823002/diff/20001/cc/playback/display_lis...
cc/playback/display_list_recording_source_unittest.cc:428: class
DisplayListRecordingSourceTestDiscardableImagesBaseNonDiscardable
Failing in serialized mode.  Investigating.

[David Trainor- moved to gerrit, 2015-12-10 00:32:51]

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

[David Trainor- moved to gerrit, 2015-12-10 00:32:51]

ptal thanks!

[vmpstr, 2015-12-10 01:12:19]

https://codereview.chromium.org/1512823002/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.cc (left):

https://codereview.chromium.org/1512823002/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.cc:330: 
Undo this change please :)

https://codereview.chromium.org/1512823002/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1512823002/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.cc:214: // DCHECK_EQ(items_.size(),
visual_rects_.size());
FYI, when this DCHECK gets uncommented, then this code will break. I'm not sure
what we should be doing here.

Here's what happens right now (and by now I mean in the near future):

We build up display item list, which builds up the items and visual_rects. In
finalize, we use the visual rects to build an rtree and clear the visual rects.
Then, at raster time we can use the rtree for the indices that index into items.

At what point do you expect that this list will be ToProtof-ed? We either need
the visual rects or the rtree serialized, since that's important for raster
performance. That is, the client side can't reconstruct these rects.

(this is also possibly my bad that visual rects were added without the
serialization code being updated).

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

https://codereview.chromium.org/1512823002/diff/60001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:80: FinalizeDisplayItemList();
I would call this something different not to confuse it with
display_list->Finalize()

https://codereview.chromium.org/1512823002/diff/60001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:99:
display_list_->EmitTraceSnapshot();
Is tracing expected to work on the client side the same way it does now?

https://codereview.chromium.org/1512823002/diff/60001/cc/proto/display_list_r...
File cc/proto/display_list_recording_source.proto (right):

https://codereview.chromium.org/1512823002/diff/60001/cc/proto/display_list_r...
cc/proto/display_list_recording_source.proto:17: // required
Skip these comments.. Maybe just say a blanket "all fields are required by the
protocol, but are optional in order for it to be able to change." Or maybe just
don't comment.

https://codereview.chromium.org/1512823002/diff/60001/cc/test/fake_display_li...
File cc/test/fake_display_list_recording_source.h (right):

https://codereview.chromium.org/1512823002/diff/60001/cc/test/fake_display_li...
cc/test/fake_display_list_recording_source.h:129: bool EqualsForTesting(const
FakeDisplayListRecordingSource& other);
For Fake* classes, you don't need ForTesting.. Just say EqualsTo

https://codereview.chromium.org/1512823002/diff/60001/cc/test/skia_common.cc
File cc/test/skia_common.cc (right):

https://codereview.chromium.org/1512823002/diff/60001/cc/test/skia_common.cc#...
cc/test/skia_common.cc:50: return 0 == memcmp(pixels_a.get(), pixels_b.get(),
pixel_size);
return !memcmp(...);

[David Trainor- moved to gerrit, 2015-12-10 17:10:10]

Thanks.  addressed comments.

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/playback/disp...
File cc/playback/display_item_list.cc (left):

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/playback/disp...
cc/playback/display_item_list.cc:330: 
On 2015/12/10 01:12:18, vmpstr wrote:
> Undo this change please :)

Oops.

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/playback/disp...
File cc/playback/display_item_list.cc (right):

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/playback/disp...
cc/playback/display_item_list.cc:214: // DCHECK_EQ(items_.size(),
visual_rects_.size());
On 2015/12/10 01:12:18, vmpstr wrote:
> FYI, when this DCHECK gets uncommented, then this code will break. I'm not
sure
> what we should be doing here.
> 
> Here's what happens right now (and by now I mean in the near future):
> 
> We build up display item list, which builds up the items and visual_rects. In
> finalize, we use the visual rects to build an rtree and clear the visual
rects.
> Then, at raster time we can use the rtree for the indices that index into
items.
> 
> At what point do you expect that this list will be ToProtof-ed? We either need
> the visual rects or the rtree serialized, since that's important for raster
> performance. That is, the client side can't reconstruct these rects.
> 
> (this is also possibly my bad that visual rects were added without the
> serialization code being updated).

We protobuf it during the commit phase and don't actually do any
PushPropertiesTo call on the layer in the engine (although we'll simulate what
we have to after the serialization to keep state working properly).

What would be easiest for us to send and cache on the client?  Ideally we'll
start caching DisplayItems on the client and only sending new ones.  I'll take a
look at this in a follow up patch today/tomorrow.

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

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/playback/disp...
cc/playback/display_list_recording_source.cc:80: FinalizeDisplayItemList();
On 2015/12/10 01:12:19, vmpstr wrote:
> I would call this something different not to confuse it with
> display_list->Finalize()

Having trouble with a name... came up with FinishDisplayItemListUpdate().  Let
me know if you're good with that.

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/playback/disp...
cc/playback/display_list_recording_source.cc:99:
display_list_->EmitTraceSnapshot();
On 2015/12/10 01:12:18, vmpstr wrote:
> Is tracing expected to work on the client side the same way it does now?

I actually need to investigate that.  I don't know.  But I'm assuming the client
will hit a lot of other tracing call sites throughout the compositor, so it
might be best to make sure we call the right methods at the right time and
disable it explicitly somewhere else?

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/proto/display...
File cc/proto/display_list_recording_source.proto (right):

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/proto/display...
cc/proto/display_list_recording_source.proto:17: // required
On 2015/12/10 01:12:19, vmpstr wrote:
> Skip these comments.. Maybe just say a blanket "all fields are required by the
> protocol, but are optional in order for it to be able to change." Or maybe
just
> don't comment.

Done.

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/test/fake_dis...
File cc/test/fake_display_list_recording_source.h (right):

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/test/fake_dis...
cc/test/fake_display_list_recording_source.h:129: bool EqualsForTesting(const
FakeDisplayListRecordingSource& other);
On 2015/12/10 01:12:19, vmpstr wrote:
> For Fake* classes, you don't need ForTesting.. Just say EqualsTo

Done.

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/test/skia_com...
File cc/test/skia_common.cc (right):

https://chromiumcodereview.appspot.com/1512823002/diff/60001/cc/test/skia_com...
cc/test/skia_common.cc:50: return 0 == memcmp(pixels_a.get(), pixels_b.get(),
pixel_size);
On 2015/12/10 01:12:19, vmpstr wrote:
> return !memcmp(...);

Done.

[vmpstr, 2015-12-10 18:55:44]

https://codereview.chromium.org/1512823002/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1512823002/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.cc:214: // DCHECK_EQ(items_.size(),
visual_rects_.size());
On 2015/12/10 17:10:09, David Trainor wrote:
> On 2015/12/10 01:12:18, vmpstr wrote:
> > FYI, when this DCHECK gets uncommented, then this code will break. I'm not
> sure
> > what we should be doing here.
> > 
> > Here's what happens right now (and by now I mean in the near future):
> > 
> > We build up display item list, which builds up the items and visual_rects.
In
> > finalize, we use the visual rects to build an rtree and clear the visual
> rects.
> > Then, at raster time we can use the rtree for the indices that index into
> items.
> > 
> > At what point do you expect that this list will be ToProtof-ed? We either
need
> > the visual rects or the rtree serialized, since that's important for raster
> > performance. That is, the client side can't reconstruct these rects.
> > 
> > (this is also possibly my bad that visual rects were added without the
> > serialization code being updated).
> 
> We protobuf it during the commit phase and don't actually do any
> PushPropertiesTo call on the layer in the engine (although we'll simulate what
> we have to after the serialization to keep state working properly).
> 
> What would be easiest for us to send and cache on the client?  Ideally we'll
> start caching DisplayItems on the client and only sending new ones.  I'll take
a
> look at this in a follow up patch today/tomorrow.

We need both the display list and visual rects on the client. The problem is
that when we call finalize, the visual rects are converted into an rtree. That's
why it's not clear to me if we serialize to protobuf before finalize or after
finalize? I image it's after. If that's the case we have two options:

1. Save off visual rects before finalize and serialize those
2. Serialize the rtree post the fact.

I would prefer 1... 

Can you file a bug and add a TODO probably around this line that says that we
have to deal with serialization of visual rects.

https://codereview.chromium.org/1512823002/diff/80001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/1512823002/diff/80001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:88:
CompareDisplayListDrawingResults(gfx::Rect(layer_size), list, new_list));
naming nit: If this is going to return true maybe consider
"AreDisplayListDrawingResultsSame" or something like that

https://codereview.chromium.org/1512823002/diff/80001/cc/playback/display_lis...
File cc/playback/display_list_recording_source_unittest.cc (right):

https://codereview.chromium.org/1512823002/diff/80001/cc/playback/display_lis...
cc/playback/display_list_recording_source_unittest.cc:34: void
ValidateRecordingSourceRoundTripSerialization(
nit: s/RoundTrip//

[David Trainor- moved to gerrit, 2015-12-10 21:51:27]

done! :)

https://chromiumcodereview.appspot.com/1512823002/diff/80001/cc/playback/disp...
File cc/playback/display_item_list_unittest.cc (right):

https://chromiumcodereview.appspot.com/1512823002/diff/80001/cc/playback/disp...
cc/playback/display_item_list_unittest.cc:88:
CompareDisplayListDrawingResults(gfx::Rect(layer_size), list, new_list));
On 2015/12/10 18:55:44, vmpstr wrote:
> naming nit: If this is going to return true maybe consider
> "AreDisplayListDrawingResultsSame" or something like that

Exactly the same length.  Wut.

https://chromiumcodereview.appspot.com/1512823002/diff/80001/cc/playback/disp...
File cc/playback/display_list_recording_source_unittest.cc (right):

https://chromiumcodereview.appspot.com/1512823002/diff/80001/cc/playback/disp...
cc/playback/display_list_recording_source_unittest.cc:34: void
ValidateRecordingSourceRoundTripSerialization(
On 2015/12/10 18:55:44, vmpstr wrote:
> nit: s/RoundTrip//

Done.

[vmpstr, 2015-12-10 21:55:50]

lgtm

https://chromiumcodereview.appspot.com/1512823002/diff/80001/cc/playback/disp...
File cc/playback/display_item_list_unittest.cc (right):

https://chromiumcodereview.appspot.com/1512823002/diff/80001/cc/playback/disp...
cc/playback/display_item_list_unittest.cc:88:
CompareDisplayListDrawingResults(gfx::Rect(layer_size), list, new_list));
On 2015/12/10 21:51:27, David Trainor wrote:
> On 2015/12/10 18:55:44, vmpstr wrote:
> > naming nit: If this is going to return true maybe consider
> > "AreDisplayListDrawingResultsSame" or something like that
> 
> Exactly the same length.  Wut.

Skillz.

[David Trainor- moved to gerrit, 2015-12-11 00:34:59]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-12-11 00:35:00]

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/1512823002/#ps140001 (title: "Rebased")

[commit-bot: I haz the power, 2015-12-11 00:37:03]

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

[commit-bot: I haz the power, 2015-12-11 00:52:35]

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

[commit-bot: I haz the power, 2015-12-11 00:52:36]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Trainor- moved to gerrit, 2015-12-11 16:04:19]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2015-12-11 16:04:50]

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

[commit-bot: I haz the power, 2015-12-11 16:20:26]

Description was changed from

==========
Serialize DisplayListRecordingSource to protos.

Add protobuf serialization to DisplayListRecordingSource.  Also refactor
the unittests to run all tests in both serialization and
non-serialization mode.

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

to

==========
Serialize DisplayListRecordingSource to protos.

Add protobuf serialization to DisplayListRecordingSource.  Also refactor
the unittests to run all tests in both serialization and
non-serialization mode.

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

[commit-bot: I haz the power, 2015-12-11 16:20:27]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-11 16:21:09]

Description was changed from

==========
Serialize DisplayListRecordingSource to protos.

Add protobuf serialization to DisplayListRecordingSource.  Also refactor
the unittests to run all tests in both serialization and
non-serialization mode.

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

to

==========
Serialize DisplayListRecordingSource to protos.

Add protobuf serialization to DisplayListRecordingSource.  Also refactor
the unittests to run all tests in both serialization and
non-serialization mode.

BUG=541321
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2015-12-11 16:21:10]

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