Create a new action triggered when a BeginMainFrame is not expected before vsync

Create new action sent when a BeginMainFrame not expected before vsync.

The renderer scheduler can then use this information to schedule short idle
work as would have happened after a BeginMainFrame. This allows idle work to be
scheduled for instance in cases where a compositor animation is occurring which
prevents BeginMainFrameNotExpectedSoon from being sent.

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

Review-Url: https://codereview.chromium.org/2753843003
Cr-Commit-Position: refs/heads/master@{#469619}
Committed: https://chromium.googlesource.com/chromium/src/+/9db74aa87f97c709baf5d38b82fb987713a40951

[Dan Elphick, 2017-03-15 15:51:24]

Description was changed from

==========
Create a new idle work action triggered when a main frame is not required
but otherwise could be scheduled.

BUG=683962
==========

to

==========
Create a new idle work action triggered when a main frame is not required
but otherwise could be scheduled.

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

[Sami, 2017-03-15 16:33:08]

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

[Sami, 2017-03-15 16:33:08]

Seems like a good direction, although we can probably simplify the logic a bit.

https://codereview.chromium.org/2753843003/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:48: do_idle_work_funnel_(true),
Maybe add a comment why this starts out as true?

[Dan Elphick, 2017-03-15 16:44:09]

https://codereview.chromium.org/2753843003/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:48: do_idle_work_funnel_(true),
On 2017/03/15 16:33:08, Sami wrote:
> Maybe add a comment why this starts out as true?

oops that would be a typo.

[Sami, 2017-03-20 16:58:50]

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:634: BeginImplFrameNotExpectedSoon();
Let's introduce a new notification which kicks off a short idle period rather
than a long one to avoid accidental jank.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:436: if (needs_begin_main_frame_)
// BeginMainFrame already triggers idle work, so we don't need to schedule any
extra idle work here.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:439: // We can not perform commits if we
are not visible.
// Only do FooIdle work when we're visible.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:444: // so should also stop
BeginMainFrames.
...stop FooIdle work

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:449: if (defer_commits_)
This one we can drop.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:468: if (begin_main_frame_state_ !=
BEGIN_MAIN_FRAME_STATE_IDLE)
// If a BeginMainFrame is in progress, don't do any extra idle work.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:472: if
(!settings_.main_frame_before_activation_enabled && has_pending_tree_)
Drop this.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:476: if (settings_.commit_to_active_tree
&&
Drop this.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:483: if (ImplLatencyTakesPriority() &&
Drop this.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:494: if
(!settings_.using_synchronous_renderer_compositor &&
Drop this.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:501: if (forced_redraw_state_ ==
FORCED_REDRAW_STATE_WAITING_FOR_COMMIT)
Drop this.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:505: if
(!HasInitializedCompositorFrameSink())
Drop this.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:508: if
(!settings_.main_frame_while_submit_frame_throttled_enabled) {
Drop this.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:521: if
(skip_next_begin_main_frame_to_reduce_latency_)
Drop this.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:693: if (ShouldDoIdleWork())
Let's move this to the end.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:794: DCHECK(!has_pending_tree_ ||
settings_.main_frame_before_activation_enabled);
Drop this.

[Dan Elphick, 2017-03-21 11:23:51]

https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:436: if (needs_begin_main_frame_ ||
send_begin_main_frame_funnel_)
I made this change here because needs_begin_main_frame gets reset after it's
handled. So essentially we'd get idle work after every main frame.

I've tested the two cases we had though and they still work after this change.

[Sami, 2017-03-21 11:35:21]

https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:436: if (needs_begin_main_frame_ ||
send_begin_main_frame_funnel_)
On 2017/03/21 11:23:51, Dan Elphick wrote:
> I made this change here because needs_begin_main_frame gets reset after it's
> handled. So essentially we'd get idle work after every main frame.
> 
> I've tested the two cases we had though and they still work after this change.

I wonder if we should look at begin_main_frame_state_ instead? What states does
it cycle through? Basically we want to avoid doing any extra idle work if
there's a BeginMainFrame in any part of the pipeline I think (perhaps that means
we should look at pending trees too)

[Sami, 2017-03-21 11:36:39]

https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:436: if (needs_begin_main_frame_ ||
send_begin_main_frame_funnel_)
On 2017/03/21 11:35:21, Sami wrote:
> On 2017/03/21 11:23:51, Dan Elphick wrote:
> > I made this change here because needs_begin_main_frame gets reset after it's
> > handled. So essentially we'd get idle work after every main frame.
> > 
> > I've tested the two cases we had though and they still work after this
change.
> 
> I wonder if we should look at begin_main_frame_state_ instead? What states
does
> it cycle through? Basically we want to avoid doing any extra idle work if
> there's a BeginMainFrame in any part of the pipeline I think (perhaps that
means
> we should look at pending trees too)

Just to clarify: the problem with looking at the funnel is that it's really a
rate limiting mechanism for BeginMainFrames, and if we decide to allow more
BeginMainFrames in the future it wouldn't be sufficient here anymore.

[Dan Elphick, 2017-03-21 11:58:38]

On 2017/03/21 11:36:39, Sami wrote:
>
https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
> File cc/scheduler/scheduler_state_machine.cc (right):
> 
>
https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
> cc/scheduler/scheduler_state_machine.cc:436: if (needs_begin_main_frame_ ||
> send_begin_main_frame_funnel_)
> On 2017/03/21 11:35:21, Sami wrote:
> > On 2017/03/21 11:23:51, Dan Elphick wrote:
> > > I made this change here because needs_begin_main_frame gets reset after
it's
> > > handled. So essentially we'd get idle work after every main frame.
> > > 
> > > I've tested the two cases we had though and they still work after this
> change.
> > 
> > I wonder if we should look at begin_main_frame_state_ instead? What states
> does
> > it cycle through? Basically we want to avoid doing any extra idle work if
> > there's a BeginMainFrame in any part of the pipeline I think (perhaps that
> means
> > we should look at pending trees too)
> 
> Just to clarify: the problem with looking at the funnel is that it's really a
> rate limiting mechanism for BeginMainFrames, and if we decide to allow more
> BeginMainFrames in the future it wouldn't be sufficient here anymore.

We already have:

if (begin_main_frame_state_ != BEGIN_MAIN_FRAME_STATE_IDLE)
  return false;

The reason I was looking at this was SchedulerStateMachineTest.TestFullCycle
(along with lots of other tests failed).

It fails at
https://cs.chromium.org/chromium/src/cc/scheduler/scheduler_state_machine_uni...
because the action is DO_IDLE_WORK rather than NONE. begin_main_frame_state has
already returned to idle at this point.

I could add another bool to the statemachine that indicates that a main frame
has been requested and which is cleared in OnBeginImplFrame - although then its
state exactly mirrors the funnel.

Perhaps we could rename the funnel variable to be more descriptive. A
counter-argument to the rate-limiting argument would be that if it was used in
that way the type would have to change anyway which would make it quickly
apparent that it was being used to limit both IdleWork and MainFrames.

[Sami, 2017-03-21 12:09:59]

On 2017/03/21 11:58:38, Dan Elphick wrote:
> On 2017/03/21 11:36:39, Sami wrote:
> >
>
https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
> > File cc/scheduler/scheduler_state_machine.cc (right):
> > 
> >
>
https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
> > cc/scheduler/scheduler_state_machine.cc:436: if (needs_begin_main_frame_ ||
> > send_begin_main_frame_funnel_)
> > On 2017/03/21 11:35:21, Sami wrote:
> > > On 2017/03/21 11:23:51, Dan Elphick wrote:
> > > > I made this change here because needs_begin_main_frame gets reset after
> it's
> > > > handled. So essentially we'd get idle work after every main frame.
> > > > 
> > > > I've tested the two cases we had though and they still work after this
> > change.
> > > 
> > > I wonder if we should look at begin_main_frame_state_ instead? What states
> > does
> > > it cycle through? Basically we want to avoid doing any extra idle work if
> > > there's a BeginMainFrame in any part of the pipeline I think (perhaps that
> > means
> > > we should look at pending trees too)
> > 
> > Just to clarify: the problem with looking at the funnel is that it's really
a
> > rate limiting mechanism for BeginMainFrames, and if we decide to allow more
> > BeginMainFrames in the future it wouldn't be sufficient here anymore.
> 
> We already have:
> 
> if (begin_main_frame_state_ != BEGIN_MAIN_FRAME_STATE_IDLE)
>   return false;
> 
> The reason I was looking at this was SchedulerStateMachineTest.TestFullCycle
> (along with lots of other tests failed).
> 
> It fails at
>
https://cs.chromium.org/chromium/src/cc/scheduler/scheduler_state_machine_uni...
> because the action is DO_IDLE_WORK rather than NONE. begin_main_frame_state
has
> already returned to idle at this point.
> 
> I could add another bool to the statemachine that indicates that a main frame
> has been requested and which is cleared in OnBeginImplFrame - although then
its
> state exactly mirrors the funnel.
> 
> Perhaps we could rename the funnel variable to be more descriptive. A
> counter-argument to the rate-limiting argument would be that if it was used in
> that way the type would have to change anyway which would make it quickly
> apparent that it was being used to limit both IdleWork and MainFrames.

