Move throttling of background timers into the renderer scheduler

Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402
Committed: https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}

Committed: https://crrev.com/de233a29aa0ef1b89d35a9045a864c9ad6fa12fe
Cr-Commit-Position: refs/heads/master@{#361978}

[alex clarke (OOO till 29th), 2015-11-13 11:19:21]

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

[commit-bot: I haz the power, 2015-11-13 11:19:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/1

[alex clarke (OOO till 29th), 2015-11-13 11:27:50]

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

[commit-bot: I haz the power, 2015-11-13 11:28:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/20001

[commit-bot: I haz the power, 2015-11-13 11:54:54]

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

[commit-bot: I haz the power, 2015-11-13 11:54:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alex clarke (OOO till 29th), 2015-11-13 12:00:11]

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

[commit-bot: I haz the power, 2015-11-13 12:00:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/40001

[alex clarke (OOO till 29th), 2015-11-13 12:28:16]

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

[commit-bot: I haz the power, 2015-11-13 12:28:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/60001

[alex clarke (OOO till 29th), 2015-11-13 12:28:48]

alexclarke@chromium.org changed reviewers:
+ jochen@chromium.org, skyostil@chromium.org

[commit-bot: I haz the power, 2015-11-13 12:57:27]

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

[commit-bot: I haz the power, 2015-11-13 12:57:28]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[alex clarke (OOO till 29th), 2015-11-13 13:31:14]

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

[commit-bot: I haz the power, 2015-11-13 13:31:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/80001

[commit-bot: I haz the power, 2015-11-13 14:04:08]

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

[commit-bot: I haz the power, 2015-11-13 14:04:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jochen (gone - plz use gerrit), 2015-11-16 11:51:44]

https://codereview.chromium.org/1441073006/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebPassOwnPtr.h (right):

https://codereview.chromium.org/1441073006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebPassOwnPtr.h:55: T* get() const
that breaks the ownership semantics of this class. can we do without this?

[Sami, 2015-11-17 10:12:21]

Thanks, looks like an excellent cleanup!

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:180: return
helper_.ControlTaskRunner();
nit: helper_.CheckOnValidThread();

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_view_scheduler_impl.cc (right):

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:23:
pump_throttled_tasks_closure_.Reset(base::Bind(
Doing this here is fine for now, but I wonder how we're going to do something
similar for out-of-view frames? I guess we can refactor this into a generic
throttling helper once we get there.

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:72:
SchedulePumpThrottledTasks();
Will this ever stop ticking if there's no work to do?

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:78: return
ceilf(now_in_seconds + 0.2) - now_in_seconds;
This probably deserves a comment and a named constant. Wasn't the previous
throttling interval 1s?

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:85:
base::TimeDelta::FromSecondsD(DelayToNextRunTimeInSeconds(
Is there a way to do this calculation only using TimeTicks/TimeDelta? I hope we
can limit the double time math to blink.

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_view_scheduler_impl.h (right):

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.h:57: bool background_;
nit: could you rename this to page_in_background_ for consistency?

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_view_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl_unittest.cc:114:
blink::WebPassOwnPtr<blink::WebFrameScheduler> frame(
Looks like this could just be a regular OwnPtr, which would avoid the need to
change WebPassOwnPTr.

https://codereview.chromium.org/1441073006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/TimerTest.cpp (right):

https://codereview.chromium.org/1441073006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:701: fprintf(stderr,
"m_startTime = %f\n", m_startTime);
Might want to remove this :)

[alex clarke (OOO till 29th), 2015-11-23 17:13:32]

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

[alex clarke (OOO till 29th), 2015-11-23 17:13:46]

PTAL

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:180: return
helper_.ControlTaskRunner();
On 2015/11/17 10:12:21, Sami wrote:
> nit: helper_.CheckOnValidThread();

Done.

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_view_scheduler_impl.cc (right):

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:23:
pump_throttled_tasks_closure_.Reset(base::Bind(
On 2015/11/17 10:12:21, Sami wrote:
> Doing this here is fine for now, but I wonder how we're going to do something
> similar for out-of-view frames? I guess we can refactor this into a generic
> throttling helper once we get there.

Yes we probably will add a helper.

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:72:
SchedulePumpThrottledTasks();
On 2015/11/17 10:12:21, Sami wrote:
> Will this ever stop ticking if there's no work to do?

I've changed it so it does.

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:78: return
ceilf(now_in_seconds + 0.2) - now_in_seconds;
On 2015/11/17 10:12:21, Sami wrote:
> This probably deserves a comment and a named constant. Wasn't the previous
> throttling interval 1s?

Yes and this also ticks once per second too.

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.cc:85:
base::TimeDelta::FromSecondsD(DelayToNextRunTimeInSeconds(
On 2015/11/17 10:12:21, Sami wrote:
> Is there a way to do this calculation only using TimeTicks/TimeDelta? I hope
we
> can limit the double time math to blink.

Done.

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_view_scheduler_impl.h (right):

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl.h:57: bool background_;
On 2015/11/17 10:12:21, Sami wrote:
> nit: could you rename this to page_in_background_ for consistency?

Done.

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_view_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1441073006/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_view_scheduler_impl_unittest.cc:114:
blink::WebPassOwnPtr<blink::WebFrameScheduler> frame(
On 2015/11/17 10:12:21, Sami wrote:
> Looks like this could just be a regular OwnPtr, which would avoid the need to
> change WebPassOwnPTr.

As discussed I ended up adding a new method to expose the Impl.

https://codereview.chromium.org/1441073006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/TimerTest.cpp (right):

https://codereview.chromium.org/1441073006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:701: fprintf(stderr,
"m_startTime = %f\n", m_startTime);
On 2015/11/17 10:12:21, Sami wrote:
> Might want to remove this :)

Done.

https://codereview.chromium.org/1441073006/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebPassOwnPtr.h (right):

https://codereview.chromium.org/1441073006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebPassOwnPtr.h:55: T* get() const
On 2015/11/16 11:51:44, jochen wrote:
> that breaks the ownership semantics of this class. can we do without this?

Done.

[commit-bot: I haz the power, 2015-11-23 17:13:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/160001

[alex clarke (OOO till 29th), 2015-11-23 17:38:21]

Description was changed from

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953
==========

to

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402
==========

[alex clarke (OOO till 29th), 2015-11-23 17:45:31]

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

[commit-bot: I haz the power, 2015-11-23 17:46:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/180001

[commit-bot: I haz the power, 2015-11-23 18:31:16]

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

[commit-bot: I haz the power, 2015-11-23 18:31:17]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alex clarke (OOO till 29th), 2015-11-24 10:41:02]

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

[commit-bot: I haz the power, 2015-11-24 10:42:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/200001

[commit-bot: I haz the power, 2015-11-24 12:04:26]

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

[commit-bot: I haz the power, 2015-11-24 12:04:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2015-11-24 12:48:48]

Great stuff!

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
File components/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue.h:146: virtual bool IsQueueEmpty() const;
Should we rename this to HasPendingImmediateTask to make the difference more
obvious?

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
File components/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
components/scheduler/base/time_domain.cc:157: void
TimeDomain::SetObserver(Observer* observer) {
Can we require this to be passed in at construction time and to outlive the time
domain? Otherwise this needs a lock :\

Guaranteeing the lifetime will be tricky with TimeDomain being refcounted,
though. I wonder if that can be changed?

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
components/scheduler/base/time_domain.h:52: virtual void
OnRegisterAsUpdatableTaskQueue() = 0;
How about OnHasNewWork, OnNewIncomingWork, OnNewImmediateWork or something like
that? I think the registration business is really an implementation detail.

Another idea:

OnTimeDomainHasImmediateWork()
OnTimeDomainHasDelayedWork()

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
File components/scheduler/base/virtual_time_domain.cc (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
components/scheduler/base/virtual_time_domain.cc:50:
WakeupReadyDelayedQueues(&lazy_now);
Isn't the DoWork going to do this? Why do it twice?

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.h:259:
scoped_refptr<VirtualTimeDomain> virtual_time_domain_;
Is this used for anything?

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.cc (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:81:
"ThrottlingHelper::OnRegisterAsUpdatableTaskQueue");
OnRequestWakeup

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:90: work_to_do_ = false;
Should this be inside the lock too?

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:105: const base::TimeDelta
one_second = base::TimeDelta::FromSeconds(1);
Could we use TimeTicks::SnappedToNextTick?

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:110: if
(pending_pump_throttled_tasks_ || !work_to_do_)
Assert that we're holding the lock?

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:117:
renderer_scheduler_->real_time_domain()->CreateLazyNow().Now()));
Looks like real_time_domain() is main thread only

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.h:32: void
ThrottleBackgroundFrame(WebFrameSchedulerImpl* frame_scheduler);
What if the unit that the ThrottlingHelper works with wasn't a
WebFrameSchedulerImpl but instead a TaskQueue? I think that way all the pumping
logic could live here and the frame scheduler would just need to add/remove the
timer queue here. WDYT?

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.h:52: // Protects members from
the contigious block below.
typo: contiguous

[alex clarke (OOO till 29th), 2015-11-25 12:29:37]

PTAL

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
File components/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
components/scheduler/base/task_queue.h:146: virtual bool IsQueueEmpty() const;
On 2015/11/24 12:48:47, Sami wrote:
> Should we rename this to HasPendingImmediateTask to make the difference more
> obvious?

Done.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
File components/scheduler/base/time_domain.cc (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
components/scheduler/base/time_domain.cc:157: void
TimeDomain::SetObserver(Observer* observer) {
On 2015/11/24 12:48:48, Sami wrote:
> Can we require this to be passed in at construction time and to outlive the
time
> domain? Otherwise this needs a lock :\
> 
> Guaranteeing the lifetime will be tricky with TimeDomain being refcounted,
> though. I wonder if that can be changed?

Agreed this is a bit subtle thanks to the ownership.

I think we are OK with this being passed in via the constructor.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
components/scheduler/base/time_domain.h:52: virtual void
OnRegisterAsUpdatableTaskQueue() = 0;
On 2015/11/24 12:48:48, Sami wrote:
> How about OnHasNewWork, OnNewIncomingWork, OnNewImmediateWork or something
like
> that? I think the registration business is really an implementation detail.
> 
> Another idea:
> 
> OnTimeDomainHasImmediateWork()
> OnTimeDomainHasDelayedWork()

Done.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
File components/scheduler/base/virtual_time_domain.cc (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/b...
components/scheduler/base/virtual_time_domain.cc:50:
WakeupReadyDelayedQueues(&lazy_now);
On 2015/11/24 12:48:48, Sami wrote:
> Isn't the DoWork going to do this? Why do it twice?

Seems it isn't needed.  It definitely was at one stage however.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.h:259:
scoped_refptr<VirtualTimeDomain> virtual_time_domain_;
On 2015/11/24 12:48:48, Sami wrote:
> Is this used for anything?

Done.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.cc (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:81:
"ThrottlingHelper::OnRegisterAsUpdatableTaskQueue");
On 2015/11/24 12:48:48, Sami wrote:
> OnRequestWakeup

Done.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:90: work_to_do_ = false;
On 2015/11/24 12:48:48, Sami wrote:
> Should this be inside the lock too?

I got rid of the locking.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:105: const base::TimeDelta
one_second = base::TimeDelta::FromSeconds(1);
On 2015/11/24 12:48:48, Sami wrote:
> Could we use TimeTicks::SnappedToNextTick?

It doesn't seem to do what I wanted.  Specifically the following fails because
it returned 0. 


EXPECT_EQ(base::TimeDelta::FromSecondsD(1.0),
            ThrottlingHelper::DelayToNextRunTimeInSeconds(
                base::TimeTicks() + base::TimeDelta::FromSecondsD(0.0)));

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:110: if
(pending_pump_throttled_tasks_ || !work_to_do_)
On 2015/11/24 12:48:48, Sami wrote:
> Assert that we're holding the lock?

Acknowledged.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:117:
renderer_scheduler_->real_time_domain()->CreateLazyNow().Now()));
On 2015/11/24 12:48:48, Sami wrote:
> Looks like real_time_domain() is main thread only

Acknowledged.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.h:52: // Protects members from
the contigious block below.
On 2015/11/24 12:48:48, Sami wrote:
> typo: contiguous

Acknowledged.

[alex clarke (OOO till 29th), 2015-11-25 12:30:12]

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.h:32: void
ThrottleBackgroundFrame(WebFrameSchedulerImpl* frame_scheduler);
On 2015/11/24 12:48:48, Sami wrote:
> What if the unit that the ThrottlingHelper works with wasn't a
> WebFrameSchedulerImpl but instead a TaskQueue? I think that way all the
pumping
> logic could live here and the frame scheduler would just need to add/remove
the
> timer queue here. WDYT?

Done.

[alex clarke (OOO till 29th), 2015-11-25 12:30:31]

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

[commit-bot: I haz the power, 2015-11-25 12:32:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/220001

[alex clarke (OOO till 29th), 2015-11-25 13:27:39]

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

[commit-bot: I haz the power, 2015-11-25 13:27:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/240001

[commit-bot: I haz the power, 2015-11-25 14:42:15]

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

[commit-bot: I haz the power, 2015-11-25 14:42:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2015-11-25 17:12:25]

Looks great. Just one question about the throttling helper's logic.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.cc (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:105: const base::TimeDelta
one_second = base::TimeDelta::FromSeconds(1);
On 2015/11/25 12:29:36, alexclarke1 wrote:
> On 2015/11/24 12:48:48, Sami wrote:
> > Could we use TimeTicks::SnappedToNextTick?
> 
> It doesn't seem to do what I wanted.  Specifically the following fails because
> it returned 0. 
> 
> 
> EXPECT_EQ(base::TimeDelta::FromSecondsD(1.0),
>             ThrottlingHelper::DelayToNextRunTimeInSeconds(
>                 base::TimeTicks() + base::TimeDelta::FromSecondsD(0.0)));

Ah, different boundary condition, got it.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.h:32: void
ThrottleBackgroundFrame(WebFrameSchedulerImpl* frame_scheduler);
On 2015/11/25 12:30:12, alexclarke1 wrote:
> On 2015/11/24 12:48:48, Sami wrote:
> > What if the unit that the ThrottlingHelper works with wasn't a
> > WebFrameSchedulerImpl but instead a TaskQueue? I think that way all the
> pumping
> > logic could live here and the frame scheduler would just need to add/remove
> the
> > timer queue here. WDYT?
> 
> Done.

Thanks, this seems like a nice simplification.

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/b...
File components/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/b...
components/scheduler/base/task_queue.h:146: virtual bool
HasPendingImmediateTask() const;
Just remembered that we also have GetQueueState(). Could you add a TODO for
maybe getting rid of these two helpers if favor of a new queue state?

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/b...
components/scheduler/base/time_domain.h:32: // Called when
RegisterAsUpdatableTaskQueue is called. Can fire from any
nit: these two comments don't really make sense unless you know how TimeDomain
works internally. Maybe just describe them more generally?

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.cc (right):

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:62: 
DCHECK that the task queue isn't still in the virtual time domain?

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:91: work_to_do_ = false;
Do we need this flag? Won't OnTimeDomainHasImmediateWork or
OnTimeDomainHasDelayedWork always be called when there are things to do, and
we'd only need to make sure we've scheduled a pump?

[alex clarke (OOO till 29th), 2015-11-26 18:43:27]

PTAL

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.cc (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:105: const base::TimeDelta
one_second = base::TimeDelta::FromSeconds(1);
On 2015/11/25 17:12:25, Sami wrote:
> On 2015/11/25 12:29:36, alexclarke1 wrote:
> > On 2015/11/24 12:48:48, Sami wrote:
> > > Could we use TimeTicks::SnappedToNextTick?
> > 
> > It doesn't seem to do what I wanted.  Specifically the following fails
because
> > it returned 0. 
> > 
> > 
> > EXPECT_EQ(base::TimeDelta::FromSecondsD(1.0),
> >             ThrottlingHelper::DelayToNextRunTimeInSeconds(
> >                 base::TimeTicks() + base::TimeDelta::FromSecondsD(0.0)));
> 
> Ah, different boundary condition, got it.

Acknowledged.

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.h (right):

https://codereview.chromium.org/1441073006/diff/200001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.h:32: void
ThrottleBackgroundFrame(WebFrameSchedulerImpl* frame_scheduler);
On 2015/11/25 17:12:25, Sami wrote:
> On 2015/11/25 12:30:12, alexclarke1 wrote:
> > On 2015/11/24 12:48:48, Sami wrote:
> > > What if the unit that the ThrottlingHelper works with wasn't a
> > > WebFrameSchedulerImpl but instead a TaskQueue? I think that way all the
> > pumping
> > > logic could live here and the frame scheduler would just need to
add/remove
> > the
> > > timer queue here. WDYT?
> > 
> > Done.
> 
> Thanks, this seems like a nice simplification.

Acknowledged.

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/b...
File components/scheduler/base/task_queue.h (right):

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/b...
components/scheduler/base/task_queue.h:146: virtual bool
HasPendingImmediateTask() const;
On 2015/11/25 17:12:25, Sami wrote:
> Just remembered that we also have GetQueueState(). Could you add a TODO for
> maybe getting rid of these two helpers if favor of a new queue state?

I mostly did that :)  I suppose I could get rid of HasPendingImmediateTask too,
but it makes the two call sites ugly.

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/b...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/b...
components/scheduler/base/time_domain.h:32: // Called when
RegisterAsUpdatableTaskQueue is called. Can fire from any
On 2015/11/25 17:12:25, Sami wrote:
> nit: these two comments don't really make sense unless you know how TimeDomain
> works internally. Maybe just describe them more generally?

Done.

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/r...
File components/scheduler/renderer/throttling_helper.cc (right):

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:62: 
On 2015/11/25 17:12:25, Sami wrote:
> DCHECK that the task queue isn't still in the virtual time domain?

Acknowledged.

https://codereview.chromium.org/1441073006/diff/240001/components/scheduler/r...
components/scheduler/renderer/throttling_helper.cc:91: work_to_do_ = false;
On 2015/11/25 17:12:25, Sami wrote:
> Do we need this flag? Won't OnTimeDomainHasImmediateWork or
> OnTimeDomainHasDelayedWork always be called when there are things to do, and
> we'd only need to make sure we've scheduled a pump?

Done.

[alex clarke (OOO till 29th), 2015-11-26 18:43:39]

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

[commit-bot: I haz the power, 2015-11-26 18:44:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441073006/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441073006/260001

[Sami, 2015-11-26 19:27:50]

scheduler parts lgtm. Just one thought about the Blink changes.

https://codereview.chromium.org/1441073006/diff/260001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (left):

https://codereview.chromium.org/1441073006/diff/260001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:228: 
Intentional?

https://codereview.chromium.org/1441073006/diff/260001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1441073006/diff/260001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:35: TaskQueueImpl::~TaskQueueImpl()
{}
nit: DCHECK that we no longer have a time domain? (or do we need to unregister?)

https://codereview.chromium.org/1441073006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1441073006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5743: if (m_importsController)
I wonder if this should be done in a separate patch which is landed before this
one? This part might uncover some flake and it might help to know that it's from
here and not the overall throttling change.

[commit-bot: I haz the power, 2015-11-26 19:35:59]

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

[commit-bot: I haz the power, 2015-11-26 19:36:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alex clarke (OOO till 29th), 2015-11-26 22:03:39]

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

[commit-bot: I haz the power, 2015-11-26 22:04:02]

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

[commit-bot: I haz the power, 2015-11-26 23:24:42]

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

[commit-bot: I haz the power, 2015-11-26 23:24:44]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2015-11-27 08:23:38]

lgtm

[alex clarke (OOO till 29th), 2015-11-27 10:01:59]

Thanks all!

https://codereview.chromium.org/1441073006/diff/260001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (left):

https://codereview.chromium.org/1441073006/diff/260001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:228: 
On 2015/11/26 19:27:50, Sami wrote:
> Intentional?

Done.

https://codereview.chromium.org/1441073006/diff/260001/components/scheduler/b...
File components/scheduler/base/task_queue_impl.cc (right):

https://codereview.chromium.org/1441073006/diff/260001/components/scheduler/b...
components/scheduler/base/task_queue_impl.cc:35: TaskQueueImpl::~TaskQueueImpl()
{}
On 2015/11/26 19:27:50, Sami wrote:
> nit: DCHECK that we no longer have a time domain? (or do we need to
unregister?)

I added a de-register here too.  It was needed in some of the tests.

https://codereview.chromium.org/1441073006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1441073006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5743: if (m_importsController)
On 2015/11/26 19:27:50, Sami wrote:
> I wonder if this should be done in a separate patch which is landed before
this
> one? This part might uncover some flake and it might help to know that it's
from
> here and not the overall throttling change.

I think this is a good idea.  I don't think we need to game this patch on it,
instead I shall do that in a follow up.

[alex clarke (OOO till 29th), 2015-11-27 10:02:15]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-11-27 10:02:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1441073006/#ps300001
(title: "Better fix")

[commit-bot: I haz the power, 2015-11-27 10:03:09]

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

[commit-bot: I haz the power, 2015-11-27 10:08:49]

Description was changed from

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402
==========

to

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402
==========

[commit-bot: I haz the power, 2015-11-27 10:08:50]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-11-27 10:09:45]

Description was changed from

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402
==========

to

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402

Committed: https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}
==========

[commit-bot: I haz the power, 2015-11-27 10:09:46]

Patchset 16 (id:??) landed as
https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}

[alex clarke (OOO till 29th), 2015-11-27 10:11:11]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/1477353002/ by alexclarke@chromium.org.

The reason for reverting is: Oops I'd meant to upload one final patchset before
committing..

[alex clarke (OOO till 29th), 2015-11-27 10:11:35]

Description was changed from

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402

Committed: https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}
==========

to

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402

Committed: https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}
==========

[alex clarke (OOO till 29th), 2015-11-27 10:13:25]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-11-27 10:13:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1441073006/#ps320001
(title: "Last few changes Sami requested")

[commit-bot: I haz the power, 2015-11-27 10:14:02]

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

[commit-bot: I haz the power, 2015-11-27 11:43:01]

Description was changed from

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402

Committed: https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}
==========

to

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402

Committed: https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}
==========

[commit-bot: I haz the power, 2015-11-27 11:43:02]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-11-27 11:43:50]

Description was changed from

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402

Committed: https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}
==========

to

==========
Move throttling of background timers into the renderer scheduler

Not only does this simplify the code, it's more efficent since
previously setting the timer alignment resulted in mass cancellation
and reposting of timers.

BUG=510398, 546953, 560402

Committed: https://crrev.com/ec5adec0a9879a31866e98c65ddc7b506b9f49c3
Cr-Commit-Position: refs/heads/master@{#361971}

Committed: https://crrev.com/de233a29aa0ef1b89d35a9045a864c9ad6fa12fe
Cr-Commit-Position: refs/heads/master@{#361978}
==========

[commit-bot: I haz the power, 2015-11-27 11:43:51]

Patchset 17 (id:??) landed as
https://crrev.com/de233a29aa0ef1b89d35a9045a864c9ad6fa12fe
Cr-Commit-Position: refs/heads/master@{#361978}