Introduce DisplayCompositor Mojo API for Mus

Introduce DisplayCompositor Mojo API for Mus

This CL introduces the following Mojo interfaces:

1. DisplayCompositor: represents a connection to the DisplayCompositor
    service in Mus-Gpu.
2. Display: represents a compositor for a single physical display.
3. CompositorFrameSinkFactory per display client process.
4. CompositorFrameSink: per client.
5. CompositorFrameSinkClient: events from the display compositor
pertaining to a specific client.

This CL only provides skeleton implementations for
CompositorFrameSink and CompositorFrameSinkFactory. Other
interfaces and integration in Mus will follow.

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

Committed: https://crrev.com/7c7022d40aae6dcb714b2453b349068f4ccced5d
Cr-Commit-Position: refs/heads/master@{#396364}

[Fady Samuel, 2016-05-18 22:03:58]

Description was changed from

==========
WIP: Introduce DisplayCompositor Mojo API for Mus

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

to

==========
Introduce DisplayCompositor Mojo API for Mus

This CL introduces the following Mojo interfaces:

1. DisplayCompositor: represents a connection to the DisplayCompositor 
    service in Mus-Gpu.
2. Display: represents a compositor for a single physical display.
3. CompositorFrameSinkFactory per display client process. 
4. CompositorFrameSink: per client.
5. CompositorFrameSinkClient: events from the display compositor
pertaining to a specific client.

This CL only provides skeleton implementations for 
CompositorFrameSink and CompositorFrameSinkFactory. Other 
interfaces and integration in Mus will follow.

BUG=610937
==========

[Fady Samuel, 2016-05-18 22:04:09]

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

[Fady Samuel, 2016-05-19 03:03:20]

PTAL Rob! I have iterated a bit more on this API. I'm fairly happy with it at
this point:

Display and DisplayCompositor are privileged interfaces that only mus-ws can
access.

mus-ws is considered a root client.

DisplayClient is implemented by both mus-ws, and other clients. mus-ws gets
OnRootClientCreated(..) on its DisplayClientImpl.

CompositorFrameSinkFactory is only available on non-root clients.

Both mus-ws and non-root clients get a CompositorFrameSink, and implement a
CompositorFrameSinkClient.

mus/public can wrap non-root clients as ServerWindowSurfaces as usual.

Display lets us set compositing permissions. Client A can embed Client B if A is
an ancestor of B. Note that this API allows for a DAG, not just a tree.
Technically this API allows for cycles but SurfaceAggregator knows how to
resolve cycles.

[Fady Samuel, 2016-05-19 03:16:34]

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:67: CreateClient(int32
client_id, DisplayClient& client);
The more I think about this, the more I think the signature of CreateClient
should be this:

CreateClient(int32 parent_id, int32 client_id, DisplayClient& client);

This API will only be used for non-root clients, and non-root clients need to
have a parent to be displayable. For top-level windows in Mus+Ash, the parent_id
will always be 0 (or whatever we use to represent the root display client).

[Fady Samuel, 2016-05-19 15:17:42]

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:38:
SetNeedsBeginFrame(bool needs_begin_frame);
ReadbackBitmap API makes sense here.

[rjkroege, 2016-05-19 22:08:38]

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:17: // A
CompositorFrameSinkFactory represents a single Display client.
I thought you were renaming the CFSF to DisplayCompositor?

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:22:
CreateCompositorFrameSink(int32 id,
document the arguments.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:28: // A
CompositorFrame contains the complete output meant for display.
Please say how a CFS is going to be used? Remember that these mojom are an
interface boundary

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:33:
SubmitCompositorFrame(CompositorFrame frame) =>
How does the client provide its async callback to receive status? When does the
ack arrive? Perhaps the ack should be part of the CompositorFrameSinkClient? The
callback should perhaps include the CF

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:36: // Lets the
display know that the client wishes to receive the next
Display is ambiguous. Indicate what kind of display you mean.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:38:
SetNeedsBeginFrame(bool needs_begin_frame);
On 2016/05/19 15:17:42, Fady Samuel wrote:
> ReadbackBitmap API makes sense here.

add a TODO for it?

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:45: // This is a
public interface implemented by Display clients.
Only the window server will ever be a DisplayClient? Can you say this?

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:57: // TODO(fsamuel):
Add BeginFrameArgs.
what did you have in mind?

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:62: // There is one
Display per physical display on Chrome OS,
is there an 1-1 correspondence between Display, PlatformWindow, accelerated
widget? If so, say so

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
File components/mus/surfaces/compositor_frame_sink_delegate.h (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_delegate.h:12: class
CompositorFrameSinkDelegate {
What's it for?

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
File components/mus/surfaces/compositor_frame_sink_factory_impl.h (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_factory_impl.h:27: void
CompositorFrameSinkConnectionLost(int sink_id) override;
// CompositorFrameSinkDelegate implementation?

And, you envisage the need for Delegate how?

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
File components/mus/surfaces/compositor_frame_sink_impl.cc (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_impl.cc:60: // TODO(fsamuel):
Validate that SurfaceDrawQuad refer to allowable surface
the validator should be a delegate.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
File components/mus/surfaces/compositor_frame_sink_impl.h (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_impl.h:21: // A client presents
visuals to screen by submitting CompositorFrames to a
I think that the .mojom should be the repository of docs. And it would be nice
to have longer more helpful docs: e.g. base/callback.h lets one figure out how
to use the API.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_impl.h:39: const
SubmitCompositorFrameCallback& callback) override;
I want to know what happens when without reading lots or code. In particular:
there is a temporal relationship between callback's invocation and OnBeginFrame.
Sometimes. When and what is it?

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_impl.h:43: void
ReturnResources(const cc::ReturnedResourceArray& resources) override;
why is this a SurfaceFactoryClient implementation?

isn't a CFS a Surface?

[Fady Samuel, 2016-05-25 12:37:01]

PTAL Rob! Thanks!

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:17: // A
CompositorFrameSinkFactory represents a single Display client.
On 2016/05/19 22:08:37, rjkroege wrote:
> I thought you were renaming the CFSF to DisplayCompositor?

I needed to separate privileged API and unprivileged API so I couldn't make that
work. I'm sure we'll end up renaming everything a few times. I'd like to get the
plumbing done first then we can rework naming.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:22:
CreateCompositorFrameSink(int32 id,
On 2016/05/19 22:08:37, rjkroege wrote:
> document the arguments.

Done.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:28: // A
CompositorFrame contains the complete output meant for display.
On 2016/05/19 22:08:37, rjkroege wrote:
> Please say how a CFS is going to be used? Remember that these mojom are an
> interface boundary

Done.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:33:
SubmitCompositorFrame(CompositorFrame frame) =>
On 2016/05/19 22:08:37, rjkroege wrote:
> How does the client provide its async callback to receive status? When does
the
> ack arrive? Perhaps the ack should be part of the CompositorFrameSinkClient?
The
> callback should perhaps include the CF

I don't think that makes sense. This callback will be used by the outputsurface
I believe. It could indeed live on the client interface, but it doesn't have to
I think. I don't think there are any compelling reasons to go one way or
another. I'll stick with this for now, and add more comments.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:36: // Lets the
display know that the client wishes to receive the next
On 2016/05/19 22:08:37, rjkroege wrote:
> Display is ambiguous. Indicate what kind of display you mean.

Done.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:38:
SetNeedsBeginFrame(bool needs_begin_frame);
On 2016/05/19 22:08:37, rjkroege wrote:
> On 2016/05/19 15:17:42, Fady Samuel wrote:
> > ReadbackBitmap API makes sense here.
> 
> add a TODO for it?

Done.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:45: // This is a
public interface implemented by Display clients.
On 2016/05/19 22:08:37, rjkroege wrote:
> Only the window server will ever be a DisplayClient? Can you say this?

That is not the case. All things that talk to the Display are DisplayClients
even unprivileged clients.

There are two callbacks: "OnRootClientCreated" which only the window server gets
and "OnClientCreated" which non-root clients get.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:57: // TODO(fsamuel):
Add BeginFrameArgs.
On 2016/05/19 22:08:37, rjkroege wrote:
> what did you have in mind?

See cc/output/begin_frame_args.h. I added a comment.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:62: // There is one
Display per physical display on Chrome OS,
On 2016/05/19 22:08:37, rjkroege wrote:
> is there an 1-1 correspondence between Display, PlatformWindow, accelerated
> widget? If so, say so

Done.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:67: CreateClient(int32
client_id, DisplayClient& client);
On 2016/05/19 03:16:34, Fady Samuel wrote:
> The more I think about this, the more I think the signature of CreateClient
> should be this:
> 
> CreateClient(int32 parent_id, int32 client_id, DisplayClient& client);
> 
> This API will only be used for non-root clients, and non-root clients need to
> have a parent to be displayable. For top-level windows in Mus+Ash, the
parent_id
> will always be 0 (or whatever we use to represent the root display client).

Now that the security issue has been resolved here:
https://codereview.chromium.org/1996783002/ we can keep this API simple.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
File components/mus/surfaces/compositor_frame_sink_delegate.h (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_delegate.h:12: class
CompositorFrameSinkDelegate {
On 2016/05/19 22:08:38, rjkroege wrote:
> What's it for?

For future unittesting.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
File components/mus/surfaces/compositor_frame_sink_factory_impl.h (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_factory_impl.h:27: void
CompositorFrameSinkConnectionLost(int sink_id) override;
On 2016/05/19 22:08:38, rjkroege wrote:
> // CompositorFrameSinkDelegate implementation?
> 
> And, you envisage the need for Delegate how?

For unit testing. One can test this class without having to go out to the
factory.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
File components/mus/surfaces/compositor_frame_sink_impl.cc (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_impl.cc:60: // TODO(fsamuel):
Validate that SurfaceDrawQuad refer to allowable surface
On 2016/05/19 22:08:38, rjkroege wrote:
> the validator should be a delegate.

Acknowledged.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
File components/mus/surfaces/compositor_frame_sink_impl.h (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_impl.h:21: // A client presents
visuals to screen by submitting CompositorFrames to a
On 2016/05/19 22:08:38, rjkroege wrote:
> I think that the .mojom should be the repository of docs. And it would be nice
> to have longer more helpful docs: e.g. base/callback.h lets one figure out how
> to use the API.

Agreed, but I don't think we're ready for that yet. I'd like to iterate over
this API rapidly upstream first.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_impl.h:39: const
SubmitCompositorFrameCallback& callback) override;
On 2016/05/19 22:08:38, rjkroege wrote:
> I want to know what happens when without reading lots or code. In particular:
> there is a temporal relationship between callback's invocation and
OnBeginFrame.
> Sometimes. When and what is it?

I'm not sure if we've figured this out yet. Chrome has both. I need to figure
out if we actually need this callback anymore.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/surfaces...
components/mus/surfaces/compositor_frame_sink_impl.h:43: void
ReturnResources(const cc::ReturnedResourceArray& resources) override;
On 2016/05/19 22:08:38, rjkroege wrote:
> why is this a SurfaceFactoryClient implementation?
> 
> isn't a CFS a Surface?

CompositorFrameSinkImpl is a SurfaceFactoryClient in order to send messages back
to the client via CompositorFrameSinkClient.

[rjkroege, 2016-05-25 19:14:20]

ask: can you come up with a naming scheme that makes it obvious at a glance
which interfaces are for all clients and which are only for the WS?

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:17: // A
CompositorFrameSinkFactory represents a single Display client.
On 2016/05/25 12:37:00, Fady Samuel wrote:
> On 2016/05/19 22:08:37, rjkroege wrote:
> > I thought you were renaming the CFSF to DisplayCompositor?
> 
> I needed to separate privileged API and unprivileged API so I couldn't make
that
> work. I'm sure we'll end up renaming everything a few times. I'd like to get
the
> plumbing done first then we can rework naming.

please indicate very precisely which interfaces are for all and which are only
for mus-ws.

https://codereview.chromium.org/1989693002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:33:
SubmitCompositorFrame(CompositorFrame frame) =>
On 2016/05/25 12:37:00, Fady Samuel wrote:
> On 2016/05/19 22:08:37, rjkroege wrote:
> > How does the client provide its async callback to receive status? When does
> the
> > ack arrive? Perhaps the ack should be part of the CompositorFrameSinkClient?
> The
> > callback should perhaps include the CF
> 
> I don't think that makes sense. This callback will be used by the
outputsurface
> I believe. It could indeed live on the client interface, but it doesn't have
to
> I think. 

OK

> I don't think there are any compelling reasons to go one way or
> another. I'll stick with this for now, and add more comments.

I however believe that the ack should specify (in some way) which
CompositorFrame the ack is for -- I could easily imagine a situation where the
client has many CFs in flight -- e.g. blimp.

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:28: // 3. |nonce| is a
cryptographically secure random number making this Sink
the client picks the nonce? Clients can lie. Should mus be providing the nonce?

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:45: // submissions.
Why would the CompositorFrame be discarded?

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:61: interface
DisplayClient {
If only musws get OnRootClientCreated, why is this part of the same interface? A
!ws client would never get OnRootClientCreated?

If so, the interface should be separate yes?

Moreover, the manifest file structure would suggest that the WS API be in a
different mojom from the general client?

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:70:
OnClientCreated(uint32 client_id, CompositorFrameSinkFactory factory);
question for my education: mojom maps uint32 to uint32_t in C++?

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:73: // See
cc/output/begin_frame_args.h.
words to the effect of "OnBeginFrame needs to take a BeginFrameArgs instance per
cc/output/begin_frame_args.h" would make your intent even clearer.

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:83:
CreateClient(uint32 client_id, DisplayClient& client);
bike-shedding: CreateRootClient or CreateWindowTreeClient?

[Fady Samuel, 2016-05-25 21:55:35]

PTAL Rob!

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:28: // 3. |nonce| is a
cryptographically secure random number making this Sink
On 2016/05/25 19:14:20, rjkroege wrote:
> the client picks the nonce? Clients can lie. Should mus be providing the
nonce?

Privileged clients will pick a cryptographically secure number. If a bad client
wishes to be embedded so be it. The goal here is for one client to bike an
unguessable id so that another client cannot embed it.

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:45: // submissions.
On 2016/05/25 19:14:19, rjkroege wrote:
> Why would the CompositorFrame be discarded?

If the client is submitting frames faster than the display is ticking.

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:61: interface
DisplayClient {
On 2016/05/25 19:14:20, rjkroege wrote:
> If only musws get OnRootClientCreated, why is this part of the same interface?
A
> !ws client would never get OnRootClientCreated?
> 
> If so, the interface should be separate yes?
> 
> Moreover, the manifest file structure would suggest that the WS API be in a
> different mojom from the general client?

Things like OnBeginFrame would overlap. I don't want to create two interfaces
when the concept of a "DisplayClient" is universal.

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:70:
OnClientCreated(uint32 client_id, CompositorFrameSinkFactory factory);
On 2016/05/25 19:14:20, rjkroege wrote:
> question for my education: mojom maps uint32 to uint32_t in C++?

Yes.

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:73: // See
cc/output/begin_frame_args.h.
On 2016/05/25 19:14:20, rjkroege wrote:
> words to the effect of "OnBeginFrame needs to take a BeginFrameArgs instance
per
> cc/output/begin_frame_args.h" would make your intent even clearer.

Acknowledged.

https://codereview.chromium.org/1989693002/diff/60001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:83:
CreateClient(uint32 client_id, DisplayClient& client);
On 2016/05/25 19:14:20, rjkroege wrote:
> bike-shedding: CreateRootClient or CreateWindowTreeClient?

This does not create a root client and it should not know about the window tree.

Maybe CreateDisplayClient?

[rjkroege, 2016-05-25 22:25:33]

https://codereview.chromium.org/1989693002/diff/80001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/80001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:78: // This is a
privileged API to be used only by the Mus Window Server.
I thought we'd agreed that you'd split apart the mojom?

[Fady Samuel, 2016-05-25 22:50:22]

PTAL Rob!

https://codereview.chromium.org/1989693002/diff/80001/components/mus/public/i...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/80001/components/mus/public/i...
components/mus/public/interfaces/display_compositor.mojom:78: // This is a
privileged API to be used only by the Mus Window Server.
On 2016/05/25 22:25:33, rjkroege wrote:
> I thought we'd agreed that you'd split apart the mojom?

We did. Done.

[rjkroege, 2016-05-25 23:01:55]

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:28: // 3. |nonce| is a
cryptographically secure random number making this Sink
I am not convinced that nonce generation should be a client responsibility. It
seems that this puts the onus on clients to do the "right" thing instead of code
in mus doing it once for everybody. Mojo apps cannot necessarily be relied to do
the right thing.

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:37: // A
CompositorFrame contains the complete output meant for display. Each time a
nit: This isn't quite true. But doesn't matter.

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:47:
(CompositorFrameDrawStatus status);
see previous comment set: I think we should support identifying the CF in the
callback. You address this a TODO.

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:66:
OnRootClientCreated(CompositorFrameSink sink,
you are still forcing DisplayClient implementations other than the Window server
to implement this interface. I am opposed.

[Fady Samuel, 2016-05-26 00:30:07]

PTAL Rob!

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:28: // 3. |nonce| is a
cryptographically secure random number making this Sink
On 2016/05/25 23:01:55, rjkroege wrote:
> I am not convinced that nonce generation should be a client responsibility. It
> seems that this puts the onus on clients to do the "right" thing instead of
code
> in mus doing it once for everybody. Mojo apps cannot necessarily be relied to
do
> the right thing.

Doing nonce more securely on the service is harder I think. If we do it naively
then we only get one nonce per client as opposed to a nonce per sink meaning if
a nonce is somehow guessed then all sinks within a client would be guessable.

If we create a nonce per sink in the browser, then we would need to:

1. Pass the nonce back to the client so it can embed things it owns. Doing this
asynchronously is hard.
2. Use a sync IPC, which is bad for performance.

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:37: // A
CompositorFrame contains the complete output meant for display. Each time a
On 2016/05/25 23:01:55, rjkroege wrote:
> nit: This isn't quite true. But doesn't matter.

Acknowledged.

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:47:
(CompositorFrameDrawStatus status);
On 2016/05/25 23:01:55, rjkroege wrote:
> see previous comment set: I think we should support identifying the CF in the
> callback. You address this a TODO.

Done.

https://codereview.chromium.org/1989693002/diff/100001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:66:
OnRootClientCreated(CompositorFrameSink sink,
On 2016/05/25 23:01:55, rjkroege wrote:
> you are still forcing DisplayClient implementations other than the Window
server
> to implement this interface. I am opposed.

Done. We now have a DisplayClient and DisplayHost. DisplayCompositor has gotten
a bit more complicated but oh well. We can clean things up in a client lib.

[rjkroege, 2016-05-26 00:48:57]

lgtm

[Fady Samuel, 2016-05-26 01:05:50]

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

[Fady Samuel, 2016-05-26 01:05:51]

+sky for mus/public

[sky, 2016-05-26 16:00:45]

https://codereview.chromium.org/1989693002/diff/120001/components/mus/public/...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/120001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:7: import
"components/mus/public/interfaces/compositor_frame.mojom";
WDYT of putting these interfaces in a different namespaces? I'm particularly
concerned about the Display* names you are adding, which conflict with
display.mojom and window_manager_constants and have totally different use.
Alternatively change the Display names to better reflect they are internal,
maybe prefix with Platform.

https://codereview.chromium.org/1989693002/diff/120001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:25: //   
CompositorFrameSinkFactory
nit: end with '.'

[Fady Samuel, 2016-05-26 19:35:26]

Description was changed from

==========
Introduce DisplayCompositor Mojo API for Mus

This CL introduces the following Mojo interfaces:

1. DisplayCompositor: represents a connection to the DisplayCompositor 
    service in Mus-Gpu.
2. Display: represents a compositor for a single physical display.
3. CompositorFrameSinkFactory per display client process. 
4. CompositorFrameSink: per client.
5. CompositorFrameSinkClient: events from the display compositor
pertaining to a specific client.

This CL only provides skeleton implementations for 
CompositorFrameSink and CompositorFrameSinkFactory. Other 
interfaces and integration in Mus will follow.

BUG=610937
==========

to

==========
Introduce DisplayCompositor Mojo API for Mus

This CL introduces the following Mojo interfaces:

1. DisplayCompositor: represents a connection to the DisplayCompositor 
    service in Mus-Gpu.
2. Display: represents a compositor for a single physical display.
3. CompositorFrameSinkFactory per display client process. 
4. CompositorFrameSink: per client.
5. CompositorFrameSinkClient: events from the display compositor
pertaining to a specific client.

This CL only provides skeleton implementations for 
CompositorFrameSink and CompositorFrameSinkFactory. Other 
interfaces and integration in Mus will follow.

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

[Fady Samuel, 2016-05-26 19:37:30]

PTAL Scott!

https://codereview.chromium.org/1989693002/diff/120001/components/mus/public/...
File components/mus/public/interfaces/display_compositor.mojom (right):

https://codereview.chromium.org/1989693002/diff/120001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:7: import
"components/mus/public/interfaces/compositor_frame.mojom";
On 2016/05/26 16:00:45, sky wrote:
> WDYT of putting these interfaces in a different namespaces? I'm particularly
> concerned about the Display* names you are adding, which conflict with
> display.mojom and window_manager_constants and have totally different use.
> Alternatively change the Display names to better reflect they are internal,
> maybe prefix with Platform.

Moved to mus.gpu.mojom and moved mojoms. I've updated the Impls, but they're now
in the wrong directory. I'll move surfaces mus/gpu in a subsequent CL.

https://codereview.chromium.org/1989693002/diff/120001/components/mus/public/...
components/mus/public/interfaces/display_compositor.mojom:25: //   
CompositorFrameSinkFactory
On 2016/05/26 16:00:45, sky wrote:
> nit: end with '.'

Done.

[Fady Samuel, 2016-05-26 19:49:19]

PTAL

[sky, 2016-05-26 21:05:27]

LGTM

[Fady Samuel, 2016-05-26 21:05:58]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-05-26 21:05:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/1989693002/#ps160001
(title: "Addressed nit")

[Fady Samuel, 2016-05-26 21:06:16]

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

[Fady Samuel, 2016-05-26 21:06:37]

+tsepez@ for ipc

[commit-bot: I haz the power, 2016-05-26 21:06:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989693002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989693002/160001

[commit-bot: I haz the power, 2016-05-26 21:16:18]

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

[commit-bot: I haz the power, 2016-05-26 21:16:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tom Sepez, 2016-05-26 23:41:20]

lgtm

https://codereview.chromium.org/1989693002/diff/160001/components/mus/surface...
File components/mus/surfaces/compositor_frame_sink_factory_impl.cc (right):

https://codereview.chromium.org/1989693002/diff/160001/components/mus/surface...
components/mus/surfaces/compositor_frame_sink_factory_impl.cc:39: //
TODO(fsamuel): Use nonce once patch lands:
ok.

[Fady Samuel, 2016-05-26 23:49:24]

CQ'ing.

https://codereview.chromium.org/1989693002/diff/160001/components/mus/surface...
File components/mus/surfaces/compositor_frame_sink_factory_impl.cc (right):

https://codereview.chromium.org/1989693002/diff/160001/components/mus/surface...
components/mus/surfaces/compositor_frame_sink_factory_impl.cc:39: //
TODO(fsamuel): Use nonce once patch lands:
On 2016/05/26 23:41:20, Tom Sepez wrote:
> ok.

Ahh, it has landed, I'll plumb nonce in a followup CL.

[Fady Samuel, 2016-05-26 23:49:34]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-05-26 23:49:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, tsepez@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1989693002/#ps180001
(title: "Rebased")

[commit-bot: I haz the power, 2016-05-26 23:50:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989693002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989693002/180001

[commit-bot: I haz the power, 2016-05-27 00:05:26]

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

[commit-bot: I haz the power, 2016-05-27 00:05:28]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Fady Samuel, 2016-05-27 00:44:29]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-05-27 00:44:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, tsepez@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1989693002/#ps200001
(title: "Fix client_id")

[commit-bot: I haz the power, 2016-05-27 00:45:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989693002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989693002/200001

[commit-bot: I haz the power, 2016-05-27 02:44:27]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-27 02:47:47]

Description was changed from

==========
Introduce DisplayCompositor Mojo API for Mus

This CL introduces the following Mojo interfaces:

1. DisplayCompositor: represents a connection to the DisplayCompositor 
    service in Mus-Gpu.
2. Display: represents a compositor for a single physical display.
3. CompositorFrameSinkFactory per display client process. 
4. CompositorFrameSink: per client.
5. CompositorFrameSinkClient: events from the display compositor
pertaining to a specific client.

This CL only provides skeleton implementations for 
CompositorFrameSink and CompositorFrameSinkFactory. Other 
interfaces and integration in Mus will follow.

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

to

==========
Introduce DisplayCompositor Mojo API for Mus

This CL introduces the following Mojo interfaces:

1. DisplayCompositor: represents a connection to the DisplayCompositor
    service in Mus-Gpu.
2. Display: represents a compositor for a single physical display.
3. CompositorFrameSinkFactory per display client process.
4. CompositorFrameSink: per client.
5. CompositorFrameSinkClient: events from the display compositor
pertaining to a specific client.

This CL only provides skeleton implementations for
CompositorFrameSink and CompositorFrameSinkFactory. Other
interfaces and integration in Mus will follow.

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

Committed: https://crrev.com/7c7022d40aae6dcb714b2453b349068f4ccced5d
Cr-Commit-Position: refs/heads/master@{#396364}
==========

[commit-bot: I haz the power, 2016-05-27 02:47:48]

Patchset 11 (id:??) landed as
https://crrev.com/7c7022d40aae6dcb714b2453b349068f4ccced5d
Cr-Commit-Position: refs/heads/master@{#396364}