Yeah maybe it should be renamed -- did_send_begin_main_frame_for_current_frame?

[Dan Elphick, 2017-03-21 12:16:52]

On 2017/03/21 12:09:59, Sami wrote:
> On 2017/03/21 11:58:38, Dan Elphick wrote:
> > Perhaps we could rename the funnel variable to be more descriptive. A
> > counter-argument to the rate-limiting argument would be that if it was used
in
> > that way the type would have to change anyway which would make it quickly
> > apparent that it was being used to limit both IdleWork and MainFrames.
> 
> Yeah maybe it should be renamed --
did_send_begin_main_frame_for_current_frame?

I'll rename the idle work funnel as well as that won't ever be a rate-limiter.

[Dan Elphick, 2017-03-21 15:56:25]

I've started going through the test failures due to this and have resolved all
of the state machine ones. Mostly what happens is that it expects to be in
ACTION_NONE but instead gets ACTION_DO_IDLE_WORK. The fix is to just add an
extra check and update before the NONE check.

However, this affects a lot of tests, so we need to make sure these are all
valid times.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:436: if (needs_begin_main_frame_)
On 2017/03/20 16:58:49, Sami wrote:
> // BeginMainFrame already triggers idle work, so we don't need to schedule any
> extra idle work here.

Done.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:439: // We can not perform commits if we
are not visible.
On 2017/03/20 16:58:49, Sami wrote:
> // Only do FooIdle work when we're visible.

