Closed
Bug 1248532
Opened 8 years ago
Closed 8 years ago
steps-start does not produce correct value at the beginning of the interval
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: birtles, Assigned: daisuke)
References
Details
Attachments
(7 files, 2 obsolete files)
426 bytes,
text/html
|
Details | |
4.49 KB,
patch
|
Details | Diff | Splinter Review | |
434 bytes,
text/html
|
Details | |
12.57 KB,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
For a steps(start) timing function, we should step up *at* the x=0 point but instead we produce y=0 at that point. In the attached test case, there should be blue box visible but instead nothing is visible. In Chrome the blue box is visible. There's a comment in the code which indicates we took advantage of the fact that the spec is unclear at this point to do what was most convenient for us.[1] [1] https://dxr.mozilla.org/mozilla-central/rev/ea39d4a6232c278dd8d805608a07cf9f4cc4c76b/dom/animation/ComputedTimingFunction.cpp#40
Reporter | ||
Comment 1•8 years ago
|
||
I notice that Edge does the same as us here.
I think what we implement is the traditional way to define a step function, and I think the diagrams in the spec (which have no corresponding prose) seem wrong to me.
Comment 3•8 years ago
|
||
I also prefer current our implementation, it is contrastive.
Comment 4•8 years ago
|
||
For what it is worth, this is a patch to match steps(start) to the diagrams in the spec.
Reporter | ||
Comment 5•8 years ago
|
||
Hmm, it seems inconsistent to me that for a 2-step function at x=0.5 we're at the top of the second step but at x=0 we're not at the top of the first step. Everywhere else in the timing model we're endpoint exclusive so I don't see why this should be an exception. We do, however, need to make sure we are at progress zero when we fill backwards (not the top of the first step).
Comment 6•8 years ago
|
||
After writing a fix for bug 1223249 (using steps(end) with reverse instead of steps(start)), I have changed my mind. The steps(start) in our current implementation can be easily replaced, but the steps(start) on the diagrams in the spec seems not to be easily replaced (need iterationStart?). CSS transition does not have 'reverse', of course. (In reply to Brian Birtles (:birtles) from comment #5) > Hmm, it seems inconsistent to me that for a 2-step function at x=0.5 we're > at the top of the second step but at x=0 we're not at the top of the first > step. I did not notice it.
Reporter | ||
Comment 7•8 years ago
|
||
Reporter | ||
Comment 8•8 years ago
|
||
Attachment #8720048 -
Attachment is obsolete: true
Reporter | ||
Comment 9•8 years ago
|
||
From my testing: During delay: Chrome: Output = Top of first step Firefox, Edge, Safari: Output = Bottom of first step At t=0: Chrome: Output = Top of first step Firefox, Edge, Safari: Output = Bottom of first step According to the spec, during the delay phase we should actually produce the bottom of the first step.[1] I'm surprised Chrome doesn't seem to be doing this since I think they were the ones who asked for this to be added to the spec. [1] http://w3c.github.io/web-animations/#step-timing-function (although this spec text has a clearly buggy sentence, "when a step timing function is applied to an animation effect or applied to an animation effect associated with an animation effect")
Comment 10•8 years ago
|
||
This patch does not contain any test cases for cubic-bezier yet, but I believe it is worthwhile.
Comment 11•8 years ago
|
||
The previous patch had a change against ComputedTimingFunction.
Attachment #8720121 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → daisuke
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7170de14b36
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42623/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42623/
Attachment #8735172 -
Flags: review?(bbirtles)
Attachment #8735173 -
Flags: review?(bbirtles)
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42625/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42625/
Comment 15•8 years ago
|
||
https://reviewboard.mozilla.org/r/42625/#review39115 ::: layout/style/test/file_animations_effect_timing_duration.html:52 (Diff revision 1) > + // Setter of timing option should set up the changes to animations for the > + // next layer transaction but it won't schedule a paint immediately so we need > + // to tick the refresh driver before we can wait on the next paint. > + advance_clock(0); > > yield waitForPaints(); IMO, we should call advance_clock(0) inside waitForPaints if we are under test control.
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8735172 [details] MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42623/diff/1-2/
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8735173 [details] MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42625/diff/1-2/
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0db1289e2443
Assignee | ||
Comment 19•8 years ago
|
||
Now, the new patch failed to compile on Linux. So, please wait to review.
Comment 20•8 years ago
|
||
You should undef True and False in ComputedTimingFunction.h
Assignee | ||
Comment 21•8 years ago
|
||
Thank you for your advice, Hiro!
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e755343fa45
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8735172 [details] MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42623/diff/2-3/
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8735173 [details] MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42625/diff/2-3/
Reporter | ||
Comment 25•8 years ago
|
||
Comment on attachment 8735172 [details] MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles https://reviewboard.mozilla.org/r/42623/#review39393 This looks good but I don't think it handles steps-end correctly. ::: dom/animation/ComputedTimingFunction.h:23 (Diff revision 3) > + // StepTimingBeforeFlag is used in step timing function. > + // http://w3c.github.io/web-animations/#step-timing-function > + enum class StepTimingBeforeFlag { > + No, > + Yes > + }; How about calling this just "BeforeFlag" and making the values "Set" and "Unset"? ::: dom/animation/ComputedTimingFunction.cpp:105 (Diff revision 3) > - // up to zero, so we can't do that. And it's not clear the spec > - // really meant it. > - return 1.0 - StepEnd(mSteps, 1.0 - aPortion); > } > MOZ_ASSERT(mType == nsTimingFunction::Type::StepEnd, "bad type"); > return StepEnd(mSteps, aPortion); I think we need to use the before flag when evaluating StepEnd too such as when you have iterationStart = 0.5. Maybe once we do that we can combine the implementation of StepStart and StepEnd somehow.
Attachment #8735172 -
Flags: review?(bbirtles)
Reporter | ||
Comment 26•8 years ago
|
||
Comment on attachment 8735173 [details] MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r=birtles https://reviewboard.mozilla.org/r/42625/#review39397 This is good but we still lack the following tests: * With iterationStart = 0.5 and steps(2, start) -- i.e. the second step lines up with the beginning * The above but in reverse * With iterationStart = 0.75 and steps(4, start) -- i.e. the fourth step lines up with the beginning * With iterationStart = 0.5 and steps(2, end) * With direction alternate and alternate-reverse * With the step timing function applied to a keyframe * With the step timing function applied to both the effect and a keyframe ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:446 (Diff revision 3) > + { width: '100px' } ], > + { delay: 1000, > + duration: 1000, > + fill: 'both', > + easing: 'steps(2, start)' }); > + anim.pause(); I think pausing isn't actually necessary here. ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:448 (Diff revision 3) > + duration: 1000, > + fill: 'both', > + easing: 'steps(2, start)' }); > + anim.pause(); > + anim.currentTime = 0; > + assert_equals(getComputedStyle(target).width, '0px'); I think it would be better to test the getComputedTiming().progress value instead. (That also makes it easier to differentiate between the width being 0px due to the backwards fill as opposed to 0px because that's the specified style.) ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:449 (Diff revision 3) > + fill: 'both', > + easing: 'steps(2, start)' }); > + anim.pause(); > + anim.currentTime = 0; > + assert_equals(getComputedStyle(target).width, '0px'); > + anim.currentTime = 1000; Should we test 999 as well? ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:451 (Diff revision 3) > + anim.pause(); > + anim.currentTime = 0; > + assert_equals(getComputedStyle(target).width, '0px'); > + anim.currentTime = 1000; > + assert_equals(getComputedStyle(target).width, '50px'); > + anim.currentTime = 1500; Should we test 1499? ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:457 (Diff revision 3) > +test(function(t) { > + var target = createDiv(t); > + var anim = target.animate([ { opacity: 0 }, > + { opacity: 1 } ], > + { delay: 1000, > + duration: 1000, > + fill: 'both', > + easing: 'steps(2, start)' }); > + anim.pause(); > + anim.currentTime = 0; > + assert_equals(getComputedStyle(target).opacity, '0'); > + anim.currentTime = 1000; > + assert_equals(getComputedStyle(target).opacity, '0.5'); > + anim.currentTime = 1500; > + assert_equals(getComputedStyle(target).opacity, '1'); > + anim.currentTime = 2000; > + assert_equals(getComputedStyle(target).opacity, '1'); > +}, 'Test bounds point of step-start easing with compositor'); I don't think this test actually adds anything. getComputedStyle() doesn't query the compositor. ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:490 (Diff revision 3) > + anim.pause(); > + anim.currentTime = 0; > + assert_equals(getComputedStyle(target).width, '100px'); > + anim.currentTime = 1001; > + assert_equals(getComputedStyle(target).width, '100px'); > + anim.currentTime = 1501; Add a test for 1500? ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:492 (Diff revision 3) > + assert_equals(getComputedStyle(target).width, '100px'); > + anim.currentTime = 1001; > + assert_equals(getComputedStyle(target).width, '100px'); > + anim.currentTime = 1501; > + assert_equals(getComputedStyle(target).width, '50px'); > + anim.currentTime = 2000; Add a test for 2500 (just in case an implementation special-cases the boundary condition) ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:495 (Diff revision 3) > + > + Nit: We only need one blank line here.
Attachment #8735173 -
Flags: review?(bbirtles)
Assignee | ||
Comment 27•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43181/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43181/
Attachment #8735173 -
Attachment description: MozReview Request: Bug 1248532 - Part 2: Add tests r?birtles → MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r?birtles
Attachment #8736235 -
Flags: review?(bbirtles)
Attachment #8735172 -
Flags: review?(bbirtles)
Attachment #8735173 -
Flags: review?(bbirtles)
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8735172 [details] MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42623/diff/3-4/
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8735173 [details] MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42625/diff/3-4/
Assignee | ||
Comment 30•8 years ago
|
||
Now, try server is closing. I'll push to try server later.
Assignee | ||
Comment 31•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #26) > Comment on attachment 8735173 [details] > MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r?birtles > > https://reviewboard.mozilla.org/r/42625/#review39397 > > This is good but we still lack the following tests: > > * With iterationStart = 0.5 and steps(2, end) I tested this condition. It looks work well without any modifying for step-end function.
Assignee | ||
Comment 32•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e94d18e3ed67
Reporter | ||
Comment 33•8 years ago
|
||
Comment on attachment 8735172 [details] MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles https://reviewboard.mozilla.org/r/42623/#review39973 As discussed, in the test case added in part 3, the progress should be zero during the delay. So I think we need to update StepEnd. Cancelling this review for the time being. ::: dom/animation/ComputedTimingFunction.h:19 (Diff revision 4) > > class ComputedTimingFunction > { > public: > + // BeforeFlag is used in step timing function. > + // http://w3c.github.io/web-animations/#step-timing-function Nit: https Also, I think we can link to: https://w3c.github.io/web-animations/#before-flag
Attachment #8735172 -
Flags: review?(bbirtles)
Reporter | ||
Updated•8 years ago
|
Attachment #8735173 -
Flags: review?(bbirtles) → review+
Reporter | ||
Comment 34•8 years ago
|
||
Comment on attachment 8735173 [details] MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r=birtles https://reviewboard.mozilla.org/r/42625/#review39977 This looks good. r=me with comments addressed, particularly extracting out extra functions to make these tests easier to read. ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:514 (Diff revisions 3 - 4) > + anim.currentTime = 0; > + assert_equals(anim.effect.getComputedTiming().progress, 0.5); > + anim.currentTime = 999; > + assert_equals(anim.effect.getComputedTiming().progress, 0.5); > + anim.currentTime = 1000; > + assert_equals(anim.effect.getComputedTiming().progress, 1); > + anim.currentTime = 1499; > + assert_equals(anim.effect.getComputedTiming().progress, 1); > + anim.currentTime = 1500; > + assert_equals(anim.effect.getComputedTiming().progress, 0.5); > + anim.currentTime = 2000; We have this pattern a lot in this file. Can we extract a function that takes an array like: [ 0, 0.5 ], [ 999, 0.5 ], [ 1000, 1 ], [ 1499, 1 ], ... And then steps through the array and calls currentTime and assert_equals? When we call assert_equals we can add a message like: 'Progress at ' + time + 'ms' ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:561 (Diff revisions 3 - 4) > + > +test(function(t) { > + var target = createDiv(t); > + var anim = target.animate([ { width: '0px' }, > + { width: '100px' } ], > + { duration: 1000, Oh, I think what I wanted to test here was the delay phase. i.e. it should be 0.75 during the delay. ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:570 (Diff revisions 3 - 4) > + assert_equals(anim.effect.getComputedTiming().progress, 0.25); > + anim.currentTime = 499; > + assert_equals(anim.effect.getComputedTiming().progress, 0.25); > + anim.currentTime = 500; > + assert_equals(anim.effect.getComputedTiming().progress, 0.5); > + anim.currentTime = 749; > + assert_equals(anim.effect.getComputedTiming().progress, 0.5); > + anim.currentTime = 750; > + assert_equals(anim.effect.getComputedTiming().progress, 0.75); > + anim.currentTime = 999; > + assert_equals(anim.effect.getComputedTiming().progress, 0.75); > + anim.currentTime = 1000; > + assert_equals(anim.effect.getComputedTiming().progress, 1); We probably don't need this part. ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:591 (Diff revisions 3 - 4) > + { duration: 1000, > + iterations: 2, > + direction: 'alternate', > + fill: 'both', > + easing: 'steps(2, start)' }); Sorry, I wasn't clear. I think what I want to test is something like: delay: 1000, iterations: 2, iterationStart: 1.5, direction: 'alternate', fill: 'both', easing: 'steps(2, start)' Then just check that we do the right thing during the backwards and forwards fill. ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:618 (Diff revisions 3 - 4) > + { duration: 1000, > + iterations: 2, > + direction: 'alternate-reverse', > + fill: 'both', > + easing: 'steps(2, start)' }); Here too, I think I was thinking more about: { delay: 1000, duration: 1000, iterations: 2, iterationStart: 0.5, direction: 'alternate-reverse', fill: 'both', easing: 'steps(2, start)' } And checking we do the right thing in the forwards and backwards fill. ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:650 (Diff revisions 3 - 4) > + assert_equals(getComputedStyle(target).width, '0px'); > + assert_equals(anim.effect.getComputedTiming().progress, 0); > + anim.currentTime = 999; > + assert_equals(getComputedStyle(target).width, '0px'); > + assert_equals(anim.effect.getComputedTiming().progress, 0); > + anim.currentTime = 1000; > + assert_equals(getComputedStyle(target).width, '50px'); > + assert_equals(anim.effect.getComputedTiming().progress, 0); > + anim.currentTime = 1499; > + assert_equals(getComputedStyle(target).width, '50px'); > + assert_equals(anim.effect.getComputedTiming().progress, 0.499); > + anim.currentTime = 1500; > + assert_equals(getComputedStyle(target).width, '100px'); > + assert_equals(anim.effect.getComputedTiming().progress, 0.5); > + anim.currentTime = 2000; > + assert_equals(getComputedStyle(target).width, '100px'); > + assert_equals(anim.effect.getComputedTiming().progress, 1); > + anim.currentTime = 2500; This is good, but I guess we can just extract another function that takes an array with: [ 0, '0px' ], [ 999, '0px' ], [ 1000, '50px' ], ... ? (I'm not sure we need to test the progress in this case) ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:674 (Diff revisions 3 - 4) > + var anim = target.animate([ { width: '0px', easing: 'linear' }, > + { width: '100px'} ], > + { delay: 1000, > + duration: 1000, > + fill: 'both', > + easing: 'steps(2, start)' }); I'm not sure using easing: 'linear' really helps here (it's the default). Let's just drop this test.
Reporter | ||
Comment 35•8 years ago
|
||
Comment on attachment 8736235 [details] MozReview Request: Bug 1248532 - Part 3: add a test for step-end with iterationStart. r=birtles https://reviewboard.mozilla.org/r/43181/#review39981 r=me with the following change made and the test converted to use whatever convenience function we create in part 2. ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:712 (Diff revision 1) > + anim.currentTime = 0; > + assert_equals(anim.effect.getComputedTiming().progress, 0.5); > + anim.currentTime = 999; > + assert_equals(anim.effect.getComputedTiming().progress, 0.5); As discussed, I think progress should be 0 here.
Attachment #8736235 -
Flags: review?(bbirtles)
Assignee | ||
Comment 36•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=483aea1a1a6f
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8735172 [details] MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42623/diff/4-5/
Attachment #8735173 -
Attachment description: MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r?birtles → MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r=birtles
Attachment #8736235 -
Attachment description: MozReview Request: Bug 1248532 - Part 3: add a test for step-end with iterationStart. r?birtles → MozReview Request: Bug 1248532 - Part 3: add a test for step-end with iterationStart. r=birtles
Attachment #8735172 -
Flags: review?(bbirtles)
Attachment #8736235 -
Flags: review?(bbirtles)
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8735173 [details] MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42625/diff/4-5/
Assignee | ||
Comment 39•8 years ago
|
||
Comment on attachment 8736235 [details] MozReview Request: Bug 1248532 - Part 3: add a test for step-end with iterationStart. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43181/diff/1-2/
Reporter | ||
Comment 40•8 years ago
|
||
Comment on attachment 8735172 [details] MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles https://reviewboard.mozilla.org/r/42623/#review40203 r=me with the simplifications to the StepTiming function I proposed (assuming they work--or something similar to that) ::: dom/animation/ComputedTimingFunction.cpp:36 (Diff revision 5) > + if (aPortion == 1.0) { > + return 1.0; > + } > + > + bool isBeforeTransaction = > + aBeforeFlag == ComputedTimingFunction::BeforeFlag::Set && > + (aPortion == 0 || fmod(aSteps * aPortion, 1) == 0); // is transition point ? > + > + uint32_t step = uint32_t(aPortion * aSteps) + > + (aType == nsTimingFunction::Type::StepStart ? 1 : 0); > + > + if (isBeforeTransaction && step > 0) { > + return double(step - 1) / double(aSteps); > + } > return double(step) / double(aSteps); I wonder if we can make this whole function simpler and easier to read and debug by calculating 'step' first like so: // Calculate current step using step-end behavior uint32_t step = uint32_t(aPortion * aSteps); // floor // step-start is one step ahead if (aType == nsTimingFunction::Type::StepStart) { step++; } // If the "before flag" is set and we are at a transition point, // drop back a step (but only if we are not already at the zero point-- // we do this clamping here since |step| is an unsigned integer) if (step != 0 && aBeforeFlag == ComputedTimingFunction::BeforeFlag::Set && fmod(aPortion * aSteps, 1) == 0) { step--; } // Don't step up or down if we are already at the limit step = std::min(step, 1); // Convert to a progress value return double(step) / double(aSteps); Does that work? ::: dom/animation/ComputedTimingFunction.cpp:40 (Diff revision 5) > + > + if (aPortion == 1.0) { > + return 1.0; > + } > + > + bool isBeforeTransaction = isBeforeStep? ::: dom/animation/ComputedTimingFunction.cpp:42 (Diff revision 5) > + return 1.0; > + } > + > + bool isBeforeTransaction = > + aBeforeFlag == ComputedTimingFunction::BeforeFlag::Set && > + (aPortion == 0 || fmod(aSteps * aPortion, 1) == 0); // is transition point ? I think fmod(0, 1) == 0? So we could just drop the aPortion == 0 test? Also, a very minor nit, but later we have (aPortion * aSteps) and here we have (aSteps * aPortion). This could we be a little easier to read if we used the same order in both places, i.e. bool isBeforeStep = aBeforeFlag == ComputedTiming::BeforeFlag::Set && fmod(aPortion * aSteps) == 0; ::: dom/animation/ComputedTimingFunction.cpp:55 (Diff revision 5) > return double(step) / double(aSteps); > } > > double > -ComputedTimingFunction::GetValue(double aPortion) const > +ComputedTimingFunction::GetValue(double aPortion, > + ComputedTimingFunction::BeforeFlag aBeforeFlag) const Nit: If we are going to split the parameters over multiple lines, we should line them up. So normally you'd write: double ComputedTimingFunction::GetValue(double aPortion, ComputedTimingFunction::BeforeFlag aBeforeFlag) const But since that doesn't fit within 80 chars, in this case you'd do: double ComputedTimingFunction::GetValue( double aPortion, ComputedTimingFunction::BeforeFlag aBeforeFlag) const (i.e. recently I notice a lot of people use a 4-space indent for the parameters in this case) ::: dom/animation/KeyframeEffect.cpp:397 (Diff revision 5) > + result.mBeforeFlag = ComputedTimingFunction::BeforeFlag::Set; > + } > + > if (aTiming.mFunction) { > result.mProgress.SetValue( > - aTiming.mFunction->GetValue(result.mProgress.Value())); > + aTiming.mFunction->GetValue(result.mProgress.Value(), result.mBeforeFlag)); Nit: I think this might be just over 80 characters?
Attachment #8735172 -
Flags: review?(bbirtles) → review+
Reporter | ||
Comment 41•8 years ago
|
||
Comment on attachment 8736235 [details] MozReview Request: Bug 1248532 - Part 3: add a test for step-end with iterationStart. r=birtles https://reviewboard.mozilla.org/r/43181/#review40219 Looks great. Thanks! ::: testing/web-platform/tests/web-animations/keyframe-effect/effect-easing.html:676 (Diff revision 2) > + { currentTime: 2500, progress: 0.5 } > + ] > + }, > + { > + description: 'Test bounds point of step-end easing ' + > + 'with iterationStart of not transition point and delay', 'with iterationStart not at a transition point' (I don't think you need to mention the delay--it's just there to help us test the initial transition point)
Attachment #8736235 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 42•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33fb9e868918
Assignee | ||
Comment 43•8 years ago
|
||
Comment on attachment 8735172 [details] MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42623/diff/5-6/
Attachment #8735172 -
Attachment description: MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r?birtles → MozReview Request: Bug 1248532 - Part 1: steps-start does not produce correct value at the beginning of the interval. r=birtles
Assignee | ||
Comment 44•8 years ago
|
||
Comment on attachment 8735173 [details] MozReview Request: Bug 1248532 - Part 2: Add tests for step-start. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42625/diff/5-6/
Assignee | ||
Comment 45•8 years ago
|
||
Comment on attachment 8736235 [details] MozReview Request: Bug 1248532 - Part 3: add a test for step-end with iterationStart. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43181/diff/2-3/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 46•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/817264a13c52 https://hg.mozilla.org/integration/mozilla-inbound/rev/be1060e48a5a https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8bfb08d273
Keywords: checkin-needed
Comment 47•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/817264a13c52 https://hg.mozilla.org/mozilla-central/rev/be1060e48a5a https://hg.mozilla.org/mozilla-central/rev/fe8bfb08d273
Status: NEW → RESOLVED
Closed: 8 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
•