Implement CompositorFrame Struct Traits

Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL
deletes surfaces type converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to
ParamTraits.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=ben@chromium.org for content/renderer/mus
BUG=611802
Committed: https://crrev.com/ce098fb67017c4afe44b6c23664723a0a057b737
Cr-Commit-Position: refs/heads/master@{#402199}

[Fady Samuel, 2016-06-23 18:29:45]

Description was changed from

==========
Implement CompositorFrame Struct Traits
==========

to

==========
Implement CompositorFrame Struct Traits
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-06-23 19:06:42]

Description was changed from

==========
Implement CompositorFrame Struct Traits
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL deletes surfaces type
converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

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

[Fady Samuel, 2016-06-23 19:07:02]

Description was changed from

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL deletes surfaces type
converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

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

to

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL deletes surfaces type
converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

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

[Fady Samuel, 2016-06-23 19:07:29]

Description was changed from

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL deletes surfaces type
converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

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

to

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL 
deletes surfaces type converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

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

[Fady Samuel, 2016-06-23 19:07:37]

fsamuel@chromium.org changed reviewers:
+ rjkroege@chromium.org

[rjkroege, 2016-06-23 19:13:56]

lgtm

https://codereview.chromium.org/2092843002/diff/20001/cc/ipc/compositor_frame...
File cc/ipc/compositor_frame.typemap (right):

https://codereview.chromium.org/2092843002/diff/20001/cc/ipc/compositor_frame...
cc/ipc/compositor_frame.typemap:11: type_mappings = [
"cc.mojom.CompositorFrame=std::unique_ptr<cc::CompositorFrame>[pass_by_value]" ]
This pass by value thing is new. What does it do?

https://codereview.chromium.org/2092843002/diff/20001/components/mus/public/c...
File components/mus/public/cpp/window_surface.h (right):

https://codereview.chromium.org/2092843002/diff/20001/components/mus/public/c...
components/mus/public/cpp/window_surface.h:38: void
SubmitCompositorFrame(std::unique_ptr<cc::CompositorFrame> frame,
my preference would be to not have client code unless absolutely necessary
because it makes things more tedious for use from other languages. Are you
adverse to removing the client lib eventually?

[Fady Samuel, 2016-06-25 04:31:11]

fsamuel@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org, sky@chromium.org

[Fady Samuel, 2016-06-25 04:31:12]

+dcheng@ for security
+danakj@ for cc
+sky@ for mus and ui

Hooray! surfaces type converters are GONE!

https://codereview.chromium.org/2092843002/diff/20001/cc/ipc/compositor_frame...
File cc/ipc/compositor_frame.typemap (right):

https://codereview.chromium.org/2092843002/diff/20001/cc/ipc/compositor_frame...
cc/ipc/compositor_frame.typemap:11: type_mappings = [
"cc.mojom.CompositorFrame=std::unique_ptr<cc::CompositorFrame>[pass_by_value]" ]
On 2016/06/23 19:13:55, rjkroege wrote:
> This pass by value thing is new. What does it do?

Normally mojo will pass things around by const reference, but you don't want to
pass around unique_ptrs by reference you want them to be uniquely owned and thus
passed by value.

https://codereview.chromium.org/2092843002/diff/20001/components/mus/public/c...
File components/mus/public/cpp/window_surface.h (right):

https://codereview.chromium.org/2092843002/diff/20001/components/mus/public/c...
components/mus/public/cpp/window_surface.h:38: void
SubmitCompositorFrame(std::unique_ptr<cc::CompositorFrame> frame,
On 2016/06/23 19:13:55, rjkroege wrote:
> my preference would be to not have client code unless absolutely necessary
> because it makes things more tedious for use from other languages. Are you
> adverse to removing the client lib eventually?

This is a fairly thin wrapper around mojo interfaces. I didn't intend to remove
this any time soon but we can discuss further offline.

[Fady Samuel, 2016-06-25 05:07:56]

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

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

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-06-25 06:10:20]

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

[commit-bot: I haz the power, 2016-06-25 06:10:21]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-06-27 02:05:00]

https://codereview.chromium.org/2092843002/diff/110001/cc/ipc/compositor_fram...
File cc/ipc/compositor_frame_struct_traits.h (right):

https://codereview.chromium.org/2092843002/diff/110001/cc/ipc/compositor_fram...
cc/ipc/compositor_frame_struct_traits.h:31: if
(!data.ReadMetadata(&out->metadata))
Let's move this to a .cc, since it's non-trivial.