Done.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:444: // so should also stop
BeginMainFrames.
On 2017/03/20 16:58:49, Sami wrote:
> ...stop FooIdle work

Done.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:449: if (defer_commits_)
On 2017/03/20 16:58:49, Sami wrote:
> This one we can drop.

Done.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:468: if (begin_main_frame_state_ !=
BEGIN_MAIN_FRAME_STATE_IDLE)
On 2017/03/20 16:58:49, Sami wrote:
> // If a BeginMainFrame is in progress, don't do any extra idle work.

Done.

https://codereview.chromium.org/2753843003/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:472: if
(!settings_.main_frame_before_activation_enabled && has_pending_tree_)
On 2017/03/20 16:58:49, Sami wrote:
> Drop this.

Done.

https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:436: if (needs_begin_main_frame_ ||
send_begin_main_frame_funnel_)
On 2017/03/21 11:36:39, Sami wrote:
> On 2017/03/21 11:35:21, Sami wrote:
> > On 2017/03/21 11:23:51, Dan Elphick wrote:
> > > I made this change here because needs_begin_main_frame gets reset after
it's
> > > handled. So essentially we'd get idle work after every main frame.
> > > 
> > > I've tested the two cases we had though and they still work after this
> change.
> > 
> > I wonder if we should look at begin_main_frame_state_ instead? What states
> does
> > it cycle through? Basically we want to avoid doing any extra idle work if
> > there's a BeginMainFrame in any part of the pipeline I think (perhaps that
> means
> > we should look at pending trees too)
> 
> Just to clarify: the problem with looking at the funnel is that it's really a
> rate limiting mechanism for BeginMainFrames, and if we decide to allow more
> BeginMainFrames in the future it wouldn't be sufficient here anymore.

I've renamed the funnel as discussed in comments elsewhere.

[Sami, 2017-03-21 17:12:25]

On 2017/03/21 15:56:25, Dan Elphick wrote:
> I've started going through the test failures due to this and have resolved all
> of the state machine ones. Mostly what happens is that it expects to be in
> ACTION_NONE but instead gets ACTION_DO_IDLE_WORK. The fix is to just add an
> extra check and update before the NONE check.
> 
> However, this affects a lot of tests, so we need to make sure these are all
> valid times.

Sounds good. Let me know if some cases seem unclear. The general idea is that
BeginMainFrame and this idle work should never happen "together".

[Dan Elphick, 2017-03-23 17:30:49]

On 2017/03/21 17:12:25, Sami wrote:
> On 2017/03/21 15:56:25, Dan Elphick wrote:
> > I've started going through the test failures due to this and have resolved
all
> > of the state machine ones. Mostly what happens is that it expects to be in
> > ACTION_NONE but instead gets ACTION_DO_IDLE_WORK. The fix is to just add an
> > extra check and update before the NONE check.
> > 
> > However, this affects a lot of tests, so we need to make sure these are all
> > valid times.
> 
> Sounds good. Let me know if some cases seem unclear. The general idea is that
> BeginMainFrame and this idle work should never happen "together".

