Make cc::CompositorFrames movable [Part 1 of 2]

Make cc::CompositorFrames movable [Part 1 of 2]

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping
CompositorFrames over mojo. CompositorFrames are now
movable. SurfaceFactory still uses a unique_ptr and so we
create a new CompositorFrame, and move into that to pass to
SurfaceFactory in this CL. In a subsequent CL,
SurfaceFactory holds a CompositorFrame by value.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[Fady Samuel, 2016-06-22 23:41:29]

Description was changed from

==========
Always pass cc::CompositorFrames through std::unique_ptrs

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping CompositorFrames
over mojo.

BUG=611802
==========

to

==========
Always pass cc::CompositorFrames through std::unique_ptrs

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping CompositorFrames
over mojo.

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

[Fady Samuel, 2016-06-22 23:47:16]

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

[Fady Samuel, 2016-06-22 23:47:16]

PTAL Dana! RenderWidgetHostImpl::OnSwapCompositorFrame still looks funky to me
but I didn't want this CL to get too large and non-trivial so I'll poke at it in
a separate CL. Hopefully this is more sane than the previous code we had.

[Fady Samuel, 2016-06-22 23:47:23]

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

[commit-bot: I haz the power, 2016-06-22 23:48:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/20001

[commit-bot: I haz the power, 2016-06-22 23:51:10]

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

[commit-bot: I haz the power, 2016-06-22 23:51:11]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Fady Samuel, 2016-06-23 03:28:10]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/40001

[commit-bot: I haz the power, 2016-06-23 03:38:40]

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

[commit-bot: I haz the power, 2016-06-23 03:38:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Fady Samuel, 2016-06-23 03:49:49]

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

