Implement aura::Window::CreateCompositorFrameSink()

Implement aura::Window::CreateCompositorFrameSink()

Currently, the exo creates cc::CompositorFrameSinkSupport by itself.
To create cc::CompositorFrameSinkSupport, exo has to access
ContextFactoryPrivate for allocating a FrameSinkId and accessing
cc::SurfaceManager, etc. But all those objects are not available in
Mus clients, so the exo doesn't work in Mushrome and Mustash
To support mushrome and mustash, this CL adds
aura::Window::CreateCompositorFrameSink(), this function hides the
detail of CompositorFrameSink creation within Cash and Mus.

Note: this CL only implements aura::Window::CreateCompositorFrameSink()
for Cash. A follow up CL will implements it for Mus.


BUG=718443
TEST=tested exo with weston-terminal, wayland_rects_client
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2868473002
Cr-Commit-Position: refs/heads/master@{#470450}
Committed: https://chromium.googlesource.com/chromium/src/+/4c5a2a2c64c2b54355139eb9970fb1992e8363bd

[Peng, 2017-05-05 15:55:43]

Description was changed from

==========
WIP


WIP

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

to

==========
WIP


WIP

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

[Peng, 2017-05-05 18:53:01]

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

[commit-bot: I haz the power, 2017-05-05 18:53:40]

Dry run: CQ is trying da patch.

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

[Peng, 2017-05-05 19:24:45]

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

[commit-bot: I haz the power, 2017-05-05 19:24:46]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-05 22:52:04]

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

[commit-bot: I haz the power, 2017-05-05 22:52:05]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Peng, 2017-05-06 00:44:39]

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

[commit-bot: I haz the power, 2017-05-06 00:45:06]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-06 02:24:21]

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

[commit-bot: I haz the power, 2017-05-06 02:24:22]

Dry run: This issue passed the CQ dry run.

[Peng, 2017-05-08 14:58:49]

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

[commit-bot: I haz the power, 2017-05-08 14:59:49]

Dry run: CQ is trying da patch.

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

[Peng, 2017-05-08 15:11:15]

Description was changed from

==========
WIP


WIP

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

to

==========
Implement aura::Window::CreateCompositorFrameSink()

Currently, the exo creates cc::CompositorFrameSinkSupport by itself.
To create cc::CompositorFrameSinkSupport, exo has to access
ContextFactoryPrivate for allocating a FrameSinkId and accessing
cc::SurfaceManager, etc. But all those objects are not available in
Mus clients, so the exo doesn't work in Mushrome and Mustash
To support mushrome and mustash, this CL adds
aura::Window::CreateCompositorFrameSink(), this function hides the
detail of CompositorFrameSink creation within Cash and Mus.

Note: this CL only implements aura::Window::CreateCompositorFrameSink()
for Cash. A follow up CL will implements it for Mus.


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

[Peng, 2017-05-08 15:12:43]

Description was changed from

==========
Implement aura::Window::CreateCompositorFrameSink()

Currently, the exo creates cc::CompositorFrameSinkSupport by itself.
To create cc::CompositorFrameSinkSupport, exo has to access
ContextFactoryPrivate for allocating a FrameSinkId and accessing
cc::SurfaceManager, etc. But all those objects are not available in
Mus clients, so the exo doesn't work in Mushrome and Mustash
To support mushrome and mustash, this CL adds
aura::Window::CreateCompositorFrameSink(), this function hides the
detail of CompositorFrameSink creation within Cash and Mus.

Note: this CL only implements aura::Window::CreateCompositorFrameSink()
for Cash. A follow up CL will implements it for Mus.


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

to

==========
Implement aura::Window::CreateCompositorFrameSink()

Currently, the exo creates cc::CompositorFrameSinkSupport by itself.
To create cc::CompositorFrameSinkSupport, exo has to access
ContextFactoryPrivate for allocating a FrameSinkId and accessing
cc::SurfaceManager, etc. But all those objects are not available in
Mus clients, so the exo doesn't work in Mushrome and Mustash
To support mushrome and mustash, this CL adds
aura::Window::CreateCompositorFrameSink(), this function hides the
detail of CompositorFrameSink creation within Cash and Mus.