PTAL. 

The scheduler now wires up the DoShortIdleWork all the way through to
RendererSchedulerImpl, which explicitly schedules short idle work rather the
long idle work mechanism used by MainFrameNotExpectedSoon.

Upshot is that it now does 50% more idle work on a page that contains an
animated GIF set to 50fps, while keeping the page running at that frame rate.

All existing tests now pass, although we need some explicit tests for the new
behaviour.

[Sami, 2017-03-24 15:24:11]

Looks great!

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler.h:51: virtual void ScheduledActionDoShortIdleWork(const
BeginFrameArgs& args) = 0;
I think my entry to the great bikeshedding race of 2017 is still
ScheduledActionBeginMainFrameNotExpectedUntil(const base::TimeTicks&). The
reason is that the cc scheduler doesn't really know or care about what the main
thread does with this notification -- it doesn't necessarily need to be idle
work.

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:467:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_DO_SHORT_IDLE_WORK);
I was thinking we might wanna avoid idling here, but looks like the next
BeginMainFrame comes in after the vsync so this should be fine.

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:534:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_DO_SHORT_IDLE_WORK);
Is this right? We just did a BeginMainFrame.

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2017: bool
expectIdleWorkBeforeImplFrame) {
nit: hacker_style

Also expect_idle_work_before_impl_frame_deadline would be a little more
accurate.

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2609:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_DO_SHORT_IDLE_WORK);
Here a begin main frame seems to have recently completeted
(NotifyBeginMainFrameStarted + commit) -- do we wanna idle?

https://codereview.chromium.org/2753843003/diff/120001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2753843003/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:358: // TODO(delphick): do something here?
Unless we're testing something at this level this can just be blank.

[Dan Elphick, 2017-03-29 16:02:44]

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler.h:51: virtual void ScheduledActionDoShortIdleWork(const
BeginFrameArgs& args) = 0;
On 2017/03/24 15:24:10, Sami wrote:
> I think my entry to the great bikeshedding race of 2017 is still
> ScheduledActionBeginMainFrameNotExpectedUntil(const base::TimeTicks&). The
> reason is that the cc scheduler doesn't really know or care about what the
main
> thread does with this notification -- it doesn't necessarily need to be idle
> work.

Done.

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:467:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_DO_SHORT_IDLE_WORK);
On 2017/03/24 15:24:10, Sami wrote:
> I was thinking we might wanna avoid idling here, but looks like the next
> BeginMainFrame comes in after the vsync so this should be fine.

Acknowledged.

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:534:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_DO_SHORT_IDLE_WORK);
On 2017/03/24 15:24:11, Sami wrote:
> Is this right? We just did a BeginMainFrame.

It's right in that the tests don't pass without it.

The reason it happens is because the IssueNextBeginImplFrame on line 523, which
clears the did_do_idle_work_ and did_send_begin_main_frame_for_current_frame_
flags. The Commit action then sets the begin_main_frame_state_ flag to IDLE
which is sufficient to allow short idle work to go.

What condition do you think should prevent this?

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2017: bool
expectIdleWorkBeforeImplFrame) {
On 2017/03/24 15:24:11, Sami wrote:
> nit: hacker_style
> 
> Also expect_idle_work_before_impl_frame_deadline would be a little more
> accurate.

Done.

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2609:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_DO_SHORT_IDLE_WORK);
On 2017/03/24 15:24:10, Sami wrote:
> Here a begin main frame seems to have recently completeted
> (NotifyBeginMainFrameStarted + commit) -- do we wanna idle?

Seems like this is the same as the last one. What would clear that condition?

https://codereview.chromium.org/2753843003/diff/120001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2753843003/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:358: // TODO(delphick): do something here?
On 2017/03/24 15:24:11, Sami wrote:
> Unless we're testing something at this level this can just be blank.

Yeah it was just a reminder to check if we needed a test here since
BeginMainFrameNotExpectedSoon has hooks here. For now I'll remove it.

