Hook up ui::Compositor to Display's BeginFrameSource

Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

The BeginFrameSource is created by the BrowserCompositorOutputSurface
which updates it based on vsync information that it receives.  This
BeginFrameSource is passed to the Display (via OutputSurfaceClient),
which informs the SurfaceManager that the compositor using
that Display should be driven by that BeginFrameSource.  The
SurfaceManager then informs the SurfaceDisplayOutputSurface about
the BeginFrameSource, which passes it into the single thread proxy's
cc::Scheduler for that ui::Compositor's instance.  Plumbing!

The path from SurfaceManager to SurfaceDisplayOutputSurface was
added in https://codereview.chromium.org/1673783004, but the rest
of the plumbing is new to this patch.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/19c108580b99c469ed192066310e3162bf8c196e
Cr-Commit-Position: refs/heads/master@{#387228}

[enne (OOO), 2016-03-21 22:08:33]

Description was changed from

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

Currently, the BrowserCompositorOutputSurface is out the loop and
doesn't implement the SetBeginFrameSource, as the real begin frame
source is owned by OnScreenDisplayClient and it doesn't participate in
the cc::Surfaces sytem directly.
==========

to

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

Currently, the BrowserCompositorOutputSurface is out the loop and
doesn't implement the SetBeginFrameSource, as the real begin frame
source is owned by OnScreenDisplayClient and it doesn't participate in
the cc::Surfaces sytem directly.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-03-21 22:10:07]

enne@chromium.org changed reviewers:
+ sievers@chromium.org, skyostil@chromium.org, sunnyps@chromium.org

[enne (OOO), 2016-03-22 21:05:18]

This seems to work properly and is mostly plumbing, so I'm a bit unsure about
how to test this.

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:285: NOTREACHED();
This is a bit awkward, but because the BrowserCompositorOutputSurface doesn't
actually participate in surfaces, it never gets this call.  Also, the
BeginFrameSource lives on OnScreenDisplayClient, which is the owner of the
Display, so it doesn't make any sense for the BrowserCompositorOutputSurface
(which also is owned by the OnScreenDisplayClient) to give an output surface
here.

Maybe this means the BeginFrameSource is in an awkward place, or we need a
better split between OutputSurfaces for surfaces and "real" OutputSurfaces, or
that maybe this is just ok.  There are already a number of other NOTREACHED
functions in Display for similar reasons, so this isn't anything new, either.

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/display_cli...
File cc/surfaces/display_client.h (right):

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/display_cli...
cc/surfaces/display_client.h:22: // This must be non-null and always the same.
This has to always the same because the SurfaceDisplayOutputSurface just pulls
this value and has no way of knowing if its changes.

[sunnyps, 2016-03-22 21:52:44]

Left a few comments. Still trying to think about how this fits into the overall
picture.

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:285: NOTREACHED();
On 2016/03/22 21:05:18, enne wrote:
> This is a bit awkward, but because the BrowserCompositorOutputSurface doesn't
> actually participate in surfaces, it never gets this call.  Also, the
> BeginFrameSource lives on OnScreenDisplayClient, which is the owner of the
> Display, so it doesn't make any sense for the BrowserCompositorOutputSurface
> (which also is owned by the OnScreenDisplayClient) to give an output surface
> here.
> 
> Maybe this means the BeginFrameSource is in an awkward place, or we need a
> better split between OutputSurfaces for surfaces and "real" OutputSurfaces, or
> that maybe this is just ok.  There are already a number of other NOTREACHED
> functions in Display for similar reasons, so this isn't anything new, either.

Can't we move the BFS from the display scheduler to display? And have the output
surface set it? There are two different BFS here - the BFS of the output surface
(backed by OS vsync signal or a synthetic one based on vsync interval/timebase)
and the BFS used for child surfaces which should be the scheduler's concern. For
the initial implementation both these could be the same.

also nit: NOTIMPLEMENTED() instead of NOTREACHED()

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/surface_dis...
File cc/surfaces/surface_display_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/surface_dis...
cc/surfaces/surface_display_output_surface.cc:83: allocator_->id_namespace());
I think eventually we want the scheduler to make these associations and change
them if required. I would rather that we associate cc::OutputSurfaces with
surface id namespaces so that the scheduler can set the BFS on the output
surface directly. Or something along those lines.

[enne (OOO), 2016-03-22 22:16:21]

Thanks!

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:285: NOTREACHED();
On 2016/03/22 at 21:52:44, sunnyps wrote:
> On 2016/03/22 21:05:18, enne wrote:
> > This is a bit awkward, but because the BrowserCompositorOutputSurface
doesn't
> > actually participate in surfaces, it never gets this call.  Also, the
> > BeginFrameSource lives on OnScreenDisplayClient, which is the owner of the
> > Display, so it doesn't make any sense for the BrowserCompositorOutputSurface
> > (which also is owned by the OnScreenDisplayClient) to give an output surface
> > here.
> > 
> > Maybe this means the BeginFrameSource is in an awkward place, or we need a
> > better split between OutputSurfaces for surfaces and "real" OutputSurfaces,
or
> > that maybe this is just ok.  There are already a number of other NOTREACHED
> > functions in Display for similar reasons, so this isn't anything new,
either.
> 
> Can't we move the BFS from the display scheduler to display? And have the
output surface set it? There are two different BFS here - the BFS of the output
surface (backed by OS vsync signal or a synthetic one based on vsync
interval/timebase) and the BFS used for child surfaces which should be the
scheduler's concern. For the initial implementation both these could be the
same.

