Moves ownership of the cc::Display's BeginFrameSource out of Display.

[cc] Move ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336, 580357
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/36f6bc8fb0f973f02e39f49f4ebdea68c4abe4d1
Cr-Commit-Position: refs/heads/master@{#438478}

[Eric Seckler, 2016-12-09 15:01:33]

Description was changed from

==========
Moves ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336,580357
==========

to

==========
Moves ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336,580357
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2016-12-09 15:02:02]

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

[commit-bot: I haz the power, 2016-12-09 15:02:29]

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

[Eric Seckler, 2016-12-09 15:49:12]

Description was changed from

==========
Moves ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336,580357
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Move ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336,580357
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2016-12-09 16:03:38]

eseckler@chromium.org changed reviewers:
+ boliu@chromium.org, brianderson@chromium.org, khushalsagar@chromium.org,
sky@chromium.org

[Eric Seckler, 2016-12-09 16:03:39]

brianderson@ for overall review and ownership of cc/

boliu@ for android_webview/ and content/browser/
khushalsagar@ for blimp/
sky@ for ui/ and services/ui/

Thanks guys!

[commit-bot: I haz the power, 2016-12-09 16:03:54]

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

[commit-bot: I haz the power, 2016-12-09 16:03:55]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-12-09 16:23:31]

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

[Fady Samuel, 2016-12-09 16:23:34]

I'm confused about the long term intent of this change.
WindowCompositorFrameSink lives in the client process whereas
GpuCompositorFrameSink lives in the gpu process.

[Eric Seckler, 2016-12-09 16:59:57]

On 2016/12/09 16:23:34, Fady Samuel wrote:
> I'm confused about the long term intent of this change.
> WindowCompositorFrameSink lives in the client process whereas
> GpuCompositorFrameSink lives in the gpu process.

For Mus, I'm looking to change the hook-up of the BeginFrameSource in the client
process: I'd like to provide the ExternalBeginFrameSource that
WindowCompositorFrameSink owns to the browser's cc::Display (via
MusBrowserCompositorOutputSurface). The changes to GpuCompositorFrameSink in
this patch are only to handle the ownership of the BeginFrameSource for its
cc::Display. I'm not planning to change anything about BeginFrameSource hookup
there.

For Android, I'd like to move the BeginFrameSource from CompositorImpl up into
WindowAndroid, which would then own it. That will allow us to hook up
RenderWidgetHostViewAndroid to a BeginFrameSource instead of listening for the
window's vsyncs. On Android Chrome, that'll be the cc::Display's
BeginFrameSource provided by the SurfaceManager (via DelegatedFrameHostAndroid);
on Android WebView it'll be  the WindowAndroid's BeginFrameSource directly.

[brianderson, 2016-12-09 19:01:57]

cc lgtm.

Thanks for being mindful of the destruction order of member variables.

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/compositor_fram...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/compositor_fram...
cc/surfaces/compositor_frame_sink_support.h:79: // created with non-null
gpu::SurfaceHandle. In the window server, the display
While we are here: "if it was created"

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/display_schedul...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/display_schedul...
cc/surfaces/display_scheduler.cc:18: : client_(nullptr),
Thanks for fixing this!

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/display_schedul...
cc/surfaces/display_scheduler.cc:47: void DisplayScheduler::SetBeginFrameSource(
For my understanding, was were the reasons to use SetBeginFrameSource rather
than passing it in to the constructor?

I see that it is:
Clearer from Display that its DisplayScheduler will have the same
BeginFrameSource.
Sets the DisplayScheduler's BeginFrameSource from fewer places.

Any others?

[Khushal, 2016-12-09 19:19:23]

blimp lgtm.

[boliu, 2016-12-09 21:11:34]

lgtm

[Fady Samuel, 2016-12-09 21:15:16]

lgtm

[Fady Samuel, 2016-12-09 21:15:16]

lgtm

lgtm

[Eric Seckler, 2016-12-12 09:27:17]

eseckler@chromium.org changed reviewers:
+ vollick@chromium.org
- sky@chromium.org

[Eric Seckler, 2016-12-12 09:27:18]

+vollick@
-sky@

Ian, please have a quick look at:
  ui/compositor/test/in_process_context_factory.cc

Thanks! :)

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/compositor_fram...
File cc/surfaces/compositor_frame_sink_support.h (right):

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/compositor_fram...
cc/surfaces/compositor_frame_sink_support.h:79: // created with non-null
gpu::SurfaceHandle. In the window server, the display
On 2016/12/09 19:01:57, brianderson wrote:
> While we are here: "if it was created"

