Closed
Bug 1244635
Opened 9 years ago
Closed 9 years ago
implement AnimationEffectTiming endDelay
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: motozawa, Assigned: motozawa)
References
()
Details
Attachments
(5 files, 4 obsolete files)
No description provided.
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
I spoke to Google and they agreed with the proposed changes, assuming they don't break the rest of the model.
Comment 4•9 years ago
|
||
I implement a setter for AnimationEffectTiming endDelay.
Attachment #8725141 -
Flags: review?(hiikezoe)
Comment 5•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37351/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37351/
Attachment #8725155 -
Flags: review?(hiikezoe)
Comment 6•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37353/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37353/
Attachment #8725158 -
Flags: review?(hiikezoe)
Comment 7•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37355/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37355/
Attachment #8725159 -
Flags: review?(hiikezoe)
Comment 8•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37357/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37357/
Attachment #8725160 -
Flags: review?(hiikezoe)
Comment 9•9 years ago
|
||
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.
Attachment #8725155 -
Flags: review?(hiikezoe)
Updated•9 years ago
|
Attachment #8725159 -
Flags: review?(hiikezoe)
Comment 10•9 years ago
|
||
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•9 years ago
|
||
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?
Attachment #8725158 -
Flags: review?(hiikezoe)
Comment 12•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
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
Attachment #8725160 -
Flags: review?(hiikezoe)
Comment 15•9 years ago
|
||
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/
Attachment #8725155 -
Flags: review?(hiikezoe)
Comment 16•9 years ago
|
||
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/
Attachment #8725158 -
Attachment description: MozReview Request: Bug 1244635 - Part2 Add duration tests in dom/animation/test/chrome r?hiro → MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r?hiro
Attachment #8725158 -
Flags: review?(hiikezoe)
Updated•9 years ago
|
Attachment #8725159 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8725160 -
Attachment is obsolete: true
Comment 17•9 years ago
|
||
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?
Attachment #8725155 -
Flags: review?(hiikezoe)
Comment 18•9 years ago
|
||
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?
Attachment #8725158 -
Flags: review?(hiikezoe)
Comment 19•9 years ago
|
||
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/
Attachment #8725155 -
Flags: review?(hiikezoe)
Updated•9 years ago
|
Attachment #8725158 -
Flags: review?(hiikezoe)
Comment 20•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8725155 -
Flags: review?(hiikezoe) → review+
Comment 22•9 years ago
|
||
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.
Attachment #8725158 -
Flags: review?(hiikezoe) → review+
Comment 23•9 years ago
|
||
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/
Attachment #8725155 -
Attachment description: MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r?hiro → MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro
Comment 24•9 years ago
|
||
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/
Attachment #8725158 -
Attachment description: MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r?hiro → MozReview Request: Bug 1244635 - Part2 Add enddelay tests in dom/animation/test/chrome r=hiro
Updated•9 years ago
|
Attachment #8725155 -
Flags: review?(bzbarsky)
Comment 25•9 years ago
|
||
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.
Attachment #8725155 -
Flags: review?(bzbarsky) → review+
Comment 26•9 years ago
|
||
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/
Attachment #8725155 -
Attachment description: MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro → MozReview Request: Bug 1244635 - Part1 Add enddelay implementation in dom/animation/AnimationEffectTiming.cpp r=hiro r=bz
Updated•9 years ago
|
Attachment #8725158 -
Attachment is obsolete: true
Comment 27•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8725141 -
Attachment is obsolete: true
Attachment #8725141 -
Flags: review?(hiikezoe)
Comment 30•9 years ago
|
||
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•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38073/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38073/
Attachment #8726544 -
Flags: review?(hiikezoe)
Comment 32•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38075/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38075/
Attachment #8726545 -
Flags: review?(hiikezoe)
Comment 33•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38077/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38077/
Attachment #8726546 -
Flags: review?(hiikezoe)
Comment 34•9 years ago
|
||
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 ...
Attachment #8726544 -
Flags: review?(hiikezoe) → review+
Comment 35•9 years ago
|
||
(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•9 years ago
|
||
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.
Attachment #8726545 -
Flags: review?(hiikezoe)
Comment 37•9 years ago
|
||
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.
Attachment #8726546 -
Flags: review?(hiikezoe) → review+
Comment 38•9 years ago
|
||
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•9 years ago
|
||
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/
Attachment #8726545 -
Flags: review?(hiikezoe)
Comment 40•9 years ago
|
||
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/
Attachment #8726546 -
Attachment description: MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r?hiro → MozReview Request: Bug 1244635 - Part4 Add enddelay tests in layout/style/test r=hiro
Updated•9 years ago
|
Attachment #8726545 -
Flags: review?(hiikezoe)
Comment 41•9 years ago
|
||
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•9 years ago
|
||
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/
Attachment #8726545 -
Flags: review?(hiikezoe)
Comment 43•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8726545 -
Flags: review?(hiikezoe)
Comment 45•9 years ago
|
||
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•9 years ago
|
||
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/
Attachment #8726545 -
Flags: review?(hiikezoe)
Comment 47•9 years ago
|
||
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•9 years ago
|
||
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
});
Attachment #8726545 -
Flags: review?(hiikezoe) → review+
Comment 49•9 years ago
|
||
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/
Attachment #8726545 -
Attachment description: MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r?hiro → MozReview Request: Bug 1244635 - Part3 Add enddelay tests in testing/web-platform/tests/web-animations r=hiro
Comment 50•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Comment 55•9 years ago
|
||
A test in test_animations_effect_timing_enddelay.html failed:
https://treeherder.mozilla.org/logviewer.html#?job_id=17609538&repo=try
Comment 56•9 years ago
|
||
Status: NEW → ASSIGNED
Comment 57•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Comment 62•9 years ago
|
||
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?
Attachment #8726546 -
Flags: review+
Comment 63•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
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
Attachment #8726546 -
Flags: review+
Comment 67•9 years ago
|
||
Adding checkin-needed on behalf of Ryo because he doesn't have editbugs privilege yet.
Keywords: checkin-needed
Comment 68•9 years ago
|
||
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
Keywords: checkin-needed
Comment 69•9 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•