The BFS is currently in the OnScreenDisplayClient and not the DisplayScheduler. 
I can make the BrowserCompositorOuptutSurface own the real output surface and
pass it to Display.  In that case, the Display could hand it to the
DisplayScheduler and the DisplayScheduler could register things?  I'll see how
that looks.

> also nit: NOTIMPLEMENTED() instead of NOTREACHED()

nit nit: I don't really plan on implementing it though, so I think it's
NOTREACHED.

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/surface_dis...
File cc/surfaces/surface_display_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/surface_dis...
cc/surfaces/surface_display_output_surface.cc:83: allocator_->id_namespace());
On 2016/03/22 at 21:52:44, sunnyps wrote:
> I think eventually we want the scheduler to make these associations and change
them if required. I would rather that we associate cc::OutputSurfaces with
surface id namespaces so that the scheduler can set the BFS on the output
surface directly. Or something along those lines.

I'm not sure what you mean by associating cc::OutputSurfaces with surface id
namespaces.  BrowserCompositorOutputSurface doesn't involve itself with surfaces
at all, although arguably a single surface from the display is drawn to it. 
It'd be a little out of place.

The general pattern here is that SurfaceFactoryClients are consumers of
BeginFrameSources.  It also turns out that SurfaceDisplayOutputSurface is also
(awkwardly) the producer of a BeginFrameSource.  So, in this world it registers
a BFS with the SurfaceFactory which will turn around and SetBeginFrameSource on
it.  One option would be to hoist that BFS registration up to the Display or
DisplayScheduler, which would make SurfaceDisplayOutputSurface only a consumer
and then the Display(Scheduler) could change that as needed.

[sunnyps, 2016-03-24 23:14:26]

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/surface_dis...
File cc/surfaces/surface_display_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/20001/cc/surfaces/surface_dis...
cc/surfaces/surface_display_output_surface.cc:83: allocator_->id_namespace());
On 2016/03/22 22:16:21, enne wrote:
> On 2016/03/22 at 21:52:44, sunnyps wrote:
> > I think eventually we want the scheduler to make these associations and
change
> them if required. I would rather that we associate cc::OutputSurfaces with
> surface id namespaces so that the scheduler can set the BFS on the output
> surface directly. Or something along those lines.
> 
> I'm not sure what you mean by associating cc::OutputSurfaces with surface id
> namespaces.  BrowserCompositorOutputSurface doesn't involve itself with
surfaces
> at all, although arguably a single surface from the display is drawn to it. 
> It'd be a little out of place.

Sorry, I keep forgetting that the BFS is associated with the
SurfaceFactoryClient on the browser.

> 
> The general pattern here is that SurfaceFactoryClients are consumers of
> BeginFrameSources.  It also turns out that SurfaceDisplayOutputSurface is also
> (awkwardly) the producer of a BeginFrameSource.  So, in this world it
registers
> a BFS with the SurfaceFactory which will turn around and SetBeginFrameSource
on
> it.  One option would be to hoist that BFS registration up to the Display or
> DisplayScheduler, which would make SurfaceDisplayOutputSurface only a consumer
> and then the Display(Scheduler) could change that as needed.

Hmmm, I think maybe GpuBrowserCompositorOutputSurface (and it's siblings like
the surfaceless one) should own a BFS - it's the place in the browser which gets
vsync parameters from the GPU process and we could make it get vsync signals on
android too. That is also the output surface cc::Display is initialized with so
it also ends up being the BFS of the root surface.

[enne (OOO), 2016-03-25 00:40:45]

sunnyps: patchset #3 here isn't quite ready to land yet, but looks like that
approach is feasible.  Does this look more like what you're aiming for?

[enne (OOO), 2016-03-25 17:55:36]

https://codereview.chromium.org/1821863002/diff/40001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1821863002/diff/40001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:311: // It's expected that there's only a single source
from the
I feel like this comment explains what has to happen here.

I do think this is a bit of an awkward variable push in the middle of
BindToClient where a pull would make more sense since it can only happen once. 
It does the trick, though.

https://codereview.chromium.org/1821863002/diff/40001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1821863002/diff/40001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:42:
client_->UpdateSchedulerBeginFrameSource(begin_frame_source_);
I'm not sure about the split between DisplayScheduler and Display in terms of
registering BeginFrameSources.  Right now I left it as the DisplayScheduler
picking the top level BFS it wants to use (which could be unthrottled, and isn't
in this patch yet), and then the Display registering it with the current surface
id and handling surface id changes (which I don't think happen, but that's not
asserted anywhere).