[commit-bot: I haz the power, 2016-06-23 03:50:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/60001

[commit-bot: I haz the power, 2016-06-23 04:02:18]

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

[commit-bot: I haz the power, 2016-06-23 04:02:19]

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

[Fady Samuel, 2016-06-23 04:22:11]

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

[commit-bot: I haz the power, 2016-06-23 04:22:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/80001

[commit-bot: I haz the power, 2016-06-23 04:32:27]

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

[commit-bot: I haz the power, 2016-06-23 04:32:28]

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

[Fady Samuel, 2016-06-23 04:47:29]

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

[commit-bot: I haz the power, 2016-06-23 04:47:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/100001

[commit-bot: I haz the power, 2016-06-23 05:04:34]

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

[commit-bot: I haz the power, 2016-06-23 05:04:35]

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

[Fady Samuel, 2016-06-23 10:55:10]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/120001

[commit-bot: I haz the power, 2016-06-23 11:16:54]

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

[commit-bot: I haz the power, 2016-06-23 11:16:55]

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

[Fady Samuel, 2016-06-23 12:25:35]

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

[commit-bot: I haz the power, 2016-06-23 12:25:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/140001

[Fady Samuel, 2016-06-23 12:45:00]

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

[commit-bot: I haz the power, 2016-06-23 12:45:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/160001

[Fady Samuel, 2016-06-23 13:12:24]

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

[commit-bot: I haz the power, 2016-06-23 13:12:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/180001

[Fady Samuel, 2016-06-23 13:54:30]

fsamuel@chromium.org changed reviewers:
+ boliu@chromium.org, dcheng@chromium.org, piman@chromium.org, sky@chromium.org,
wez@chromium.org

[Fady Samuel, 2016-06-23 13:54:31]

+piman@ for content (once Dana is happy)
+sky@ for mus
+boliu@ for webview
+wez@ for blimp
+dcheng@ for ipc

[commit-bot: I haz the power, 2016-06-23 14:02:11]

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

[commit-bot: I haz the power, 2016-06-23 14:02:12]

Dry run: This issue passed the CQ dry run.

[sky, 2016-06-23 15:54:47]

mus LGTM

[Wez, 2016-06-23 17:30:35]

blimp/ LGTM

[dcheng, 2016-06-23 18:39:40]

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:677: void
ParamTraits<std::unique_ptr<cc::CompositorFrame>>::Write(
Shouldn't the existing ParamTraits partial specialization for std::unique_ptr<T>
handle this already?

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

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:677: void
ParamTraits<std::unique_ptr<cc::CompositorFrame>>::Write(
On 2016/06/23 18:39:40, dcheng wrote:
> Shouldn't the existing ParamTraits partial specialization for
std::unique_ptr<T>
> handle this already?

One exists but it doesn't do what I want. In particular, I don't want to call
"new" directly. The only way to create a CompositorFrame in this patch is
through CompositorFrame::Create() which returns a
unique_ptr<cc::CompositorFrame>.

[dcheng, 2016-06-23 19:16:37]

lgtm

[piman, 2016-06-23 19:30:32]

The original reason for this is that pre-c++11 we didn't have movable types.
CompositorFrame can be quite big (lists of quads) and we wanted to make sure not
to have to make unnecessary copies.

My suggestion would have been to make CompositorFrame a movable type instead to
restore sanity. It has virtually the same semantics, but without the extra
indirection / memory allocation. WDYT?

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
File cc/output/compositor_frame.h (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
cc/output/compositor_frame.h:31: protected:
How about private? I don't think we want to have classes derive from this.

[danakj, 2016-06-23 20:34:42]

I think Antoine's right and using a move-only type would be nice.

IPCing a unique_ptr is weird, also, as that ties the usage of the type to a
smartpointer/container.

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:677: void
ParamTraits<std::unique_ptr<cc::CompositorFrame>>::Write(
On 2016/06/23 18:47:29, Fady Samuel wrote:
> On 2016/06/23 18:39:40, dcheng wrote:
> > Shouldn't the existing ParamTraits partial specialization for
> std::unique_ptr<T>
> > handle this already?
> 
> One exists but it doesn't do what I want. In particular, I don't want to call
> "new" directly. The only way to create a CompositorFrame in this patch is
> through CompositorFrame::Create() which returns a
> unique_ptr<cc::CompositorFrame>.

I want to remove all Create() methods that are not needed in cc/. So I don't
think you need this.

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits.h
File cc/ipc/cc_param_traits.h (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.h:86: struct CC_IPC_EXPORT
ParamTraits<cc::CompositorFrame> {
delete this one then?

https://codereview.chromium.org/2096493002/diff/180001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.h:96: struct CC_IPC_EXPORT
ParamTraits<std::unique_ptr<cc::CompositorFrame>> {
its a bit weird tho, why do you need to do it this way instead of create a
compositor frame and read into it?

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
File cc/output/compositor_frame.cc (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
cc/output/compositor_frame.cc:12: return base::WrapUnique(new
CompositorFrame());
this would have been better done as base::MakeUnique.

and you don't need the () after the type name when you write new.

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
File cc/output/compositor_frame.h (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
cc/output/compositor_frame.h:25: static std::unique_ptr<CompositorFrame>
Create();
Please just leave the constructor public and drop the Create().

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
cc/output/compositor_frame.h:31: protected:
On 2016/06/23 19:30:32, piman wrote:
> How about private? I don't think we want to have classes derive from this.

I'd rather just not have a Create() method at all.

https://codereview.chromium.org/2096493002/diff/180001/cc/test/fake_output_su...
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/test/fake_output_su...
cc/test/fake_output_surface.h:173: std::unique_ptr<CompositorFrame>
last_sent_frame_;
even if you make CompositorFrame moveonly and passed around by value, using a
unique_ptr here is nice to denote the "no frame sent yet" state, i like the
changes to tests that came from this.

https://codereview.chromium.org/2096493002/diff/180001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/2096493002/diff/180001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:153:
std::unique_ptr<cc::CompositorFrame> frame(cc::CompositorFrame::Create());
just allocate this on the stack, since ownership never leaves

https://codereview.chromium.org/2096493002/diff/180001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:216:
std::unique_ptr<cc::CompositorFrame> frame(cc::CompositorFrame::Create());
ditto

https://codereview.chromium.org/2096493002/diff/180001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/2096493002/diff/180001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:182:
DCHECK(frame.get());
no .get() needed. unique_ptr is testable

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

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

[commit-bot: I haz the power, 2016-06-23 23:38:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/220001

[Fady Samuel, 2016-06-23 23:39:38]

Description was changed from

==========
Always pass cc::CompositorFrames through std::unique_ptrs

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping CompositorFrames
over mojo.

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

to

==========
Make cc::CompositorFrames movable [Part 1 of 2]

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping CompositorFrames
over mojo. CompositorFrames are now movable. SurfaceFactory
still uses a unique_ptr and so we create a new CompositorFrame, and move into
that to pass to SurfaceFactory in this CL. In a subsequent CL, SurfaceFactory
holds a CompositorFrame by value.

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

[Fady Samuel, 2016-06-23 23:40:05]

Description was changed from

==========
Make cc::CompositorFrames movable [Part 1 of 2]

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping CompositorFrames
over mojo. CompositorFrames are now movable. SurfaceFactory
still uses a unique_ptr and so we create a new CompositorFrame, and move into
that to pass to SurfaceFactory in this CL. In a subsequent CL, SurfaceFactory
holds a CompositorFrame by value.

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

to

==========
Make cc::CompositorFrames movable [Part 1 of 2]

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping 
CompositorFrames over mojo. CompositorFrames are now 
movable. SurfaceFactory still uses a unique_ptr and so we 
create a new CompositorFrame, and move into that to pass to 
SurfaceFactory in this CL. In a subsequent CL, 
SurfaceFactory holds a CompositorFrame by value.

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

[Fady Samuel, 2016-06-23 23:41:00]

PTAL everyone. We've decided to make cc::CompositorFrame movable instead of
using unique_ptrs to move it around. This is CL 1 of 2. The SurfaceFactory
boundary still uses unique_ptr. That will be fixed in the next CL.

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
File cc/output/compositor_frame.h (right):

https://codereview.chromium.org/2096493002/diff/180001/cc/output/compositor_f...
cc/output/compositor_frame.h:31: protected:
On 2016/06/23 19:30:32, piman wrote:
> How about private? I don't think we want to have classes derive from this.

Done.

[Fady Samuel, 2016-06-23 23:48:35]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/240001

[Wez, 2016-06-24 00:03:07]

Are we sure that making cc::CompositorFrame movable is the right approach here?
It contains CompositorFrameMetadata, which seems to be a non-trivially large
struct, and will end up being copied at the various call-sites, right?

[Fady Samuel, 2016-06-24 00:06:31]

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

[commit-bot: I haz the power, 2016-06-24 00:07:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/260001

[piman, 2016-06-24 00:07:50]

lgtm

[piman, 2016-06-24 00:10:11]

On 2016/06/24 00:03:07, Wez wrote:
> Are we sure that making cc::CompositorFrame movable is the right approach
here?
> It contains CompositorFrameMetadata, which seems to be a non-trivially large
> struct, and will end up being copied at the various call-sites, right?

Oh, we should make it movable too. I missed that it wasn't in this patch.

Allocations+indirections feel more costly than copying a handful of fields.

[Wez, 2016-06-24 00:25:44]

On 2016/06/24 00:10:11, piman wrote:
> On 2016/06/24 00:03:07, Wez wrote:
> > Are we sure that making cc::CompositorFrame movable is the right approach
> here?
> > It contains CompositorFrameMetadata, which seems to be a non-trivially large
> > struct, and will end up being copied at the various call-sites, right?
> 
> Oh, we should make it movable too. I missed that it wasn't in this patch.
> 
> Allocations+indirections feel more costly than copying a handful of fields.

OK, if cc folks are happy w/ this approach then blimp/ LGTM

[Fady Samuel, 2016-06-24 00:48:33]

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

[Fady Samuel, 2016-06-24 00:49:27]

PTAL Antoine! PTAL boliu@ Thanks!

[commit-bot: I haz the power, 2016-06-24 00:49:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/280001

[piman, 2016-06-24 00:56:25]

lgtm

https://codereview.chromium.org/2096493002/diff/280001/cc/output/delegating_r...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/2096493002/diff/280001/cc/output/delegating_r...
cc/output/delegating_renderer.cc:102: void DelegatingRenderer::SwapBuffers(const
CompositorFrameMetadata& metadata) {
nit: can we make this take a CompositorFrameMetadata (non-ref), and have the
callers use std::move? Maybe as a follow-up, this patch is getting big, but
LayerTreeHostImpl::SwapBuffers creates the metadata as a temporary, so it seems
worth it.

[Fady Samuel, 2016-06-24 01:10:43]

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

[commit-bot: I haz the power, 2016-06-24 01:11:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/300001

[Fady Samuel, 2016-06-24 01:20:34]

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

[commit-bot: I haz the power, 2016-06-24 01:21:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/320001

[commit-bot: I haz the power, 2016-06-24 01:29:54]

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

[commit-bot: I haz the power, 2016-06-24 01:29:57]

Dry run: 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_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2016-06-24 01:55:37]

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

[commit-bot: I haz the power, 2016-06-24 01:56:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2096493002/340001

[commit-bot: I haz the power, 2016-06-24 02:04:43]

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

[commit-bot: I haz the power, 2016-06-24 02:04:44]

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

[Fady Samuel, 2016-06-24 02:29:15]

So it looks like android_webview relies on unique_ptr to identify whether a
CompositorFrame exists or not in some places.

With CompositorFrames being movable instead of std::unique_ptr'ed, the check for
null is no longer possible. base::Optional ends up producing messy code. An
alternative is to always check delegated_frame_data as signal for whether the
CompositorFrame is populated or not. WDYT Dana, Antoine? Thanks!

[boliu, 2016-06-24 02:32:28]

On 2016/06/24 02:29:15, Fady Samuel wrote:
> So it looks like android_webview relies on unique_ptr to identify whether a
> CompositorFrame exists or not in some places.
> 
> With CompositorFrames being movable instead of std::unique_ptr'ed, the check
for
> null is no longer possible. base::Optional ends up producing messy code. An
> alternative is to always check delegated_frame_data as signal for whether the
> CompositorFrame is populated or not. WDYT Dana, Antoine? Thanks!

I wrote the same comment at the same time. Here's my version:

So current PS uses base::Optional<cc::CompositorFrame> for a bunch of places in
webview, because webview needs a null check in some cases.

And we found out the hard way moving an Optional doesn't null it out, so need to
do it manually.

So instead of Optional, can we make stronger guarantees about
cc::CompositorFrame after it's moved? eg guarantee that it's same as immediately
after default constructor is called? I mean the fact there is a default
constructor suggests the conceput of an empty fram exists..?

[piman, 2016-06-24 02:38:14]

On Thu, Jun 23, 2016 at 7:32 PM, <boliu@chromium.org> wrote:

> On 2016/06/24 02:29:15, Fady Samuel wrote:
> > So it looks like android_webview relies on unique_ptr to identify
> whether a
> > CompositorFrame exists or not in some places.
> >
> > With CompositorFrames being movable instead of std::unique_ptr'ed, the
> check
> for
> > null is no longer possible. base::Optional ends up producing messy code.
> An
> > alternative is to always check delegated_frame_data as signal for
> whether the
> > CompositorFrame is populated or not. WDYT Dana, Antoine? Thanks!
>
> I wrote the same comment at the same time. Here's my version:
>
> So current PS uses base::Optional<cc::CompositorFrame> for a bunch of
> places in
> webview, because webview needs a null check in some cases.
>
> And we found out the hard way moving an Optional doesn't null it out, so
> need to
> do it manually.
>
> So instead of Optional, can we make stronger guarantees about
> cc::CompositorFrame after it's moved? eg guarantee that it's same as
> immediately
> after default constructor is called? I mean the fact there is a default
> constructor suggests the conceput of an empty fram exists..?
>

I don't think it'd be easy to provide and maintain that guarantee - we'd
need an explicit move constructor that goes and resets the fields.
We could also use a unique_ptr only in webview code, in the places that
need it.


> https://codereview.chromium.org/2096493002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2016-06-24 02:42:57]

Could you provide an explicit operator bool() so it can be checked for
validity without assumptions about internal structure, and tie that to some
appropriate bit of the internal state (e.g. one of the unique_ptrs<>)?

On 23 June 2016 at 19:37, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Thu, Jun 23, 2016 at 7:32 PM, <boliu@chromium.org> wrote:
>
>> On 2016/06/24 02:29:15, Fady Samuel wrote:
>> > So it looks like android_webview relies on unique_ptr to identify
>> whether a
>> > CompositorFrame exists or not in some places.
>> >
>> > With CompositorFrames being movable instead of std::unique_ptr'ed, the
>> check
>> for
>> > null is no longer possible. base::Optional ends up producing messy
>> code. An
>> > alternative is to always check delegated_frame_data as signal for
>> whether the
>> > CompositorFrame is populated or not. WDYT Dana, Antoine? Thanks!
>>
>> I wrote the same comment at the same time. Here's my version:
>>
>> So current PS uses base::Optional<cc::CompositorFrame> for a bunch of
>> places in
>> webview, because webview needs a null check in some cases.
>>
>> And we found out the hard way moving an Optional doesn't null it out, so
>> need to
>> do it manually.
>>
>> So instead of Optional, can we make stronger guarantees about
>> cc::CompositorFrame after it's moved? eg guarantee that it's same as
>> immediately
>> after default constructor is called? I mean the fact there is a default
>> constructor suggests the conceput of an empty fram exists..?
>>
>
> I don't think it'd be easy to provide and maintain that guarantee - we'd
> need an explicit move constructor that goes and resets the fields.
> We could also use a unique_ptr only in webview code, in the places that
> need it.
>
>
>> https://codereview.chromium.org/2096493002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2016-06-24 02:56:59]

On Thu, Jun 23, 2016 at 7:42 PM, Wez <wez@chromium.org> wrote:

> Could you provide an explicit operator bool() so it can be checked for
> validity without assumptions about internal structure, and tie that to some
> appropriate bit of the internal state (e.g. one of the unique_ptrs<>)?
>

I think an empty frame is kinda valid, or at least I'm not sure I want to
coalesce "I didn't receive a frame" and "I received an empty frame" into
the same state. I don't think other fields would be reset after a move.
One could add a canary field to detect a move and use that as a basis for
validity, but if feels like a giant ugly workaround.

Maybe we could fix base::Optional to reset on move, though I don't know if
the c++17 version of std::optional does provide the guarantee. If it
doesn't, then maybe we shouldn't make base::Optional a superset.
The last possibility of course is to explicitly reset the base::Optional
after the std::move.

Antoine


> On 23 June 2016 at 19:37, Antoine Labour <piman@chromium.org> wrote:
>
>>
>>
>> On Thu, Jun 23, 2016 at 7:32 PM, <boliu@chromium.org> wrote:
>>
>>> On 2016/06/24 02:29:15, Fady Samuel wrote:
>>> > So it looks like android_webview relies on unique_ptr to identify
>>> whether a
>>> > CompositorFrame exists or not in some places.
>>> >
>>> > With CompositorFrames being movable instead of std::unique_ptr'ed, the
>>> check
>>> for
>>> > null is no longer possible. base::Optional ends up producing messy
>>> code. An
>>> > alternative is to always check delegated_frame_data as signal for
>>> whether the
>>> > CompositorFrame is populated or not. WDYT Dana, Antoine? Thanks!
>>>
>>> I wrote the same comment at the same time. Here's my version:
>>>
>>> So current PS uses base::Optional<cc::CompositorFrame> for a bunch of
>>> places in
>>> webview, because webview needs a null check in some cases.
>>>
>>> And we found out the hard way moving an Optional doesn't null it out, so
>>> need to
>>> do it manually.
>>>
>>> So instead of Optional, can we make stronger guarantees about
>>> cc::CompositorFrame after it's moved? eg guarantee that it's same as
>>> immediately
>>> after default constructor is called? I mean the fact there is a default
>>> constructor suggests the conceput of an empty fram exists..?
>>>
>>
>> I don't think it'd be easy to provide and maintain that guarantee - we'd
>> need an explicit move constructor that goes and resets the fields.
>> We could also use a unique_ptr only in webview code, in the places that
>> need it.
>>
>>
>>> https://codereview.chromium.org/2096493002/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2016-06-24 03:04:52]

On 2016/06/24 02:56:59, piman wrote:
> On Thu, Jun 23, 2016 at 7:42 PM, Wez <mailto:wez@chromium.org> wrote:
> 
> > Could you provide an explicit operator bool() so it can be checked for
> > validity without assumptions about internal structure, and tie that to some
> > appropriate bit of the internal state (e.g. one of the unique_ptrs<>)?
> >
> 
> I think an empty frame is kinda valid, or at least I'm not sure I want to
> coalesce "I didn't receive a frame" and "I received an empty frame" into
> the same state. I don't think other fields would be reset after a move.
> One could add a canary field to detect a move and use that as a basis for
> validity, but if feels like a giant ugly workaround.
> 
> Maybe we could fix base::Optional to reset on move, though I don't know if
> the c++17 version of std::optional does provide the guarantee. If it
> doesn't, then maybe we shouldn't make base::Optional a superset.

std::optional does not null out a moved from value:
http://en.cppreference.com/w/cpp/utility/optional/optional

Move constructor: If other contains a value, initializes the contained value as
if direct-initializing (but not direct-list-initializing) an object of type T
with the expression std::move(*other) and does not make other empty: a
moved-from optional still contains a value, but the value itself is moved from.
If other does not contain a value, constructs an object that does not contain a
value.

> The last possibility of course is to explicitly reset the base::Optional
> after the std::move.

This CL missed the reset in two places already. So I vote for sticking with
unique_ptrs for webview

> 
> Antoine
> 
> 
> > On 23 June 2016 at 19:37, Antoine Labour <mailto:piman@chromium.org> wrote:
> >
> >>
> >>
> >> On Thu, Jun 23, 2016 at 7:32 PM, <mailto:boliu@chromium.org> wrote:
> >>
> >>> On 2016/06/24 02:29:15, Fady Samuel wrote:
> >>> > So it looks like android_webview relies on unique_ptr to identify
> >>> whether a
> >>> > CompositorFrame exists or not in some places.
> >>> >
> >>> > With CompositorFrames being movable instead of std::unique_ptr'ed, the
> >>> check
> >>> for
> >>> > null is no longer possible. base::Optional ends up producing messy
> >>> code. An
> >>> > alternative is to always check delegated_frame_data as signal for
> >>> whether the
> >>> > CompositorFrame is populated or not. WDYT Dana, Antoine? Thanks!
> >>>
> >>> I wrote the same comment at the same time. Here's my version:
> >>>
> >>> So current PS uses base::Optional<cc::CompositorFrame> for a bunch of
> >>> places in
> >>> webview, because webview needs a null check in some cases.
> >>>
> >>> And we found out the hard way moving an Optional doesn't null it out, so
> >>> need to
> >>> do it manually.
> >>>
> >>> So instead of Optional, can we make stronger guarantees about
> >>> cc::CompositorFrame after it's moved? eg guarantee that it's same as
> >>> immediately
> >>> after default constructor is called? I mean the fact there is a default
> >>> constructor suggests the conceput of an empty fram exists..?
> >>>
> >>
> >> I don't think it'd be easy to provide and maintain that guarantee - we'd
> >> need an explicit move constructor that goes and resets the fields.
> >> We could also use a unique_ptr only in webview code, in the places that
> >> need it.
> >>
> >>
> >>> https://codereview.chromium.org/2096493002/
> >>>
> >>
> >>
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2016-06-24 03:16:03]

Ultimately, I'd like to port all platforms to using a mojo-IPC-based display
compositor so consistency would be ideal. In the meantime, I'm OK with giving
android webview a moved CompositorFrame in a unique_ptr to preserve semantics.
Separately, we can think about cleanups to avoid the extra copying.

On 2016/06/24 03:04:52, boliu wrote:
> On 2016/06/24 02:56:59, piman wrote:
> > On Thu, Jun 23, 2016 at 7:42 PM, Wez <mailto:wez@chromium.org> wrote:
> > 
> > > Could you provide an explicit operator bool() so it can be checked for
> > > validity without assumptions about internal structure, and tie that to
some
> > > appropriate bit of the internal state (e.g. one of the unique_ptrs<>)?
> > >
> > 
> > I think an empty frame is kinda valid, or at least I'm not sure I want to
> > coalesce "I didn't receive a frame" and "I received an empty frame" into
> > the same state. I don't think other fields would be reset after a move.
> > One could add a canary field to detect a move and use that as a basis for
> > validity, but if feels like a giant ugly workaround.
> > 
> > Maybe we could fix base::Optional to reset on move, though I don't know if
> > the c++17 version of std::optional does provide the guarantee. If it
> > doesn't, then maybe we shouldn't make base::Optional a superset.
> 
> std::optional does not null out a moved from value:
> http://en.cppreference.com/w/cpp/utility/optional/optional
> 
> Move constructor: If other contains a value, initializes the contained value
as
> if direct-initializing (but not direct-list-initializing) an object of type T
> with the expression std::move(*other) and does not make other empty: a
> moved-from optional still contains a value, but the value itself is moved
from.
> If other does not contain a value, constructs an object that does not contain
a
> value.
> 
> > The last possibility of course is to explicitly reset the base::Optional
> > after the std::move.
> 
> This CL missed the reset in two places already. So I vote for sticking with
> unique_ptrs for webview
> 
> > 
> > Antoine
> > 
> > 
> > > On 23 June 2016 at 19:37, Antoine Labour <mailto:piman@chromium.org>
wrote:
> > >
> > >>
> > >>
> > >> On Thu, Jun 23, 2016 at 7:32 PM, <mailto:boliu@chromium.org> wrote:
> > >>
> > >>> On 2016/06/24 02:29:15, Fady Samuel wrote:
> > >>> > So it looks like android_webview relies on unique_ptr to identify
> > >>> whether a
> > >>> > CompositorFrame exists or not in some places.
> > >>> >
> > >>> > With CompositorFrames being movable instead of std::unique_ptr'ed, the
> > >>> check
> > >>> for
> > >>> > null is no longer possible. base::Optional ends up producing messy
> > >>> code. An
> > >>> > alternative is to always check delegated_frame_data as signal for
> > >>> whether the
> > >>> > CompositorFrame is populated or not. WDYT Dana, Antoine? Thanks!
> > >>>
> > >>> I wrote the same comment at the same time. Here's my version:
> > >>>
> > >>> So current PS uses base::Optional<cc::CompositorFrame> for a bunch of
> > >>> places in
> > >>> webview, because webview needs a null check in some cases.
> > >>>
> > >>> And we found out the hard way moving an Optional doesn't null it out, so
> > >>> need to
> > >>> do it manually.
> > >>>
> > >>> So instead of Optional, can we make stronger guarantees about
> > >>> cc::CompositorFrame after it's moved? eg guarantee that it's same as
> > >>> immediately
> > >>> after default constructor is called? I mean the fact there is a default
> > >>> constructor suggests the conceput of an empty fram exists..?
> > >>>
> > >>
> > >> I don't think it'd be easy to provide and maintain that guarantee - we'd
> > >> need an explicit move constructor that goes and resets the fields.
> > >> We could also use a unique_ptr only in webview code, in the places that
> > >> need it.
> > >>
> > >>
> > >>> https://codereview.chromium.org/2096493002/
> > >>>
> > >>
> > >>
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2016-06-24 17:52:20]

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

[Fady Samuel, 2016-06-24 17:52:48]

Hey Bo! I've simplified android_webview code significantly based on our
conversations yesterday. Could you please take a look? Thanks!

[commit-bot: I haz the power, 2016-06-24 17:53:03]

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

[boliu, 2016-06-24 18:13:14]

still need to revert changes to test_synchronous_compositor_android.h/cc to make
webview unittests compile

lgtm after that

[commit-bot: I haz the power, 2016-06-24 18:17:00]

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

[commit-bot: I haz the power, 2016-06-24 18:17:01]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Fady Samuel, 2016-06-24 18:24:30]

On 2016/06/24 18:13:14, boliu wrote:
> still need to revert changes to test_synchronous_compositor_android.h/cc to
make
> webview unittests compile
> 
> lgtm after that

Fixed. I'm CQ'ing now. Please yell at me if you think I shouldn't land this!

[Fady Samuel, 2016-06-24 18:24:53]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-24 18:24:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
dcheng@chromium.org, wez@chromium.org, piman@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2096493002/#ps380001
(title: "Fix android webview test")

[commit-bot: I haz the power, 2016-06-24 18:25:24]

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

[danakj, 2016-06-24 18:27:35]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-06-24 18:27:43]

On 2016/06/24 18:24:30, Fady Samuel wrote:
> On 2016/06/24 18:13:14, boliu wrote:
> > still need to revert changes to test_synchronous_compositor_android.h/cc to
> make
> > webview unittests compile
> > 
> > lgtm after that
> 
> Fixed. I'm CQ'ing now. Please yell at me if you think I shouldn't land this!

Im still reviewing

[danakj, 2016-06-24 18:35:11]

https://codereview.chromium.org/2096493002/diff/360001/cc/output/compositor_f...
File cc/output/compositor_frame.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/compositor_f...
cc/output/compositor_frame.cc:7: #include "base/memory/ptr_util.h"
not used

https://codereview.chromium.org/2096493002/diff/360001/cc/output/compositor_f...
File cc/output/compositor_frame.h (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/compositor_f...
cc/output/compositor_frame.h:20: // TODO(fsamuel): Write more here.
^_^ maybe now is a good time?

https://codereview.chromium.org/2096493002/diff/360001/cc/output/delegating_r...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/delegating_r...
cc/output/delegating_renderer.cc:105: compositor_frame.metadata =
CompositorFrameMetadata(metadata);
what is this? do you need to define a copy operator= to match the constructor?

or make it move-only and pass it by value to here?

https://codereview.chromium.org/2096493002/diff/360001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2721: compositor_frame.metadata =
CompositorFrameMetadata(metadata);
dittos

https://codereview.chromium.org/2096493002/diff/360001/cc/output/renderer_uni...
File cc/output/renderer_unittest.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/renderer_uni...
cc/output/renderer_unittest.cc:24: // OutputSurface implementation
put the period back please :)

https://codereview.chromium.org/2096493002/diff/360001/cc/output/software_ren...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/software_ren...
cc/output/software_renderer.cc:115: compositor_frame.metadata =
CompositorFrameMetadata(metadata);
dittos

https://codereview.chromium.org/2096493002/diff/360001/cc/test/fake_output_su...
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/test/fake_output_su...
cc/test/fake_output_surface.h:173: CompositorFrame last_sent_frame_;
I kinda liked this not being a default CompositorFrame, instead being null when
nothing was sent yet, in the older patchset.

https://codereview.chromium.org/2096493002/diff/360001/components/mus/public/...
File components/mus/public/cpp/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/components/mus/public/...
components/mus/public/cpp/surfaces/surfaces_type_converters.cc:42:
frame->metadata = cc::CompositorFrameMetadata(input.metadata);
dittos

https://codereview.chromium.org/2096493002/diff/360001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:149:
last_compositor_frame_metadata_ = cc::CompositorFrameMetadata(frame_metadata);
dittos

https://codereview.chromium.org/2096493002/diff/360001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:161: ?
cc::CompositorFrameMetadata(next_compositor_frame_metadata_)
dittos

https://codereview.chromium.org/2096493002/diff/360001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:163:
next_compositor_frame_metadata_ = cc::CompositorFrameMetadata(frame_metadata);
A copy needs to be made here tho, so if you make it move-only you'll need a
Clone() method or something for this.

https://codereview.chromium.org/2096493002/diff/360001/content/public/browser...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/2096493002/diff/360001/content/public/browser...
content/public/browser/android/synchronous_compositor.h:14: #include
"cc/output/compositor_frame.h"
Dont need to do this now

https://codereview.chromium.org/2096493002/diff/360001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:256:
SendDemandDrawSwReply(false, std::move(empty_frame), reply_message);
you can just pass a cc::CompositorFrame() here, no variable needed

[Fady Samuel, 2016-06-24 20:00:24]

PTAL Dana!

https://codereview.chromium.org/2096493002/diff/360001/cc/output/compositor_f...
File cc/output/compositor_frame.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/compositor_f...
cc/output/compositor_frame.cc:7: #include "base/memory/ptr_util.h"
On 2016/06/24 18:35:10, danakj wrote:
> not used

Done.

https://codereview.chromium.org/2096493002/diff/360001/cc/output/compositor_f...
File cc/output/compositor_frame.h (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/compositor_f...
cc/output/compositor_frame.h:20: // TODO(fsamuel): Write more here.
On 2016/06/24 18:35:10, danakj wrote:
> ^_^ maybe now is a good time?

Done. Let me know if I could improve this further? Thanks!

https://codereview.chromium.org/2096493002/diff/360001/cc/output/delegating_r...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/delegating_r...
cc/output/delegating_renderer.cc:105: compositor_frame.metadata =
CompositorFrameMetadata(metadata);
On 2016/06/24 18:35:10, danakj wrote:
> what is this? do you need to define a copy operator= to match the constructor?
> 
> or make it move-only and pass it by value to here?

Done.

https://codereview.chromium.org/2096493002/diff/360001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2721: compositor_frame.metadata =
CompositorFrameMetadata(metadata);
On 2016/06/24 18:35:10, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/2096493002/diff/360001/cc/output/renderer_uni...
File cc/output/renderer_unittest.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/renderer_uni...
cc/output/renderer_unittest.cc:24: // OutputSurface implementation
On 2016/06/24 18:35:11, danakj wrote:
> put the period back please :)

Done.

https://codereview.chromium.org/2096493002/diff/360001/cc/output/software_ren...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/output/software_ren...
cc/output/software_renderer.cc:115: compositor_frame.metadata =
CompositorFrameMetadata(metadata);
On 2016/06/24 18:35:11, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/2096493002/diff/360001/cc/test/fake_output_su...
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/2096493002/diff/360001/cc/test/fake_output_su...
cc/test/fake_output_surface.h:173: CompositorFrame last_sent_frame_;
On 2016/06/24 18:35:11, danakj wrote:
> I kinda liked this not being a default CompositorFrame, instead being null
when
> nothing was sent yet, in the older patchset.

Done.

https://codereview.chromium.org/2096493002/diff/360001/components/mus/public/...
File components/mus/public/cpp/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/components/mus/public/...
components/mus/public/cpp/surfaces/surfaces_type_converters.cc:42:
frame->metadata = cc::CompositorFrameMetadata(input.metadata);
On 2016/06/24 18:35:11, danakj wrote:
> dittos

This is getting cloned for now. I'm deleting this entire file in the next CL:
https://codereview.chromium.org/2092843002/

https://codereview.chromium.org/2096493002/diff/360001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:149:
last_compositor_frame_metadata_ = cc::CompositorFrameMetadata(frame_metadata);
On 2016/06/24 18:35:11, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/2096493002/diff/360001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:161: ?
cc::CompositorFrameMetadata(next_compositor_frame_metadata_)
On 2016/06/24 18:35:11, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/2096493002/diff/360001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:163:
next_compositor_frame_metadata_ = cc::CompositorFrameMetadata(frame_metadata);
On 2016/06/24 18:35:11, danakj wrote:
> A copy needs to be made here tho, so if you make it move-only you'll need a
> Clone() method or something for this.

I made the above Clone() and using std::move on htis.

https://codereview.chromium.org/2096493002/diff/360001/content/public/browser...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/2096493002/diff/360001/content/public/browser...
content/public/browser/android/synchronous_compositor.h:14: #include
"cc/output/compositor_frame.h"
On 2016/06/24 18:35:11, danakj wrote:
> Dont need to do this now

Done.

https://codereview.chromium.org/2096493002/diff/360001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/2096493002/diff/360001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:256:
SendDemandDrawSwReply(false, std::move(empty_frame), reply_message);
On 2016/06/24 18:35:11, danakj wrote:
> you can just pass a cc::CompositorFrame() here, no variable needed

Done.

[Fady Samuel, 2016-06-24 20:00:37]

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

[commit-bot: I haz the power, 2016-06-24 20:01:44]

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

[danakj, 2016-06-24 20:20:44]

Thanks, looks nice. Few things left

https://codereview.chromium.org/2096493002/diff/400001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2096493002/diff/400001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:27: CompositorFrameMetadata(const
CompositorFrameMetadata& other);
can you make this private then, and "delete" the operator= for copy?

https://codereview.chromium.org/2096493002/diff/400001/cc/output/delegating_r...
File cc/output/delegating_renderer_unittest.cc (right):

https://codereview.chromium.org/2096493002/diff/400001/cc/output/delegating_r...
cc/output/delegating_renderer_unittest.cc:115: CompositorFrame* last_frame =
output_surface_->last_sent_frame();
nit: can stay const

https://codereview.chromium.org/2096493002/diff/400001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2096493002/diff/400001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:149:
last_compositor_frame_metadata_ = frame_metadata.Clone();
why not move?

https://codereview.chromium.org/2096493002/diff/400001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:160:
has_compositor_frame_metadata_ ? next_compositor_frame_metadata_.Clone()
you can move here, but you probably need to use if/else

[commit-bot: I haz the power, 2016-06-24 20:47:58]

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

[commit-bot: I haz the power, 2016-06-24 20:48:01]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Fady Samuel, 2016-06-24 21:31:03]

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

[Fady Samuel, 2016-06-24 21:31:16]

PTAL Dana!

https://codereview.chromium.org/2096493002/diff/400001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2096493002/diff/400001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:27: CompositorFrameMetadata(const
CompositorFrameMetadata& other);
On 2016/06/24 20:20:43, danakj wrote:
> can you make this private then, and "delete" the operator= for copy?

Done.

https://codereview.chromium.org/2096493002/diff/400001/cc/output/delegating_r...
File cc/output/delegating_renderer_unittest.cc (right):

https://codereview.chromium.org/2096493002/diff/400001/cc/output/delegating_r...
cc/output/delegating_renderer_unittest.cc:115: CompositorFrame* last_frame =
output_surface_->last_sent_frame();
On 2016/06/24 20:20:43, danakj wrote:
> nit: can stay const

Done.

https://codereview.chromium.org/2096493002/diff/400001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2096493002/diff/400001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:149:
last_compositor_frame_metadata_ = frame_metadata.Clone();
On 2016/06/24 20:20:43, danakj wrote:
> why not move?

Done.

https://codereview.chromium.org/2096493002/diff/400001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:160:
has_compositor_frame_metadata_ ? next_compositor_frame_metadata_.Clone()
On 2016/06/24 20:20:43, danakj wrote:
> you can move here, but you probably need to use if/else

Done.

[commit-bot: I haz the power, 2016-06-24 21:31:52]

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

[danakj, 2016-06-24 21:45:19]

LGTM

https://codereview.chromium.org/2096493002/diff/420001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2096493002/diff/420001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:74: CompositorFrameMetadata(const
CompositorFrameMetadata& other);
also explicitly delete the operator= please. Literally "CFM operator=(const
CFM&) = delete;"

https://codereview.chromium.org/2096493002/diff/420001/components/mus/public/...
File components/mus/public/cpp/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/2096493002/diff/420001/components/mus/public/...
components/mus/public/cpp/surfaces/surfaces_type_converters.cc:62:
frame->metadata = input->metadata.Clone();
can this not move(input->metadata)?

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:164:
last_compositor_frame_metadata_ = std::move(frame_metadata);
nit: you could keep the next_cfm part outside the if/else

if (has)
  last = std::move(next);
else
  last = frame_metadata.Clone();
next = std::move(frame_metadata);

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:437:
last_compositor_frame_metadata_.Clone()),
use base::Passed( of Clone()) here

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:444: const
cc::CompositorFrameMetadata& metadata,
take this by value

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:463:
weak_factory_.GetWeakPtr(), metadata.Clone(),
use base::Passed(&metadata) here,

then you avoid this copy, and the copy into the Bind in each place.

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:468: const
cc::CompositorFrameMetadata& metadata,
and take this by value

[commit-bot: I haz the power, 2016-06-24 21:58:05]

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

[commit-bot: I haz the power, 2016-06-24 21:58:08]

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

[Fady Samuel, 2016-06-24 22:15:46]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-24 22:15:47]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wez@chromium.org, boliu@chromium.org, dcheng@chromium.org, piman@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2096493002/#ps440001
(title: "Addressed Dana's nits")

[Fady Samuel, 2016-06-24 22:15:47]

The CQ bit was unchecked by fsamuel@chromium.org

[Fady Samuel, 2016-06-24 22:20:06]

Patchset #23 (id:440001) has been deleted

[Fady Samuel, 2016-06-24 22:24:37]

Thanks Dana (and everyone!) for the code review! CQ'ing!

https://codereview.chromium.org/2096493002/diff/420001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2096493002/diff/420001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:74: CompositorFrameMetadata(const
CompositorFrameMetadata& other);
On 2016/06/24 21:45:18, danakj wrote:
> also explicitly delete the operator= please. Literally "CFM operator=(const
> CFM&) = delete;"

Done.

https://codereview.chromium.org/2096493002/diff/420001/components/mus/public/...
File components/mus/public/cpp/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/2096493002/diff/420001/components/mus/public/...
components/mus/public/cpp/surfaces/surfaces_type_converters.cc:62:
frame->metadata = input->metadata.Clone();
On 2016/06/24 21:45:18, danakj wrote:
> can this not move(input->metadata)?

No, since input is a const ref. This code is all going away in my next CL
anyway.

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:164:
last_compositor_frame_metadata_ = std::move(frame_metadata);
On 2016/06/24 21:45:18, danakj wrote:
> nit: you could keep the next_cfm part outside the if/else
> 
> if (has)
>   last = std::move(next);
> else
>   last = frame_metadata.Clone();
> next = std::move(frame_metadata);

Done.

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:437:
last_compositor_frame_metadata_.Clone()),
On 2016/06/24 21:45:18, danakj wrote:
> use base::Passed( of Clone()) here

Done.

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:444: const
cc::CompositorFrameMetadata& metadata,
On 2016/06/24 21:45:18, danakj wrote:
> take this by value

Done.

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:463:
weak_factory_.GetWeakPtr(), metadata.Clone(),
On 2016/06/24 21:45:18, danakj wrote:
> use base::Passed(&metadata) here,
> 
> then you avoid this copy, and the copy into the Bind in each place.

Done.

https://codereview.chromium.org/2096493002/diff/420001/content/browser/devtoo...
content/browser/devtools/protocol/page_handler.cc:468: const
cc::CompositorFrameMetadata& metadata,
On 2016/06/24 21:45:18, danakj wrote:
> and take this by value

Done.

[Fady Samuel, 2016-06-24 22:24:49]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-24 22:24:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wez@chromium.org, boliu@chromium.org, dcheng@chromium.org, piman@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2096493002/#ps460001
(title: "Addressed Dana's nits")

[commit-bot: I haz the power, 2016-06-24 22:25:18]

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

[danakj, 2016-06-24 22:25:30]

LGTM

[commit-bot: I haz the power, 2016-06-24 23:40:05]

Description was changed from

==========
Make cc::CompositorFrames movable [Part 1 of 2]

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping 
CompositorFrames over mojo. CompositorFrames are now 
movable. SurfaceFactory still uses a unique_ptr and so we 
create a new CompositorFrame, and move into that to pass to 
SurfaceFactory in this CL. In a subsequent CL, 
SurfaceFactory holds a CompositorFrame by value.

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

to

==========
Make cc::CompositorFrames movable [Part 1 of 2]

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping 
CompositorFrames over mojo. CompositorFrames are now 
movable. SurfaceFactory still uses a unique_ptr and so we 
create a new CompositorFrame, and move into that to pass to 
SurfaceFactory in this CL. In a subsequent CL, 
SurfaceFactory holds a CompositorFrame by value.

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

[commit-bot: I haz the power, 2016-06-24 23:40:07]

Committed patchset #23 (id:460001)

[commit-bot: I haz the power, 2016-06-24 23:41:49]

Description was changed from

==========
Make cc::CompositorFrames movable [Part 1 of 2]

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping 
CompositorFrames over mojo. CompositorFrames are now 
movable. SurfaceFactory still uses a unique_ptr and so we 
create a new CompositorFrame, and move into that to pass to 
SurfaceFactory in this CL. In a subsequent CL, 
SurfaceFactory holds a CompositorFrame by value.

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

to

==========
Make cc::CompositorFrames movable [Part 1 of 2]

There was some odd code where we created CompositorFrames on the stack
and passed them around by raw pointers, and then on the other other side
of the IPC boundary we wrapped the CompositorFrame in a unique_ptr. Odder
still, there are cases where we create a copy of the CompositorFrame just
to pass it to a SurfaceFactory or elsewhere.

This CL cleans up the insanity in preparation of shipping
CompositorFrames over mojo. CompositorFrames are now
movable. SurfaceFactory still uses a unique_ptr and so we
create a new CompositorFrame, and move into that to pass to
SurfaceFactory in this CL. In a subsequent CL,
SurfaceFactory holds a CompositorFrame by value.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-06-24 23:41:52]

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