https://codereview.chromium.org/2092843002/diff/110001/components/mus/gpu/dis...
File components/mus/gpu/display_compositor/compositor_frame_sink_impl.cc
(right):

https://codereview.chromium.org/2092843002/diff/110001/components/mus/gpu/dis...
components/mus/gpu/display_compositor/compositor_frame_sink_impl.cc:73:
*compositor_frame_copy = std::move(compositor_frame);
Is it possible to skip the intermediate and just move once in the call below?

Similar question below.

[Fady Samuel, 2016-06-27 03:05:10]

PTAL Daniel et al! Thanks!

https://codereview.chromium.org/2092843002/diff/110001/cc/ipc/compositor_fram...
File cc/ipc/compositor_frame_struct_traits.h (right):

https://codereview.chromium.org/2092843002/diff/110001/cc/ipc/compositor_fram...
cc/ipc/compositor_frame_struct_traits.h:31: if
(!data.ReadMetadata(&out->metadata))
On 2016/06/27 02:05:00, dcheng wrote:
> Let's move this to a .cc, since it's non-trivial.

Done. I ended up also pulling out non-trivial Read methods for
CompositorFrameMetadata and TransferableResource to cc files to make the include
list in the cc sane.

https://codereview.chromium.org/2092843002/diff/110001/components/mus/gpu/dis...
File components/mus/gpu/display_compositor/compositor_frame_sink_impl.cc
(right):

https://codereview.chromium.org/2092843002/diff/110001/components/mus/gpu/dis...
components/mus/gpu/display_compositor/compositor_frame_sink_impl.cc:73:
*compositor_frame_copy = std::move(compositor_frame);
On 2016/06/27 02:05:00, dcheng wrote:
> Is it possible to skip the intermediate and just move once in the call below?
> 
> Similar question below.


Yes, it is possible. I am doing that cleanup here:
https://codereview.chromium.org/2098953003/

I will land either this patch or the other first. Depending on which lands first
one will get a rebase.

[Fady Samuel, 2016-06-27 03:09:13]

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

[commit-bot: I haz the power, 2016-06-27 03:09:23]

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

[dcheng, 2016-06-27 03:41:57]

mojom lgtm

[commit-bot: I haz the power, 2016-06-27 04:07:02]

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

[commit-bot: I haz the power, 2016-06-27 04:07:03]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-06-27 15:34:51]

fsamuel@chromium.org changed reviewers:
+ ajuma@chromium.org
- danakj@chromium.org

[Fady Samuel, 2016-06-27 15:34:52]

+ajuma@
-danakj@ in the interest of load balancing reviewers  Feel free to chime in and
I'll make changes.

[ajuma, 2016-06-27 15:44:03]

cc lgtm

[sky, 2016-06-27 16:15:38]

LGTM

[Fady Samuel, 2016-06-27 16:20:41]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-27 16:20:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/2092843002/#ps130001
(title: "Addressed dcheng's comments")

[commit-bot: I haz the power, 2016-06-27 16:21:11]

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-06-27 16:29:34]

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

[commit-bot: I haz the power, 2016-06-27 16:29:36]

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

[Fady Samuel, 2016-06-27 16:37:02]

Description was changed from

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL 
deletes surfaces type converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

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

to

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL 
deletes surfaces type converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=ben@chromium.org for content/renderer/mus
BUG=611802
==========

[Fady Samuel, 2016-06-27 16:37:30]

fsamuel@chromium.org changed reviewers:
+ ben@chromium.org

[Fady Samuel, 2016-06-27 16:37:31]

+ben@ I'm TBR'ing you for a trivial change to content/renderer/mus Thanks!

[Fady Samuel, 2016-06-27 16:37:41]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-06-27 16:38:07]

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-06-27 16:43:22]

Description was changed from

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL 
deletes surfaces type converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=ben@chromium.org for content/renderer/mus
BUG=611802
==========

to

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL 
deletes surfaces type converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=ben@chromium.org for content/renderer/mus
BUG=611802
==========

[commit-bot: I haz the power, 2016-06-27 16:43:24]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-06-27 16:45:04]

Description was changed from

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL 
deletes surfaces type converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to 
ParamTraits.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=ben@chromium.org for content/renderer/mus
BUG=611802
==========

to

==========
Implement CompositorFrame StructTraits

This CL implements CompositorFrame StructTraits. This CL
deletes surfaces type converters, and updates call sites.

In a subsequent CL, I'll add a perf test comparable to
ParamTraits.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=ben@chromium.org for content/renderer/mus
BUG=611802

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

[commit-bot: I haz the power, 2016-06-27 16:45:05]

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