[no sievers, 2016-03-25 18:55:09]

https://codereview.chromium.org/1821863002/diff/40001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1821863002/diff/40001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:67: DCHECK(vsync_begin_frame_source_);
Then it looks like OutputSurfaceWithoutParent in compositor_impl_android.cc has
to call SetBeginFrameSource().

That's great actually since it already has ExternalBeginFrameSource there (for
the browser compositor, passed through
LayerTreeSettings.external_begin_frame_source) while awkwardly it looks like the
on screen display was using CommitVSyncParameters() with the synthetic one. Now
they can use the same, right?

https://codereview.chromium.org/1821863002/diff/40001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/1821863002/diff/40001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:495: // TODO(enne): this overrides any
preexisting begin frame source.  Those
Yea see other comment wrt Android. Since it currently uses
|external_begin_frame_source| there, one would probably override the other. But
it's probably straightforward to just have them both use the same source now.

[enne (OOO), 2016-03-28 21:43:27]

PTAL.  This passes all the cc and compositor unittests, so should be ready
enough for a real review.  I think these unittests are adequate coverage for
what's going on here, but I'm happy to add more testing if you think there's
something else.

Re: CompositorImpl.  Yeah, I think that's exactly it. 
OutputSurfaceWithoutParent will act like BrowserCompositorOutputSurface.  It'll
own the ExternalBeginFrameSource, and we can slowly get rid of some of the vsync
parameter setting.

The hope is also to get rid of the cc external begin frame source and instead
just have all output sources provide begin frame sources directly.  That means
that RenderWidget will have to pass a begin frame source to output surfaces when
it's creating them, I think.  This will unify the way the begin frame sources
get to the compositor.

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h#n...
cc/surfaces/display.h:104: virtual void
CreateScheduler(base::SingleThreadTaskRunner* task_runner);
This used to be passed in, so that a test scheduler could be created, but that's
awkward with the way the begin frame source gets to the DisplayScheduler so now
there's a virtual to create this for the test that wants a TestDisplayScheduler.

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:38: if (settings.disable_display_vsync) {
I like the way this worked out, in terms of the DisplayScheduler making the
decision here about BeginFrameSource.

https://codereview.chromium.org/1821863002/diff/80001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.h:44: bool use_output_surface_begin_frame_source;
In order to stage Android (and renderers) separately, I've put in a
LayerTreeSetting for whether or not the output surface's begin frame source
should be used.  If the setting is off, then
OutputSurfaceClient::SetBeginFrameSource is ignored.  If the setting is on, then
SingleThreadProxy ignores external/unthrottled/synthetic begin frame sources and
only uses the one set by the OutputSurface and is otherwise null.  That seems
like a reasonable way to stage this for different compositors and platforms, and
also to make sure that this is behaving in a "real" way rather than depending
(briefly) on a fake begin frame source that gets clobbered.

[enne (OOO), 2016-03-28 21:50:07]

I should mention that I'm least happy about what Display ends up looking like,
because it feels like it's doing too much.  It gets a BeginFrameSource from the
BrowserCompositorOutputSurface and hands it to the scheduler, which hands one
back to the Display to register (since it's the one that knows about
SurfaceManager).  I could teach DisplayScheduler about SurfaceManager and the
surface namespace for the compositor and not have that callback, but that's a
little weird too.

[sunnyps, 2016-03-28 23:09:48]

Left a few comments. Looks mostly good.

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h#n...
cc/surfaces/display.h:63: base::SingleThreadTaskRunner* task_runner);
nit: The task runner is base::ThreadTaskRunnerHandle except for tests. So
DisplayScheduler could have a static factory function that passes the right task
runner to its constructor.

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:38: if (settings.disable_display_vsync) {
On 2016/03/28 21:43:27, enne wrote:
> I like the way this worked out, in terms of the DisplayScheduler making the
> decision here about BeginFrameSource.

It feels like Display or BrowserCompositorOutputSurface should decide this. That
way we don't need an UpdateSchedulerBFS method or pass RendererSettings here.
But I'm OK with this approach too.

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_uni...
File cc/surfaces/display_unittest.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_uni...
cc/surfaces/display_unittest.cc:187: SurfaceIdAllocator allocator(3);
nit: Why 3? Can we abstract this into a method on the test class so that we
never have to remember what the next valid namespace is?

https://codereview.chromium.org/1821863002/diff/80001/content/browser/composi...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:25:
scoped_ptr<cc::SyntheticBeginFrameSource> synthetic_begin_frame_source,
BrowserCompositorOutputSurface can create and own the synthetic BFS instead of
it being passed in.

[enne (OOO), 2016-03-29 20:39:53]

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h#n...
cc/surfaces/display.h:63: base::SingleThreadTaskRunner* task_runner);
On 2016/03/28 at 23:09:48, sunnyps wrote:
> nit: The task runner is base::ThreadTaskRunnerHandle except for tests. So
DisplayScheduler could have a static factory function that passes the right task
runner to its constructor.

