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

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

This CL is part 2 of 2 for making cc::CompositorFrames
movable. This CL primarily tackles making cc::Surface
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2d9cc5abb4745280a002822a2380e925f835ef51
Committed: https://crrev.com/5222b6cb6ea4cb34ce34a6b8572bdd6cf081fa7f
Cr-Original-Commit-Position: refs/heads/master@{#402545}
Cr-Commit-Position: refs/heads/master@{#402615}

[Fady Samuel, 2016-06-25 14:01:32]

Description was changed from

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

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

to

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

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-06-25 14:01:32]

Description was changed from

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

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

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

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

[Fady Samuel, 2016-06-25 14:01:50]

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

[commit-bot: I haz the power, 2016-06-25 14:02:00]

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 14:16:31]

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

[commit-bot: I haz the power, 2016-06-25 14:16:32]

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

[Fady Samuel, 2016-06-25 14:49:01]

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

[commit-bot: I haz the power, 2016-06-25 14:49:14]

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

[Fady Samuel, 2016-06-25 15:43:07]

Description was changed from

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

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

to

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

This CL is part 2 of 2 for making cc::CompositorFrames movable. This CL
primarily tackles making cc::Surface hold
a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

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

[Fady Samuel, 2016-06-25 15:43:43]

Description was changed from

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

This CL is part 2 of 2 for making cc::CompositorFrames movable. This CL
primarily tackles making cc::Surface hold
a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

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

to

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

This CL is part 2 of 2 for making cc::CompositorFrames 
movable. This CL primarily tackles making cc::Surface 
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

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

[Fady Samuel, 2016-06-25 15:45:05]

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

[Fady Samuel, 2016-06-25 15:45:05]

+danakj@ for overall review
+piman@ for content (and anything else, will wait for both you and Dana to LGTM)
+sky@ for mus
+boliu@ for android_webview

[commit-bot: I haz the power, 2016-06-25 16:19:43]

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

[commit-bot: I haz the power, 2016-06-25 16:19:44]

Dry run: This issue passed the CQ dry run.

[piman, 2016-06-27 15:13:24]

lgtm

[sky, 2016-06-27 16:12:35]

LGTM

[boliu, 2016-06-27 16:19:38]

lgtm

[danakj, 2016-06-27 19:53:01]

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:93:
current_frame_.metadata.satisfies_sequences.swap(satisfies_sequences);
you can use move instead for this, and merge it with the variable definition.

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:145: return &current_frame_;
There are callers that check if this is null. What does it mean that it is never
null for them? And we should change them accordingly. I suspect we need to
maintain a "has current_frame_" vs "does not have" state but I'd like to hear
your analysis

[Fady Samuel, 2016-06-28 17:19:10]

PTAL Dana! Please let me know what you think about my changing GetEligibleFrame.
Is it reasonable or do we want an explicit "has frame"?

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:93:
current_frame_.metadata.satisfies_sequences.swap(satisfies_sequences);
On 2016/06/27 19:53:00, danakj wrote:
> you can use move instead for this, and merge it with the variable definition.

Done.

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:145: return &current_frame_;
On 2016/06/27 19:53:00, danakj wrote:
> There are callers that check if this is null. What does it mean that it is
never
> null for them? And we should change them accordingly. I suspect we need to
> maintain a "has current_frame_" vs "does not have" state but I'd like to hear
> your analysis

I've let the compiler help me with the analysis. :-) I've made GetEligibleFrame
return a const reference instead and I've updated callsites to check
delegated_frame_data instead. WDYT?

[Fady Samuel, 2016-06-28 17:20:18]

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

[commit-bot: I haz the power, 2016-06-28 17:21: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-28 17:54:27]

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

[commit-bot: I haz the power, 2016-06-28 17:54:28]

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

[danakj, 2016-06-28 19:01:09]

LGTM

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:145: return &current_frame_;
On 2016/06/28 17:19:10, Fady Samuel wrote:
> On 2016/06/27 19:53:00, danakj wrote:
> > There are callers that check if this is null. What does it mean that it is
> never
> > null for them? And we should change them accordingly. I suspect we need to
> > maintain a "has current_frame_" vs "does not have" state but I'd like to
hear
> > your analysis
> 
> I've let the compiler help me with the analysis. :-) I've made
GetEligibleFrame
> return a const reference instead and I've updated callsites to check
> delegated_frame_data instead. WDYT?

Ya ok this should work for now. Eventually we're not going to have a
DelegatedFrameData I don't think, we'll just collapse it into
CompositorFrameData, so we'll have to write this patch again basically. I think
you could avoid that by using a "has a frame" state instead in this CL.

https://codereview.chromium.org/2098953003/diff/40001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2098953003/diff/40001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:65: // Returns the most recent frame that is eligible to
be rendered.
Mention here how to tell if there is no frame at all.

[Fady Samuel, 2016-06-28 19:24:19]

CQ'ing this CL. I can delete DelegatedFrameData in a separate CL and introduce a
HasFrame.

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2098953003/diff/20001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:145: return &current_frame_;
On 2016/06/28 19:01:09, danakj wrote:
> On 2016/06/28 17:19:10, Fady Samuel wrote:
> > On 2016/06/27 19:53:00, danakj wrote:
> > > There are callers that check if this is null. What does it mean that it is
> > never
> > > null for them? And we should change them accordingly. I suspect we need to
> > > maintain a "has current_frame_" vs "does not have" state but I'd like to
> hear
> > > your analysis
> > 
> > I've let the compiler help me with the analysis. :-) I've made
> GetEligibleFrame
> > return a const reference instead and I've updated callsites to check
> > delegated_frame_data instead. WDYT?
> 
> Ya ok this should work for now. Eventually we're not going to have a
> DelegatedFrameData I don't think, we'll just collapse it into
> CompositorFrameData, so we'll have to write this patch again basically. I
think
> you could avoid that by using a "has a frame" state instead in this CL.