Done.

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/display_schedul...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2565783002/diff/1/cc/surfaces/display_schedul...
cc/surfaces/display_scheduler.cc:47: void DisplayScheduler::SetBeginFrameSource(
On 2016/12/09 19:01:57, brianderson wrote:
> For my understanding, was were the reasons to use SetBeginFrameSource rather
> than passing it in to the constructor?
> 
> I see that it is:
> Clearer from Display that its DisplayScheduler will have the same
> BeginFrameSource.
> Sets the DisplayScheduler's BeginFrameSource from fewer places.
> 
> Any others?

What you said :) Also, it's useful for later wrapping the Display's BFS  (with
something that can keep track of outstanding BeginFrames / observer responses,
see DisplayBeginFrameSource in https://codereview.chromium.org/2527283003/) and
providing the wrapped source to the scheduler.

[Eric Seckler, 2016-12-12 10:58:59]

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

[commit-bot: I haz the power, 2016-12-12 10:59: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-12-12 12:03:43]

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

[commit-bot: I haz the power, 2016-12-12 12:03:44]

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

[Eric Seckler, 2016-12-12 12:16:54]

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

[Eric Seckler, 2016-12-12 12:16:54]

+reveman@ for
  components/exo/compositor_frame_sink.cc

Thanks!

[reveman, 2016-12-12 15:25:39]

components/exo lgtm

[Eric Seckler, 2016-12-13 15:25:26]

eseckler@chromium.org changed reviewers:
+ sky@chromium.org
- vollick@chromium.org

[Eric Seckler, 2016-12-13 15:25:27]

+sky@
-vollick@

Looks like everone from ui/compositor/ is out at the moment.

Scott, could you take a quick look at
ui/compositor/test/in_process_context_factory.cc ?

Thanks!

[sky, 2016-12-13 17:50:49]

LGTM

[Eric Seckler, 2016-12-14 09:55:04]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-12-14 09:55:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, khushalsagar@chromium.org, boliu@chromium.org,
brianderson@chromium.org, reveman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2565783002/#ps80001
(title: "rebase.")

[commit-bot: I haz the power, 2016-12-14 09:55:18]

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-12-14 11:15:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481709304215610,
"parent_rev": "9541b103155432da0ed451ae12d55d5e5bfb0766", "commit_rev":
"208727b67a0e9d2985617cea070fb9787e680311"}

[commit-bot: I haz the power, 2016-12-14 11:15:45]

Description was changed from

==========
[cc] Move ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336,580357
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Move ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336,580357
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2565783002
==========

[commit-bot: I haz the power, 2016-12-14 11:15:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-14 11:17:54]

Description was changed from

==========
[cc] Move ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336,580357
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2565783002
==========

to

==========
[cc] Move ownership of the cc::Display's BeginFrameSource out of Display.

This is in preparation for Android and Mus to provide a BeginFrameSource
that is owned further up in the object hierarchy (e.g. by WindowAndroid
/ WindowCompositorFrameSink).

BUG=401336,580357
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/36f6bc8fb0f973f02e39f49f4ebdea68c4abe4d1
Cr-Commit-Position: refs/heads/master@{#438478}
==========

[commit-bot: I haz the power, 2016-12-14 11:17:56]

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