Ensure serviceOnNextFrame isn't put off indefinitely.

Ensure serviceOnNextFrame isn't put off indefinitely.

A constant series of compositor updates would force the serviceOnNextFrame
to be kept being put off. The wakeAfter was always called with a fixed
duration so it would just keep getting cancelled and rescheduled.

Since the serviceOnNextFrame ends up calling setNeedsLayout inside the
scheduleAnimation callback; the LayoutTree would get updated in that call.
The hit testing problem identified in the bug was related to the fact that
the LayoutTree's layer transforms weren't updated correctly until it was
told it needed to do a Layout.

The fix is to check the remaining duration vs the new scheduled duration
and avoid the schedule if the new duration is longer.


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

[dtapuska, 2015-08-14 15:35:47]

dtapuska@chromium.org changed reviewers:
+ alancutter@chromium.org, dstockwell@chromium.org

[dtapuska, 2015-08-14 15:35:48]

I emailed Alan about this issue yesterday but I didn't hear back. Here is my
proposed fix of understanding the code.

[alancutter (OOO until 2018), 2015-08-17 07:30:36]

Deferring to dstockwell for full review as he is more familiar with this area
than I am.

https://codereview.chromium.org/1287013003/diff/1/Source/core/animation/Anima...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/1287013003/diff/1/Source/core/animation/Anima...
Source/core/animation/AnimationTimeline.cpp:177: double nextFireTime =
m_timeline->currentTime() + duration;
currentTime() is affected by m_playbackRate making it different to wall clock
time.
I suspect duration is in terms of wall clock time so you should probably use
document()->animationClock() here instead.

[dstockwell, 2015-08-17 08:05:36]

Is it possible to write a layout test to simulate the hit testing behavior?

https://codereview.chromium.org/1287013003/diff/1/Source/core/animation/Anima...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/1287013003/diff/1/Source/core/animation/Anima...
Source/core/animation/AnimationTimeline.cpp:178: if (m_timer.isActive() &&
nextFireTime >= m_currentScheduledTime)
You should be able to retrieve the next fire time from the timer directly (see
TimerBase::nextFireInterval), and reduce the patch to this one line.

[dtapuska, 2015-08-17 17:50:44]

I attempted to create a layout test for this but I haven't had much success to
created it so that it works consistently. I'm wondering if the different call
flow of the eventSender API are causing it to always pass when under the test I
wrote.

https://codereview.chromium.org/1287013003/diff/1/Source/core/animation/Anima...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/1287013003/diff/1/Source/core/animation/Anima...
Source/core/animation/AnimationTimeline.cpp:177: double nextFireTime =
m_timeline->currentTime() + duration;
On 2015/08/17 07:30:36, alancutter wrote:
> currentTime() is affected by m_playbackRate making it different to wall clock
> time.
> I suspect duration is in terms of wall clock time so you should probably use
> document()->animationClock() here instead.

Acknowledged.

https://codereview.chromium.org/1287013003/diff/1/Source/core/animation/Anima...
Source/core/animation/AnimationTimeline.cpp:178: if (m_timer.isActive() &&
nextFireTime >= m_currentScheduledTime)
On 2015/08/17 08:05:36, dstockwell wrote:
> You should be able to retrieve the next fire time from the timer directly (see
> TimerBase::nextFireInterval), and reduce the patch to this one line.

Done.

[dstockwell, 2015-08-18 02:53:42]

lgtm, please wait for the test in https://codereview.chromium.org/1297563003 to
land first and then remove the failing expectation.

[dtapuska, 2015-08-18 13:56:15]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-08-18 13:56:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dstockwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1287013003/#ps40001
(title: "Adjust layout test")

[commit-bot: I haz the power, 2015-08-18 13:56:25]

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

[commit-bot: I haz the power, 2015-08-18 16:09:02]

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

[commit-bot: I haz the power, 2015-08-18 16:09:03]

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

[dtapuska, 2015-08-18 17:07:38]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-08-18 17:07:50]

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

[commit-bot: I haz the power, 2015-08-18 18:16:44]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200737