Rename DisplayCompositor to MojoFrameSinkManager.

Rename DisplayCompositor to MojoFrameSinkManager.

The DisplayCompositor class and interface names are inaccurate. It is
not a per display thing and it doesn't composite. We also already have a
"display compositor" which is something separate.

What was called DisplayCompositor actually manages CompositorFrameSinks.
In the future it will manage other kinds of FrameSinks.

Rename the interface to mojom::FrameSinkManager and implementation to
MojoFrameSinkManager.

Also remove cc::mojom::FrameSinkManager from manifests. This is a
privileged interface only for service:ui not window_manager + apps.

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

Review-Url: https://codereview.chromium.org/2797453002
Cr-Commit-Position: refs/heads/master@{#461727}
Committed: https://chromium.googlesource.com/chromium/src/+/486c4556acd867e89461ff8c53f6badfe2f1b3a4

[kylechar, 2017-04-03 16:23:08]

Description was changed from

==========
Rename DisplayCompositor to MojoFrameSinkManager.

The DisplayCompositor class and interface names are inaccurate. It is
not a per display thing and it doesn't composite. We also already have a
"display compositor" which is something separate.

What was called DisplayCompositor actually manages CompositorFrameSinks.
In the future it will manage other kinds of FrameSinks.

Rename the interface to mojom::FrameSinkManager and implementation to
MojoFrameSinkManager.

BUG=705630
==========

to

==========
Rename DisplayCompositor to MojoFrameSinkManager.

The DisplayCompositor class and interface names are inaccurate. It is
not a per display thing and it doesn't composite. We also already have a
"display compositor" which is something separate.

What was called DisplayCompositor actually manages CompositorFrameSinks.
In the future it will manage other kinds of FrameSinks.

Rename the interface to mojom::FrameSinkManager and implementation to
MojoFrameSinkManager.

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

[kylechar, 2017-04-03 17:26:06]

Patchset #2 (id:20001) has been deleted

[kylechar, 2017-04-03 17:27:43]

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

[kylechar, 2017-04-03 17:27:44]

fsamuel for confirmation before sending out to all OWNERS.

[Fady Samuel, 2017-04-03 18:23:51]

lgtm

[kylechar, 2017-04-03 18:30:07]

kylechar@chromium.org changed reviewers:
+ danakj@chromium.org, msw@chromium.org, tsepez@chromium.org, xlai@chromium.org

[kylechar, 2017-04-03 18:30:07]

+tsepez for *.mojom and security
+xlai for third_party/WebKit/* and
content/browser/renderer_host/*offscreen_canvas*
+msw for services/ui/*
+piman for components/display_compositor/* and content/gpu/*

[Tom Sepez, 2017-04-03 18:32:57]

Straight rename? Then RS LGTM.

[kylechar, 2017-04-03 18:33:02]

kylechar@chromium.org changed reviewers:
+ piman@chromium.org
- danakj@chromium.org

[kylechar, 2017-04-03 18:33:03]

+piman for real

[danakj, 2017-04-03 18:37:18]

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

[danakj, 2017-04-03 18:37:19]

Thank you :) Sorry for all the naming round-abouts. LGTM

[kylechar, 2017-04-03 18:47:45]

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

[xlai (Olivia), 2017-04-03 18:48:12]

xlai@chromium.org changed reviewers:
+ junov@chromium.org

[xlai (Olivia), 2017-04-03 18:48:13]

lgtm for content/browser/renderer_host/*offscreen_canvas* but I'm not the owner
of third_party/WebKit/graphics. +junov@ to stamp.

[commit-bot: I haz the power, 2017-04-03 18:48:23]

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

[msw, 2017-04-03 19:22:13]

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/gpu_main.h
File services/ui/gpu/gpu_main.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/gpu_mai...
services/ui/gpu/gpu_main.h:83: cc::mojom::FrameSinkManagerRequest
pending_display_compositor_request_;
Update these member names? Also grep services/ui (and clients) for references to
'display compositor'/'display_compositor'/'DisplayCompositor' and update those?
Ditto for docs, bugs, etc. that live outside the codebase.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/gpu_mai...
services/ui/gpu/gpu_main.h:87: std::unique_ptr<MojoFrameSinkManager>
frame_sink_manager_;
nit: add a comment

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/interfa...
File services/ui/gpu/interfaces/gpu_main.mojom (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/interfa...
services/ui/gpu/interfaces/gpu_main.mojom:16: cc.mojom.FrameSinkManager&
display_compositor,
Update arg names

https://codereview.chromium.org/2797453002/diff/40001/services/ui/manifest.json
File services/ui/manifest.json (left):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/manifest.js...
services/ui/manifest.json:14: "ui::mojom::DisplayCompositor",
Describe this change in the CL description; this isn't a rename...

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
File services/ui/surfaces/mojo_frame_sink_manager.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:32: class MojoFrameSinkManager
q: Why use Mojo in this name? Maybe explain in comment?

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/server_w...
File services/ui/ws/server_window_delegate.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/server_w...
services/ui/ws/server_window_delegate.h:10: #include
"cc/ipc/frame_sink_manager.mojom.h"
fwd decl instead

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/server_w...
services/ui/ws/server_window_delegate.h:22: // Returns a display compositor
interface pointer. There is only one
update comment

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/test_ser...
File services/ui/ws/test_server_window_delegate.cc (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/test_ser...
services/ui/ws/test_server_window_delegate.cc:7: #include
"services/ui/ws/server_window.h"
nit: not needed

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/test_ser...
File services/ui/ws/test_server_window_delegate.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/test_ser...
services/ui/ws/test_server_window_delegate.h:13: class FrameSinkManager;
nit: not needed (should be pulled in via server_window_delegate.h)

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:48: // TODO(fsamuel): FrameSinkManager should be
a mojo interface dispensed by
q: Is this not the case?

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server.h:385:
mojo::Binding<cc::mojom::FrameSinkManagerClient>
nit: comments

[kylechar, 2017-04-03 19:44:40]

Description was changed from

==========
Rename DisplayCompositor to MojoFrameSinkManager.

The DisplayCompositor class and interface names are inaccurate. It is
not a per display thing and it doesn't composite. We also already have a
"display compositor" which is something separate.

What was called DisplayCompositor actually manages CompositorFrameSinks.
In the future it will manage other kinds of FrameSinks.

Rename the interface to mojom::FrameSinkManager and implementation to
MojoFrameSinkManager.

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

to

==========
Rename DisplayCompositor to MojoFrameSinkManager.

The DisplayCompositor class and interface names are inaccurate. It is
not a per display thing and it doesn't composite. We also already have a
"display compositor" which is something separate.

What was called DisplayCompositor actually manages CompositorFrameSinks.
In the future it will manage other kinds of FrameSinks.

Rename the interface to mojom::FrameSinkManager and implementation to
MojoFrameSinkManager.

Also remove cc::mojom::FrameSinkManager from manifests. This is a
privileged interface only for service:ui not window_manager + apps.

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

[kylechar, 2017-04-03 19:54:06]

Patchset #3 (id:60001) has been deleted

[kylechar, 2017-04-03 19:54:16]

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/gpu_main.h
File services/ui/gpu/gpu_main.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/gpu_mai...
services/ui/gpu/gpu_main.h:83: cc::mojom::FrameSinkManagerRequest
pending_display_compositor_request_;
On 2017/04/03 19:22:13, msw wrote:
> Update these member names? Also grep services/ui (and clients) for references
to
> 'display compositor'/'display_compositor'/'DisplayCompositor' and update
those?
> Ditto for docs, bugs, etc. that live outside the codebase.

Done.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/gpu_mai...
services/ui/gpu/gpu_main.h:87: std::unique_ptr<MojoFrameSinkManager>
frame_sink_manager_;
On 2017/04/03 19:22:13, msw wrote:
> nit: add a comment

Done.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/interfa...
File services/ui/gpu/interfaces/gpu_main.mojom (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/gpu/interfa...
services/ui/gpu/interfaces/gpu_main.mojom:16: cc.mojom.FrameSinkManager&
display_compositor,
On 2017/04/03 19:22:13, msw wrote:
> Update arg names

Done.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/manifest.json
File services/ui/manifest.json (left):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/manifest.js...
services/ui/manifest.json:14: "ui::mojom::DisplayCompositor",
On 2017/04/03 19:22:13, msw wrote:
> Describe this change in the CL description; this isn't a rename...

Okay, I added a line about this in the CL description. This interface should not
have been in the manifest, since "app" and "window_manager" aren't allowed to
connect to it.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
File services/ui/surfaces/mojo_frame_sink_manager.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:32: class MojoFrameSinkManager
On 2017/04/03 19:22:13, msw wrote:
> q: Why use Mojo in this name? Maybe explain in comment?

There is a cc::FrameSinkManager already :( MojoFrameSinkManager was the best
compromise. I'll rewrite the class comment since I'm changing other comments in
this CL too.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/server_w...
File services/ui/ws/server_window_delegate.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/server_w...
services/ui/ws/server_window_delegate.h:10: #include
"cc/ipc/frame_sink_manager.mojom.h"
On 2017/04/03 19:22:13, msw wrote:
> fwd decl instead

Done.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/server_w...
services/ui/ws/server_window_delegate.h:22: // Returns a display compositor
interface pointer. There is only one
On 2017/04/03 19:22:13, msw wrote:
> update comment

Done.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/test_ser...
File services/ui/ws/test_server_window_delegate.cc (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/test_ser...
services/ui/ws/test_server_window_delegate.cc:7: #include
"services/ui/ws/server_window.h"
On 2017/04/03 19:22:13, msw wrote:
> nit: not needed

This is a bit outside the scope of this rename CL, so I feel it's best not to
modify an unrelated include.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/test_ser...
File services/ui/ws/test_server_window_delegate.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/test_ser...
services/ui/ws/test_server_window_delegate.h:13: class FrameSinkManager;
On 2017/04/03 19:22:13, msw wrote:
> nit: not needed (should be pulled in via server_window_delegate.h)

Done.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:48: // TODO(fsamuel): FrameSinkManager should be
a mojo interface dispensed by
On 2017/04/03 19:22:13, msw wrote:
> q: Is this not the case?

Done.

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server.h:385:
mojo::Binding<cc::mojom::FrameSinkManagerClient>
On 2017/04/03 19:22:13, msw wrote:
> nit: comments

Done.

[danakj, 2017-04-03 19:55:15]

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
File services/ui/surfaces/mojo_frame_sink_manager.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:32: class MojoFrameSinkManager
On 2017/04/03 19:54:15, kylechar wrote:
> On 2017/04/03 19:22:13, msw wrote:
> > q: Why use Mojo in this name? Maybe explain in comment?
> 
> There is a cc::FrameSinkManager already :( MojoFrameSinkManager was the best
> compromise. I'll rewrite the class comment since I'm changing other comments
in
> this CL too.

Also this is managing MojoCompositorFrameSink, so it's literally a manager of
Mojo*FrameSink.

[msw, 2017-04-03 20:07:34]

lgtm with nits

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
File services/ui/surfaces/mojo_frame_sink_manager.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:32: class MojoFrameSinkManager
On 2017/04/03 19:55:15, danakj wrote:
> On 2017/04/03 19:54:15, kylechar wrote:
> > On 2017/04/03 19:22:13, msw wrote:
> > > q: Why use Mojo in this name? Maybe explain in comment?
> > 
> > There is a cc::FrameSinkManager already :( MojoFrameSinkManager was the best
> > compromise. I'll rewrite the class comment since I'm changing other comments
> in
> > this CL too.
> 
> Also this is managing MojoCompositorFrameSink, so it's literally a manager of
> Mojo*FrameSink.

Then I'd ask the same question of MojoCompositorFrameSink. It doesn't make sense
if every Mojo interface in prefixed with 'Mojo'. I guess the idea in this case
is "it's just like that other thing we have, but this one uses Mojo."

https://codereview.chromium.org/2797453002/diff/80001/services/ui/surfaces/mo...
File services/ui/surfaces/mojo_frame_sink_manager.h (right):

https://codereview.chromium.org/2797453002/diff/80001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:28: // provides a Mojo interface
to create CompositorFrameSinks, manage BeginFrame
nit: 'manages' here and on the next line?

https://codereview.chromium.org/2797453002/diff/80001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:31: // This is intended to
created in the viz or GPU process. For mus+ash this will
nit: 'be created'? Is 'viz' the appropriate process name?

[danakj, 2017-04-03 20:11:15]

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
File services/ui/surfaces/mojo_frame_sink_manager.h (right):

https://codereview.chromium.org/2797453002/diff/40001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:32: class MojoFrameSinkManager
On 2017/04/03 20:07:34, msw wrote:
> On 2017/04/03 19:55:15, danakj wrote:
> > On 2017/04/03 19:54:15, kylechar wrote:
> > > On 2017/04/03 19:22:13, msw wrote:
> > > > q: Why use Mojo in this name? Maybe explain in comment?
> > > 
> > > There is a cc::FrameSinkManager already :( MojoFrameSinkManager was the
best
> > > compromise. I'll rewrite the class comment since I'm changing other
comments
> > in
> > > this CL too.
> > 
> > Also this is managing MojoCompositorFrameSink, so it's literally a manager
of
> > Mojo*FrameSink.
> 
> Then I'd ask the same question of MojoCompositorFrameSink. It doesn't make
sense
> if every Mojo interface in prefixed with 'Mojo'. I guess the idea in this case
> is "it's just like that other thing we have, but this one uses Mojo."

We had that discussion on the CL that introduced the name, which you can find if
you want. But yeah there is a cc API and we made a Mojo api like it, and I did
not like depending on a namespace only to differentiate.

[kylechar, 2017-04-03 20:32:06]

https://codereview.chromium.org/2797453002/diff/80001/services/ui/surfaces/mo...
File services/ui/surfaces/mojo_frame_sink_manager.h (right):

https://codereview.chromium.org/2797453002/diff/80001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:28: // provides a Mojo interface
to create CompositorFrameSinks, manage BeginFrame
On 2017/04/03 20:07:34, msw wrote:
> nit: 'manages' here and on the next line?

Done.

https://codereview.chromium.org/2797453002/diff/80001/services/ui/surfaces/mo...
services/ui/surfaces/mojo_frame_sink_manager.h:31: // This is intended to
created in the viz or GPU process. For mus+ash this will
On 2017/04/03 20:07:34, msw wrote:
> nit: 'be created'? Is 'viz' the appropriate process name?

Changed. Also, yep, basically GPU process => viz process basically.

[piman, 2017-04-03 23:58:26]

lgtm

[kylechar, 2017-04-04 13:50:22]

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

[commit-bot: I haz the power, 2017-04-04 13:50:34]

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

[Justin Novosad, 2017-04-04 14:29:52]

lgtm

[kylechar, 2017-04-04 14:35:54]

The CQ bit was unchecked by kylechar@chromium.org

[kylechar, 2017-04-04 14:35:55]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-04-04 14:35:55]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
tsepez@chromium.org, fsamuel@chromium.org, xlai@chromium.org, msw@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2797453002/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-04 14:36:11]

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-04-04 15:31:48]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491316555366240,
"parent_rev": "a758d4d5c23e748d3db4d07e5f9ee310238419c3", "commit_rev":
"486c4556acd867e89461ff8c53f6badfe2f1b3a4"}

[commit-bot: I haz the power, 2017-04-04 15:34:40]

Description was changed from

==========
Rename DisplayCompositor to MojoFrameSinkManager.

The DisplayCompositor class and interface names are inaccurate. It is
not a per display thing and it doesn't composite. We also already have a
"display compositor" which is something separate.

What was called DisplayCompositor actually manages CompositorFrameSinks.
In the future it will manage other kinds of FrameSinks.

Rename the interface to mojom::FrameSinkManager and implementation to
MojoFrameSinkManager.

Also remove cc::mojom::FrameSinkManager from manifests. This is a
privileged interface only for service:ui not window_manager + apps.

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

to

==========
Rename DisplayCompositor to MojoFrameSinkManager.

The DisplayCompositor class and interface names are inaccurate. It is
not a per display thing and it doesn't composite. We also already have a
"display compositor" which is something separate.

What was called DisplayCompositor actually manages CompositorFrameSinks.
In the future it will manage other kinds of FrameSinks.

Rename the interface to mojom::FrameSinkManager and implementation to
MojoFrameSinkManager.

Also remove cc::mojom::FrameSinkManager from manifests. This is a
privileged interface only for service:ui not window_manager + apps.

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

Review-Url: https://codereview.chromium.org/2797453002
Cr-Commit-Position: refs/heads/master@{#461727}
Committed:
https://chromium.googlesource.com/chromium/src/+/486c4556acd867e89461ff8c53f6...
==========

[commit-bot: I haz the power, 2017-04-04 15:34:44]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/486c4556acd867e89461ff8c53f6...