Mus: Implement GpuMain mojo interface

Mus: Implement GpuMain mojo interface

The GpuMain mojo interface is the single primordial mojo interface used
by the window server to start and restart the gpu process in order to
ensure that requesting other sub-interfaces doesn't restart a crashed
GPU process and put it in a bad state.

GpuMain provides factory methods to create two sub-services: the gpu
service and the display compositor service. The gpu service does not
directly know about the display compositor service, and the display
compositor service does not know about the gpu service. GpuMain's
implementation is the glue that puts both together.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/2a2351a9d9ae6af2c9222217aa21272a17d24cc9
Cr-Commit-Position: refs/heads/master@{#435509}

[Fady Samuel, 2016-11-29 18:41:01]

Description was changed from

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

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

to

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Fady Samuel, 2016-11-29 18:41:02]

Description was changed from

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2016-11-29 19:00:33]

Description was changed from

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2016-11-29 19:00:33]

Description was changed from

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2016-11-29 19:00:54]

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

[Fady Samuel, 2016-11-29 19:03:29]

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

[commit-bot: I haz the power, 2016-11-29 19:03:52]

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

[sadrul, 2016-11-29 19:46:41]

https://codereview.chromium.org/2539893002/diff/40001/services/ui/gpu/gpu_mai...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2539893002/diff/40001/services/ui/gpu/gpu_mai...
services/ui/gpu/gpu_main.cc:118: // initialized.
DCHECK()?

https://codereview.chromium.org/2539893002/diff/40001/services/ui/ws/gpu_serv...
File services/ui/ws/gpu_service_proxy.cc (right):

https://codereview.chromium.org/2539893002/diff/40001/services/ui/ws/gpu_serv...
services/ui/ws/gpu_service_proxy.cc:57: std::move(client));
Dumb question: why can't WindowServer do this:
  connector->ConnectToInterface("gpu", &display_compositor_);
  display_compositor_->SetClient(std::move(display_compositor_client_binding_));

[commit-bot: I haz the power, 2016-11-29 19:46:58]

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

[commit-bot: I haz the power, 2016-11-29 19:46:59]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[Fady Samuel, 2016-11-29 20:08:58]

I didn't apply your suggestions because they're hard. :P If you really want me
to, then I will.

https://codereview.chromium.org/2539893002/diff/40001/services/ui/gpu/gpu_mai...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2539893002/diff/40001/services/ui/gpu/gpu_mai...
services/ui/gpu/gpu_main.cc:118: // initialized.
On 2016/11/29 19:46:41, sadrul wrote:
> DCHECK()?

GpuMain doesn't know about GpuServiceInternal::Initialize so there's no easy way
to add a DCHECK that doesn't involve a bunch of extra plumbing. If you feel it's
sufficiently important I am happy to add.

https://codereview.chromium.org/2539893002/diff/40001/services/ui/ws/gpu_serv...
File services/ui/ws/gpu_service_proxy.cc (right):

https://codereview.chromium.org/2539893002/diff/40001/services/ui/ws/gpu_serv...
services/ui/ws/gpu_service_proxy.cc:57: std::move(client));
On 2016/11/29 19:46:41, sadrul wrote:
> Dumb question: why can't WindowServer do this:
>   connector->ConnectToInterface("gpu", &display_compositor_);
>  
display_compositor_->SetClient(std::move(display_compositor_client_binding_));

It can. It's a matter of style I guess. I see no compelling reason to go one way
over the other. If you feel strongly I'll change it. For now, I'm leaving it as
is.

[Fady Samuel, 2016-11-29 20:09:08]

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

[commit-bot: I haz the power, 2016-11-29 20:09:43]

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-11-29 22:20:57]

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

[commit-bot: I haz the power, 2016-11-29 22:20:58]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-11-30 19:13:10]

Description was changed from

==========
Mus: Implement DisplayCompositorFactory mojo interface

The Gpu service should not know about the display compositor as that's a
layer above gpu: the display compositor is a consumer of the gpu service.
This implements the same DisplayCompositorFactory interface used in Jellyfish.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Mus: Implement GpuMain mojo interface

The GpuMain mojo interface is the single primordial mojo interface used 
by the window server to start and restart the gpu process in order to 
ensure that requesting other sub-interfaces doesn't restart a crashed GPU
process and put it in a bad state.

