cc: Send all begin frames using a PostTask.

cc: Send all begin frames using a PostTask.

The scheduler retro frame logic turns the BeginFrame that's sent during
DelayBasedBeginFrameSource::AddObserver to a PostTask. That's needed
because AddObserver is called in Scheduler::ProcessScheduledActions and
that method guards against reentrancy.

Also includes cleanup of DelayBasedTimeSource usage.

BUG=602485
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[sunnyps, 2016-07-13 20:41:30]

Description was changed from

==========
cc: Send all begin frames using a PostTask.

The scheduler retro frame logic turns the BeginFrame that's sent during
DelayBasedBeginFrameSource::AddObserver to a PostTask. That's needed
because AddObserver is called in Scheduler::ProcessScheduledActions and
that method guards against reentrancy.

Also includes cleanup of DelayBasedTimeSource usage.

R=brianderson@chromium.org
BUG=602485
==========

to

==========
cc: Send all begin frames using a PostTask.

The scheduler retro frame logic turns the BeginFrame that's sent during
DelayBasedBeginFrameSource::AddObserver to a PostTask. That's needed
because AddObserver is called in Scheduler::ProcessScheduledActions and
that method guards against reentrancy.

Also includes cleanup of DelayBasedTimeSource usage.

R=brianderson@chromium.org
BUG=602485
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunnyps, 2016-07-13 20:42:51]

Description was changed from

==========
cc: Send all begin frames using a PostTask.

The scheduler retro frame logic turns the BeginFrame that's sent during
DelayBasedBeginFrameSource::AddObserver to a PostTask. That's needed
because AddObserver is called in Scheduler::ProcessScheduledActions and
that method guards against reentrancy.

Also includes cleanup of DelayBasedTimeSource usage.

R=brianderson@chromium.org
BUG=602485
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Send all begin frames using a PostTask.

The scheduler retro frame logic turns the BeginFrame that's sent during
DelayBasedBeginFrameSource::AddObserver to a PostTask. That's needed
because AddObserver is called in Scheduler::ProcessScheduledActions and
that method guards against reentrancy.

Also includes cleanup of DelayBasedTimeSource usage.

BUG=602485
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunnyps, 2016-07-13 20:47:22]

sunnyps@chromium.org changed reviewers:
+ danakj@chromium.org, fsamuel@chromium.org

[sunnyps, 2016-07-13 20:47:23]

PTAL

danakj: Please RS content/browser and ui/compositor
fsamuel: Please RS services/ui

[danakj, 2016-07-13 21:00:59]

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.h:143: // For testing.
Can we avoid reintroducing for-testing constructors?

[danakj, 2016-07-13 21:03:46]

> Also includes cleanup of DelayBasedTimeSource usage.

I feel like you're undoing some stuff I did here so let me explain what I did.

I made the constructors always use DelayBasedTimeSource because
1) that lets tests control "Now" when they want to, without adding For-Testing
overrides/methods.
2) DelayBasedTimeSource deals with task posting
3) The BFS doesn't need to know about a TaskRunner*, DelayBasedTimeSource
encapsulated that.

I could be convinced that some of these are not valuable (anymore?). But I
really don't want to revert #1.

[brianderson, 2016-07-13 21:14:49]

lgtm, pending Dana's comments and nits.

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:105: clock_ ? clock_->NowTicks() :
base::TimeTicks::Now();
Maybe use a base::DefaultTickClock instead of checking for NULL?

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/delay_based_ti...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/delay_based_ti...
cc/scheduler/delay_based_time_source.cc:60: return clock_ ? clock_->NowTicks() :
base::TimeTicks::Now();
DefaultTickClock?

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/delay_based_ti...
File cc/scheduler/delay_based_time_source.h (right):

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/delay_based_ti...
cc/scheduler/delay_based_time_source.h:66: base::TimeTicks Now();
Should this just go away now that there's a tick clock?

[sunnyps, 2016-07-13 21:22:20]

On 2016/07/13 21:03:46, danakj wrote:
> > Also includes cleanup of DelayBasedTimeSource usage.
> 
> I feel like you're undoing some stuff I did here so let me explain what I did.
> 
> I made the constructors always use DelayBasedTimeSource because
> 1) that lets tests control "Now" when they want to, without adding For-Testing
> overrides/methods.

