Fix the drift in repeating timers (try #2)

Fix the drift in repeating timers (try #2)

Currently repeating blink timers ignore the difference between when the
timer was due to fire and when it actually did.  In addition the delay
reported to chromium was rounded up to the nearest millisecond,
introducing unnecessary extra delay.

(Note seperatly DOMTimers have additional clamping).

This fairly broken behaviour has been there for a long time, and I
really hope nothing has come to depend on it!

Judging by
http://www.duckware.com/test/chrome/timeraccuracy.html this patch makes
things a lot better at least on linux. The actual delays are much
closer to the requested ones, and the run time modulo interval shows
the drift is gone.

Based on https://codereview.chromium.org/1261993006/ but without the
assert.  I've added test coverage to make sure nothing bad happens
if the timer fires a little earlier than expected (which can happen
since the delay gets quantized down to milliseconds on some platforms).

BUG=328700
Committed: https://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0
Cr-Commit-Position: refs/heads/master@{#351295}

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

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

[alex clarke (OOO till 29th), 2015-09-25 10:50:25]

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

[commit-bot: I haz the power, 2015-09-25 10:50:45]

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

[commit-bot: I haz the power, 2015-09-25 11:06:29]

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

[commit-bot: I haz the power, 2015-09-25 11:06:30]

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

[Sami, 2015-09-25 11:11:11]

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

[Sami, 2015-09-25 11:17:31]

lgtm, thanks for giving this another shot.

+Elliott, could you confirm that increasing the resolution of timers like is
okay? Basically the 1 ms minimum delay is still there, but with this patch
delays above 1 ms can have sub-ms precision.

If it turns out we need to keep the 1 ms resolution, we could still eliminate
drifting in setInterval() separately.

[alex clarke (OOO till 29th), 2015-09-25 11:40:48]

On 2015/09/25 11:17:31, Sami wrote:
> lgtm, thanks for giving this another shot.
> 
> +Elliott, could you confirm that increasing the resolution of timers like is
> okay? Basically the 1 ms minimum delay is still there, but with this patch
> delays above 1 ms can have sub-ms precision.

I note that DOM timers specify the interval as an integer number of
milliseconds, so from JS it's not possible to do setInterval(..., 1.5) for
example.

> 
> If it turns out we need to keep the 1 ms resolution, we could still eliminate
> drifting in setInterval() separately.

[alex clarke (OOO till 29th), 2015-09-25 11:41:07]

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

[commit-bot: I haz the power, 2015-09-25 11:41:19]

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

[Sami, 2015-09-25 12:13:53]

On 2015/09/25 11:40:48, alexclarke1 wrote:
> I note that DOM timers specify the interval as an integer number of
> milliseconds, so from JS it's not possible to do setInterval(..., 1.5) for
> example.

Right, good point, I was thinking that might be the case. I still wanted to make
sure since the rounding in Timer might be there to guard against other APIs that
don't have that kind of guard.

[commit-bot: I haz the power, 2015-09-25 12:22:10]

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

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

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

[alex clarke (OOO till 29th), 2015-09-25 17:07:14]

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

[commit-bot: I haz the power, 2015-09-25 17:07:44]

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

[commit-bot: I haz the power, 2015-09-25 17:22:07]

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

[commit-bot: I haz the power, 2015-09-25 17:22:11]

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

[alex clarke (OOO till 29th), 2015-09-28 09:14:06]

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

[commit-bot: I haz the power, 2015-09-28 09:14:35]

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

[commit-bot: I haz the power, 2015-09-28 10:54:24]

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

[commit-bot: I haz the power, 2015-09-28 10:54:25]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-09-28 17:23:48]

Elliot does this look safe to you? Thanks!

[esprehn, 2015-09-28 23:34:43]

Yeah this seems fine, you can do the renames and fix the std::vector in a future
patch, but please do fix them. :)

lgtm

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

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:44:
DelayedTask(WebTaskRunner::Task* task, double delaySecs)
delaySeconds

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:47: , m_delaySecs(delaySecs) {
}
m_delaySeconds

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:64: double delaySecs() const
ditto, don't abbreviate

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:71: double m_runTimeSecs;
This one should be fixed too

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:342: std::vector<double>
m_nextFireTimes;
These should be WTF::Vector, we don't use std::vector in blink. Both of these
should be fixed.

[alex clarke (OOO till 29th), 2015-09-29 10:58:36]

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

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:44:
DelayedTask(WebTaskRunner::Task* task, double delaySecs)
On 2015/09/28 23:34:43, esprehn wrote:
> delaySeconds

Done.

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:47: , m_delaySecs(delaySecs) {
}
On 2015/09/28 23:34:43, esprehn wrote:
> m_delaySeconds

Done.

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:64: double delaySecs() const
On 2015/09/28 23:34:43, esprehn wrote:
> ditto, don't abbreviate

Done.

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:71: double m_runTimeSecs;
On 2015/09/28 23:34:43, esprehn wrote:
> This one should be fixed too

Done.

https://codereview.chromium.org/1373503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TimerTest.cpp:342: std::vector<double>
m_nextFireTimes;
On 2015/09/28 23:34:43, esprehn wrote:
> These should be WTF::Vector, we don't use std::vector in blink. Both of these
> should be fixed.

I've added a todo.

[alex clarke (OOO till 29th), 2015-09-29 10:59:21]

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

[alex clarke (OOO till 29th), 2015-09-29 10:59:22]

+jochen@ for blink platform owners review.

[jochen (gone - plz use gerrit), 2015-09-29 11:09:36]

lgtm

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

The CQ bit was checked by alexclarke@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1373503002/#ps80001
(title: "Rename some variables and add a todo")

[commit-bot: I haz the power, 2015-09-29 11:10:39]

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

[commit-bot: I haz the power, 2015-09-29 12:37:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-29 12:38:03]

Patchset 5 (id:??) landed as
https://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0
Cr-Commit-Position: refs/heads/master@{#351295}