Note: this CL only implements aura::Window::CreateCompositorFrameSink()
for Cash. A follow up CL will implements it for Mus.


BUG=718443
TEST=tested exo with weston-terminal, wayland_rects_client
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Peng, 2017-05-08 15:44:15]

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

[Peng, 2017-05-08 15:44:16]

Hi Fady, PTAL. Thanks.

[Fady Samuel, 2017-05-08 15:52:09]

I like this. This seems to be moving in a generally good direction. Thanks! I
have a few comments but otherwise, looks good.

https://codereview.chromium.org/2868473002/diff/160001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/160001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
Is this necessary? Can we avoid adding this?

https://codereview.chromium.org/2868473002/diff/160001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:130: virtual SurfaceId GetSurfaceId() const;
Is this necessary, can we avoid adding this?

https://codereview.chromium.org/2868473002/diff/160001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2868473002/diff/160001/components/exo/surface...
components/exo/surface.cc:224: ->GetSurfaceId();
Perhaps this can be part of aura::Window instead?

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/mus/window_por...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/mus/window_por...
ui/aura/mus/window_port_mus.cc:525: return nullptr;
TODO here to implement this.

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/window.cc#newc...
ui/aura/window.cc:995: return port_->OnCreateCompositorFrameSink();
Maybe just call this CreateCompositorFrameSink?

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/window_port.h
File ui/aura/window_port.h (right):

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/window_port.h#...
ui/aura/window_port.h:80: OnCreateCompositorFrameSink() = 0;
CreateCompositorFrameSink.

[Peng, 2017-05-08 17:44:39]

https://codereview.chromium.org/2868473002/diff/160001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/160001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
On 2017/05/08 15:52:09, Fady Samuel wrote:
> Is this necessary? Can we avoid adding this?

The exo need call this function to flush frames before destroying the
CompositorFrameSink. And exo need keep CompositorFrameSink alive before all
submitted resources are released by ReclaimResources(). So flushing frame during
destruction or DetachFromCliend() doesn't work.

https://codereview.chromium.org/2868473002/diff/160001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:130: virtual SurfaceId GetSurfaceId() const;
On 2017/05/08 15:52:09, Fady Samuel wrote:
> Is this necessary, can we avoid adding this?
Done

https://codereview.chromium.org/2868473002/diff/160001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2868473002/diff/160001/components/exo/surface...
components/exo/surface.cc:224: ->GetSurfaceId();
On 2017/05/08 15:52:09, Fady Samuel wrote:
> Perhaps this can be part of aura::Window instead?

Done.

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/mus/window_por...
File ui/aura/mus/window_port_mus.cc (right):

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/mus/window_por...
ui/aura/mus/window_port_mus.cc:525: return nullptr;
On 2017/05/08 15:52:09, Fady Samuel wrote:
> TODO here to implement this.

Done.

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/window.cc#newc...
ui/aura/window.cc:995: return port_->OnCreateCompositorFrameSink();
On 2017/05/08 15:52:09, Fady Samuel wrote:
> Maybe just call this CreateCompositorFrameSink?

Done.

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/window_port.h
File ui/aura/window_port.h (right):

https://codereview.chromium.org/2868473002/diff/160001/ui/aura/window_port.h#...
ui/aura/window_port.h:80: OnCreateCompositorFrameSink() = 0;
On 2017/05/08 15:52:09, Fady Samuel wrote:
> CreateCompositorFrameSink.

Done.

[Fady Samuel, 2017-05-08 18:17:35]

LGTM + nits

https://codereview.chromium.org/2868473002/diff/160001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/160001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
On 2017/05/08 17:44:39, Peng wrote:
> On 2017/05/08 15:52:09, Fady Samuel wrote:
> > Is this necessary? Can we avoid adding this?
> 
> The exo need call this function to flush frames before destroying the
> CompositorFrameSink. And exo need keep CompositorFrameSink alive before all
> submitted resources are released by ReclaimResources(). So flushing frame
during
> destruction or DetachFromCliend() doesn't work.