Do you think it's better if we provided a std::unique_ptr<base::TickClock> to
DelayBasedTimeSource? That way we won't have any for test constructors / methods
/ overrides.

> 2) DelayBasedTimeSource deals with task posting
> 3) The BFS doesn't need to know about a TaskRunner*, DelayBasedTimeSource
> encapsulated that.

I'll see if I can make DelayBasedTimeSource handle missed ticks - when we call
AddObserver we want to PostTask a missed tick. DelayBasedTimeSource doesn't
really know about observers so I can't see an easy way to do this so
DelayBasedBeginFrameSource will most likely need to know about task runners.

> 
> I could be convinced that some of these are not valuable (anymore?). But I
> really don't want to revert #1.

[sunnyps, 2016-07-13 21:24:53]

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2150533004/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:105: clock_ ? clock_->NowTicks() :
base::TimeTicks::Now();
On 2016/07/13 21:14:49, brianderson wrote:
> Maybe use a base::DefaultTickClock instead of checking for NULL?

I tried that but then DelayBasedTimeSource ended up owning the clock (unless you
want to instantiate a base::DefaultTickClock everywhere) which is fine except
that tests instantiate both back to back and delay based begin frame sources.
That can obviously be worked around and I'll probably do that.

[danakj, 2016-07-13 22:10:12]

On 2016/07/13 21:22:20, sunnyps wrote:
> On 2016/07/13 21:03:46, danakj wrote:
> > > Also includes cleanup of DelayBasedTimeSource usage.
> > 
> > I feel like you're undoing some stuff I did here so let me explain what I
did.
> > 
> > I made the constructors always use DelayBasedTimeSource because
> > 1) that lets tests control "Now" when they want to, without adding
For-Testing
> > overrides/methods.
> 
> Do you think it's better if we provided a std::unique_ptr<base::TickClock> to
> DelayBasedTimeSource? That way we won't have any for test constructors /
methods
> / overrides.

Ya I think that the BFS needs something that provides time, and we should let
that thing be replaced/controlled by tests as needed. Either
DelayBasedTimeSource can be swapped out,
or it can own/use something else that can be swapped out. Previously, we used a
TestDelayBasedTimeSource, but just having it always be DelayBasedTimeSource with
a
different kinda of TickClock sounds good too, ownership questions aside.

> 
> > 2) DelayBasedTimeSource deals with task posting
> > 3) The BFS doesn't need to know about a TaskRunner*, DelayBasedTimeSource
> > encapsulated that.
> 
> I'll see if I can make DelayBasedTimeSource handle missed ticks - when we call
> AddObserver we want to PostTask a missed tick. DelayBasedTimeSource doesn't
> really know about observers so I can't see an easy way to do this so
> DelayBasedBeginFrameSource will most likely need to know about task runners.

Can it tell the timesource to tick immediately or something?

> > 
> > I could be convinced that some of these are not valuable (anymore?). But I
> > really don't want to revert #1.

[Fady Samuel, 2016-07-13 22:21:02]

services/ui/surfaces lgtm

[sunnyps, 2016-07-15 23:50:16]

I decided to work around the PostTask issue in the scheduler instead of
modifying all begin frame sources. Closing this CL.

[sunnyps, 2016-07-15 23:50:23]

Description was changed from

==========
cc: Send all begin frames using a PostTask.

The scheduler retro frame logic turns the BeginFrame that's sent during
DelayBasedBeginFrameSource::AddObserver to a PostTask. That's needed
because AddObserver is called in Scheduler::ProcessScheduledActions and
that method guards against reentrancy.

Also includes cleanup of DelayBasedTimeSource usage.

BUG=602485
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Send all begin frames using a PostTask.

The scheduler retro frame logic turns the BeginFrame that's sent during
DelayBasedBeginFrameSource::AddObserver to a PostTask. That's needed
because AddObserver is called in Scheduler::ProcessScheduledActions and
that method guards against reentrancy.

Also includes cleanup of DelayBasedTimeSource usage.

BUG=602485
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========