GpuMain provides factory methods to create two sub-services: the gpu
service and the display compositor service. The gpu service does not
directly know about the display compositor service, and the display
compositor service does not know about the gpu service. GpuMain's
implementation is the glue that puts both together.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2016-11-30 19:13:29]

Description was changed from

==========
Mus: Implement GpuMain mojo interface

The GpuMain mojo interface is the single primordial mojo interface used 
by the window server to start and restart the gpu process in order to 
ensure that requesting other sub-interfaces doesn't restart a crashed GPU
process and put it in a bad state.

GpuMain provides factory methods to create two sub-services: the gpu
service and the display compositor service. The gpu service does not
directly know about the display compositor service, and the display
compositor service does not know about the gpu service. GpuMain's
implementation is the glue that puts both together.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Mus: Implement GpuMain mojo interface

The GpuMain mojo interface is the single primordial mojo interface used 
by the window server to start and restart the gpu process in order to 
ensure that requesting other sub-interfaces doesn't restart a crashed 
GPU process and put it in a bad state.

GpuMain provides factory methods to create two sub-services: the gpu
service and the display compositor service. The gpu service does not
directly know about the display compositor service, and the display
compositor service does not know about the gpu service. GpuMain's
implementation is the glue that puts both together.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2016-11-30 19:16:07]

PTAL sadrul@: I've reworked this CL based on offline conversations.

GpuMain is the central interface to the gpu process. It
allows you to create a display compositor and gpu service.
Requesting these interfaces can happen in either order.

[Fady Samuel, 2016-11-30 19:16:32]

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

[commit-bot: I haz the power, 2016-11-30 19:17:01]

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-11-30 20:24:11]

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

[commit-bot: I haz the power, 2016-11-30 20:24:12]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-11-30 21:05:55]