Where in the pipeline do you want me to change this? The SingleThreadTaskRunner
parameter goes GpuProcessTransportFactory -> OnScreenDisplayClient -> Display ->
DisplayScheduler -> BeginFrameSource.  Should I change all of these?

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:38: if (settings.disable_display_vsync) {
On 2016/03/28 at 23:09:48, sunnyps wrote:
> On 2016/03/28 21:43:27, enne wrote:
> > I like the way this worked out, in terms of the DisplayScheduler making the
> > decision here about BeginFrameSource.
> 
> It feels like Display or BrowserCompositorOutputSurface should decide this.
That way we don't need an UpdateSchedulerBFS method or pass RendererSettings
here. But I'm OK with this approach too.

I'll make Display do it, because Display already has the renderer settings.  I
don't know how this change will jive with a future world where DisplayScheduler
throttles slow renderers when vsync is disabled, but it shouldn't be hard to
solve that when we get there.

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_uni...
File cc/surfaces/display_unittest.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display_uni...
cc/surfaces/display_unittest.cc:187: SurfaceIdAllocator allocator(3);
On 2016/03/28 at 23:09:48, sunnyps wrote:
> nit: Why 3? Can we abstract this into a method on the test class so that we
never have to remember what the next valid namespace is?

It's an arbitrary number.  Put it in a constant to make this more obvious.

https://codereview.chromium.org/1821863002/diff/80001/content/browser/composi...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:25:
scoped_ptr<cc::SyntheticBeginFrameSource> synthetic_begin_frame_source,
On 2016/03/28 at 23:09:48, sunnyps wrote:
> BrowserCompositorOutputSurface can create and own the synthetic BFS instead of
it being passed in.

It was passed in because creating a BFS required a task runner.

[enne (OOO), 2016-03-29 21:57:33]

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h#n...
cc/surfaces/display.h:63: base::SingleThreadTaskRunner* task_runner);
On 2016/03/29 at 20:39:52, enne wrote:
> On 2016/03/28 at 23:09:48, sunnyps wrote:
> > nit: The task runner is base::ThreadTaskRunnerHandle except for tests. So
DisplayScheduler could have a static factory function that passes the right task
runner to its constructor.
> 
> Where in the pipeline do you want me to change this? The
SingleThreadTaskRunner parameter goes GpuProcessTransportFactory ->
OnScreenDisplayClient -> Display -> DisplayScheduler -> BeginFrameSource. 
Should I change all of these?

Talked to danakj for base ownership guidance.  She said that explicit plumbing
of the task runner was better than using global accessors so that in the future
this could all be switched to using thread pools.  She suggested switching
SingleThreadTaskRunner to SequencedTaskRunner as well, but given things like
RenderThreadImpl local storage, I don't think the compositor is ready to be run
on multiple threads.

So, end result is that I'm going to leave this as-is.

[sunnyps, 2016-03-29 22:00:10]

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/80001/cc/surfaces/display.h#n...
cc/surfaces/display.h:63: base::SingleThreadTaskRunner* task_runner);
On 2016/03/29 21:57:33, enne wrote:
> On 2016/03/29 at 20:39:52, enne wrote:
> > On 2016/03/28 at 23:09:48, sunnyps wrote:
> > > nit: The task runner is base::ThreadTaskRunnerHandle except for tests. So
> DisplayScheduler could have a static factory function that passes the right
task
> runner to its constructor.
> > 
> > Where in the pipeline do you want me to change this? The
> SingleThreadTaskRunner parameter goes GpuProcessTransportFactory ->
> OnScreenDisplayClient -> Display -> DisplayScheduler -> BeginFrameSource. 
> Should I change all of these?
> 
> Talked to danakj for base ownership guidance.  She said that explicit plumbing
> of the task runner was better than using global accessors so that in the
future
> this could all be switched to using thread pools.  She suggested switching
> SingleThreadTaskRunner to SequencedTaskRunner as well, but given things like
> RenderThreadImpl local storage, I don't think the compositor is ready to be
run
> on multiple threads.
> 
> So, end result is that I'm going to leave this as-is.

Ok. Sounds good.

[enne (OOO), 2016-03-29 22:15:28]

Description was changed from

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

Currently, the BrowserCompositorOutputSurface is out the loop and
doesn't implement the SetBeginFrameSource, as the real begin frame
source is owned by OnScreenDisplayClient and it doesn't participate in
the cc::Surfaces sytem directly.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-03-29 22:21:30]

Description was changed from

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

The BeginFrameSource is created by the BrowserCompositorOutputSurface
which updates it based on vsync information that it receives.  This
BeginFrameSource is passed to the Display (via OutputSurfaceClient)
and then to the DisplayScheduler.  The DisplayScheduler makes a
decision about what BeginFrameSource to use and tells the Display
about it, which informs the SurfaceManager that the compositor using
that Display should be driven by that BeginFrameSource.  The
SurfaceManager then informs the SurfaceDisplayOutputSurface about
the BeginFrameSource, which passes it into the single thread proxy's
cc::Scheduler for that ui::Compositor's instance.  Plumbing!