The primary goal of this CL is to make Surface hold a cc::CompositorFrame
directly.

I think the easiest way to introduce a HasFrame() is to get rid of
delegated_frame_data and update all checks for not-nullness to use HasFrame().
The compiler can help us verify we did it right. 

My goal is also to get cc/output/compositor_frame.h and
cc/ipc/compositor_frame.mojom to look the same so I'm happy to make that change
in a separate CL.

https://codereview.chromium.org/2098953003/diff/40001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2098953003/diff/40001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:65: // Returns the most recent frame that is eligible to
be rendered.
On 2016/06/28 19:01:09, danakj wrote:
> Mention here how to tell if there is no frame at all.

Done.

[Fady Samuel, 2016-06-28 19:24:30]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-28 19:24:31]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
boliu@chromium.org, sky@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2098953003/#ps60001
(title: "Updated")

[commit-bot: I haz the power, 2016-06-28 19:26:10]

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-28 19:46:59]

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

[commit-bot: I haz the power, 2016-06-28 19:47:00]

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-28 21:09:20]

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

[Fady Samuel, 2016-06-28 21:09:20]

+reveman@ for exo

[reveman, 2016-06-28 21:10:43]

components/exo lgtm

[Fady Samuel, 2016-06-28 21:11:12]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-06-28 21:12:34]

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-28 21:22:51]

Description was changed from

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

This CL is part 2 of 2 for making cc::CompositorFrames 
movable. This CL primarily tackles making cc::Surface 
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

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

to

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

This CL is part 2 of 2 for making cc::CompositorFrames 
movable. This CL primarily tackles making cc::Surface 
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

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

[commit-bot: I haz the power, 2016-06-28 21:22:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-28 21:24:45]

Description was changed from

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

This CL is part 2 of 2 for making cc::CompositorFrames 
movable. This CL primarily tackles making cc::Surface 
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

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

to

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

This CL is part 2 of 2 for making cc::CompositorFrames
movable. This CL primarily tackles making cc::Surface
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2d9cc5abb4745280a002822a2380e925f835ef51
Cr-Commit-Position: refs/heads/master@{#402545}
==========

[commit-bot: I haz the power, 2016-06-28 21:24:46]

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

[Pete Williamson, 2016-06-28 21:59:50]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2103333002/ by petewil@chromium.org.

The reason for reverting is: It looks like this might have broken the tree
compile:
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

Reverting as Sheriff.

[Fady Samuel, 2016-06-28 22:29:37]

Description was changed from

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

This CL is part 2 of 2 for making cc::CompositorFrames
movable. This CL primarily tackles making cc::Surface
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2d9cc5abb4745280a002822a2380e925f835ef51
Cr-Commit-Position: refs/heads/master@{#402545}
==========

to

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

This CL is part 2 of 2 for making cc::CompositorFrames
movable. This CL primarily tackles making cc::Surface
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2d9cc5abb4745280a002822a2380e925f835ef51
Cr-Commit-Position: refs/heads/master@{#402545}
==========

[Fady Samuel, 2016-06-28 22:29:51]

On 2016/06/28 21:59:50, Pete Williamson wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2103333002/ by mailto:petewil@chromium.org.
> 
> The reason for reverting is: It looks like this might have broken the tree
> compile:
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
> 
> Reverting as Sheriff.

I've fixed the issue and am relanding. Thanks!

[Fady Samuel, 2016-06-28 22:29:58]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-28 22:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
piman@chromium.org, reveman@chromium.org, sky@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2098953003/#ps80001
(title: "Fixed exo unittests")

[commit-bot: I haz the power, 2016-06-28 22:34:32]

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-29 00:37:08]

Description was changed from

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

This CL is part 2 of 2 for making cc::CompositorFrames
movable. This CL primarily tackles making cc::Surface
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2d9cc5abb4745280a002822a2380e925f835ef51
Cr-Commit-Position: refs/heads/master@{#402545}
==========

to

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

This CL is part 2 of 2 for making cc::CompositorFrames
movable. This CL primarily tackles making cc::Surface
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2d9cc5abb4745280a002822a2380e925f835ef51
Cr-Commit-Position: refs/heads/master@{#402545}
==========

[commit-bot: I haz the power, 2016-06-29 00:37:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-29 00:39:20]

Description was changed from

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

This CL is part 2 of 2 for making cc::CompositorFrames
movable. This CL primarily tackles making cc::Surface
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2d9cc5abb4745280a002822a2380e925f835ef51
Cr-Commit-Position: refs/heads/master@{#402545}
==========

to

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

This CL is part 2 of 2 for making cc::CompositorFrames
movable. This CL primarily tackles making cc::Surface
hold a cc::CompositorFrame by value. This impacts tons of
callsites which are updated in this patch.

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

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2d9cc5abb4745280a002822a2380e925f835ef51
Committed: https://crrev.com/5222b6cb6ea4cb34ce34a6b8572bdd6cf081fa7f
Cr-Original-Commit-Position: refs/heads/master@{#402545}
Cr-Commit-Position: refs/heads/master@{#402615}
==========

[commit-bot: I haz the power, 2016-06-29 00:39:22]

Patchset 5 (id:??) landed as
https://crrev.com/5222b6cb6ea4cb34ce34a6b8572bdd6cf081fa7f
Cr-Commit-Position: refs/heads/master@{#402615}