[Sami, 2017-03-30 15:30:37]

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:218: void Scheduler::ScheduleShortIdleWork(const
BeginFrameArgs& args) {
Let's rename this to BeginMainFrameNotExpectedUntil(base::TimeTicks)

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.h:126: ACTION_DO_SHORT_IDLE_WORK,
ACTION_NOTIFY_BEGIN_MAIN_FRAME_NOT_SENT

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.h:365: bool did_do_idle_work_;
Let's also add a flag saying whether we did a commit in this frame and avoid
idling in that case.

https://codereview.chromium.org/2753843003/diff/180001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2753843003/diff/180001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:360: void
BeginMainFrameNotExpectedUntil(base::TimeTicks frame_time) override {}
s/frame_time/time/

[Dan Elphick, 2017-04-06 16:10:17]

Thanks!

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:534:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_DO_SHORT_IDLE_WORK);
On 2017/03/29 16:02:44, Dan Elphick wrote:
> On 2017/03/24 15:24:11, Sami wrote:
> > Is this right? We just did a BeginMainFrame.
> 
> It's right in that the tests don't pass without it.
> 
> The reason it happens is because the IssueNextBeginImplFrame on line 523,
which
> clears the did_do_idle_work_ and did_send_begin_main_frame_for_current_frame_
> flags. The Commit action then sets the begin_main_frame_state_ flag to IDLE
> which is sufficient to allow short idle work to go.
> 
> What condition do you think should prevent this?

This is now removed after tweaking the state machine.

https://codereview.chromium.org/2753843003/diff/120001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2609:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_DO_SHORT_IDLE_WORK);
On 2017/03/29 16:02:44, Dan Elphick wrote:
> On 2017/03/24 15:24:10, Sami wrote:
> > Here a begin main frame seems to have recently completeted
> > (NotifyBeginMainFrameStarted + commit) -- do we wanna idle?
> 
> Seems like this is the same as the last one. What would clear that condition?

Done.

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:218: void Scheduler::ScheduleShortIdleWork(const
BeginFrameArgs& args) {
On 2017/03/30 15:30:36, Sami wrote:
> Let's rename this to BeginMainFrameNotExpectedUntil(base::TimeTicks)

Done.

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.h:126: ACTION_DO_SHORT_IDLE_WORK,
On 2017/03/30 15:30:36, Sami wrote:
> ACTION_NOTIFY_BEGIN_MAIN_FRAME_NOT_SENT

Done.

https://codereview.chromium.org/2753843003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.h:365: bool did_do_idle_work_;
On 2017/03/30 15:30:37, Sami wrote:
> Let's also add a flag saying whether we did a commit in this frame and avoid
> idling in that case.

Done.

https://codereview.chromium.org/2753843003/diff/180001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/2753843003/diff/180001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:360: void
BeginMainFrameNotExpectedUntil(base::TimeTicks frame_time) override {}
On 2017/03/30 15:30:37, Sami wrote:
> s/frame_time/time/

Done.

[Sami, 2017-04-06 17:38:41]

Description was changed from

==========
Create a new idle work action triggered when a main frame is not required
but otherwise could be scheduled.

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

to

==========
Create a new idle work action triggered when a main frame is not required
but otherwise could be scheduled.

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

[Sami, 2017-04-06 17:38:41]

skyostil@chromium.org changed reviewers:
+ brianderson@chromium.org

[Sami, 2017-04-06 17:39:10]

Thanks! I think this looks good now. Brian, would you mind verifying the logic
here?

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:219: // TODO(delphick): Do we need to log this?
Let's add:

TRACE_EVENT0("cc", "Scheduler::BeginMainFrameNotExpectedUntil");

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler.h:52: base::TimeTicks frameTime) = 0;
nit: frame_time (or just time)

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:440: // BeginMainFrame already triggers
idle work, so we don't need to schedule
I'm wondering if we should rephrase all these comments so they don't mention
idle work, since it's not supposed to be a thing at this level. Maybe I'm just
being too dogmatic about these layers :P

https://codereview.chromium.org/2753843003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2753843003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:2081:
base::TimeTicks frame_time = now + tenMillis;
nit: ten_millis

https://codereview.chromium.org/2753843003/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h
(right):

https://codereview.chromium.org/2753843003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:91:
virtual void BeginMainFrameNotExpectedUntil(const base::TimeTicks& time) = 0;
nit: pass by value

[Dan Elphick, 2017-04-07 09:05:02]

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:219: // TODO(delphick): Do we need to log this?
On 2017/04/06 17:39:09, Sami wrote:
> Let's add:
> 
> TRACE_EVENT0("cc", "Scheduler::BeginMainFrameNotExpectedUntil");

Done.

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler.h:52: base::TimeTicks frameTime) = 0;
On 2017/04/06 17:39:09, Sami wrote:
> nit: frame_time (or just time)

Thanks thought I'd got them all.
Done.

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:440: // BeginMainFrame already triggers
idle work, so we don't need to schedule
On 2017/04/06 17:39:09, Sami wrote:
> I'm wondering if we should rephrase all these comments so they don't mention
> idle work, since it's not supposed to be a thing at this level. Maybe I'm just
> being too dogmatic about these layers :P

I think you're right. I've phrased most of the comments in terms of letting the
main thread know that it can do something else without specifying idle work.