The path from SurfaceManager to SurfaceDisplayOutputSurface was
added in https://codereview.chromium.org/1673783004, but the rest
of the plumbing is new to this patch.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-03-30 17:34:41]

https://codereview.chromium.org/1821863002/diff/80001/content/browser/composi...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:25:
scoped_ptr<cc::SyntheticBeginFrameSource> synthetic_begin_frame_source,
On 2016/03/29 at 20:39:52, enne wrote:
> On 2016/03/28 at 23:09:48, sunnyps wrote:
> > BrowserCompositorOutputSurface can create and own the synthetic BFS instead
of it being passed in.
> 
> It was passed in because creating a BFS required a task runner.

Would you rather I just pass in a task runner?

[sunnyps, 2016-03-30 18:56:02]

https://codereview.chromium.org/1821863002/diff/80001/content/browser/composi...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/80001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:25:
scoped_ptr<cc::SyntheticBeginFrameSource> synthetic_begin_frame_source,
On 2016/03/30 17:34:41, enne wrote:
> On 2016/03/29 at 20:39:52, enne wrote:
> > On 2016/03/28 at 23:09:48, sunnyps wrote:
> > > BrowserCompositorOutputSurface can create and own the synthetic BFS
instead
> of it being passed in.
> > 
> > It was passed in because creating a BFS required a task runner.
> 
> Would you rather I just pass in a task runner?

I think either way is fine. Don't have a strong opinion on it.

[enne (OOO), 2016-03-31 20:14:55]

sunnyps: Do you have more comments about this? I think I have addressed what you
asked about before.

[sunnyps, 2016-04-01 22:35:26]

