1244635 - implement AnimationEffectTiming endDelay

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I tried a negative endDelay on Chromium as of 2/24.

    var target = document.getElementById("target");
    var anim = target.animate(
      [ { opacity: 0 },
        { opacity: 1 } ],
      { duration: 100, endDelay: -120 });
    anim.ready.then(function() {
      console.log("ready");
    });
    anim.finished.then(function() {
      console.log("finished");
      console.log(getComputedStyle(target, "").opacity);
    });

The output in console was:
 ready
 finished
 0

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Motozawa-san and I discussed some failing tests and how an endDelay should behave when it is negative and greater in magnitude than (start delay + active duration). It's not clear what the intended behavior here should be. After some consideration I think we should try changing the definition of phases[1] as follows:

  An animation effect is in the *before phase* if the animation effect’s local
  time is not unresolved and is less than min(start delay, end time).

  An animation effect is in the *active phase* if /all/ of the following
  conditions are met:

    * the animation effect’s local time is not unresolved, and
    * the animation effect’s local time is greater than or equal to its
      start delay, and
    * the animation effect’s local time is less than
      min(start delay + active duration, end time)

  An animation effect is in the *after phase* if the animation effect’s local time
  is not unresolved and is greater than or equal to min(start delay + active
  duration, end time).

There are lots of different ways we could go here, particularly for the definition of the after phase. In effect, the above definition means that when we have a positive end delay, we apply the fill mode between the end of the active interval and the end of the end delay (rather than, say, maintaining the last value regardless of the fill mode--which is what we would do if we defined the after phase as simply being after the end time).

I think the above provides the most consistent behavior with start delay and means we don't need to change the definition of in-effect / current. It seems the best of the alternatives I considered, anyway.

We need to try implementing to see if this works. Also, I want to get Google's feedback on this before changing the spec.

[1] https://w3c.github.io/web-animations/#animation-effect-phases-and-states

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: IRC log of discussion with Google about spec changes

I spoke to Google and they agreed with the proposed changes, assuming they don't break the rest of the model.

Comment 4

[Ryo Motozawa [:ryo]]

Attachment: Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp

I implement a setter for AnimationEffectTiming endDelay.

Comment 5

[Ryo Motozawa [:ryo]]

Attachment: MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

Review commit: https://reviewboard.mozilla.org/r/37351/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37351/

Comment 6

[Ryo Motozawa [:ryo]]

Attachment: MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

Review commit: https://reviewboard.mozilla.org/r/37353/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37353/

Comment 7

[Ryo Motozawa [:ryo]]

Attachment: MozReview Request: Bug 1244635 - Part3 Add duration tests in testing/web-platform/tests/web-animations r?hiro

Review commit: https://reviewboard.mozilla.org/r/37355/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37355/

Comment 8

[Ryo Motozawa [:ryo]]

Attachment: MozReview Request: Bug 1244635 - Part4 Add duration tests in layout/style/test r?hiro

Review commit: https://reviewboard.mozilla.org/r/37357/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37357/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

https://reviewboard.mozilla.org/r/37351/#review33907

::: dom/animation/AnimationEffectTiming.h:27
(Diff revision 1)
> +  void SetEndDelay(const double aEndDelay);

Nit: I think these line should be:

void Unlink();

void SetEndDelay();
void SetDuration();

::: dom/animation/AnimationEffectTiming.cpp:29
(Diff revision 1)
> +  if (mEffect) {
> +    mEffect->NotifySpecifiedTimingUpdated();
> +  }

I think now it's the time to create a utility method to check mEffect and call NotifySpecifiedTimingUpdated if the mEffect is not nullptr.

::: dom/animation/AnimationEffectTimingReadOnly.h:57
(Diff revision 1)
> +  TimeDuration mEndDelay;

Nit: This line should be just below mDelay.

::: dom/animation/KeyframeEffect.cpp:249
(Diff revision 1)
> -  // Bug 1244635: Add endDelay to the end time calculation
> +

Nit: A blank line.

::: dom/animation/KeyframeEffect.cpp:269
(Diff revision 1)
> -  if (localTime >= aTiming.mDelay + result.mActiveDuration) {
> +  if (localTime.ToMilliseconds() >=
> +      fmin(aTiming.mDelay.ToMilliseconds() +
> +           result.mActiveDuration.ToMilliseconds(),
> +           result.mEndTime.ToMilliseconds())) {

Is there any readon we don't use operators for TimeDuration or StickyTimeDuration at all?
I think there might be some possibility of causing over flow.

::: dom/animation/KeyframeEffect.cpp:284
(Diff revision 1)
> -  } else if (localTime < aTiming.mDelay) {
> +  } else if (localTime.ToMilliseconds() <
> +             fmin(aTiming.mDelay.ToMilliseconds(),
> +                  result.mEndTime.ToMilliseconds())) {

Same question here.
I think we should use operators instead of fmin.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8725159 [details]
MozReview Request: Bug 1244635 - Part3 Add duration tests in testing/web-platform/tests/web-animations r?hiro

https://reviewboard.mozilla.org/r/37355/#review33915

This is not duration test. The commit message should be revised.

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:30
(Diff revision 1)
> +  assert_equals(anim.effect.timing.endDelay, -1000, 'set endDelay 1000');

set endDelay -1000

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:33
(Diff revision 1)
> +}, 'set endDelay 1000');

