Disable scheduler deadline task on battery power in Windows

Disable scheduler deadline task on battery power in Windows

On windows the timer resolution on battery power is quite low (15 ms).
The scheduler should take this into account while posting it's
deadline task - otherwise it risks giving way too much time to the
main thread which means frame rate is reduced by half in the worst
case.

BUG=407182
Committed: https://crrev.com/34093979365b781fa6b9472ed8e4cdbfcef9a683
Cr-Commit-Position: refs/heads/master@{#299984}

[sunnyps, 2014-09-09 01:21:46]

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

[sunnyps, 2014-09-09 01:21:46]

Can you take a look at this, Brian?

I'm sure you're the owner for most of the files I changed - I'm not sure about
LayerTreeSettings.

[danakj, 2014-09-09 14:59:55]

Where's the tests?

[brianderson, 2014-09-09 17:59:05]

As Dana already pointed out, you'll want to add unit tests to
scheduler_unittests.cc for this.

I was going to say you could probably piggy back off a synchronous_compositor
test, however it looks like we let the synchronous_compositor stuff slip through
without unit tests. We can figure out how to proceed in person.

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

https://codereview.chromium.org/554973002/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:527: // within this call.
this call -> within the BeginFrame call stack.

https://codereview.chromium.org/554973002/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:585: if (state_machine_.needs_redraw()) {
AdjustedBeginImplFrameDeadline is used in more than one place. Are you sure this
won't have side effects with the BeginRetroFrames?

https://codereview.chromium.org/554973002/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/554973002/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_settings.cc:21: high_latency_mode_on_battery(true) {
We need to make sure this is only the case on Windows, since it's the only
platform with the timer precision issue.

[sunnyps, 2014-09-12 19:34:32]

I've addressed the issues pointed out and added tests. Please take a look again.

[brianderson, 2014-09-12 22:05:28]

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:544: if (settings_.high_latency_mode_on_battery &&
on_battery_power_)
Please add a comment above this if.

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:584: OnBeginImplFrameDeadline();
After thinking about this some more, I am a bit concerned with what will happen
if every call to BeginImplFrame takes more than one BeginFrame period.

Once we move Windows over to the unified BeginFrame message, many BeginFrame
messages could pile up onto our message queue.

To avoid pile ups, we need to skip some BeginFrames, which we currently only do
if they are converted into BeginRetroFrames. Calling the deadline synchronously
here will prevent any BeginRetroFrames from being created though.

I wonder if we should just post an immediate task for the deadline instead to
give any pending BeginFrames on the message queue a chance to execute before the
deadline tasks is run (since the deadline task should get queued behind the
BeginFrames).

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:207: bool on_battery_power_;
Can you make sure this flag is included in the trace events?

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler_u...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:102: void SendBeginFrame() {
Can you call this AdvanceFrameSynchronousDeadline or something like that and
DCHECK that we do not have a deadline pending before returning.

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1918: class FakePowerMonitorSource : public
base::PowerMonitorSource {
+bajones to make sure this FakePowerMonitorSource makes sense.

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1935: TEST(SchedulerTest,
HighLatencyModeOnBattery) {
This test looks good, however please create a version of this test that has
BeginRetroFrames on the transition to battery power.

[sunnyps, 2014-09-15 21:52:48]

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

[sunnyps, 2014-09-15 21:52:49]

Addressed most of your comments, brianderson.

bajones, can you please review the FakePowerMonitorSource code in
scheduler_unittest.cc

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:584: OnBeginImplFrameDeadline();
On 2014/09/12 22:05:27, brianderson wrote:
> After thinking about this some more, I am a bit concerned with what will
happen
> if every call to BeginImplFrame takes more than one BeginFrame period.
> 
> Once we move Windows over to the unified BeginFrame message, many BeginFrame
> messages could pile up onto our message queue.
> 
> To avoid pile ups, we need to skip some BeginFrames, which we currently only
do
> if they are converted into BeginRetroFrames. Calling the deadline
synchronously
> here will prevent any BeginRetroFrames from being created though.
> 
> I wonder if we should just post an immediate task for the deadline instead to
> give any pending BeginFrames on the message queue a chance to execute before
the
> deadline tasks is run (since the deadline task should get queued behind the
> BeginFrames).
> 
> 

An immediate task will also take 15 ms before it executes, right? My
understanding is that immediate tasks happen on the next iteration of the
MessageLoop which should be tied to the system timer resolution.

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:207: bool on_battery_power_;
On 2014/09/12 22:05:27, brianderson wrote:
> Can you make sure this flag is included in the trace events?

Done.

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler_u...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:102: void SendBeginFrame() {
On 2014/09/12 22:05:27, brianderson wrote:
> Can you call this AdvanceFrameSynchronousDeadline or something like that and
> DCHECK that we do not have a deadline pending before returning.

Done.

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1918: class FakePowerMonitorSource : public
base::PowerMonitorSource {
On 2014/09/12 22:05:27, brianderson wrote:
> +bajones to make sure this FakePowerMonitorSource makes sense.

Done.

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1935: TEST(SchedulerTest,
HighLatencyModeOnBattery) {
On 2014/09/12 22:05:27, brianderson wrote:
> This test looks good, however please create a version of this test that has
> BeginRetroFrames on the transition to battery power.

I thought we decided to remove that test because retro frames are discarded
incorrectly right now.

[bajones, 2014-09-16 01:10:47]

On 2014/09/15 at 21:52:49, sunnyps wrote:
> bajones, can you please review the FakePowerMonitorSource code in
scheduler_unittest.cc

FakePowerMonitorSource LGTM.

[sunnyps, 2014-09-16 21:30:31]

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/554973002/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:584: OnBeginImplFrameDeadline();
On 2014/09/15 21:52:48, sunnyps wrote:
> On 2014/09/12 22:05:27, brianderson wrote:
> > After thinking about this some more, I am a bit concerned with what will
> happen
> > if every call to BeginImplFrame takes more than one BeginFrame period.
> > 
> > Once we move Windows over to the unified BeginFrame message, many BeginFrame
> > messages could pile up onto our message queue.
> > 
> > To avoid pile ups, we need to skip some BeginFrames, which we currently only
> do
> > if they are converted into BeginRetroFrames. Calling the deadline
> synchronously
> > here will prevent any BeginRetroFrames from being created though.
> > 
> > I wonder if we should just post an immediate task for the deadline instead
to
> > give any pending BeginFrames on the message queue a chance to execute before
> the
> > deadline tasks is run (since the deadline task should get queued behind the
> > BeginFrames).
> > 
> > 
> 
> An immediate task will also take 15 ms before it executes, right? My
> understanding is that immediate tasks happen on the next iteration of the
> MessageLoop which should be tied to the system timer resolution.

I've gone through some of the MessageLoop/MessagePump source code and I think
what you're saying is correct. We can post a deadline task with delay 0 and that
should run immediately (after the current run loop iteration).

I'm also wondering if the scheduler state machine should know about any of this.

[mithro-old, 2014-09-23 15:11:14]

Hi sunnyps,

Sami pointed me in the direction of this change. With the additions I have made
to the OrderedSimpleTaskRunner we can actually make a test case which simulates
the Windows behaviour on battery power (the 15ms resolution timer). It would be
really awesome if we could build a test which shows the issues with the current
behaviour as that way you can check there aren't any other similar issues hiding
here.

Basically, you want to turn off the AutoAdvanceNowToPendingTasks feature of the
OrderedSimpleTaskRunner (which is on by default) and then manually advance the
now_src by the required resolution.

Tim

[sunnyps, 2014-09-24 18:34:59]

On 2014/09/23 15:11:14, mithro wrote:
> Hi sunnyps,
> 
> Sami pointed me in the direction of this change. With the additions I have
made
> to the OrderedSimpleTaskRunner we can actually make a test case which
simulates
> the Windows behaviour on battery power (the 15ms resolution timer). It would
be
> really awesome if we could build a test which shows the issues with the
current
> behaviour as that way you can check there aren't any other similar issues
hiding
> here.
> 
> Basically, you want to turn off the AutoAdvanceNowToPendingTasks feature of
the
> OrderedSimpleTaskRunner (which is on by default) and then manually advance the
> now_src by the required resolution.
> 
> Tim

Attached: http://pastebin.com/BHjymi4V

[sunnyps, 2014-09-24 18:42:01]

On 2014/09/24 18:34:59, sunnyps wrote:
> On 2014/09/23 15:11:14, mithro wrote:
> > Hi sunnyps,
> > 
> > Sami pointed me in the direction of this change. With the additions I have
> made
> > to the OrderedSimpleTaskRunner we can actually make a test case which
> simulates
> > the Windows behaviour on battery power (the 15ms resolution timer). It would
> be
> > really awesome if we could build a test which shows the issues with the
> current
> > behaviour as that way you can check there aren't any other similar issues
> hiding
> > here.
> > 
> > Basically, you want to turn off the AutoAdvanceNowToPendingTasks feature of
> the
> > OrderedSimpleTaskRunner (which is on by default) and then manually advance
the
> > now_src by the required resolution.
> > 
> > Tim
> 
> Attached: http://pastebin.com/BHjymi4V

Oops, wrong CL.

[sunnyps, 2014-10-09 01:16:31]

On 2014/09/23 15:11:14, mithro wrote:
> Hi sunnyps,
> 
> Sami pointed me in the direction of this change. With the additions I have
made
> to the OrderedSimpleTaskRunner we can actually make a test case which
simulates
> the Windows behaviour on battery power (the 15ms resolution timer). It would
be
> really awesome if we could build a test which shows the issues with the
current
> behaviour as that way you can check there aren't any other similar issues
hiding
> here.
> 
> Basically, you want to turn off the AutoAdvanceNowToPendingTasks feature of
the
> OrderedSimpleTaskRunner (which is on by default) and then manually advance the
> now_src by the required resolution.
> 
> Tim

Done.

[sunnyps, 2014-10-09 01:17:40]

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

[sunnyps, 2014-10-09 01:17:41]

danakj, can you please review the changes in compositor.cc and overall too if
you're interested?

[danakj, 2014-10-09 17:04:01]

LGTM

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

https://codereview.chromium.org/554973002/diff/120001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:78: : frame_source_(),
initialize your new bool to false

https://codereview.chromium.org/554973002/diff/120001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:552: } else if
(settings_.prioritize_impl_latency_on_battery &&
just if (on_battery_power_) should be enough?

[sunnyps, 2014-10-09 21:03:38]

I've fixed two issues with the latest patch set:

1. Improved the design of power observing in the scheduler. We want to rely on
as few global variables as we can - the singleton PowerMonitor fell into this
category. I've changed the design so that the SchedulerClient provides a
PowerMonitor to the Scheduler. The Scheduler always tries to observe power
events using the PowerMonitor if provided by the SchedulerClient. In my opinion
this simplifies the testing code and also is a cleaner design. 

2. I've moved the condition that causes the early deadline into the existing
ShouldTriggerBeginImplFrameDeadlineEarly function in SchedulerStateMachine. This
takes care of some corner cases (like having a lot of pending swaps, no output
surface, etc.) and also only triggers the deadline early if the impl thread has
work to do. This necessitated changing the unit test so that this is explicitly
tested.

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

https://codereview.chromium.org/554973002/diff/120001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:78: : frame_source_(),
On 2014/10/09 17:04:01, danakj wrote:
> initialize your new bool to false

Done.

[sunnyps, 2014-10-09 21:03:38]

I've fixed two issues with the latest patch set:

1. Improved the design of power observing in the scheduler. We want to rely on
as few global variables as we can - the singleton PowerMonitor fell into this
category. I've changed the design so that the SchedulerClient provides a
PowerMonitor to the Scheduler. The Scheduler always tries to observe power
events using the PowerMonitor if provided by the SchedulerClient. In my opinion
this simplifies the testing code and also is a cleaner design. 

2. I've moved the condition that causes the early deadline into the existing
ShouldTriggerBeginImplFrameDeadlineEarly function in SchedulerStateMachine. This
takes care of some corner cases (like having a lot of pending swaps, no output
surface, etc.) and also only triggers the deadline early if the impl thread has
work to do. This necessitated changing the unit test so that this is explicitly
tested.

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

https://codereview.chromium.org/554973002/diff/120001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:78: : frame_source_(),
On 2014/10/09 17:04:01, danakj wrote:
> initialize your new bool to false

Done.

[sunnyps, 2014-10-09 21:07:42]

On 2014/10/09 17:04:01, danakj wrote:
> LGTM
> 
>
https://codereview.chromium.org/554973002/diff/120001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/554973002/diff/120001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:78: : frame_source_(),
> initialize your new bool to false
> 
>
https://codereview.chromium.org/554973002/diff/120001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:552: } else if
> (settings_.prioritize_impl_latency_on_battery &&
> just if (on_battery_power_) should be enough?

I've changed the design so that the scheduler *always* gets power state events
so it is necessary to check the settings flag too - this check is now in
SchedulerStateMachine. I think this communicates the intent better.

[danakj, 2014-10-09 21:18:03]

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:140: power_monitor != NULL);
!= NULL is redundant here and below

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

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:886: if
(settings_.prioritize_impl_latency_on_battery && on_battery_power_)
i'd say leave on_battery_power_ always false if the setting is off, ie don't set
up the listener when the setting if off.

otherwise you're adding a listener and setting states, but not to use them.

[danakj, 2014-10-09 21:18:18]

Going through the client sounds like a good idea for tests.

[sunnyps, 2014-10-09 22:04:28]

On 2014/10/09 21:18:03, danakj wrote:
>
https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:140: power_monitor != NULL);
> != NULL is redundant here and below
> 
>
https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler_...
> File cc/scheduler/scheduler_state_machine.cc (right):
> 
>
https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler_...
> cc/scheduler/scheduler_state_machine.cc:886: if
> (settings_.prioritize_impl_latency_on_battery && on_battery_power_)
> i'd say leave on_battery_power_ always false if the setting is off, ie don't
set
> up the listener when the setting if off.
> 
> otherwise you're adding a listener and setting states, but not to use them.

I believe that latest patch addresses all the issues you pointed out. PTAL.

[sunnyps, 2014-10-09 22:04:39]

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:140: power_monitor != NULL);
On 2014/10/09 21:18:02, danakj wrote:
> != NULL is redundant here and below

I don't think it is. The tests which deal the scheduler exclusively setup a fake
scheduler client which returns a valid power monitor however other tests which
rely on the scheduler indirectly (like the layer tree host impl tests) have a
NULL power monitor (since base::PowerMonitor()::Get() returns NULL).

Now that I've changed the code so that we only register with the power monitor
when the settings flag tells us to this redundancy should be gone.

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

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:886: if
(settings_.prioritize_impl_latency_on_battery && on_battery_power_)
On 2014/10/09 21:18:02, danakj wrote:
> i'd say leave on_battery_power_ always false if the setting is off, ie don't
set
> up the listener when the setting if off.
> 
> otherwise you're adding a listener and setting states, but not to use them.

I agree. I'll rename on_battery_power_ to
impl_latency_takes_priority_on_battery_ so that it's clear that it takes both
the settings flag and power state into account. Also I believe that the name
"on_battery_power" might confuse someone if it doesn't actually reflect the
power state of the system and means something else.

[danakj, 2014-10-09 22:08:27]

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:140: power_monitor != NULL);
On 2014/10/09 22:04:39, sunnyps wrote:
> On 2014/10/09 21:18:02, danakj wrote:
> > != NULL is redundant here and below
> 
> I don't think it is. The tests which deal the scheduler exclusively setup a
fake
> scheduler client which returns a valid power monitor however other tests which
> rely on the scheduler indirectly (like the layer tree host impl tests) have a
> NULL power monitor (since base::PowerMonitor()::Get() returns NULL).
> 
> Now that I've changed the code so that we only register with the power monitor
> when the settings flag tells us to this redundancy should be gone.

Sorry, what I meant is if (a != NULL) is more simply written as if (a)

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

https://codereview.chromium.org/554973002/diff/250001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:886: if
(settings_.prioritize_impl_latency_on_battery && on_battery_power_)
On 2014/10/09 22:04:39, sunnyps wrote:
> On 2014/10/09 21:18:02, danakj wrote:
> > i'd say leave on_battery_power_ always false if the setting is off, ie don't
> set
> > up the listener when the setting if off.
> > 
> > otherwise you're adding a listener and setting states, but not to use them.
> 
> I agree. I'll rename on_battery_power_ to
> impl_latency_takes_priority_on_battery_ so that it's clear that it takes both
> the settings flag and power state into account. Also I believe that the name
> "on_battery_power" might confuse someone if it doesn't actually reflect the
> power state of the system and means something else. 

Eh, maybe it's fine to have it just be false when its not known. I don't find
the current name bad at all, but whatever you and the other scheduler folks
perfer :)

[sunnyps, 2014-10-09 22:30:21]

On 2014/10/09 22:08:27, danakj wrote:
 
> Sorry, what I meant is if (a != NULL) is more simply written as if (a)

Done.

[mithro-old, 2014-10-13 23:25:57]

mithro@mithis.com changed required reviewers:
+ brianderson@chromium.org

[mithro-old, 2014-10-13 23:25:58]

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:520: // The synchronous compositor needs to draw right
away.
Is this being removed in your CL, or was this removed in a rebase?

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:544: if
(settings_.using_synchronous_renderer_compositor) {
Is this being removed in your CL, or was this removed in a rebase?

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:2034: EXPECT_LT(before_deadline,
after_deadline);
There is a posted_begin_impl_frame_deadline function which should return the
value you are actually after here.

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:2051: EXPECT_EQ(before_deadline,
after_deadline);
See above.

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:2110:
EXPECT_FALSE(scheduler->BeginImplFrameDeadlinePending());
Can you advance a couple more frames here?

[sunnyps, 2014-10-13 23:59:43]

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:520: // The synchronous compositor needs to draw right
away.
On 2014/10/13 23:25:57, mithro wrote:
> Is this being removed in your CL, or was this removed in a rebase?

I'll revert this and other changes around here that are not related to power
monitoring and make those a part of the retro frame CL.

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:544: if
(settings_.using_synchronous_renderer_compositor) {
On 2014/10/13 23:25:57, mithro wrote:
> Is this being removed in your CL, or was this removed in a rebase?

Will revert this part.

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:518:
DCHECK(begin_impl_frame_deadline_task_.IsCancelled());
Will revert this too as it's not related to this change directly.

[brianderson, 2014-10-14 00:44:57]

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:38: virtual base::PowerMonitor* PowerMonitor() = 0;
Since this isn't expected to change, should it be passed in as part of the
constructor instead?

[sunnyps, 2014-10-15 01:11:11]

Addressed all comments. PTAL.

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/554973002/diff/370001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:38: virtual base::PowerMonitor* PowerMonitor() = 0;
On 2014/10/14 00:44:57, brianderson wrote:
> Since this isn't expected to change, should it be passed in as part of the
> constructor instead?

Done.

[danakj, 2014-10-15 15:42:26]

https://codereview.chromium.org/554973002/diff/530001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/554973002/diff/530001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:90: base::PowerMonitor::Get()));
I think brian meant that you'd pass it to Create() which would forward it to the
constructor. We don't usually do anything inside Create() except forward
parameters.

[sunnyps, 2014-10-15 19:38:07]

On 2014/10/15 15:42:26, danakj wrote:
> https://codereview.chromium.org/554973002/diff/530001/cc/scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
> 
>
https://codereview.chromium.org/554973002/diff/530001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.h:90: base::PowerMonitor::Get()));
> I think brian meant that you'd pass it to Create() which would forward it to
the
> constructor. We don't usually do anything inside Create() except forward
> parameters.

Done. I've also refactored similar issues in the scheduler test support classes
and removed some magic from TestScheduler::Create and moved that to
FakeSchedulerClient.

[brianderson, 2014-10-15 23:16:45]

lgtm.

Nit: I wonder if we should rename the setting to
"post_task_delay_precision_is_low_on_battery" or something like that? (But keep
the impl_latency_takes_priority_on_battery name in the SchedulerStateMachine.)
That way, the "why" is linked to the "what" in the code explicitly.

Thanks for cleaning up the other Create methods.

[sunnyps, 2014-10-15 23:45:11]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2014-10-15 23:45:12]

The CQ bit was unchecked by sunnyps@chromium.org

[sunnyps, 2014-10-15 23:52:17]

On 2014/10/15 23:16:45, brianderson wrote:
> lgtm.
> 
> Nit: I wonder if we should rename the setting to
> "post_task_delay_precision_is_low_on_battery" or something like that? (But
keep
> the impl_latency_takes_priority_on_battery name in the SchedulerStateMachine.)
> That way, the "why" is linked to the "what" in the code explicitly.
> 
> Thanks for cleaning up the other Create methods.

Done.

[sunnyps, 2014-10-16 19:01:05]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2014-10-16 19:02:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/554973002/570001

[sunnyps, 2014-10-16 19:08:05]

The CQ bit was unchecked by sunnyps@chromium.org

[sunnyps, 2014-10-16 20:25:26]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2014-10-16 20:26:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/554973002/590001

[commit-bot: I haz the power, 2014-10-16 22:00:07]

Committed patchset #15 (id:590001)

[commit-bot: I haz the power, 2014-10-16 22:01:29]

Patchset 15 (id:??) landed as
https://crrev.com/34093979365b781fa6b9472ed8e4cdbfcef9a683
Cr-Commit-Position: refs/heads/master@{#299984}