Make cc::Display not own its BeginFrameSource

Make cc::Display not own its BeginFrameSource

This is refactoring to allow MusBrowserCompositorOutputSurface to
provide an ExternalBeginFrameSource that it owns rather than
passing ownership to Display.

This should also make it possible to remove begin frame sources from
OutputSurface in the future after the OutputSurface/CompositorFrameSink
split.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/9848a61393772cc8a9aa8349c68f7d436c743369
Cr-Commit-Position: refs/heads/master@{#415738}

[enne (OOO), 2016-08-30 01:00:40]

Description was changed from

==========
Make cc::Display not own its BeginFrameSource

This is refactoring to allow MusBrowserCompositorOutputSurface to
provide an ExternalBeginFrameSource that it owns rather than
passing ownership to Display.

This should also make it possible to remove begin frame sources from
OutputSurface in the future after the OutputSurface/CompositorFrameSink
split.
==========

to

==========
Make cc::Display not own its BeginFrameSource

This is refactoring to allow MusBrowserCompositorOutputSurface to
provide an ExternalBeginFrameSource that it owns rather than
passing ownership to Display.

This should also make it possible to remove begin frame sources from
OutputSurface in the future after the OutputSurface/CompositorFrameSink
split.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[enne (OOO), 2016-08-30 01:01:01]

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

[danakj, 2016-08-30 01:10:56]

LGTM~

https://codereview.chromium.org/2286273003/diff/1/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/2286273003/diff/1/cc/surfaces/display.h#newco...
cc/surfaces/display.h:136: BeginFrameSource* begin_frame_source_;
can it be a const member maybe up with the bitmap_manager_ etc

https://codereview.chromium.org/2286273003/diff/1/cc/test/test_delegating_out...
File cc/test/test_delegating_output_surface.h (right):

https://codereview.chromium.org/2286273003/diff/1/cc/test/test_delegating_out...
cc/test/test_delegating_output_surface.h:91:
std::unique_ptr<SyntheticBeginFrameSource> begin_frame_source_;
Can you put this right on top of display_ since that's all that uses it?

https://codereview.chromium.org/2286273003/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2286273003/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:443:
data->begin_frame_source.reset(new cc::DelayBasedBeginFrameSource(
I don't think you should put this into |data| until you have put the Display
into it. The old Display will still be in |data| pointing ot the old bfs. This
is the things-get-hairy part I guess

https://codereview.chromium.org/2286273003/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:543: data->display =
base::MakeUnique<cc::Display>(
So just insert it into |data| after this, with a comment?

https://codereview.chromium.org/2286273003/diff/1/ui/compositor/test/in_proce...
File ui/compositor/test/in_process_context_factory.cc (right):

https://codereview.chromium.org/2286273003/diff/1/ui/compositor/test/in_proce...
ui/compositor/test/in_process_context_factory.cc:177:
data->begin_frame_source.reset(new cc::DelayBasedBeginFrameSource(
it works here cuz you're making a new |data|. maybe a comment explaining?

[enne (OOO), 2016-08-30 20:17:28]

https://codereview.chromium.org/2286273003/diff/1/ui/compositor/test/in_proce...
File ui/compositor/test/in_process_context_factory.cc (right):

https://codereview.chromium.org/2286273003/diff/1/ui/compositor/test/in_proce...
ui/compositor/test/in_process_context_factory.cc:177:
data->begin_frame_source.reset(new cc::DelayBasedBeginFrameSource(
On 2016/08/30 at 01:10:56, danakj wrote:
> it works here cuz you're making a new |data|. maybe a comment explaining?

I don't think this one needs a comment.  I think if this ever persists the
PerCompositorData across multiple output surfaces, then it can get some more
explanation then.

[enne (OOO), 2016-08-30 20:17:46]

enne@chromium.org changed reviewers:
+ fsamuel@chromium.org, nyquist@chromium.org

[enne (OOO), 2016-08-30 20:18:21]

fsamuel: services/ui/surfaces/ OWNERS
nyquist: blimp/client/app/compositor/ OWNERS

[enne (OOO), 2016-08-30 20:18:35]

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

[commit-bot: I haz the power, 2016-08-30 20:19:04]

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-08-30 20:20:14]

lgtm

[commit-bot: I haz the power, 2016-08-30 20:38:02]

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

[commit-bot: I haz the power, 2016-08-30 20:38:03]

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...)

[nyquist, 2016-08-30 21:52:05]

lgtm

[enne (OOO), 2016-08-30 22:18:08]

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

[commit-bot: I haz the power, 2016-08-30 22:18:30]

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

[enne (OOO), 2016-08-30 22:19:00]

enne@chromium.org changed reviewers:
+ sievers@chromium.org

[enne (OOO), 2016-08-30 22:19:00]

sievers: content/browser/renderer_host/ OWNERS

[enne (OOO), 2016-08-30 22:41:14]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-08-30 23:41:49]

enne@chromium.org changed reviewers:
+ boliu@chromium.org

[enne (OOO), 2016-08-30 23:41:58]

boliu: android_webview OWNERS

[boliu, 2016-08-30 23:42:58]

On 2016/08/30 23:41:58, enne wrote:
> boliu: android_webview OWNERS

lgtm

[enne (OOO), 2016-08-31 17:43:40]

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

[commit-bot: I haz the power, 2016-08-31 17:44:16]

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-08-31 18:57:52]

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

[commit-bot: I haz the power, 2016-08-31 18:57:53]

Dry run: This issue passed the CQ dry run.

[no sievers, 2016-08-31 19:10:57]

On 2016/08/30 22:19:00, enne wrote:
> sievers: content/browser/renderer_host/ OWNERS

lgtm

[enne (OOO), 2016-08-31 20:15:09]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-08-31 20:15:10]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
fsamuel@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2286273003/#ps80001
(title: "Fix webview")

[commit-bot: I haz the power, 2016-08-31 20:15:39]

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-08-31 20:20:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-31 20:23:25]

Description was changed from

==========
Make cc::Display not own its BeginFrameSource

This is refactoring to allow MusBrowserCompositorOutputSurface to
provide an ExternalBeginFrameSource that it owns rather than
passing ownership to Display.

This should also make it possible to remove begin frame sources from
OutputSurface in the future after the OutputSurface/CompositorFrameSink
split.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make cc::Display not own its BeginFrameSource

This is refactoring to allow MusBrowserCompositorOutputSurface to
provide an ExternalBeginFrameSource that it owns rather than
passing ownership to Display.

This should also make it possible to remove begin frame sources from
OutputSurface in the future after the OutputSurface/CompositorFrameSink
split.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/9848a61393772cc8a9aa8349c68f7d436c743369
Cr-Commit-Position: refs/heads/master@{#415738}
==========

[commit-bot: I haz the power, 2016-08-31 20:23:26]

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

[Ken Russell (switch to Gerrit), 2016-09-01 00:08:02]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2304483002/ by kbr@chromium.org.

The reason for reverting is: Suspect this may have induced renderer crashes in
the context_lost tests; see http://crbug.com/642984 .
.