-1000

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:34
(Diff revision 1)
> +

I think we need to test positive infinity and negative inifinity with non-zero duration.

I think we also need test cases to confirm that onfinish event is fired after endDelay.

::: testing/web-platform/tests/web-animations/animation-effect-timing/getAnimations.html:33
(Diff revision 1)
> +  assert_equals(div.getAnimations().length, 0, 'set negative endDelay');
> +  anim.effect.timing.endDelay = 1000;
> +  assert_equals(div.getAnimations()[0], anim, 'set positive endDelay');
> +}, 'when endDelay is changed');

I think we need to test at least below cases:

1) duration: 1000, endDelay: 1000, currentTime: 1500
2) duration: 1000, endDelay: -500, currentTime 400, 500, 1000
3) with various start delays

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:30
(Diff revision 1)
> +  div.style.opacity = '0';

Is this style necessary?

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:31
(Diff revision 1)
> +  var anim = div.animate({ opacity: [ 0, 1 ] },
> +                         { duration: 100000, fill: 'none' });
> +  anim.effect.timing.endDelay = 10000;

In this test case the endDelay can be specified as an option of animate().

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:43
(Diff revision 1)
> +  anim.currentTime = 111000;
> +  assert_equals(getComputedStyle(div).opacity, '0',
> +                'set currentTime after endDelay');
> +}, 'change endDelay and currentTime when fill none');

I'd like to see a similar test case for fill:forwards.

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:54
(Diff revision 1)
> +  anim.effect.timing.endDelay = 10000;

The endDelay can be specified in animate method in this test case too.

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:58
(Diff revision 1)
> +}, 'change endDelay and currentTime when fill backwards');

'fill forwards'

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:59
(Diff revision 1)
> +

We don't need to test negative endDelay at all?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8725158 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

https://reviewboard.mozilla.org/r/37353/#review33917

This is not duration test too.

::: dom/animation/test/chrome/test_animation_observers.html:1530
(Diff revision 1)
> +  anim.currentTime = 109000;
> +  yield await_frame();
> +  assert_records([{ added: [], changed: [], removed: [anim] }],
> +                 "records after currentTime over endTime");

It seems that 109000 is not over the endTime.  You mean that 109000 is greater than startDelay + activeDuration but less than the endTime?

::: dom/animation/test/chrome/test_animation_observers.html:1535
(Diff revision 1)
> +  anim.currentTime = 0;
> +  yield await_frame();
> +  assert_records([{ added: [anim], changed: [], removed: [] }],
> +                 "records after currentTime under endTime");

I don't quite understand what this test checks actually.

::: dom/animation/test/chrome/test_animation_observers.html:1540
(Diff revision 1)
> +  anim.effect.timing.endDelay = -110000;
> +  yield await_frame();
> +  assert_records([{ added: [], changed: [], removed: [anim] }],
> +                 "records after assigning negative value");

Though I don't remember the reason at all, why don't we receive any changedAnimation when the endDelay is changed?  You know, Brian?

Anyway, I'd like to see a test case that initial endDelay is negative.

::: dom/animation/test/chrome/test_running_on_compositor.html:342
(Diff revision 1)
> +      + ' when changed endDelay');

when endDelay is changed

