Closed Bug 1244638 Opened 4 years ago Closed 4 years ago

implement AnimationEffectTiming iterationStart

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: motozawa, Assigned: daisuke)

References

()

Details

Attachments

(3 files, 5 obsolete files)

No description provided.
Depends on: 1248338
Assignee: motozawa → daisuke
Attachment #8729215 - Flags: review?(bbirtles)
Comment on attachment 8729215 [details] [diff] [review]
Part1:implement AnimationEffectTiming iterationStart

Review of attachment 8729215 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I want to check the updated test.

::: dom/animation/AnimationEffectTiming.cpp
@@ +58,5 @@
>    NotifyTimingUpdate();
>  }
>  
> +void
> +AnimationEffectTiming::SetIterationStart(const double aIterationStart)

No need for 'const' here.

::: dom/animation/AnimationEffectTiming.h
@@ +26,5 @@
>  
>    void SetEndDelay(double aEndDelay);
>    void SetDuration(const UnrestrictedDoubleOrString& aDuration,
>                     ErrorResult& aRv);
> +  void SetIterationStart(const double aIterationStart);

Let's follow the order here:

http://w3c.github.io/web-animations/#animationeffecttiming

i.e. SetIterationStart goes before SetDuration and after SetEndDelay.

::: testing/web-platform/tests/web-animations/animation-effect-timing/iterationStart.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>iterationStart tests</title>
> +<link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-iterationstart">

Nit: https

@@ +30,5 @@
> +  anim.effect.timing.iterationStart = 2.7;
> +  assert_equals(anim.effect.timing.iterationStart, 2.7);
> +  assert_equals(anim.effect.getComputedTiming().progress, null);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, null);
> +}, 'Test progress during before and after phase when fill is none');

I think we can drop this test. It doesn't seem to be useful.

@@ +41,5 @@
> +                           fill: 'both',
> +                           duration: 100,
> +                           delay: 1 });
> +  anim.effect.timing.iterationStart = 2.5;
> +  assert_equals(anim.effect.timing.iterationStart, 2.5);

Drop this line. If the following two lines pass, we can assume this worked too.

@@ +44,5 @@
> +  anim.effect.timing.iterationStart = 2.5;
> +  assert_equals(anim.effect.timing.iterationStart, 2.5);
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.5);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, 2);
> +}, 'Test during before phase when fill is both');

'Test that changing the iterationStart affects computed timing when backwards-filling'

@@ +55,5 @@
> +                           fill: 'both',
> +                           duration: 100,
> +                           delay: 0 });
> +  anim.effect.timing.iterationStart = 2.5;
> +  assert_equals(anim.effect.timing.iterationStart, 2.5);

Drop this line.

@@ +58,5 @@
> +  anim.effect.timing.iterationStart = 2.5;
> +  assert_equals(anim.effect.timing.iterationStart, 2.5);
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.5);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, 2);
> +}, 'Test during active phase when fill is both');

'Test that changing the iterationStart affects computed timing during the active phase'

@@ +69,5 @@
> +                           fill: 'both',
> +                           duration: 100,
> +                           delay: 0 });
> +  anim.effect.timing.iterationStart = 2.5;
> +  anim.finish();

Reverse the order of these two lines. It's possible an implementation might fail to update timing until finish() is called or something affects the currentTime. By changing the iterationStart after calling finish() we check that the result is updated immediately.

@@ +73,5 @@
> +  anim.finish();
> +  assert_equals(anim.effect.timing.iterationStart, 2.5);
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.5);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, 3);
> +  anim.effect.timing.iterationStart = 0.7;

Why do we need this second test?

@@ +77,5 @@
> +  anim.effect.timing.iterationStart = 0.7;
> +  assert_equals(anim.effect.timing.iterationStart, 0.7);
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.7);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, 1);
> +}, 'Test during after phase when fill is both');

