Fix the drift in repeating timers.

Fix the drift in repeating timers.

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.

BUG=328700
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199903

[alex clarke (OOO till 29th), 2015-07-31 16:56:53]

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

[Sami, 2015-08-03 12:57:21]

https://codereview.chromium.org/1261993006/diff/1/Source/platform/Timer.cpp
File Source/platform/Timer.cpp (right):

https://codereview.chromium.org/1261993006/diff/1/Source/platform/Timer.cpp#n...
Source/platform/Timer.cpp:129: double intervalToNextFireTime = m_repeatInterval
- fmod(now - m_unalignedNextFireTime, m_repeatInterval);
If (now - m_unalignedNextFireTime) % m_repeatInterval == 0, should the delay
equal zero too?

[alex clarke (OOO till 29th), 2015-08-03 13:37:27]

PTAL

https://codereview.chromium.org/1261993006/diff/1/Source/platform/Timer.cpp
File Source/platform/Timer.cpp (right):

https://codereview.chromium.org/1261993006/diff/1/Source/platform/Timer.cpp#n...
Source/platform/Timer.cpp:129: double intervalToNextFireTime = m_repeatInterval
- fmod(now - m_unalignedNextFireTime, m_repeatInterval);
On 2015/08/03 12:57:21, Sami wrote:
> If (now - m_unalignedNextFireTime) % m_repeatInterval == 0, should the delay
> equal zero too?

Interesting point.  I think for this API there's not really a strong preference
for or against that, so I'm going to default to simpler code with less special
cases.

[Sami, 2015-08-03 15:44:32]

On 2015/08/03 13:37:27, alexclarke1 wrote:
> PTAL
> 
> https://codereview.chromium.org/1261993006/diff/1/Source/platform/Timer.cpp
> File Source/platform/Timer.cpp (right):
> 
>
https://codereview.chromium.org/1261993006/diff/1/Source/platform/Timer.cpp#n...
> Source/platform/Timer.cpp:129: double intervalToNextFireTime =
m_repeatInterval
> - fmod(now - m_unalignedNextFireTime, m_repeatInterval);
> On 2015/08/03 12:57:21, Sami wrote:
> > If (now - m_unalignedNextFireTime) % m_repeatInterval == 0, should the delay
> > equal zero too?
> 
> Interesting point.  I think for this API there's not really a strong
preference
> for or against that, so I'm going to default to simpler code with less special
> cases.

Yeah, I only brought it up since base::TimeTicks works that way, but that is a
more general API than this so lgtm.

[jochen (gone - plz use gerrit), 2015-08-03 15:49:45]

lgtm

[alex clarke (OOO till 29th), 2015-08-03 15:54:48]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-08-03 15:55:08]

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

[commit-bot: I haz the power, 2015-08-03 17:21:43]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199903

[jam, 2015-08-12 03:11:15]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1290633002/ by jam@chromium.org.

The reason for reverting is: this change caused a large amount of flakes, see
https://code.google.com/p/chromium/issues/detail?id=517488

and also graphs on 
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.linux%20Build...
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.win%20Builder...
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.win%20Builder...

this was hard to track down because browser_tests weren't printing renderer
crashes. i fixed that in an unlanded cl and tried reenabling all the tests and
they all had
ASSERTION FAILED: now >= m_unalignedNextFireTime
see the try runs on this patchset
https://codereview.chromium.org/1284083002/#ps20001
i.e. one example:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...

BUG=517488.