I like it! Some nitty/commentsy comments. I will stare at the thread-hopping
dance in GpuMain for a little longer, but I think this looks good.

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:112: // The client thread will outlive the gpu
thread and so it's safe to use
Did you mean s/The client thread/|this|/?

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:122: // The client thread will outlive the gpu
thread and so it's safe to use
ditto

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:233: FROM_HERE,
base::Bind(&CallCreateGpuServiceCallbackOnOriginThread,
Maybe just 'base::Bind(callback, gpu_info)' here?

https://codereview.chromium.org/2539893002/diff/120001/services/ui/ws/gpu_ser...
File services/ui/ws/gpu_service_proxy.cc (right):

https://codereview.chromium.org/2539893002/diff/120001/services/ui/ws/gpu_ser...
services/ui/ws/gpu_service_proxy.cc:37: //  
connector->ConnectToInterface("gpu", &gpu_service_);
Update the comment. I believe we will be getting |gpu_main_| from the connector
instead, right?

https://codereview.chromium.org/2539893002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2539893002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_server.cc:66: // the gpu service must live on a separate
thread.
This comment should in gpu_main.cc, right? It seems out of place here in
WindowServer (which should not need to know/care about that detail).

[sadrul, 2016-11-30 21:13:14]

Sorry, a couple more comments. With that, I conclude my review. lgtm

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:166: gpu_command_service_ = new
gpu::GpuInProcessThreadService(
Do we want to DCHECK() here that these don't already exist?

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:222: if (gpu_service_internal_)
Maybe add a comment mentioning when this can be null?

[Fady Samuel, 2016-11-30 21:34:48]

fsamuel@chromium.org changed reviewers:
+ jbauman@chromium.org, tsepez@chromium.org

[Fady Samuel, 2016-11-30 21:34:49]

+tsepez@ for mojoms
+jbauman for gpu

I'm working on sadrul@'s nits.

[Tom Sepez, 2016-11-30 21:45:10]

mojom LGTM

[Fady Samuel, 2016-11-30 22:53:19]

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

[Fady Samuel, 2016-11-30 22:53:20]

+sky@ for window_server

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:112: // The client thread will outlive the gpu
thread and so it's safe to use
On 2016/11/30 21:05:55, sadrul wrote:
> Did you mean s/The client thread/|this|/?

Done.

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:122: // The client thread will outlive the gpu
thread and so it's safe to use
On 2016/11/30 21:05:55, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:166: gpu_command_service_ = new
gpu::GpuInProcessThreadService(
On 2016/11/30 21:13:14, sadrul wrote:
> Do we want to DCHECK() here that these don't already exist?

Done.

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:222: if (gpu_service_internal_)
On 2016/11/30 21:13:14, sadrul wrote:
> Maybe add a comment mentioning when this can be null?

I don't think it can be actually because we create it at startup on the gpu
thread. This task happens after. I've removed the check.

https://codereview.chromium.org/2539893002/diff/120001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:233: FROM_HERE,
base::Bind(&CallCreateGpuServiceCallbackOnOriginThread,
On 2016/11/30 21:05:55, sadrul wrote:
> Maybe just 'base::Bind(callback, gpu_info)' here?

Done.

https://codereview.chromium.org/2539893002/diff/120001/services/ui/ws/gpu_ser...
File services/ui/ws/gpu_service_proxy.cc (right):

https://codereview.chromium.org/2539893002/diff/120001/services/ui/ws/gpu_ser...
services/ui/ws/gpu_service_proxy.cc:37: //  
connector->ConnectToInterface("gpu", &gpu_service_);
On 2016/11/30 21:05:55, sadrul wrote:
> Update the comment. I believe we will be getting |gpu_main_| from the
connector
> instead, right?

Done.

https://codereview.chromium.org/2539893002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2539893002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_server.cc:66: // the gpu service must live on a separate
thread.
On 2016/11/30 21:05:55, sadrul wrote:
> This comment should in gpu_main.cc, right? It seems out of place here in
> WindowServer (which should not need to know/care about that detail).

Done.

[jbauman, 2016-11-30 22:55:59]

gpu/ lgtm

[Fady Samuel, 2016-11-30 22:58:57]

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

[commit-bot: I haz the power, 2016-11-30 22:59:11]

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

[sky, 2016-11-30 23:10:50]

LGTM

[commit-bot: I haz the power, 2016-12-01 00:09:08]

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

[commit-bot: I haz the power, 2016-12-01 00:09:09]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-12-01 01:01:57]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-12-01 01:01:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2539893002/#ps140001
(title: "Addressed Sadrul's comments")

[commit-bot: I haz the power, 2016-12-01 01:02:25]

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-01 01:08:07]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480554117279360,
"parent_rev": "69c4c2fae2ade8ef33a30d250d570275e4089e0e", "commit_rev":
"d63f06a9411cf068592a0fea25b059fe69ec9a8c"}

[commit-bot: I haz the power, 2016-12-01 01:09:36]

Description was changed from

==========
Mus: Implement GpuMain mojo interface

The GpuMain mojo interface is the single primordial mojo interface used 
by the window server to start and restart the gpu process in order to 
ensure that requesting other sub-interfaces doesn't restart a crashed 
GPU process and put it in a bad state.

GpuMain provides factory methods to create two sub-services: the gpu
service and the display compositor service. The gpu service does not
directly know about the display compositor service, and the display
compositor service does not know about the gpu service. GpuMain's
implementation is the glue that puts both together.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Mus: Implement GpuMain mojo interface

The GpuMain mojo interface is the single primordial mojo interface used 
by the window server to start and restart the gpu process in order to 
ensure that requesting other sub-interfaces doesn't restart a crashed 
GPU process and put it in a bad state.

GpuMain provides factory methods to create two sub-services: the gpu
service and the display compositor service. The gpu service does not
directly know about the display compositor service, and the display
compositor service does not know about the gpu service. GpuMain's
implementation is the glue that puts both together.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2016-12-01 01:09:38]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-01 01:14:16]

Description was changed from

==========
Mus: Implement GpuMain mojo interface

The GpuMain mojo interface is the single primordial mojo interface used 
by the window server to start and restart the gpu process in order to 
ensure that requesting other sub-interfaces doesn't restart a crashed 
GPU process and put it in a bad state.

GpuMain provides factory methods to create two sub-services: the gpu
service and the display compositor service. The gpu service does not
directly know about the display compositor service, and the display
compositor service does not know about the gpu service. GpuMain's
implementation is the glue that puts both together.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Mus: Implement GpuMain mojo interface

The GpuMain mojo interface is the single primordial mojo interface used
by the window server to start and restart the gpu process in order to
ensure that requesting other sub-interfaces doesn't restart a crashed
GPU process and put it in a bad state.

GpuMain provides factory methods to create two sub-services: the gpu
service and the display compositor service. The gpu service does not
directly know about the display compositor service, and the display
compositor service does not know about the gpu service. GpuMain's
implementation is the glue that puts both together.

BUG=610937
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

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

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