'Test that changing the iterationStart affects computed timing when forwards-filling'
Attachment #8729215 - Flags: review?(bbirtles)
Comment on attachment 8729216 [details] [diff] [review]
Part2: Throw TypeError if iterationStart is NaN, negative value or Infinity

Review of attachment 8729216 [details] [diff] [review]:
-----------------------------------------------------------------

Really minor nit: when numbering patches, please put a space between "part" and the number.

e.g. Bug 1244638 - Part 2: ....

::: dom/animation/AnimationEffectTiming.cpp
@@ +67,5 @@
>      return;
>    }
>    mTiming.mIterationStart = aIterationStart;
> +
> +  NotifyTimingUpdate();

(In a separate patch we should rename NotifyTimingUpdate to PostSpecifiedTimingUpdated to match how we use these terms elsewhere. PostSpecifiedTimingUpdated should be inline too.)

::: dom/animation/TimingParams.cpp
@@ +103,5 @@
>  }
>  
> +/* static */ bool
> +TimingParams::IsValidIterationStart(double aIterationStart, ErrorResult& aRv) {
> +  if (aIterationStart >= 0 && aIterationStart != PositiveInfinity<double>()) {

aIterationStart will never be positive infinity, negative infinity or NaN because it is a 'double' and the WebIDL bindings will check this for us. WebIDL only allows infinity and NaN values if the type is 'unrestricted double'.

So all we really need here is:

  if (aIterationStart < 0) {
    aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
    return false;
  }

  return true;

::: dom/animation/TimingParams.h
@@ +66,5 @@
>  
> +  // Return true if the parameter is valid for iteration start.
> +  // Else, retrun false.
> +  // Also, set invalid reason to ErrorResult parameter to throw the exception.
> +  static bool IsValidIterationStart(double aIterationStart, ErrorResult& aRv);

I think we could make this inline since it is quite simple?

It might be nice to avoid having two ways to indicate failure:
* The bool return value
* The result of aRv.

Could we just make this:

static void ValidateIterationStart(double aIterationStart, ErrorResult& aRv) {
  if (aIterationStart < 0) {
    aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
  }
}

Then the caller is responsible for checking aRv.Failed() ?

I think if we do that we don't need a comment, either.

I thought we could actually drop this method altogether, but I think in bug 1253470 we will want to replace Throw() here with aRv.ThrowTypeError<ENFORCE_RANGE_OUT_OF_RANGE>(NS_LITERAL_STRING("iterationStart"));

::: testing/web-platform/tests/web-animations/animation-effect-timing/iterationStart.html
@@ +109,5 @@
> +                function() {
> +                  div.animate({ opacity: [ 0, 1 ] },
> +                              { iterationStart: NaN });
> +                });
> +}, 'Test invlid iterationStart value');

We don't need half of these tests since we can rely on the WebIDL bindings to produce errors for NaN / Infinity.
Attachment #8729216 - Flags: review?(bbirtles)
Thanks, Brian!
I revised the codes that you pointed.
Attachment #8729366 - Flags: review?(bbirtles)
Attachment #8729215 - Attachment is obsolete: true
I'll do aRv.ThrowTypeError<ENFORCE_RANGE_OUT_OF_RANGE> after this bug as 1253470.
Attachment #8729368 - Flags: review?(bbirtles)
Attachment #8729216 - Attachment is obsolete: true
Comment on attachment 8729366 [details] [diff] [review]
Part 1:implement AnimationEffectTiming iterationStart

Review of attachment 8729366 [details] [diff] [review]:
-----------------------------------------------------------------

Please ask if there are any comments you don't understand.

::: testing/web-platform/tests/web-animations/animation-effect-timing/iterationStart.html
@@ +25,5 @@
> +  anim.finish();
> +  assert_equals(anim.effect.timing.iterationStart, 2.5);
> +  assert_equals(anim.effect.getComputedTiming().progress, null);
> +  assert_equals(anim.effect.getComputedTiming().currentIteration, null);
> +}, 'Test progress during before and after phase when fill is none');

