BrowserMainLoop owns FrameSinkManagerHost

BrowserMainLoop owns FrameSinkManagerHost

A world with the display compositor in the GPU process is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
owned by BrowserMainLoop that can be accessed from both content/ and ui/ 
and does not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=730193
Review-Url: https://codereview.chromium.org/2932893002
Cr-Commit-Position: refs/heads/master@{#478766}
Committed: https://chromium.googlesource.com/chromium/src/+/ea6969825e1264bf84d00ead0c7f8e849e06f570

[Fady Samuel, 2017-06-09 19:55:28]

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

[commit-bot: I haz the power, 2017-06-09 19:55:56]

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, 2017-06-09 20:13:58]

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

[commit-bot: I haz the power, 2017-06-09 20:13:59]

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

[Fady Samuel, 2017-06-09 20:36:09]

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

[commit-bot: I haz the power, 2017-06-09 20:36:33]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-06-09 20:39:07]

Description was changed from

==========
Make FrameSinkManagerHost

A world with the display compositor in the GPU process, is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
a singleton that can be accessed from both content/ and ui/ and does
not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=none
==========

to

==========
Make FrameSinkManagerHost

A world with the display compositor in the GPU process is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
owned by BrowserMainLoop that can be accessed from both content/ and ui/ 
and does not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=none
==========

[Fady Samuel, 2017-06-09 20:39:34]

Description was changed from

==========
Make FrameSinkManagerHost

A world with the display compositor in the GPU process is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
owned by BrowserMainLoop that can be accessed from both content/ and ui/ 
and does not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=none
==========

to

==========
BrowserMainLoop owns FrameSinkManagerHost

A world with the display compositor in the GPU process is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
owned by BrowserMainLoop that can be accessed from both content/ and ui/ 
and does not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=none
==========

[Fady Samuel, 2017-06-09 20:40:52]

Description was changed from

==========
BrowserMainLoop owns FrameSinkManagerHost

A world with the display compositor in the GPU process is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
owned by BrowserMainLoop that can be accessed from both content/ and ui/ 
and does not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=none
==========

to

==========
BrowserMainLoop owns FrameSinkManagerHost

A world with the display compositor in the GPU process is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
owned by BrowserMainLoop that can be accessed from both content/ and ui/ 
and does not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=730193
==========

[Fady Samuel, 2017-06-09 20:41:00]

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

[commit-bot: I haz the power, 2017-06-09 21:03:30]

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

[commit-bot: I haz the power, 2017-06-09 21:03:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[piman, 2017-06-09 21:09:28]

https://codereview.chromium.org/2932893002/diff/20001/content/browser/composi...
File content/browser/compositor/surface_utils.cc (left):

https://codereview.chromium.org/2932893002/diff/20001/content/browser/composi...
content/browser/compositor/surface_utils.cc:178: return
CompositorImpl::GetFrameSinkManagerHost();
Should we remove the FrameSinkManagerHost in CompositorImpl then? Otherwise I
think we'll end up with 2 of them.

[Fady Samuel, 2017-06-09 21:49:17]

PTAL Antoine! I've fixed offscreen canvas unit tests to take in a
FrameSinkManagerHost.

https://codereview.chromium.org/2932893002/diff/20001/content/browser/composi...
File content/browser/compositor/surface_utils.cc (left):

https://codereview.chromium.org/2932893002/diff/20001/content/browser/composi...
content/browser/compositor/surface_utils.cc:178: return
CompositorImpl::GetFrameSinkManagerHost();
On 2017/06/09 21:09:28, piman wrote:
> Should we remove the FrameSinkManagerHost in CompositorImpl then? Otherwise I
> think we'll end up with 2 of them.

Done.

[Fady Samuel, 2017-06-09 21:50:17]

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

[commit-bot: I haz the power, 2017-06-09 21:50:41]

Dry run: CQ is trying da patch.

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

[piman, 2017-06-09 22:16:24]

lgtm

[danakj, 2017-06-09 22:38:11]

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

[danakj, 2017-06-09 22:38:12]

When FrameSinkManagerHost needs a mojo pointer to the Gpu process, we can't
start it until BrowserMainLoop::BrowserThreadsStarted? But it's currently in
BrowserMainLoop::PostMainMessageLoopStart, is that a problem?

[danakj, 2017-06-09 22:43:37]

https://codereview.chromium.org/2932893002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2932893002/diff/40001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_provider_impl_unittest.cc:151:
frame_sink_manager_host_ = base::MakeUnique<viz::FrameSinkManagerHost>();
Please destroy things in TearDown that are created in SetUp.

[Fady Samuel, 2017-06-09 22:52:39]

Thanks Dana! I've addressed your comments. Sending to CQ.

https://codereview.chromium.org/2932893002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2932893002/diff/40001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_provider_impl_unittest.cc:151:
frame_sink_manager_host_ = base::MakeUnique<viz::FrameSinkManagerHost>();
On 2017/06/09 22:43:37, danakj wrote:
> Please destroy things in TearDown that are created in SetUp.

Done.

[Fady Samuel, 2017-06-09 22:52:47]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-06-09 22:52:48]

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