::: dom/animation/test/chrome/test_running_on_compositor.html:344
(Diff revision 1)
> +    animation.currentTime = 11000;
> +    return waitForFrame();
> +  })).then(t.step_func(function() {
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled,

I think we should optimize off main-thread animations with endDelay.  After active duration we don't need to run animations on compositor at all, I think.

::: dom/animation/test/chrome/test_running_on_compositor.html:351
(Diff revision 1)
> +}, 'animation is remaining on compositor' +
> +   ' when timing.endDelay is made longer than the current time');

This description is something not quite right.

We should also test negative endDelay case here.  In negative case,  the compositor animation remains too?

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> ::: dom/animation/test/chrome/test_animation_observers.html:1540
> (Diff revision 1)
> > +  anim.effect.timing.endDelay = -110000;
> > +  yield await_frame();
> > +  assert_records([{ added: [], changed: [], removed: [anim] }],
> > +                 "records after assigning negative value");
> 
> Though I don't remember the reason at all, why don't we receive any
> changedAnimation when the endDelay is changed?  You know, Brian?

If updating the endDelay causes us to no longer be "in effect" or "current" (i.e. the animation is removed from the list of animations returned by getAnimations()), then the batching of mutation records (nsAutoAnimationMutationBatch) which cause us to convert "[changed], [removed]" into just "[removed]".

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #12)
> If updating the endDelay causes us to no longer be "in effect" or "current"
> (i.e. the animation is removed from the list of animations returned by
> getAnimations()), then the batching of mutation records
> (nsAutoAnimationMutationBatch) which cause us to convert "[changed],
> [removed]" into just "[removed]".

s/which cause/will cause/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8725160 [details]
MozReview Request: Bug 1244635 - Part4 Add duration tests in layout/style/test r?hiro

https://reviewboard.mozilla.org/r/37357/#review33911

This is not duration test too!

::: layout/style/test/file_animations_effect_timing_enddelay.html:8
(Diff revision 1)
> +    @keyframes anim {
> +      0% { transform: translate(0px) }
> +      100% { transform: translate(100px) }
> +    }

This keyframes should be removed.

::: layout/style/test/file_animations_effect_timing_enddelay.html:43
(Diff revision 1)
> +  div.style.transform = 'translate(0px)';

Is this really necessary?

::: layout/style/test/file_animations_effect_timing_enddelay.html:57
(Diff revision 1)
> +
> +  advance_clock(1000);
> +  yield waitForPaints();
> +  omta_is(div, "transform", { tx: 0 }, RunningOn.Mainthread,
> +          "Animation is updated on compositor");

I don't quite understand what the purpose of this test is.  What do you want to check at 1100ms?
And this result seems to be different from the result of part 2 patch.  When the currentTime is greater than activeDuration but less than endTime, the compositor animation with fill:none is still running on compositor, or not?  Am I missing something?

::: layout/style/test/file_animations_effect_timing_enddelay.html:74
(Diff revision 1)
> +  animation.effect.timing.endDelay = 1000;

This endDelay should be set as an option of animate().

::: layout/style/test/file_animations_effect_timing_enddelay.html:77
(Diff revision 1)
> +          "Animation is updated on compositor backwards");

forwards?

::: layout/style/test/file_animations_effect_timing_enddelay.html:81
(Diff revision 1)
> +

I'd like to see the result of below case with fill:none and fill:forwards:

duration: 1000, endDelay: -500, currentTime: 500, 900, 1000

Comment 15

[Ryo Motozawa [:ryo]]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37351/diff/1-2/

Comment 16

[Ryo Motozawa [:ryo]]

Comment on attachment 8725158 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37353/diff/1-2/

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

https://reviewboard.mozilla.org/r/37351/#review34085

::: dom/animation/AnimationEffectTiming.h:31
(Diff revisions 1 - 2)
> +  void NotifySpecifiedTimingUpdatedUtil();

I'd prefer NotifyTimingUpdate();

