Move part of display_compositor's interface to cc/ipc

Move part of display_compositor's interface to cc/ipc

In display_compositor, we have CompositorFrameSink and CompositorFrameSinkClient
interface. Since we already have cc/output/compositor_frame_sink class,
it makes more sense to move these two interfaces to cc/ipc.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/284a2b2247742cfa9e7039dbce4bf75900272bd6
Cr-Commit-Position: refs/heads/master@{#419248}

[xidachen, 2016-09-16 14:01:11]

Description was changed from

==========
Move part of display_compositor's interface to cc/ipc

In display_compositor, we have CompositorFrameSink and CompositorFrameSinkClient
interface. Since we already have cc/output/compositor_frame_sink class,
it makes more sense to move these two interfaces to cc/ipc.
==========

to

==========
Move part of display_compositor's interface to cc/ipc

In display_compositor, we have CompositorFrameSink and CompositorFrameSinkClient
interface. Since we already have cc/output/compositor_frame_sink class,
it makes more sense to move these two interfaces to cc/ipc.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[xidachen, 2016-09-16 14:03:25]

xidachen@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org, fsamuel@chromium.org

[xidachen, 2016-09-16 14:03:26]

PTAL

[Fady Samuel, 2016-09-16 14:08:03]

This is going in a good direction! Thanks for doing this!

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:7: 
Please place in module cc.mojom;

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:13: interface MojoCompositorFrameSink {
Please rename to CompositorFrameSink. Mojo is redundant.

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:18:
SubmitCompositorFrame(cc.mojom.CompositorFrame frame) => ();
I  don't think we need the callback? danakj@ could answer better I think. Frame
ratelimiting is now determined by unified beginframe.

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:27: interface
MojoCompositorFrameSinkClient {
Drop mojo please. It's redundant.

[xidachen, 2016-09-16 14:10:17]

cc some more people.

[xidachen, 2016-09-16 14:10:18]

cc some more people.

[xidachen, 2016-09-16 14:32:00]

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:7: 
On 2016/09/16 14:08:02, Fady Samuel wrote:
> Please place in module cc.mojom;

Done.

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:13: interface MojoCompositorFrameSink {
On 2016/09/16 14:08:02, Fady Samuel wrote:
> Please rename to CompositorFrameSink. Mojo is redundant.

This prefix is suggested by Dana. Her concern is that without this prefix, when
people talk about CompositorFrameSink, it is confusing because cc already have a
CompositorFrameSink class.

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:18:
SubmitCompositorFrame(cc.mojom.CompositorFrame frame) => ();
On 2016/09/16 14:08:03, Fady Samuel wrote:
> I  don't think we need the callback? danakj@ could answer better I think.
Frame
> ratelimiting is now determined by unified beginframe.

I will defer that to danakj@. Thanks.

[Fady Samuel, 2016-09-16 14:35:50]

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:13: interface MojoCompositorFrameSink {
On 2016/09/16 14:32:00, xidachen wrote:
> On 2016/09/16 14:08:02, Fady Samuel wrote:
> > Please rename to CompositorFrameSink. Mojo is redundant.
> 
> This prefix is suggested by Dana. Her concern is that without this prefix,
when
> people talk about CompositorFrameSink, it is confusing because cc already have
a
> CompositorFrameSink class.

Dana, Xida, we place all cc mojo interfaces in a cc::mojom namespace for this
purpose. I think the mojom namespace captures the fact that this is mojo and so
there shouldn't be any confusion, WDYT?

[xidachen, 2016-09-16 14:52:20]

On 2016/09/16 14:35:50, Fady Samuel wrote:
>
https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
> File cc/ipc/mojo_compositor_frame_sink.mojom (right):
> 
>
https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
> cc/ipc/mojo_compositor_frame_sink.mojom:13: interface MojoCompositorFrameSink
{
> On 2016/09/16 14:32:00, xidachen wrote:
> > On 2016/09/16 14:08:02, Fady Samuel wrote:
> > > Please rename to CompositorFrameSink. Mojo is redundant.
> > 
> > This prefix is suggested by Dana. Her concern is that without this prefix,
> when
> > people talk about CompositorFrameSink, it is confusing because cc already
have
> a
> > CompositorFrameSink class.
> 
> Dana, Xida, we place all cc mojo interfaces in a cc::mojom namespace for this
> purpose. I think the mojom namespace captures the fact that this is mojo and
so
> there shouldn't be any confusion, WDYT?

I personally agree with Fady. cc::mojom should be sufficient to clarify.

[danakj, 2016-09-16 17:56:37]

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:13: interface MojoCompositorFrameSink {
On 2016/09/16 14:35:50, Fady Samuel wrote:
> On 2016/09/16 14:32:00, xidachen wrote:
> > On 2016/09/16 14:08:02, Fady Samuel wrote:
> > > Please rename to CompositorFrameSink. Mojo is redundant.
> > 
> > This prefix is suggested by Dana. Her concern is that without this prefix,
> when
> > people talk about CompositorFrameSink, it is confusing because cc already
have
> a
> > CompositorFrameSink class.
> 
> Dana, Xida, we place all cc mojo interfaces in a cc::mojom namespace for this
> purpose. I think the mojom namespace captures the fact that this is mojo and
so
> there shouldn't be any confusion, WDYT?

I asked for it. When we talk about classes we talk about them by name, and
having two classes with the same name under cc/ seems like its going to be
confusing. We can rename it when there aren't two with the same name?

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:18:
SubmitCompositorFrame(cc.mojom.CompositorFrame frame) => ();
On 2016/09/16 14:32:00, xidachen wrote:
> On 2016/09/16 14:08:03, Fady Samuel wrote:
> > I  don't think we need the callback? danakj@ could answer better I think.
> Frame
> > ratelimiting is now determined by unified beginframe.
> 
> I will defer that to danakj@. Thanks.

We still need a swap ack for now, scheduler would DCHECK if it did not receive a
swap ack.

[xidachen, 2016-09-16 18:00:09]

On 2016/09/16 17:56:37, danakj wrote:
>
https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
> File cc/ipc/mojo_compositor_frame_sink.mojom (right):
> 
>
https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
> cc/ipc/mojo_compositor_frame_sink.mojom:13: interface MojoCompositorFrameSink
{
> On 2016/09/16 14:35:50, Fady Samuel wrote:
> > On 2016/09/16 14:32:00, xidachen wrote:
> > > On 2016/09/16 14:08:02, Fady Samuel wrote:
> > > > Please rename to CompositorFrameSink. Mojo is redundant.
> > > 
> > > This prefix is suggested by Dana. Her concern is that without this prefix,
> > when
> > > people talk about CompositorFrameSink, it is confusing because cc already
> have
> > a
> > > CompositorFrameSink class.
> > 
> > Dana, Xida, we place all cc mojo interfaces in a cc::mojom namespace for
this
> > purpose. I think the mojom namespace captures the fact that this is mojo and
> so
> > there shouldn't be any confusion, WDYT?
> 
> I asked for it. When we talk about classes we talk about them by name, and
> having two classes with the same name under cc/ seems like its going to be
> confusing. We can rename it when there aren't two with the same name?
> 
OK, I think that's fine then.

[Fady Samuel, 2016-09-16 18:00:36]

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:13: interface MojoCompositorFrameSink {
On 2016/09/16 17:56:37, danakj wrote:
> On 2016/09/16 14:35:50, Fady Samuel wrote:
> > On 2016/09/16 14:32:00, xidachen wrote:
> > > On 2016/09/16 14:08:02, Fady Samuel wrote:
> > > > Please rename to CompositorFrameSink. Mojo is redundant.
> > > 
> > > This prefix is suggested by Dana. Her concern is that without this prefix,
> > when
> > > people talk about CompositorFrameSink, it is confusing because cc already
> have
> > a
> > > CompositorFrameSink class.
> > 
> > Dana, Xida, we place all cc mojo interfaces in a cc::mojom namespace for
this
> > purpose. I think the mojom namespace captures the fact that this is mojo and
> so
> > there shouldn't be any confusion, WDYT?
> 
> I asked for it. When we talk about classes we talk about them by name, and
> having two classes with the same name under cc/ seems like its going to be
> confusing. We can rename it when there aren't two with the same name?

Sure. sgtm

https://codereview.chromium.org/2344183002/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:18:
SubmitCompositorFrame(cc.mojom.CompositorFrame frame) => ();
On 2016/09/16 17:56:37, danakj wrote:
> On 2016/09/16 14:32:00, xidachen wrote:
> > On 2016/09/16 14:08:03, Fady Samuel wrote:
> > > I  don't think we need the callback? danakj@ could answer better I think.
> > Frame
> > > ratelimiting is now determined by unified beginframe.
> > 
> > I will defer that to danakj@. Thanks.
> 
> We still need a swap ack for now, scheduler would DCHECK if it did not receive
a
> swap ack.

OK

[Fady Samuel, 2016-09-16 18:23:41]

services/ui/gpu lgtm

[dcheng, 2016-09-16 18:23:45]

A probably dumb question, but how does this interface differ from Surface /
SurfaceClient?

https://codereview.chromium.org/2344183002/diff/40001/services/ui/gpu/display...
File services/ui/gpu/display_compositor/compositor_frame_sink_impl.h (right):

https://codereview.chromium.org/2344183002/diff/40001/services/ui/gpu/display...
services/ui/gpu/display_compositor/compositor_frame_sink_impl.h:36: //
cc::mojom::CompositorFrameSink implementation.
Nit: MojoCompositorFrameSink

[Fady Samuel, 2016-09-16 18:25:36]

On 2016/09/16 18:23:45, dcheng wrote:
> A probably dumb question, but how does this interface differ from Surface /
> SurfaceClient?
> 
>
https://codereview.chromium.org/2344183002/diff/40001/services/ui/gpu/display...
> File services/ui/gpu/display_compositor/compositor_frame_sink_impl.h (right):
> 
>
https://codereview.chromium.org/2344183002/diff/40001/services/ui/gpu/display...
> services/ui/gpu/display_compositor/compositor_frame_sink_impl.h:36: //
> cc::mojom::CompositorFrameSink implementation.
> Nit: MojoCompositorFrameSink

It will replace it. Surface/SurfaceClient are going away. We are moving towards
the "FrameSink" terminology.

[dcheng, 2016-09-16 18:26:25]

On 2016/09/16 18:25:36, Fady Samuel wrote:
> On 2016/09/16 18:23:45, dcheng wrote:
> > A probably dumb question, but how does this interface differ from Surface /
> > SurfaceClient?
> > 
> >
>
https://codereview.chromium.org/2344183002/diff/40001/services/ui/gpu/display...
> > File services/ui/gpu/display_compositor/compositor_frame_sink_impl.h
(right):
> > 
> >
>
https://codereview.chromium.org/2344183002/diff/40001/services/ui/gpu/display...
> > services/ui/gpu/display_compositor/compositor_frame_sink_impl.h:36: //
> > cc::mojom::CompositorFrameSink implementation.
> > Nit: MojoCompositorFrameSink
> 
> It will replace it. Surface/SurfaceClient are going away. We are moving
towards
> the "FrameSink" terminology.

mojom lgtm with nit addressed

[xidachen, 2016-09-16 18:28:13]

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

[commit-bot: I haz the power, 2016-09-16 18:28:46]

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

[danakj, 2016-09-16 18:32:53]

LGTM

[xidachen, 2016-09-16 19:16:20]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-09-16 19:16:27]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-09-16 19:16:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2344183002/#ps60001
(title: "nits")

[commit-bot: I haz the power, 2016-09-16 19:17:08]

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-09-16 19:25:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-16 19:28:08]

Description was changed from

==========
Move part of display_compositor's interface to cc/ipc

In display_compositor, we have CompositorFrameSink and CompositorFrameSinkClient
interface. Since we already have cc/output/compositor_frame_sink class,
it makes more sense to move these two interfaces to cc/ipc.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Move part of display_compositor's interface to cc/ipc

In display_compositor, we have CompositorFrameSink and CompositorFrameSinkClient
interface. Since we already have cc/output/compositor_frame_sink class,
it makes more sense to move these two interfaces to cc/ipc.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/284a2b2247742cfa9e7039dbce4bf75900272bd6
Cr-Commit-Position: refs/heads/master@{#419248}
==========

[commit-bot: I haz the power, 2016-09-16 19:28:09]

Patchset 4 (id:??) landed as
https://crrev.com/284a2b2247742cfa9e7039dbce4bf75900272bd6
Cr-Commit-Position: refs/heads/master@{#419248}