https://codereview.chromium.org/2753843003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2753843003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:2081:
base::TimeTicks frame_time = now + tenMillis;
On 2017/04/06 17:39:09, Sami wrote:
> nit: ten_millis

Done.

[Dan Elphick, 2017-04-07 13:52:20]

Description was changed from

==========
Create a new idle work action triggered when a main frame is not required
but otherwise could be scheduled.

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

to

==========
Create a new action triggered when a BeginMainFrame is not expected before
vsync.

The renderer scheduler can then use this information to schedule short idle work
as would have happened after a BeginMainFrame. This allows idle work to be
scheduled for instance in cases where a compositor animation is occurring which
prevents BeginMainFrameNotExpectedSoon from being sent.

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

[brianderson, 2017-04-07 22:03:17]

Thanks for improving short idle periods!

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:220:
compositor_timing_history_->BeginImplFrameNotExpectedSoon();
Do not call compositor_timing_history_->BeingImplFrameNoExpectedSoon here since
that's used a signal that we've gone idle; but we haven't necessarily gone idle
at this point.

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:439: bool
SchedulerStateMachine::ShouldNotifyBeginMainFrameNotSent() const {
Is it okay that we'll be sending this at the start of every frame while the
compositor is active?

Should blink explicitly request this signal?

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:445: if (needs_begin_main_frame_ ||
did_send_begin_main_frame_for_current_frame_)
I think the did_send_begin_main_frame_for_current_frame_ check can be removed
since it is redundant with the combination of checks on did_commit_during_frame_
and begin_main_frame_state_.

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:471: if (begin_main_frame_state_ !=
BEGIN_MAIN_FRAME_STATE_IDLE)
Group this with the needs_begin_main_frame_ check on line 445.

[Dan Elphick, 2017-04-10 15:53:22]

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:220:
compositor_timing_history_->BeginImplFrameNotExpectedSoon();
On 2017/04/07 22:03:16, brianderson wrote:
> Do not call compositor_timing_history_->BeingImplFrameNoExpectedSoon here
since
> that's used a signal that we've gone idle; but we haven't necessarily gone
idle
> at this point.

Done.

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:439: bool
SchedulerStateMachine::ShouldNotifyBeginMainFrameNotSent() const {
On 2017/04/07 22:03:16, brianderson wrote:
> Is it okay that we'll be sending this at the start of every frame while the
> compositor is active?
> 
> Should blink explicitly request this signal?

We're aware this might cause some problems, but we'd like to assess the impact
first and then add the extra complexity later if/when we determine this is
having a negative impact.

Assuming it is a problem, I think we can modify the renderer scheduler to signal
the cc scheduler that it wants this signal whenever the idle queue changes from
empty to non-empty and vice versa.

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:445: if (needs_begin_main_frame_ ||
did_send_begin_main_frame_for_current_frame_)
It is actually necessary as it's the flag is initialized to true which stops it
sending this notification straight away (which caused every single state machine
test to fail).

However I can remove this check and fix that by just tweaking the initialization
of did_notify_begin_main_frame_not_sent_ to true.

https://codereview.chromium.org/2753843003/diff/220001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:471: if (begin_main_frame_state_ !=
BEGIN_MAIN_FRAME_STATE_IDLE)
On 2017/04/07 22:03:16, brianderson wrote:
> Group this with the needs_begin_main_frame_ check on line 445.

Done.

[brianderson, 2017-04-17 21:40:39]

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

[brianderson, 2017-04-17 21:40:40]

+Sunny for owner's review while I'm out.

[Dan Elphick, 2017-04-19 09:40:57]

On 2017/04/17 21:40:40, brianderson wrote:
> +Sunny for owner's review while I'm out.

Hi Sunny, could you take a look at this?

[Sami, 2017-04-20 11:15:10]

Thanks, lgtm with a few nits. Sunny, do you want to do a quick sanity check
before we land this?

https://codereview.chromium.org/2753843003/diff/240001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2753843003/diff/240001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2050: bool
expect_idle_work_before_impl_frame_deadline) {
nit: expect_begin_main_frame_not_sent_before_impl_frame_deadline

https://codereview.chromium.org/2753843003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2753843003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:2069:
base::TimeDelta ten_millis(base::TimeDelta::FromInternalValue(10000));
nit: prefer FromMilliseconds(...) to avoid depending on the internal
representation.

[Dan Elphick, 2017-04-20 12:39:46]

Thanks!

https://codereview.chromium.org/2753843003/diff/240001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2753843003/diff/240001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2050: bool
expect_idle_work_before_impl_frame_deadline) {
On 2017/04/20 11:15:10, Sami wrote:
> nit: expect_begin_main_frame_not_sent_before_impl_frame_deadline

Done.

https://codereview.chromium.org/2753843003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2753843003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:2069:
base::TimeDelta ten_millis(base::TimeDelta::FromInternalValue(10000));
On 2017/04/20 11:15:10, Sami wrote:
> nit: prefer FromMilliseconds(...) to avoid depending on the internal
> representation.

Done.

[chromium-reviews, 2017-04-20 15:57:42]

Yes I'll take a look at this today.

On Thu, Apr 20, 2017, 5:39 AM <delphick@chromium.org> wrote:

> Thanks!
>
>
>
>
>
https://codereview.chromium.org/2753843003/diff/240001/cc/scheduler/scheduler...
> File cc/scheduler/scheduler_state_machine_unittest.cc (right):
>
>
>
https://codereview.chromium.org/2753843003/diff/240001/cc/scheduler/scheduler...
> cc/scheduler/scheduler_state_machine_unittest.cc:2050: bool
> expect_idle_work_before_impl_frame_deadline) {
> On 2017/04/20 11:15:10, Sami wrote:
> > nit: expect_begin_main_frame_not_sent_before_impl_frame_deadline
>
> Done.
>
>
>
>
https://codereview.chromium.org/2753843003/diff/240001/third_party/WebKit/Sou...
> File
>
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
> (right):
>
>
>
https://codereview.chromium.org/2753843003/diff/240001/third_party/WebKit/Sou...
>
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:2069:
> base::TimeDelta ten_millis(base::TimeDelta::FromInternalValue(10000));
> On 2017/04/20 11:15:10, Sami wrote:
> > nit: prefer FromMilliseconds(...) to avoid depending on the internal
> > representation.
>
> Done.
>
> https://codereview.chromium.org/2753843003/
>

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

[sunnyps, 2017-04-20 18:36:48]

lgtm % nits

https://codereview.chromium.org/2753843003/diff/260001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/260001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:212: TRACE_EVENT0("cc",
"Scheduler::BeginMainFrameNotExpectedUntil");
nit: might want to log time in the trace event