https://codereview.chromium.org/1821863002/diff/160001/cc/output/output_surfa...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/1821863002/diff/160001/cc/output/output_surfa...
cc/output/output_surface_client.h:28: virtual void
CommitVSyncParameters(base::TimeTicks timebase,
nit: This only calls CommitVSyncParameters on the client. So maybe we can remove
this and move the call to the client to the renderer's output surface?

https://codereview.chromium.org/1821863002/diff/160001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/160001/cc/surfaces/display.h#...
cc/surfaces/display.h:127: BeginFrameSource* scheduler_begin_frame_source_;
Not sure why the scheduler will ever tell a Display to use a particular BFS. I
think we'll only ever want to set the BFS for renderer surfaces and never for
the Display/ui::Compositor.

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

https://codereview.chromium.org/1821863002/diff/160001/components/mus/surface...
components/mus/surfaces/direct_output_surface.cc:46: auto timebase_time_ticks =
base::TimeTicks::FromInternalValue(timebase);
nit: Don't really need the _time_ticks and _time_delta suffixes.

https://codereview.chromium.org/1821863002/diff/160001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/160001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.cc:53:
cc::switches::kEnableBeginFrameScheduling)) {
nit: Is this flag ever false? If not can you remove it?

[enne (OOO), 2016-04-02 00:18:39]

https://codereview.chromium.org/1821863002/diff/160001/cc/output/output_surfa...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/1821863002/diff/160001/cc/output/output_surfa...
cc/output/output_surface_client.h:28: virtual void
CommitVSyncParameters(base::TimeTicks timebase,
On 2016/04/01 at 22:35:25, sunnyps wrote:
> nit: This only calls CommitVSyncParameters on the client. So maybe we can
remove this and move the call to the client to the renderer's output surface?

I think you're going to need to be more specific with what you're suggesting
here.

The vsync path needs to stick around for a while until enable begin frame
scheduling is turned on everywhere.  Also, begin frame sources live outside of
output surfaces in the renderer at the moment.  So the path to set vsync
parameters on the renderer begin frame source is roughly
BrowserCompositorOutputSurface -> CompositorVSyncManager -> DelegatedFrameHost
-> RenderWidgetHostViewAura -> RenderWidgetHostImpl ->
ViewMsg_UpdateVSyncParameters -> CompositorOutputSurface -> OutputSurfaceClient
/ LayerTreeHostImpl -> ProxyImpl -> SyntheticBeginFrameSource.

I think you're suggesting moving the CommitVSyncParameters call off of the
client interface (OutputSurfaceClient) and have the renderer's OutputSurface
(CompositorOutputSurface, usually) call the client (LayerTreeHostImpl) directly.
 Unfortunately, not every OutputSurface is a CompositorOutputSurface and the
creator of CompositorOutputSurface (RenderWidget) doesn't know about
LayerTreeHostImpl, so I'd need to add some "SetLayerTreeHostImpl" virtual that
only CompositorOutputSurface would implement? I can't quite picture what this
would look like.

I think in any case, this will go away soon on its own.  I also want to hoist
the ownership of external/synthetic begin frame sources into the output surfaces
themselves as a cleanup, which would also get rid of this client call.

https://codereview.chromium.org/1821863002/diff/160001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/160001/cc/surfaces/display.h#...
cc/surfaces/display.h:127: BeginFrameSource* scheduler_begin_frame_source_;
On 2016/04/01 at 22:35:25, sunnyps wrote:
> Not sure why the scheduler will ever tell a Display to use a particular BFS. I
think we'll only ever want to set the BFS for renderer surfaces and never for
the Display/ui::Compositor.

Ok, then I think I misunderstood how you wanted to handle giving different BFS
to children and how you wanted the DisplayScheduler involved.  I've removed this
and now the Display directly registers/unregisters the BFS it wants based on
settings.

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

https://codereview.chromium.org/1821863002/diff/160001/components/mus/surface...
components/mus/surfaces/direct_output_surface.cc:46: auto timebase_time_ticks =
base::TimeTicks::FromInternalValue(timebase);
On 2016/04/01 at 22:35:25, sunnyps wrote:
> nit: Don't really need the _time_ticks and _time_delta suffixes.

This code was moved from TopLevelDisplayClient, and those suffixes differentiate
from the function parameters.  Do you have a suggestion on what I should call
this if you don't like this code?

https://codereview.chromium.org/1821863002/diff/160001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/160001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.cc:53:
cc::switches::kEnableBeginFrameScheduling)) {
On 2016/04/01 at 22:35:25, sunnyps wrote:
> nit: Is this flag ever false? If not can you remove it?

Yes.  This flag is sometimes false.  That boolean is used to sometimes send
vsync information to the renderer when it is false.  That vsync path can't be
removed until all the renderers are using begin frame scheduling, unfortunately.

[sunnyps, 2016-04-02 00:47:15]

https://codereview.chromium.org/1821863002/diff/160001/cc/output/output_surfa...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/1821863002/diff/160001/cc/output/output_surfa...
cc/output/output_surface_client.h:28: virtual void
CommitVSyncParameters(base::TimeTicks timebase,
On 2016/04/02 00:18:38, enne wrote:
> On 2016/04/01 at 22:35:25, sunnyps wrote:
> > nit: This only calls CommitVSyncParameters on the client. So maybe we can
> remove this and move the call to the client to the renderer's output surface?
> 
> I think you're going to need to be more specific with what you're suggesting
> here.
> 
> The vsync path needs to stick around for a while until enable begin frame
> scheduling is turned on everywhere.  Also, begin frame sources live outside of
> output surfaces in the renderer at the moment.  So the path to set vsync
> parameters on the renderer begin frame source is roughly
> BrowserCompositorOutputSurface -> CompositorVSyncManager -> DelegatedFrameHost
> -> RenderWidgetHostViewAura -> RenderWidgetHostImpl ->
> ViewMsg_UpdateVSyncParameters -> CompositorOutputSurface ->
OutputSurfaceClient
> / LayerTreeHostImpl -> ProxyImpl -> SyntheticBeginFrameSource.
> 
> I think you're suggesting moving the CommitVSyncParameters call off of the
> client interface (OutputSurfaceClient) and have the renderer's OutputSurface
> (CompositorOutputSurface, usually) call the client (LayerTreeHostImpl)
directly.
>  Unfortunately, not every OutputSurface is a CompositorOutputSurface and the
> creator of CompositorOutputSurface (RenderWidget) doesn't know about
> LayerTreeHostImpl, so I'd need to add some "SetLayerTreeHostImpl" virtual that
> only CompositorOutputSurface would implement? I can't quite picture what this
> would look like.

Sorry for the confusion, I thought I was looking at output_surface.h.

I was suggesting that we remove OutputSurface::CommitVSyncParameters (which just
calls the same method on the client) and that
CompositorOutputSurface::OnUpdateVSyncParametersFromBrowser call
OutputSurfaceClient::CommitVSyncParameters.

> 
> I think in any case, this will go away soon on its own.  I also want to hoist
> the ownership of external/synthetic begin frame sources into the output
surfaces
> themselves as a cleanup, which would also get rid of this client call.

https://codereview.chromium.org/1821863002/diff/160001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/160001/cc/surfaces/display.h#...
cc/surfaces/display.h:127: BeginFrameSource* scheduler_begin_frame_source_;
On 2016/04/02 00:18:38, enne wrote:
> On 2016/04/01 at 22:35:25, sunnyps wrote:
> > Not sure why the scheduler will ever tell a Display to use a particular BFS.
I
> think we'll only ever want to set the BFS for renderer surfaces and never for
> the Display/ui::Compositor.
> 
> Ok, then I think I misunderstood how you wanted to handle giving different BFS
> to children and how you wanted the DisplayScheduler involved.  I've removed
this
> and now the Display directly registers/unregisters the BFS it wants based on
> settings.

Can you also update the CL description to reflect this change?

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

https://codereview.chromium.org/1821863002/diff/160001/components/mus/surface...
components/mus/surfaces/direct_output_surface.cc:46: auto timebase_time_ticks =
base::TimeTicks::FromInternalValue(timebase);
On 2016/04/02 00:18:38, enne wrote:
> On 2016/04/01 at 22:35:25, sunnyps wrote:
> > nit: Don't really need the _time_ticks and _time_delta suffixes.
> 
> This code was moved from TopLevelDisplayClient, and those suffixes
differentiate
> from the function parameters.  Do you have a suggestion on what I should call
> this if you don't like this code?

ah, ok nvm.

[enne (OOO), 2016-04-04 17:26:21]

Description was changed from

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

The BeginFrameSource is created by the BrowserCompositorOutputSurface
which updates it based on vsync information that it receives.  This
BeginFrameSource is passed to the Display (via OutputSurfaceClient)
and then to the DisplayScheduler.  The DisplayScheduler makes a
decision about what BeginFrameSource to use and tells the Display
about it, which informs the SurfaceManager that the compositor using
that Display should be driven by that BeginFrameSource.  The
SurfaceManager then informs the SurfaceDisplayOutputSurface about
the BeginFrameSource, which passes it into the single thread proxy's
cc::Scheduler for that ui::Compositor's instance.  Plumbing!

The path from SurfaceManager to SurfaceDisplayOutputSurface was
added in https://codereview.chromium.org/1673783004, but the rest
of the plumbing is new to this patch.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

The BeginFrameSource is created by the BrowserCompositorOutputSurface
which updates it based on vsync information that it receives.  This
BeginFrameSource is passed to the Display (via OutputSurfaceClient),
which informs the SurfaceManager that the compositor using
that Display should be driven by that BeginFrameSource.  The
SurfaceManager then informs the SurfaceDisplayOutputSurface about
the BeginFrameSource, which passes it into the single thread proxy's
cc::Scheduler for that ui::Compositor's instance.  Plumbing!

The path from SurfaceManager to SurfaceDisplayOutputSurface was
added in https://codereview.chromium.org/1673783004, but the rest
of the plumbing is new to this patch.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-04-04 18:12:33]

https://codereview.chromium.org/1821863002/diff/160001/cc/output/output_surfa...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/1821863002/diff/160001/cc/output/output_surfa...
cc/output/output_surface_client.h:28: virtual void
CommitVSyncParameters(base::TimeTicks timebase,
On 2016/04/02 at 00:47:15, sunnyps wrote:

> Sorry for the confusion, I thought I was looking at output_surface.h.
> 
> I was suggesting that we remove OutputSurface::CommitVSyncParameters (which
just calls the same method on the client) and that
CompositorOutputSurface::OnUpdateVSyncParametersFromBrowser call
OutputSurfaceClient::CommitVSyncParameters.

Oh!  Good thought.  Done.  :)

https://codereview.chromium.org/1821863002/diff/160001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/1821863002/diff/160001/cc/surfaces/display.h#...
cc/surfaces/display.h:127: BeginFrameSource* scheduler_begin_frame_source_;
On 2016/04/02 at 00:47:15, sunnyps wrote:

> Can you also update the CL description to reflect this change?

Done, thanks for noticing that.  :)