As per comment 5, please drop this whole test. That is line 15-29.

@@ +36,5 @@
> +                           fill: 'both',
> +                           duration: 100,
> +                           delay: 1 });
> +  anim.effect.timing.iterationStart = 2.5;
> +  assert_equals(anim.effect.timing.iterationStart, 2.5);

As per comment 5, please drop this line.

@@ +66,5 @@
> +                           duration: 100,
> +                           delay: 0 });
> +  anim.finish();
> +  anim.effect.timing.iterationStart = 2.5;
> +  assert_equals(anim.effect.timing.iterationStart, 2.5);

As per comment 5, please drop this line.
Attachment #8729366 - Flags: review?(bbirtles) → review+
Comment on attachment 8729368 [details] [diff] [review]
Part 2: Throw TypeError if iterationStart is NaN, negative value or Infinity

Review of attachment 8729368 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/animation/TimingParams.cpp
@@ +106,5 @@
>    return TimingParamsFromOptionsUnion(aOptions, aTarget, aRv);
>  }
>  
> +/* static */ void
> +TimingParams::ValidateIterationStart(double aIterationStart, ErrorResult& aRv) {

As per comment 6, we should make this inline, i.e. put it in the header file.

::: testing/web-platform/tests/web-animations/animation-effect-timing/iterationStart.html
@@ +84,5 @@
> +                function() {
> +                  div.animate({ opacity: [ 0, 1 ] },
> +                              { iterationStart: -1 });
> +                });
> +}, 'Test invlid iterationStart value');

Test invalid iterationStart value.
Attachment #8729368 - Flags: review?(bbirtles) → review+
Attachment #8729369 - Flags: review?(bbirtles) → review+
Attachment #8729366 - Attachment is obsolete: true
Attachment #8729368 - Attachment is obsolete: true
Comment on attachment 8729405 [details] [diff] [review]
Part 2: Throw TypeError if iterationStart is NaN, negative value or Infinity

Review of attachment 8729405 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/TimingParams.h
@@ +64,5 @@
>      return result;
>    }
>  
> +  inline static void ValidateIterationStart(double aIterationStart,
> +                                            ErrorResult& aRv) {

No need for 'inline' when you put it within the class declaration.

Also, the { should go on a new line.

See the examples here.[1]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes
Parts 1 and 2 need review from a DOM peer.[1]

[1] https://wiki.mozilla.org/Modules/Core#Document_Object_Model
Attachment #8729405 - Attachment is obsolete: true
Comment on attachment 8729404 [details] [diff] [review]
Part 1:implement AnimationEffectTiming iterationStart

Hello Olli,
Nice to meet you over bugzilla!

Could you review AnimationEffectTiming.webidl ?
Attachment #8729404 - Flags: review?(bugs)
Comment on attachment 8729422 [details] [diff] [review]
Part 2: Throw TypeError if iterationStart is NaN, negative value or Infinity

Please review AnimationEffectTiming.webidl too.
Attachment #8729422 - Flags: review?(bugs)
Comment on attachment 8729404 [details] [diff] [review]
Part 1:implement AnimationEffectTiming iterationStart

Hi daisuke.
Looking good.
Attachment #8729404 - Flags: review?(bugs) → review+
Comment on attachment 8729422 [details] [diff] [review]
Part 2: Throw TypeError if iterationStart is NaN, negative value or Infinity

r+ for the .webidl also here.
Attachment #8729422 - Flags: review?(bugs) → review+
Daisuke-san, is this ready to land now?
Status: NEW → ASSIGNED
Flags: needinfo?(daisuke)
(In reply to Brian Birtles (:birtles) from comment #22)
> Daisuke-san, is this ready to land now?

Brian, please let me check with try server again at the end!
Flags: needinfo?(daisuke)
(In reply to Brian Birtles (:birtles) from comment #22)
> Daisuke-san, is this ready to land now?

It is ready to land!
You need to log in before you can comment on or make changes to this bug.