https://codereview.chromium.org/2753843003/diff/260001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/2753843003/diff/260001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.h:362: bool
did_send_begin_main_frame_for_current_frame_;
Thanks for changing this. I was planning to do the same for the other "funnels".

https://codereview.chromium.org/2753843003/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h
(right):

https://codereview.chromium.org/2753843003/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:90:
// BeginFrameNotExpectedSoon and is also called when the compositor is busy
nit: compositor may be busy

[sunnyps, 2017-04-20 19:04:46]

Forgot this: Please format your CL description to 80 cols. Thanks!

[Dan Elphick, 2017-04-26 16:06:31]

Description was changed from

==========
Create a new action triggered when a BeginMainFrame is not expected before
vsync.

The renderer scheduler can then use this information to schedule short idle work
as would have happened after a BeginMainFrame. This allows idle work to be
scheduled for instance in cases where a compositor animation is occurring which
prevents BeginMainFrameNotExpectedSoon from being sent.

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

to

==========
Create new action sent when a BeginMainFrame not expected before vsync.

The renderer scheduler can then use this information to schedule short idle
work as would have happened after a BeginMainFrame. This allows idle work to be
scheduled for instance in cases where a compositor animation is occurring which
prevents BeginMainFrameNotExpectedSoon from being sent.

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

[Dan Elphick, 2017-04-26 16:31:44]

Thanks

https://codereview.chromium.org/2753843003/diff/260001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2753843003/diff/260001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:212: TRACE_EVENT0("cc",
"Scheduler::BeginMainFrameNotExpectedUntil");
On 2017/04/20 18:36:47, sunnyps wrote:
> nit: might want to log time in the trace event

Done.

https://codereview.chromium.org/2753843003/diff/260001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/2753843003/diff/260001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.h:362: bool
did_send_begin_main_frame_for_current_frame_;
On 2017/04/20 18:36:47, sunnyps wrote:
> Thanks for changing this. I was planning to do the same for the other
"funnels".

Acknowledged.

https://codereview.chromium.org/2753843003/diff/260001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h
(right):

https://codereview.chromium.org/2753843003/diff/260001/third_party/WebKit/pub...
third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:90:
// BeginFrameNotExpectedSoon and is also called when the compositor is busy
On 2017/04/20 18:36:48, sunnyps wrote:
> nit: compositor may be busy

Done.

[Dan Elphick, 2017-04-26 16:42:45]

Description was changed from

==========
Create new action sent when a BeginMainFrame not expected before vsync.