[commit-bot: I haz the power, 2017-06-09 22:53:25]

CQ is trying da patch.

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

[danakj, 2017-06-09 22:54:09]

https://codereview.chromium.org/2932893002/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2932893002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1461: if
(!service_manager::ServiceManagerIsRemote()) {
Don't we need a FrameSinkManagerHost when the ServiceManagerIsRemote too?

[Fady Samuel, 2017-06-09 22:55:01]

https://codereview.chromium.org/2932893002/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2932893002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1461: if
(!service_manager::ServiceManagerIsRemote()) {
On 2017/06/09 22:54:09, danakj wrote:
> Don't we need a FrameSinkManagerHost when the ServiceManagerIsRemote too?

No, the FrameSinkManagerHost is in the window server not in the browser process
when running in Mus+Ash.

[danakj, 2017-06-09 22:57:45]

LGTM

https://codereview.chromium.org/2932893002/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2932893002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1461: if
(!service_manager::ServiceManagerIsRemote()) {
On 2017/06/09 22:55:00, Fady Samuel wrote:
> On 2017/06/09 22:54:09, danakj wrote:
> > Don't we need a FrameSinkManagerHost when the ServiceManagerIsRemote too?
> 
> No, the FrameSinkManagerHost is in the window server not in the browser
process
> when running in Mus+Ash.

Oh I see, this is different than FrameSinkManagerIsRemote :)

[commit-bot: I haz the power, 2017-06-10 01:58:59]

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

[commit-bot: I haz the power, 2017-06-10 01:59:00]

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, 2017-06-12 14:52:42]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2017-06-12 14:52:57]

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, 2017-06-12 16:30:23]

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

[commit-bot: I haz the power, 2017-06-12 16:30:24]

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, 2017-06-12 17:24:31]

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

[commit-bot: I haz the power, 2017-06-12 17:25:01]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-06-12 18:25:07]

PTAL Antoine! Are you OK with this CL as is for now? Dependency injecting
FrameSinkManagerHost into CompositorImpl on Android turns out to be a mess
because content::Compositor is accessed by Chrome for VR, and CompositorImpl has
a bunch of static accessors that need to be made non-static and so on.

It's doable, but it's a lot of mechanical work that isn't getting us closer to a
Chrome for Linux or Chrome OS build running with viz.

[commit-bot: I haz the power, 2017-06-12 18:45:58]

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

[commit-bot: I haz the power, 2017-06-12 18:45:59]

Dry run: This issue passed the CQ dry run.

[piman, 2017-06-12 19:14:41]

I don't want to block too much, so LGTM, but I want us to revisit. I don't think
we can punt Android forever, and we should at least think about how it will fit
before we commit to a design.

https://codereview.chromium.org/2932893002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2932893002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.h:168: #if !defined(OS_ANDROID)
nit: Could you file a bug with the breadcrumbs you have about how one would go
and unify these things, and add a TODO here?

[Fady Samuel, 2017-06-12 19:54:22]

Thanks Antoine, Dana!

https://codereview.chromium.org/2932893002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2932893002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.h:168: #if !defined(OS_ANDROID)
On 2017/06/12 19:14:41, piman wrote:
> nit: Could you file a bug with the breadcrumbs you have about how one would go
> and unify these things, and add a TODO here?

https://bugs.chromium.org/p/chromium/issues/detail?id=732507 filed. Thanks!

[Fady Samuel, 2017-06-12 19:54:27]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-06-12 19:54:27]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2932893002/#ps120001
(title: "Added a comment that works everywhere")

[commit-bot: I haz the power, 2017-06-12 19:55:06]

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, 2017-06-12 21:14:34]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497297267275190,
"parent_rev": "57cbe4e5fd1f7d7c24b404b783e0587aa97cbfd3", "commit_rev":
"ea6969825e1264bf84d00ead0c7f8e849e06f570"}

[commit-bot: I haz the power, 2017-06-12 21:14:48]

Description was changed from

==========
BrowserMainLoop owns FrameSinkManagerHost

A world with the display compositor in the GPU process is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
owned by BrowserMainLoop that can be accessed from both content/ and ui/ 
and does not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=730193
==========

to

==========
BrowserMainLoop owns FrameSinkManagerHost

A world with the display compositor in the GPU process is a world
without GpuProcessTransportFactory and ImageTransportFactory.
ImageTransportFactory is a singleton. This CL makes FrameSinkManagerHost
owned by BrowserMainLoop that can be accessed from both content/ and ui/ 
and does not depend on ImageTransportFactory/GpuProcessTransportFactory.

BUG=730193

Review-Url: https://codereview.chromium.org/2932893002
Cr-Commit-Position: refs/heads/master@{#478766}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea6969825e1264bf84d00ead0c7f...
==========

[commit-bot: I haz the power, 2017-06-12 21:14:50]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ea6969825e1264bf84d00ead0c7f...