OK Thanks.

https://codereview.chromium.org/2868473002/diff/180001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/180001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
We prefer pure virtual in cc.

https://codereview.chromium.org/2868473002/diff/180001/ui/aura/local/window_p...
File ui/aura/local/window_port_local.cc (right):

https://codereview.chromium.org/2868473002/diff/180001/ui/aura/local/window_p...
ui/aura/local/window_port_local.cc:109: cc::SurfaceId
WindowPortLocal::GetSurfaceId() const {
Maybe we should make this const cc::SurfaceId& instead since it saves a copy?

https://codereview.chromium.org/2868473002/diff/180001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2868473002/diff/180001/ui/aura/window.cc#newc...
ui/aura/window.cc:998: cc::SurfaceId Window::GetSurfaceId() const {
Maybe make this const cc::SurfaceId&

[Peng, 2017-05-08 18:21:00]

penghuang@chromium.org changed reviewers:
+ reveman@chromium.org, sky@chromium.org

[Peng, 2017-05-08 18:21:01]

reveman@chromium.org: Please review changes in //components/exo

sky@chromium.org: Please review changes in //ui/aura

Hi David & Scott, PTAL. Thanks.

[reveman, 2017-05-08 19:25:17]

components/exo lgtm

any chance you could update ash/laser/laser_pointer_view.* as well? That code is
very similar to exo code.

https://codereview.chromium.org/2868473002/diff/180001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2868473002/diff/180001/components/exo/surface...
components/exo/surface.cc:179: std::unique_ptr<cc::CompositorFrameSink>
compositor_frame_sink =
nit: feel free to remove this temporary variable if you want. doesn't seem to
provide much value

[Peng, 2017-05-08 20:14:01]

https://codereview.chromium.org/2868473002/diff/180001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/180001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
On 2017/05/08 18:17:35, Fady Samuel wrote:
> We prefer pure virtual in cc.

Tried it and found out making it pure will modify a lot of subclasses which are
only for testing. And right now, this class only has one pure virtual method.
Probably it is better to keep EvictFrame() non-pure.

https://codereview.chromium.org/2868473002/diff/180001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2868473002/diff/180001/components/exo/surface...
components/exo/surface.cc:179: std::unique_ptr<cc::CompositorFrameSink>
compositor_frame_sink =
On 2017/05/08 19:25:16, reveman wrote:
> nit: feel free to remove this temporary variable if you want. doesn't seem to
> provide much value

Done.

https://codereview.chromium.org/2868473002/diff/180001/ui/aura/local/window_p...
File ui/aura/local/window_port_local.cc (right):

https://codereview.chromium.org/2868473002/diff/180001/ui/aura/local/window_p...
ui/aura/local/window_port_local.cc:109: cc::SurfaceId
WindowPortLocal::GetSurfaceId() const {
On 2017/05/08 18:17:35, Fady Samuel wrote:
> Maybe we should make this const cc::SurfaceId& instead since it saves a copy?

I think mus implementation has to create a temporary cc::SurfaceId, and return
it. So const cc::SurfaceId& doesn't work for it.

[Peng, 2017-05-08 20:15:09]

On 2017/05/08 19:25:17, reveman wrote:
> components/exo lgtm
> 
> any chance you could update ash/laser/laser_pointer_view.* as well? That code
is
> very similar to exo code.

I will fix that issue when this CL is landed.

> 
>
https://codereview.chromium.org/2868473002/diff/180001/components/exo/surface.cc
> File components/exo/surface.cc (right):
> 
>
https://codereview.chromium.org/2868473002/diff/180001/components/exo/surface...
> components/exo/surface.cc:179: std::unique_ptr<cc::CompositorFrameSink>
> compositor_frame_sink =
> nit: feel free to remove this temporary variable if you want. doesn't seem to
> provide much value

[Peng, 2017-05-08 20:17:15]

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

[commit-bot: I haz the power, 2017-05-08 20:18:01]

Dry run: CQ is trying da patch.

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

[Peng, 2017-05-08 20:19:50]

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

[Peng, 2017-05-08 20:19:51]

piman@chromium.org: Please review changes in //content/renderer/gpu

Hi Antoine, PTAL. Thanks.

[piman, 2017-05-08 20:30:15]

https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
What calls this?

[Peng, 2017-05-08 20:41:50]

https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
On 2017/05/08 20:30:15, piman wrote:
> What calls this?

exo::Surface::~Surface() calls it to flush frames. But the
cc::CompositorFrameSink until will be alive until all resources are returned via
cc::CompositorFrameSinkClient::ReclaimResources(). So evicting frames in
cc::CompositorFrameSink::DetachFromClient or dtor doesn't work.

[commit-bot: I haz the power, 2017-05-08 21:58:55]

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

[commit-bot: I haz the power, 2017-05-08 21:58:56]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-08 22:36:43]

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/DEPS
File ui/aura/DEPS (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/DEPS#newcode2
ui/aura/DEPS:2: "+cc/output/compositor_frame_sink.h",
Is there a reason to only include certain files from cc rather than all of cc?

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/DEPS
File ui/aura/local/DEPS (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/DEPS#new...
ui/aura/local/DEPS:2: "+cc/output/compositor_frame_sink.h",
Similar comment here about cc includes.

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/composit...
File ui/aura/local/compositor_frame_sink_local.cc (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/composit...
ui/aura/local/compositor_frame_sink_local.cc:36: thread_checker_.reset(new
base::ThreadChecker());
Use MakeUnique.

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/composit...
File ui/aura/local/compositor_frame_sink_local.h (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/composit...
ui/aura/local/compositor_frame_sink_local.h:25: class CompositorFrameSinkLocal :
public cc::CompositorFrameSink,
Please add a description to all files. It may be obvious to you what this file
does, but others don't necessarily have the context you do.

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window.cc#newc...
ui/aura/window.cc:781: if (root_window && root_window != new_root_window)
Are you sure you don't care about the case when the root changes? It seems like
the frame sink ids won't be registered correctly if the window moves from one
root to another.

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window.cc#newc...
ui/aura/window.cc:860: port_->OnWindowRemovingFromRootWindow();
I have a mild preferences for calling this before notifying observers. Is that
possible?

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window_port.h
File ui/aura/window_port.h (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window_port.h#...
ui/aura/window_port.h:87: virtual void OnWindowRemovingFromRootWindow() = 0;
OnWillRemoveWindowFromRoot().

[piman, 2017-05-08 23:18:03]

https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
On 2017/05/08 20:41:49, Peng wrote:
> On 2017/05/08 20:30:15, piman wrote:
> > What calls this?
> 
> exo::Surface::~Surface() calls it to flush frames. But the
> cc::CompositorFrameSink until will be alive until all resources are returned
via
> cc::CompositorFrameSinkClient::ReclaimResources(). So evicting frames in
> cc::CompositorFrameSink::DetachFromClient or dtor doesn't work.

cc::CompositorFrameSink is the API that LayerTreeHostImpl uses to interact with
its client. I don't think we should be adding this, as LTHI doesn't call it.
In particular you've implemented this function for many subclasses, but they
will never be called.

I'm not sure why exo::Surface needs a cc::CompositorFrameSink - it didn't
before. It doesn't have a compositor to give it to. It seems an odd fit.

Did you mean to use a MojoCompositorFrameSink instead, maybe?

[Fady Samuel, 2017-05-08 23:42:32]

On 2017/05/08 23:18:03, piman wrote:
>
https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
> File cc/output/compositor_frame_sink.h (right):
> 
>
https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
> cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
> On 2017/05/08 20:41:49, Peng wrote:
> > On 2017/05/08 20:30:15, piman wrote:
> > > What calls this?
> > 
> > exo::Surface::~Surface() calls it to flush frames. But the
> > cc::CompositorFrameSink until will be alive until all resources are returned
> via
> > cc::CompositorFrameSinkClient::ReclaimResources(). So evicting frames in
> > cc::CompositorFrameSink::DetachFromClient or dtor doesn't work.
> 
> cc::CompositorFrameSink is the API that LayerTreeHostImpl uses to interact
with
> its client. I don't think we should be adding this, as LTHI doesn't call it.
> In particular you've implemented this function for many subclasses, but they
> will never be called.
> 
> I'm not sure why exo::Surface needs a cc::CompositorFrameSink - it didn't
> before. It doesn't have a compositor to give it to. It seems an odd fit.
> 
> Did you mean to use a MojoCompositorFrameSink instead, maybe?

We've been using cc::CompositorFrameSink as an abstract interface for an
endpoint to the display compositor. Anything that wishes to submit frames to the
display compositor uses cc::CompositorFrameSink. Whether that uses IPC or not is
an implementation detail. The intent here is for a client to request a
CompositorFrameSink from an aura::Window. Where the other end is is an
implementation detail. The other end may be a test or direct access to the
display compositor or IPC via MojoCompositorFrameSink.

[piman, 2017-05-08 23:58:21]

On Mon, May 8, 2017 at 4:42 PM, <fsamuel@chromium.org> wrote:

> On 2017/05/08 23:18:03, piman wrote:
> >
> https://codereview.chromium.org/2868473002/diff/200001/cc/
> output/compositor_frame_sink.h
> > File cc/output/compositor_frame_sink.h (right):
> >
> >
> https://codereview.chromium.org/2868473002/diff/200001/cc/
> output/compositor_frame_sink.h#newcode127
> > cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
> > On 2017/05/08 20:41:49, Peng wrote:
> > > On 2017/05/08 20:30:15, piman wrote:
> > > > What calls this?
> > >
> > > exo::Surface::~Surface() calls it to flush frames. But the
> > > cc::CompositorFrameSink until will be alive until all resources are
> returned
> > via
> > > cc::CompositorFrameSinkClient::ReclaimResources(). So evicting frames
> in
> > > cc::CompositorFrameSink::DetachFromClient or dtor doesn't work.
> >
> > cc::CompositorFrameSink is the API that LayerTreeHostImpl uses to
> interact
> with
> > its client. I don't think we should be adding this, as LTHI doesn't call
> it.
> > In particular you've implemented this function for many subclasses, but
> they
> > will never be called.
> >
> > I'm not sure why exo::Surface needs a cc::CompositorFrameSink - it didn't
> > before. It doesn't have a compositor to give it to. It seems an odd fit.
> >
> > Did you mean to use a MojoCompositorFrameSink instead, maybe?
>
> We've been using cc::CompositorFrameSink as an abstract interface for an
> endpoint to the display compositor. Anything that wishes to submit frames
> to the
> display compositor uses cc::CompositorFrameSink. Whether that uses IPC or
> not is
> an implementation detail. The intent here is for a client to request a
> CompositorFrameSink from an aura::Window. Where the other end is is an
> implementation detail. The other end may be a test or direct access to the
> display compositor or IPC via MojoCompositorFrameSink.
>

There seems to be a disconnect though. CompositorFrameSink is defined by
the needs of LTHI, not by the needs of DisplayCompositor. EvictFrame
doesn't belong there if LTHI won't call it.
Other option, downcast in the the exo code, or add a facility on
aura::Window->aura::WindowPort->aura::WindowPortLocal to evict the frame.


>
> https://codereview.chromium.org/2868473002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2017-05-09 17:26:57]

https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
File cc/output/compositor_frame_sink.h (right):

https://codereview.chromium.org/2868473002/diff/200001/cc/output/compositor_f...
cc/output/compositor_frame_sink.h:127: virtual void EvictFrame() {}
On 2017/05/08 23:18:03, piman wrote:
> On 2017/05/08 20:41:49, Peng wrote:
> > On 2017/05/08 20:30:15, piman wrote:
> > > What calls this?
> > 
> > exo::Surface::~Surface() calls it to flush frames. But the
> > cc::CompositorFrameSink until will be alive until all resources are returned
> via
> > cc::CompositorFrameSinkClient::ReclaimResources(). So evicting frames in
> > cc::CompositorFrameSink::DetachFromClient or dtor doesn't work.
> 
> cc::CompositorFrameSink is the API that LayerTreeHostImpl uses to interact
with
> its client. I don't think we should be adding this, as LTHI doesn't call it.
> In particular you've implemented this function for many subclasses, but they
> will never be called.
> 
> I'm not sure why exo::Surface needs a cc::CompositorFrameSink - it didn't
> before. It doesn't have a compositor to give it to. It seems an odd fit.
> 
> Did you mean to use a MojoCompositorFrameSink instead, maybe?

I removed this method. And call support_->EvictFrame() in DetachFromClient().
Done

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/DEPS
File ui/aura/DEPS (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/DEPS#newcode2
ui/aura/DEPS:2: "+cc/output/compositor_frame_sink.h",
On 2017/05/08 22:36:43, sky wrote:
> Is there a reason to only include certain files from cc rather than all of cc?

Changed it to folder. Done

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/DEPS
File ui/aura/local/DEPS (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/DEPS#new...
ui/aura/local/DEPS:2: "+cc/output/compositor_frame_sink.h",
On 2017/05/08 22:36:43, sky wrote:
> Similar comment here about cc includes.

Done.

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/composit...
File ui/aura/local/compositor_frame_sink_local.cc (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/composit...
ui/aura/local/compositor_frame_sink_local.cc:36: thread_checker_.reset(new
base::ThreadChecker());
On 2017/05/08 22:36:43, sky wrote:
> Use MakeUnique.

Done.

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/composit...
File ui/aura/local/compositor_frame_sink_local.h (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/local/composit...
ui/aura/local/compositor_frame_sink_local.h:25: class CompositorFrameSinkLocal :
public cc::CompositorFrameSink,
On 2017/05/08 22:36:43, sky wrote:
> Please add a description to all files. It may be obvious to you what this file
> does, but others don't necessarily have the context you do.

Done.

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window.cc#newc...
ui/aura/window.cc:781: if (root_window && root_window != new_root_window)
On 2017/05/08 22:36:43, sky wrote:
> Are you sure you don't care about the case when the root changes? It seems
like
> the frame sink ids won't be registered correctly if the window moves from one
> root to another.

In that case, this function must be called by Window::AddChild() at line 364,
and then the child->NotifyAddedToRoot() will be called at line 379. It will
notify window port the new root, right?

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window.cc#newc...
ui/aura/window.cc:860: port_->OnWindowRemovingFromRootWindow();
On 2017/05/08 22:36:43, sky wrote:
> I have a mild preferences for calling this before notifying observers. Is that
> possible?

Done.

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window_port.h
File ui/aura/window_port.h (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window_port.h#...
ui/aura/window_port.h:87: virtual void OnWindowRemovingFromRootWindow() = 0;
On 2017/05/08 22:36:43, sky wrote:
> OnWillRemoveWindowFromRoot().

Done.

[Peng, 2017-05-09 17:33:32]

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

[commit-bot: I haz the power, 2017-05-09 17:34:24]

Dry run: CQ is trying da patch.

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

[reveman, 2017-05-09 17:35:58]

still lgtm, just a nit

https://codereview.chromium.org/2868473002/diff/220001/components/exo/composi...
File components/exo/compositor_frame_sink_holder.cc (right):

https://codereview.chromium.org/2868473002/diff/220001/components/exo/composi...
components/exo/compositor_frame_sink_holder.cc:32: const gpu::SyncToken token;
nit: how common is it that we actually have toms release callbacks here? If we
do it's most likely one so can we remove this local variable and just do
"Run(gpu::SyncToken(), false)" below instead?

[commit-bot: I haz the power, 2017-05-09 19:27:00]

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

[commit-bot: I haz the power, 2017-05-09 19:27:01]

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...)

[sky, 2017-05-09 19:34:06]

LGTM

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2868473002/diff/200001/ui/aura/window.cc#newc...
ui/aura/window.cc:781: if (root_window && root_window != new_root_window)
On 2017/05/09 17:26:57, Peng wrote:
> On 2017/05/08 22:36:43, sky wrote:
> > Are you sure you don't care about the case when the root changes? It seems
> like
> > the frame sink ids won't be registered correctly if the window moves from
one
> > root to another.
> 
> In that case, this function must be called by Window::AddChild() at line 364,
> and then the child->NotifyAddedToRoot() will be called at line 379. It will
> notify window port the new root, right?

You are indeed right. Sorry about that.

[Peng, 2017-05-09 19:48:49]

https://codereview.chromium.org/2868473002/diff/220001/components/exo/composi...
File components/exo/compositor_frame_sink_holder.cc (right):

https://codereview.chromium.org/2868473002/diff/220001/components/exo/composi...
components/exo/compositor_frame_sink_holder.cc:32: const gpu::SyncToken token;
On 2017/05/09 17:35:58, reveman wrote:
> nit: how common is it that we actually have toms release callbacks here? If we
> do it's most likely one so can we remove this local variable and just do
> "Run(gpu::SyncToken(), false)" below instead?

I tested with several wayland clients. the size of release_callbacks_ is one. So
removed the local variable. Done

[Peng, 2017-05-09 19:52:53]

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

[commit-bot: I haz the power, 2017-05-09 19:53:49]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-09 20:57:24]

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

[commit-bot: I haz the power, 2017-05-09 20:57:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Peng, 2017-05-09 21:43:12]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2017-05-09 21:43:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, reveman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2868473002/#ps240001
(title: "Address review issues")

[commit-bot: I haz the power, 2017-05-09 21:45:02]

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-05-10 02:00:37]

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

[commit-bot: I haz the power, 2017-05-10 02:00:39]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Peng, 2017-05-10 02:34:35]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2017-05-10 02:35:53]

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-05-10 04:42:00]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1494383675250720,
"parent_rev": "25488964c9f5595d2f70edf0f25c502e9acdc281", "commit_rev":
"4c5a2a2c64c2b54355139eb9970fb1992e8363bd"}

[commit-bot: I haz the power, 2017-05-10 04:42:12]

Description was changed from

==========
Implement aura::Window::CreateCompositorFrameSink()

Currently, the exo creates cc::CompositorFrameSinkSupport by itself.
To create cc::CompositorFrameSinkSupport, exo has to access
ContextFactoryPrivate for allocating a FrameSinkId and accessing
cc::SurfaceManager, etc. But all those objects are not available in
Mus clients, so the exo doesn't work in Mushrome and Mustash
To support mushrome and mustash, this CL adds
aura::Window::CreateCompositorFrameSink(), this function hides the
detail of CompositorFrameSink creation within Cash and Mus.

Note: this CL only implements aura::Window::CreateCompositorFrameSink()
for Cash. A follow up CL will implements it for Mus.


BUG=718443
TEST=tested exo with weston-terminal, wayland_rects_client
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement aura::Window::CreateCompositorFrameSink()

Currently, the exo creates cc::CompositorFrameSinkSupport by itself.
To create cc::CompositorFrameSinkSupport, exo has to access
ContextFactoryPrivate for allocating a FrameSinkId and accessing
cc::SurfaceManager, etc. But all those objects are not available in
Mus clients, so the exo doesn't work in Mushrome and Mustash
To support mushrome and mustash, this CL adds
aura::Window::CreateCompositorFrameSink(), this function hides the
detail of CompositorFrameSink creation within Cash and Mus.

Note: this CL only implements aura::Window::CreateCompositorFrameSink()
for Cash. A follow up CL will implements it for Mus.


BUG=718443
TEST=tested exo with weston-terminal, wayland_rects_client
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2868473002
Cr-Commit-Position: refs/heads/master@{#470450}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c5a2a2c64c2b54355139eb9970f...
==========

[commit-bot: I haz the power, 2017-05-10 04:42:13]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/4c5a2a2c64c2b54355139eb9970f...