The renderer scheduler can then use this information to schedule short idle
work as would have happened after a BeginMainFrame. This allows idle work to be
scheduled for instance in cases where a compositor animation is occurring which
prevents BeginMainFrameNotExpectedSoon from being sent.

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

to

==========
Create new action sent when a BeginMainFrame not expected before vsync.

The renderer scheduler can then use this information to schedule short idle
work as would have happened after a BeginMainFrame. This allows idle work to be
scheduled for instance in cases where a compositor animation is occurring which
prevents BeginMainFrameNotExpectedSoon from being sent.

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

[Dan Elphick, 2017-04-26 16:42:45]

delphick@chromium.org changed reviewers:
+ enne@chromium.org, vollick@chromium.org

[Dan Elphick, 2017-04-26 16:44:28]

Added vollick@ for third_party/WebKit/Source/platform/scheduler OWNERS and enne@
for ui/compositor/compositor.h OWNERS.

[Dan Elphick, 2017-04-27 14:54:03]

delphick@chromium.org changed reviewers:
- brianderson@chromium.org

[Dan Elphick, 2017-05-03 11:10:45]

On 2017/04/26 16:44:28, Dan Elphick wrote:
> Added vollick@ for third_party/WebKit/Source/platform/scheduler OWNERS and
enne@
> for ui/compositor/compositor.h OWNERS.

Sorry I got the directories wrong that should be Adrienne for
content/renderer/gpu and Ian for third_party/WebKit/Source/platform/testing and
ui/compositor.

Those changes should all be very small.

[Dan Elphick, 2017-05-03 11:27:44]

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

[commit-bot: I haz the power, 2017-05-03 11:27:53]

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-03 11:39:54]

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

[commit-bot: I haz the power, 2017-05-03 11:39:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Dan Elphick, 2017-05-03 13:57:55]

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

[commit-bot: I haz the power, 2017-05-03 13:58:10]

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-03 14:21:45]

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

[commit-bot: I haz the power, 2017-05-03 14:21:46]

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

[Dan Elphick, 2017-05-03 15:51:01]

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

[commit-bot: I haz the power, 2017-05-03 15:51:29]

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-03 17:20:25]

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

[commit-bot: I haz the power, 2017-05-03 17:20:27]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-05-03 18:53:20]

content/renderer/gpu lgtm

[Ian Vollick, 2017-05-04 15:57:03]

On 2017/05/03 18:53:20, enne wrote:
> content/renderer/gpu lgtm

ui/compositor/ lgtm.

[Dan Elphick, 2017-05-04 16:03:50]

delphick@chromium.org changed reviewers:
+ aelias@chromium.org

[Dan Elphick, 2017-05-04 16:04:35]

Adding aelias@ for content/browser/renderer_host/compositor_impl_android.h
OWNERS

[aelias_OOO_until_Jul13, 2017-05-04 18:30:13]

compositor_impl_android.h lgtm

[Dan Elphick, 2017-05-05 07:55:11]

The CQ bit was checked by delphick@chromium.org

[Dan Elphick, 2017-05-05 07:55:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2753843003/#ps340001
(title: "Add BeginMainFrameNotExpectedUntil to content::CompositorImpl for
Android.")

[commit-bot: I haz the power, 2017-05-05 07:55:24]

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 10:20:55]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1493970911791950,
"parent_rev": "7a70ae843511b3e6d4f8a3924904fd3f7d1e0cba", "commit_rev":
"9db74aa87f97c709baf5d38b82fb987713a40951"}

[commit-bot: I haz the power, 2017-05-05 10:21:05]

Description was changed from

==========
Create new action sent when a BeginMainFrame not expected before vsync.

The renderer scheduler can then use this information to schedule short idle
work as would have happened after a BeginMainFrame. This allows idle work to be
scheduled for instance in cases where a compositor animation is occurring which
prevents BeginMainFrameNotExpectedSoon from being sent.

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

to

==========
Create new action sent when a BeginMainFrame not expected before vsync.

The renderer scheduler can then use this information to schedule short idle
work as would have happened after a BeginMainFrame. This allows idle work to be
scheduled for instance in cases where a compositor animation is occurring which
prevents BeginMainFrameNotExpectedSoon from being sent.

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

Review-Url: https://codereview.chromium.org/2753843003
Cr-Commit-Position: refs/heads/master@{#469619}
Committed:
https://chromium.googlesource.com/chromium/src/+/9db74aa87f97c709baf5d38b82fb...
==========

[commit-bot: I haz the power, 2017-05-05 10:21:07]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/9db74aa87f97c709baf5d38b82fb...