[sunnyps, 2016-04-04 20:46:10]

LGTM

[no sievers, 2016-04-04 21:25:15]

https://codereview.chromium.org/1821863002/diff/200001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/200001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.cc:25:
base::SingleThreadTaskRunner* task_runner,
just one nit: Since |task_runner| is a bit of plumbing through the subclasses
and it's also not immediately obvious what it's for, do you think we can just
defer BFS creation here and imply the task runner from BindToClient() and
current task runner below?

[enne (OOO), 2016-04-05 22:32:52]

enne@chromium.org changed reviewers:
+ sunnyps@chromium.orgh
- sunnyps@chromium.org

[enne (OOO), 2016-04-05 22:32:59]

sievers: I also ended up having to move the ExternalBeginFrameSource in
CompositorImpl to be owned by the real output surface there so that it would
follow the same path as the desktop browser.  PTAL

https://codereview.chromium.org/1821863002/diff/200001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/200001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.cc:25:
base::SingleThreadTaskRunner* task_runner,
On 2016/04/04 at 21:25:15, sievers wrote:
> just one nit: Since |task_runner| is a bit of plumbing through the subclasses
and it's also not immediately obvious what it's for, do you think we can just
defer BFS creation here and imply the task runner from BindToClient() and
current task runner below?

Ok, sure.  Both you and sunnyps asked about this, and it does look like this
implicit stuff is already happening in BindToClient, so I'll just be implicit
here as well.  I removed it from both the OutputSurface ctor and
Display::Initialize.

[no sievers, 2016-04-05 23:12:52]

Thanks for fixing up the Android stuff, that looks already better now.

https://codereview.chromium.org/1821863002/diff/240001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/240001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.cc:31:
base::ThreadTaskRunnerHandle::Get().get(),
Shouldn't it be the same thread that BindToClient() binds to i.e. don't you have
to do it there?
(Even if currently everything might be single-threaded.)

[enne (OOO), 2016-04-06 20:19:32]

After some offline discussion with sievers and danakj, it sounds like the
explicit task runner can't be removed, as the compositor receives a nested
message loop sometimes and shouldn't post tasks to the outer one.  So, I've
reverted those changes.

PTAL

[no sievers, 2016-04-06 21:40:19]

lgtm

https://codereview.chromium.org/1821863002/diff/280001/cc/trees/layer_tree_se...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/1821863002/diff/280001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.h:44: bool use_output_surface_begin_frame_source;
obsolete again in latest ps?