::: dom/animation/KeyframeEffect.cpp:267
(Diff revisions 1 - 2)
>    StickyTimeDuration activeTime;
> -  if (localTime.ToMilliseconds() >=
> -      fmin(aTiming.mDelay.ToMilliseconds() +
> -           result.mActiveDuration.ToMilliseconds(),
> +  if (aTiming.mDelay + result.mActiveDuration < result.mEndTime
> +      ? localTime >= aTiming.mDelay + result.mActiveDuration
> +      : localTime >= result.mEndTime) {
> -           result.mEndTime.ToMilliseconds())) {

This if statement is hard to read.  Can't we define Min for StickyTimeDuration?

::: dom/animation/KeyframeEffect.cpp:282
(Diff revisions 1 - 2)
> -  } else if (localTime.ToMilliseconds() <
> -             fmin(aTiming.mDelay.ToMilliseconds(),
> +  } else if (aTiming.mDelay < result.mEndTime ? localTime < aTiming.mDelay
> +                                              : localTime < result.mEndTime) {
> -                  result.mEndTime.ToMilliseconds())) {

I think we should define Min(const TimeDuration&, const StickyTimeDuration&) and use it here.  What do you think?

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8725158 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

https://reviewboard.mozilla.org/r/37353/#review34087

I still think we need test cases for getAnimation in case initial endDelay is negative. e.g. duration: 1000, endDelay: -1100

::: dom/animation/test/chrome/test_running_on_compositor.html:347
(Diff revisions 1 - 2)
> -    assert_equals(animation.isRunningOnCompositor, omtaEnabled,
> +    assert_equals(animation.isRunningOnCompositor, false,
>        'Animation reports that it is running on the compositor'
>        + ' when currentTime is shoter than duration + endDelay');

Now this description is something wrong.

::: dom/animation/test/chrome/test_running_on_compositor.html:360
(Diff revisions 1 - 2)
> +      'Animation reports that it is running on the compositor');
> +
> +    animation.effect.timing.endDelay = -20000;
> +
> +    assert_equals(animation.isRunningOnCompositor, false,

I think we should wait a frame here, or if we don't wait a frame, we should check isRunningOnCompositor flag in a subsequent frame to confirm that the flag is not set back true again.  IIRC, isRunningOnCompositor flag is set to false immediately whenever any timing property is changed.

::: dom/animation/test/chrome/test_running_on_compositor.html:364
(Diff revisions 1 - 2)
> +    assert_equals(animation.isRunningOnCompositor, false,
> +      'Animation reports that it is running on the compositor'
> +      + ' when endDelay is changed');

This description is wrong too.

::: dom/animation/test/chrome/test_running_on_compositor.html:368
(Diff revisions 1 - 2)
> +}, 'animation is removed from compositor' +
> +   ' when endTime is negative value');
>  

What will happen if endTime is positive and endDelay is negative?  Shouldn't we test it?

Comment 19

[Ryo Motozawa [:ryo]]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37351/diff/2-3/

Comment 20

[Ryo Motozawa [:ryo]]

Comment on attachment 8725158 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37353/diff/2-3/

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

https://reviewboard.mozilla.org/r/37351/#review34297

This patch needs to be reviewed by a DOM peer.

::: dom/animation/AnimationEffectTiming.cpp:32
(Diff revision 3)
> +  if (mTiming.mEndDelay == TimeDuration::FromMilliseconds(aEndDelay)) {
> +    return;
> +  }
> +  mTiming.mEndDelay = TimeDuration::FromMilliseconds(aEndDelay);
> +

Nit: We can store the result of TimeDuration::FromMilliseconds(aEndDelay) and use it.

::: dom/animation/KeyframeEffect.cpp:253
(Diff revision 3)
> -  // Bug 1244635: Add endDelay to the end time calculation
> +  result.mEndTime = aTiming.mDelay + result.mActiveDuration + aTiming.mEndDelay;

Nit: over 80 chars.

::: dom/animation/KeyframeEffect.cpp:272
(Diff revision 3)
> -  if (localTime >= aTiming.mDelay + result.mActiveDuration) {
> +  if (localTime >=
> +      std::min(StickyTimeDuration(aTiming.mDelay + result.mActiveDuration),
> +               result.mEndTime)) {

Nit: the line for std::min needs one more indent. I mean two spaces should be inserted there.

::: dom/animation/KeyframeEffect.cpp:287
(Diff revision 3)
> -  } else if (localTime < aTiming.mDelay) {
> +  } else if (localTime <
> +             std::min(StickyTimeDuration(aTiming.mDelay), result.mEndTime)) {

Nit: same here.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8725158 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

https://reviewboard.mozilla.org/r/37353/#review34313

::: dom/animation/test/chrome/test_animation_observers.html:1535
(Diff revision 3)
> +  anim.effect.timing.endDelay = -110000;
> +  yield await_frame();
> +  assert_records([], "records after assigning negative value");
> +
> +  anim.cancel();

I wanted to know the result of a case that endDelay is negative initially. E.g.

var anim = div.animate({..}, { duration: 100, endDelay: -100 });

assert_records(?);

::: dom/animation/test/chrome/test_running_on_compositor.html:362
(Diff revision 3)
> +    animation.effect.timing.endDelay = -20000;
> +    return waitForFrame();
> +    assert_equals(animation.isRunningOnCompositor, false,
> +      'Animation reports that it is NOT running on the compositor'

This assertion should be inside the next then block.  Otherwise the assertion will be never executed because once the promise returned by waitForFrame() is fullfilled, then the next then block is executed.

::: dom/animation/test/chrome/test_running_on_compositor.html:379
(Diff revision 3)
> +    animation.effect.timing.endDelay = -5000;
> +    return waitForFrame();
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled,
> +      'Animation reports that it is running on the compositor'
> +      + ' when endTime is positive and endDelay is negative');

Same here.

::: dom/animation/test/chrome/test_running_on_compositor.html:385
(Diff revision 3)
> +    animation.currentTime = 6000;
> +    return waitForFrame();
> +  })).then(t.step_func(function() {
> +    assert_equals(animation.isRunningOnCompositor, false,
> +      'Animation reports that it is NOT running on the compositor'

Yes! This is what I wanted to know!

::: dom/animation/test/chrome/test_running_on_compositor.html:392
(Diff revision 3)
> +}, 'animation is remaining on compositor' +
> +   'when endTime is positive and endDelay is negative');

Nit: The animation has gone. ;-)


r=me with these changes.

Comment 23

[Ryo Motozawa [:ryo]]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37351/diff/3-4/

Comment 24

[Ryo Motozawa [:ryo]]

Comment on attachment 8725158 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37353/diff/3-4/

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

https://reviewboard.mozilla.org/r/37351/#review34337

::: dom/animation/AnimationEffectTiming.h:27
(Diff revision 4)
> +  void SetEndDelay(const double aEndDelay);

Just double, not const double.  It's being passed by value anyway.

r=me with that fixed.

Comment 26

[Ryo Motozawa [:ryo]]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37351/diff/4-5/

Comment 27

[Ryo Motozawa [:ryo]]

https://reviewboard.mozilla.org/r/37355/#review33915

> Is this style necessary?

This style is need to set opacity when currentTime is outside active interval.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Ryo Motozawa [:ryo] from comment #27)
> https://reviewboard.mozilla.org/r/37355/#review33915
> 
> > Is this style necessary?
> 
> This style is need to set opacity when currentTime is outside active
> interval.

I wonder we can use opacity: [1, 0] for that purpose?

Comment 29

[Ryo Motozawa [:ryo]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> (In reply to Ryo Motozawa [:ryo] from comment #27)
> > https://reviewboard.mozilla.org/r/37355/#review33915
> > 
> > > Is this style necessary?
> > 
> > This style is need to set opacity when currentTime is outside active
> > interval.
> 
> I wonder we can use opacity: [1, 0] for that purpose?

I see. I'm going to use it.

Comment 30

[Ryo Motozawa [:ryo]]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37351/diff/5-6/

Comment 31

[Ryo Motozawa [:ryo]]

Attachment: MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

Review commit: https://reviewboard.mozilla.org/r/38073/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38073/

Comment 32

[Ryo Motozawa [:ryo]]

Attachment: MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

Review commit: https://reviewboard.mozilla.org/r/38075/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38075/

Comment 33

[Ryo Motozawa [:ryo]]

Attachment: MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

Review commit: https://reviewboard.mozilla.org/r/38077/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38077/

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726544 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

https://reviewboard.mozilla.org/r/38073/#review34603

I thought I've already reviewed this.  Unfortunately MozReview does not show the interdiff any more.

::: dom/animation/test/chrome/test_running_on_compositor.html:397
(Diff revision 1)
> +  var div = addDiv(t);
> +  var animation = div.animate({ opacity: [ 0, 1 ] },
> +                              { duration: 100, endDelay: -100});

I'd actually wanted to know this case for *getAnimations()* test, not for isRunningOnCompositor.
And 100ms is basically too short, but in this case it might not be a problem.  Could you please run this test with chaos mode?

 MOZ_CHAOSMODE=0 ./mach mochitest --run-until-failure ...

Comment 35

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> Comment on attachment 8726544 [details]
> MozReview Request: Bug 1244635 - Part2 Add enddelay tests in
> dom/animation/test/chrome r=hiro
> 
> https://reviewboard.mozilla.org/r/38073/#review34603
> 
> I thought I've already reviewed this.  Unfortunately MozReview does not show
> the interdiff any more.
> 
> ::: dom/animation/test/chrome/test_running_on_compositor.html:397
> (Diff revision 1)
> > +  var div = addDiv(t);
> > +  var animation = div.animate({ opacity: [ 0, 1 ] },
> > +                              { duration: 100, endDelay: -100});
> 
> I'd actually wanted to know this case for *getAnimations()* test

Sorry, I meant *animation observer* test.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

https://reviewboard.mozilla.org/r/38075/#review34605

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:15
(Diff revision 1)
> +function waitForAnimationFrames(frameCount, onFrame) {
> +  return new Promise(function(resolve, reject) {
> +    function handleFrame() {
> +      if (--frameCount <= 0) {
> +        resolve();
> +      } else {
> +        if (onFrame && typeof onFrame === 'function') {
> +          onFrame();
> +        }
> +        window.requestAnimationFrame(handleFrame); // wait another frame

I am not 100% sure, but we will not need |onFrame| argument in web platform tests.  And this function should be in testcommons.js.

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:75
(Diff revision 1)
> +  anim.ready.then(function() {
> +    anim.currentTime = 5000;
> +    return waitForAnimationFrames(2);
> +  }).then(t.step_func(function() {

5000ms is still in active duration.  We should set the currentTime to 10000?

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:81
(Diff revision 1)
> +}, 'set endDelay and check onfinish event');

onfinish event is not fired duration endDelay.

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:92
(Diff revision 1)
> +  anim.onfinish = t.step_func_done(function(event) {
> +    assert_equals(event.timelineTime, finishedTimelineTime,
> +      'event.timelineTime should equal to the animation timeline ' +
> +      'when finished promise is resolved');
> +  });
> +
> +  anim.ready.then(function() {
> +    anim.currentTime = 11000;
> +    return waitForAnimationFrames(2);
> +  }).then(t.step_func(function() {
> +    t.done();

Hmm, now I am confused.  I might be misunderstanding the spec.  I thought finish event is fired after endTime.  In this test case the event will be fired over 13000ms.  Am I wrong?

::: testing/web-platform/tests/web-animations/animation-effect-timing/getAnimations.html:32
(Diff revision 1)
> +  anim.effect.timing.endDelay = -3000;
> +  assert_equals(div.getAnimations().length, 0, 'set negative endDelay');

This description (and others) is not easy to understand.
'set negative endDelay so as endTime is less than currentTime'?

::: testing/web-platform/tests/web-animations/animation-effect-timing/getAnimations.html:35
(Diff revision 1)
> +  assert_equals(div.getAnimations()[0], anim, 'set positive endDelay');
> +
> +  anim.effect.timing.duration = 1000;
> +  anim.currentTime = 1500;
> +  assert_equals(div.getAnimations().length, 0, 'set positive endDelay');

Please use different description for different assertion respectively.  It will be hard to track once test fails.

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:44
(Diff revision 1)
> +}, 'change currentTime when fill none and endDelay is positive');

fill:none and positive endDelay? Or fill is none and endDelay is positive.

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:67
(Diff revision 1)
> +  anim.currentTime = 10000;
> +  assert_equals(getComputedStyle(div).opacity, '0.9',
> +                'set currentTime before endTime');
> +
> +  anim.currentTime = 100000;
> +  assert_equals(getComputedStyle(div).opacity, '1',
> +                'set currentTime after endTime');

We should check the style at 50000ms and 99999ms?
And if we don't have any asynchronouss tests here and elsewhere we should use shorter durations.  That will be easier to understand.

::: testing/web-platform/tests/web-animations/animation-effect-timing/getComputedStyle.html:83
(Diff revision 1)
> +  anim.currentTime = 10000;
> +  assert_equals(getComputedStyle(div).opacity, '0.9',
> +                'set currentTime before endTime');
> +
> +  anim.currentTime = 100000;
> +  assert_equals(getComputedStyle(div).opacity, '0',
> +                'set currentTime after endTime');

Same here.  We should check the style duration endDelay.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

https://reviewboard.mozilla.org/r/38077/#review34613

r=me with description changes and additional checks.

::: layout/style/test/file_animations_effect_timing_enddelay.html:45
(Diff revision 1)
> +  omta_is(div, "transform", { tx: 10 }, RunningOn.Compositor,
> +          "Animation is running on Compositor");

s/on Compositor/on compositor/g

::: layout/style/test/file_animations_effect_timing_enddelay.html:50
(Diff revision 1)
> +  omta_is(div, "transform", { tx: 10 }, RunningOn.Compositor,
> +          "Animation remains on Compositor");

Animation remains on compositor when endDelay is changed.

::: layout/style/test/file_animations_effect_timing_enddelay.html:55
(Diff revision 1)
> +  omta_is(div, "transform", { tx: 0 }, RunningOn.Mainthread,
> +          "Animation is updated on Mainthread");

s/on MainThread/on the main thread/g

::: layout/style/test/file_animations_effect_timing_enddelay.html:67
(Diff revision 1)
> +
> +  advance_clock(500);
> +  yield waitForPaints();
> +  omta_is(div, "transform", { tx: 0 }, RunningOn.Mainthread,
> +          "Animation is updated on Mainthread" +
> +          "duration 1000, endDelay -500, fill none, current time 500");
> +
> +  advance_clock(400);

We should check the result at 400ms, just in case.

::: layout/style/test/file_animations_effect_timing_enddelay.html:109
(Diff revision 1)
> +
> +  advance_clock(500);
> +  yield waitForPaints();
> +  omta_is(div, "transform", { tx: 100 }, RunningOn.Mainthread,
> +          "Animation is updated on Mainthread " +
> +          "duration 1000, endDelay -500, fill forwards, current time 500");

Check the result at 400ms.

Comment 38

[Ryo Motozawa [:ryo]]

Comment on attachment 8726544 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38073/diff/1-2/

Comment 39

[Ryo Motozawa [:ryo]]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38075/diff/1-2/

Comment 40

[Ryo Motozawa [:ryo]]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38077/diff/1-2/

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

https://reviewboard.mozilla.org/r/38075/#review34619

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:76
(Diff revisions 1 - 2)
>    anim.onfinish = t.step_func_done(function(event) {
>      assert_equals(event.timelineTime, finishedTimelineTime,
>        'event.timelineTime should equal to the animation timeline ' +
>        'when finished promise is resolved');
>    });
>  
>    anim.ready.then(function() {
> -    anim.currentTime = 11000;
> +    anim.currentTime = 110000;
>      return waitForAnimationFrames(2);
>    }).then(t.step_func(function() {
>      t.done();
>    }));

I am still confused..  Now do we receive the onfinish event after endTime?  It seems that 110000 is still before endTime.  You will need to move 't.done' into onfinish callback.

::: testing/web-platform/tests/web-animations/animation-effect-timing/getAnimations.html:45
(Diff revisions 1 - 2)
>    assert_equals(div.getAnimations()[0], anim, 'set currentTime 400');
>    anim.currentTime = 500;
>    assert_equals(div.getAnimations().length, 0, 'set currentTime 500');
>    anim.currentTime = 1000;
>    assert_equals(div.getAnimations().length, 0, 'set currentTime 1000');

These assertions need intuitive description too.

Comment 42

[Ryo Motozawa [:ryo]]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38075/diff/2-3/

Comment 43

[Ryo Motozawa [:ryo]]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38077/diff/2-3/

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

https://reviewboard.mozilla.org/r/38075/#review34627

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:72
(Diff revisions 1 - 3)
>    anim.finished.then(function() {
>      finishedTimelineTime = anim.timeline.currentTime;
>    });
>  
> -  anim.onfinish = t.step_func_done(function(event) {
> +  anim.onfinish = t.step_func(function(event) {
>      assert_equals(event.timelineTime, finishedTimelineTime,
>        'event.timelineTime should equal to the animation timeline ' +
>        'when finished promise is resolved');
> +    t.done();
>    });
>  
>    anim.ready.then(function() {
> -    anim.currentTime = 11000;
> -    return waitForAnimationFrames(2);
> -  }).then(t.step_func(function() {
> +    anim.currentTime = 130000;
> +  });
> +}, 'onfinish event is fired currentTime is after endTime');

Yes!  Now the assertion in onfinish is surely executed.  But one thing I am still concered is that 'when?'.
You are using the time obtained by finished promise. But 
there is no gurantee that the time is after endTime.

Comment 45

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/38075/#review34629

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:75
(Diff revisions 2 - 3)
>  
> -  anim.onfinish = t.step_func_done(function(event) {
> +  anim.onfinish = t.step_func(function(event) {
>      assert_equals(event.timelineTime, finishedTimelineTime,
>        'event.timelineTime should equal to the animation timeline ' +
>        'when finished promise is resolved');
> +    t.done();

Ah I am sorry, you don't need to replace step_func_done by step_func. step_func_done calls t.done at the end.

Comment 46

[Ryo Motozawa [:ryo]]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38075/diff/3-4/

Comment 47

[Ryo Motozawa [:ryo]]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38077/diff/3-4/

Comment 48

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

https://reviewboard.mozilla.org/r/38075/#review34635

r=me with the change.

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:76
(Diff revision 4)
> +  anim.onfinish = t.step_func_done(function(event) {
> +    assert_equals(event.timelineTime, finishedTimelineTime,
> +      'event.timelineTime should equal to the animation timeline ' +
> +      'when finished promise is resolved');
> +  });
> +
> +  anim.ready.then(function() {
> +    anim.currentTime = 110000;
> +    return waitForAnimationFrames(2);
> +  }).then(t.step_func(function() {
> +    assert_equals(anim.playState, 'running',
> +      'animation should be running play state' +
> +      'when currentTime is during endDelay');
> +    anim.currentTime = 130000;
> +  }));

Unfortunately this test finishes even if the onfinish event is fired before endTime.  My last comment must be confusable.

To fix this properly:

var receivedEvents = [];
anim.onfinish = function(event) {
  receivedEvents.push(event);
}

anim.ready.then(function() {
  anim.currentTime = 110000; // during endDelay
  return waitFor...
}).then(t.step_func(function() {
  assert_equals(receivedEvents.length, 0,.. // we have assert_empty_array?
  anim.currentTime = 130000; // after endTime
  return waitFor...
}).then(t.step_func.done(function() {
  assert_equals( // check the finished time
});

Comment 49

[Ryo Motozawa [:ryo]]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38075/diff/4-5/

Comment 50

[Ryo Motozawa [:ryo]]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38077/diff/4-5/

Comment 51

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/38075/#review34641

::: testing/web-platform/tests/web-animations/animation-effect-timing/endDelay.html:90
(Diff revisions 4 - 5)
> +  })).then(t.step_func_done(function() {
> +    assert_equals(receivedEvents[0].timelineTime, finishedTimelineTime,
> +      'receivedEvents[0].timelineTime should equal to the animation timeline '
> +      + 'when finished promise is resolved');

We should check that the length of the array equals actually to 1 before checking the time.  If there are two or more events, it's a bug. ;-)

Anyway now you can push these patches to try server through reviewboard. Yay!

Comment 52

[Ryo Motozawa [:ryo]]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38075/diff/5-6/

Comment 53

[Ryo Motozawa [:ryo]]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38077/diff/5-6/

Comment 54

[Ryo Motozawa [:ryo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=045a7c63e77e

Comment 55

[Hiroyuki Ikezoe (:hiro)]

A test in test_animations_effect_timing_enddelay.html failed:
https://treeherder.mozilla.org/logviewer.html#?job_id=17609538&repo=try

Comment 56

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Updated spec: https://github.com/w3c/web-animations/commit/05de0d1e27a25feed57c185c534ee2ddd1c28151

Comment 57

[Ryo Motozawa [:ryo]]

Comment on attachment 8725155 [details]
MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37351/diff/6-7/

Comment 58

[Ryo Motozawa [:ryo]]

Comment on attachment 8726544 [details]
MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38073/diff/2-3/

Comment 59

[Ryo Motozawa [:ryo]]

Comment on attachment 8726545 [details]
MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38075/diff/6-7/

Comment 60

[Ryo Motozawa [:ryo]]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38077/diff/6-7/

Comment 61

[Ryo Motozawa [:ryo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b02a9ecdd8d3

Comment 62

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

https://reviewboard.mozilla.org/r/38077/#review35237

::: layout/style/test/file_animations_effect_timing_enddelay.html:102
(Diff revisions 6 - 7)
>    advance_clock(1500);
> -  omta_is(div, "transform", { tx: 100 }, RunningOn.Compositor,
> -          "Animation is updated on compositor when fill forwards");
> +  yield waitForPaints();
> +  omta_is(div, "transform", { tx: 100 }, RunningOn.MainThread,

Could you please explain why the previous check did not fail on non-E10S and why it failed on E10S?

Comment 63

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #62)
> Comment on attachment 8726546 [details]
> MozReview Request: Bug 1244635 - Part4 Add enddelay tests in
> layout/style/test r=hiro
> 
> https://reviewboard.mozilla.org/r/38077/#review35237
> 
> ::: layout/style/test/file_animations_effect_timing_enddelay.html:102
> (Diff revisions 6 - 7)
> >    advance_clock(1500);
> > -  omta_is(div, "transform", { tx: 100 }, RunningOn.Compositor,
> > -          "Animation is updated on compositor when fill forwards");
> > +  yield waitForPaints();
> > +  omta_is(div, "transform", { tx: 100 }, RunningOn.MainThread,
> 
> Could you please explain why the previous check did not fail on non-E10S and
> why it failed on E10S?

I can add some explanation since Motozawa-san and I looked at it together.

Forwards-fill should not be performed on the compositor so this test should have failed everywhere. However, when using just advance_clock() on non-e10s, we don't have an opportunity to synchronize with the compositor and remove the animation from the compositor. By default compositor animations fill forwards until they are removed, hence this test passed when it should have failed.

If we add a waitForPaints(), however, it fails (as expected) on non-e10s since we remove the filling animation from the compositor.

On e10s, however, I believe paints are driven by a different refresh driver than the one we have under test control and so it is possible that we will take the animation off the compositor even without waiting for paints. You're more knowledgeable about this than me however!

Comment 64

[Ryo Motozawa [:ryo]]

https://reviewboard.mozilla.org/r/38077/#review35237

> Could you please explain why the previous check did not fail on non-E10S and why it failed on E10S?

When we are in E10S environment, probably animation running on compositor sends finish notification when it reach the end point of active interval but when non-E10S environment not.

Comment 65

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #63)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #62)
> > Comment on attachment 8726546 [details]
> > MozReview Request: Bug 1244635 - Part4 Add enddelay tests in
> > layout/style/test r=hiro
> > 
> > https://reviewboard.mozilla.org/r/38077/#review35237
> > 
> > ::: layout/style/test/file_animations_effect_timing_enddelay.html:102
> > (Diff revisions 6 - 7)
> > >    advance_clock(1500);
> > > -  omta_is(div, "transform", { tx: 100 }, RunningOn.Compositor,
> > > -          "Animation is updated on compositor when fill forwards");
> > > +  yield waitForPaints();
> > > +  omta_is(div, "transform", { tx: 100 }, RunningOn.MainThread,
> > 
> > Could you please explain why the previous check did not fail on non-E10S and
> > why it failed on E10S?
> 
> I can add some explanation since Motozawa-san and I looked at it together.
> 
> Forwards-fill should not be performed on the compositor so this test should
> have failed everywhere. However, when using just advance_clock() on
> non-e10s, we don't have an opportunity to synchronize with the compositor
> and remove the animation from the compositor. By default compositor
> animations fill forwards until they are removed, hence this test passed when
> it should have failed.
> 
> If we add a waitForPaints(), however, it fails (as expected) on non-e10s
> since we remove the filling animation from the compositor.
> 
> On e10s, however, I believe paints are driven by a different refresh driver
> than the one we have under test control and so it is possible that we will
> take the animation off the compositor even without waiting for paints.
> You're more knowledgeable about this than me however!

Ah, that's it!  I filed bug 1254381.

Comment 66

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726546 [details]
MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro

https://reviewboard.mozilla.org/r/38077/#review35245

Comment 67

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Adding checkin-needed on behalf of Ryo because he doesn't have editbugs privilege yet.

Comment 68

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46217739afcf
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5a1593c8788
https://hg.mozilla.org/integration/mozilla-inbound/rev/c167a14936ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9234d56ef2

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46217739afcf
https://hg.mozilla.org/mozilla-central/rev/e5a1593c8788
https://hg.mozilla.org/mozilla-central/rev/c167a14936ac
https://hg.mozilla.org/mozilla-central/rev/1a9234d56ef2