https://codereview.chromium.org/1821863002/diff/280001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1821863002/diff/280001/ui/compositor/composit...
ui/compositor/compositor.cc:175: settings.use_output_surface_begin_frame_source
= true;
and here, unused too?

[enne (OOO), 2016-04-06 21:42:56]

enne@chromium.org changed reviewers:
+ sunnyps@chromium.org

[enne (OOO), 2016-04-06 21:42:58]

https://codereview.chromium.org/1821863002/diff/240001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/1821863002/diff/240001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.cc:31:
base::ThreadTaskRunnerHandle::Get().get(),
On 2016/04/05 at 23:12:52, sievers wrote:
> Shouldn't it be the same thread that BindToClient() binds to i.e. don't you
have to do it there?
> (Even if currently everything might be single-threaded.)

Done.

https://codereview.chromium.org/1821863002/diff/280001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1821863002/diff/280001/ui/compositor/composit...
ui/compositor/compositor.cc:175: settings.use_output_surface_begin_frame_source
= true;
On 2016/04/06 at 21:40:19, sievers wrote:
> and here, unused too?

This is used in SingleThreadProxy.

[enne (OOO), 2016-04-07 18:38:55]

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

[enne (OOO), 2016-04-07 18:39:50]

enne@chromium.org changed reviewers:
+ boliu@chromium.org, sky@chromium.org

[enne (OOO), 2016-04-07 18:40:19]

Additional owners:

danakj: ui/compositor
boliu: android_webview
sky: components/mus

[boliu, 2016-04-07 18:43:15]

On 2016/04/07 18:40:19, enne wrote:
> Additional owners:
> 
> danakj: ui/compositor
> boliu: android_webview

rs lgtm

> sky: components/mus

[danakj, 2016-04-07 18:43:24]

LGTM

[sky, 2016-04-07 20:00:55]

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

[sky, 2016-04-07 20:00:56]

sky->fsamuel

[Fady Samuel, 2016-04-09 04:23:36]

mus lgtm

[enne (OOO), 2016-04-11 17:11:29]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-04-11 17:11:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
sunnyps@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1821863002/#ps300001
(title: "Fix webview compile")

[commit-bot: I haz the power, 2016-04-11 17:11:40]

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

[commit-bot: I haz the power, 2016-04-11 17:13:59]

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

[commit-bot: I haz the power, 2016-04-11 17:14:01]

Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[enne (OOO), 2016-04-13 00:32:17]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-04-13 00:32:18]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
danakj@chromium.org, sievers@chromium.org, fsamuel@chromium.org,
sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/1821863002/#ps360001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-13 00:32:44]

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

[commit-bot: I haz the power, 2016-04-13 00:42:08]

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

[commit-bot: I haz the power, 2016-04-13 00:42:10]

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

[enne (OOO), 2016-04-14 02:47:08]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-04-14 02:47:09]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
danakj@chromium.org, sievers@chromium.org, fsamuel@chromium.org,
sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/1821863002/#ps440001
(title: "Fix Display member destruction order")

[commit-bot: I haz the power, 2016-04-14 02:47:44]

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

[commit-bot: I haz the power, 2016-04-14 03:35:47]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-04-14 03:37:26]

Description was changed from

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

The BeginFrameSource is created by the BrowserCompositorOutputSurface
which updates it based on vsync information that it receives.  This
BeginFrameSource is passed to the Display (via OutputSurfaceClient),
which informs the SurfaceManager that the compositor using
that Display should be driven by that BeginFrameSource.  The
SurfaceManager then informs the SurfaceDisplayOutputSurface about
the BeginFrameSource, which passes it into the single thread proxy's
cc::Scheduler for that ui::Compositor's instance.  Plumbing!

The path from SurfaceManager to SurfaceDisplayOutputSurface was
added in https://codereview.chromium.org/1673783004, but the rest
of the plumbing is new to this patch.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up ui::Compositor to Display's BeginFrameSource

This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

The BeginFrameSource is created by the BrowserCompositorOutputSurface
which updates it based on vsync information that it receives.  This
BeginFrameSource is passed to the Display (via OutputSurfaceClient),
which informs the SurfaceManager that the compositor using
that Display should be driven by that BeginFrameSource.  The
SurfaceManager then informs the SurfaceDisplayOutputSurface about
the BeginFrameSource, which passes it into the single thread proxy's
cc::Scheduler for that ui::Compositor's instance.  Plumbing!

The path from SurfaceManager to SurfaceDisplayOutputSurface was
added in https://codereview.chromium.org/1673783004, but the rest
of the plumbing is new to this patch.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/19c108580b99c469ed192066310e3162bf8c196e
Cr-Commit-Position: refs/heads/master@{#387228}
==========

[commit-bot: I haz the power, 2016-04-14 03:37:28]

Patchset 23 (id:??) landed as
https://crrev.com/19c108580b99c469ed192066310e3162bf8c196e
Cr-Commit-Position: refs/heads/master@{#387228}