1215406 - implement support for KeyframeEffectOptions arguments in the KeyframeEffectReadOnly constructor

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Cameron McCormack (:heycam)]

In bug 1208951 we only support an offset, but we need to also handle a KeyframeTimingOptions object being passed in.

Comment 1

[Boris Chiou [:boris]]

I am thinking should we move FillMode(enum), PlaybackDirection(enum), ComputedTimingProperties(dictionary), AnimationEffectTimingProperties(dictionary) into a new webidl file from AnimationEffectReadOnly.webidl because

1. KeyframeEffectOptions is inherited from AnimationEffectTimingProperties, so just add KeyframeEffectOption into this new file.
2. AnimationEffectTimingReadOnly needs FillMode & PlaybackDirection, and it's better to replace NS_STYLE_ANIMATION_FILL_MODE_XXX and NS_STYLE_ANIMATION_DIRECTION_XXX with the generated scoped enums. Those cpp files using these enums or structs don't need to know what AnimationEffectReadOnly interface is. Therefore, putting these dictionaries and enums into a new file makes sense.

Comment 2

[Boris Chiou [:boris]]

Attachment: Part 1: Move AnimationEffectTimingProperties into a new file (v1)

Comment 3

[Boris Chiou [:boris]]

Attachment: Part 2: Add KeyframeEffectOptions (v1)

Comment 4

[Boris Chiou [:boris]]

Attachment: [WIP] Part 3: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v1)

Comment 5

[Boris Chiou [:boris]]

Attachment: Part 3: Make sure the mapping of FillMode and PlaybackDirection is correct (v1)

Comment 6

[Boris Chiou [:boris]]

Attachment: Part 4: Replace mDirection and mFillMode (v1)

Comment 7

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v1)

Comment 8

[Boris Chiou [:boris]]

Attachment: Part 1: Make sure the mapping of FillMode and PlaybackDirection is correct (v2)

Comment 9

[Boris Chiou [:boris]]

Attachment: Part 1: Make sure the mapping of FillMode and Direction is correct (v2)

Replace the definitions of
1. NS_STYLE_ANIMATION_DIRECTION_XXX with mozilla::dom::PlaybackDirection
2. NS_STYLE_ANIMATION_FILL_MODE_XXX with mozilla::dom::FillMode

Comment 10

[Boris Chiou [:boris]]

Attachment: Part 2: Replace mFillMode and mDirection in StyleAnimation and AnimationTiming (v2)

Use mozilla::dom::FillMode and mozilla::dom::PlaybackDirection in
AnimationTiming and StyleAnimation.

However, Animation struct still uses int32_t for direction data member in
LayersMessages.ipdlh because we cannot just include
AnimationEffectReadOnlyBinding.h in LayersMessages.ipdlh.
This will results in many compilation errors on Linux platform
due to the conflicts between some definitions in X11 header and the generated
files in webidl bindings.
E.g. X11/X.h defines "None" as 0L by "#define", which breaks
     the definition of FillMode::None.

Comment 11

[Boris Chiou [:boris]]

Attachment: Part 3: Add KeyframeEffectOptions (v1)

Add KeyframeEffectOptions in KeyframeEffect.webidl

Comment 12

[Boris Chiou [:boris]]

Attachment: Part 4: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v2)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 13

[Boris Chiou [:boris]]

Attachment: Part 1: Remove NS_STYLE_ANIMATION_DIRECTION_XXX and NS_STYLE_ANIMATION_FILL_MODE_XXX (v1)

Replace the definitions of
1. NS_STYLE_ANIMATION_DIRECTION_XXX with mozilla::dom::PlaybackDirection
2. NS_STYLE_ANIMATION_FILL_MODE_XXX with mozilla::dom::FillMode

So we can static_cast mozilla::dom::PlaybackDirection and mozilla::dom::FillMode
into integers directly.

Comment 14

[Boris Chiou [:boris]]

Attachment: Part 2: Change the types of direction and fillmode in StyleAnimation (v1)

Use scoped enum mozilla::dom::FillMode and mozilla::dom::PlaybackDirection,
instead of uint8_t in StyleAnimation.

Comment 15

[Boris Chiou [:boris]]

Attachment: Part 3: Change the types of direction and fillmode in AnimationTiming (v1)

Use mozilla::dom::FillMode and mozilla::dom::PlaybackDirection
in AnimationTiming.

Comment 16

[Boris Chiou [:boris]]

Attachment: Part 4: Add KeyframeEffectOptions (v1)

Add KeyframeEffectOptions in KeyframeEffect.webidl

Comment 17

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v2)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 18

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v3)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 19

[Boris Chiou [:boris]]

Attachment: Part 6: Test (v1)

Comment 20

[Boris Chiou [:boris]]

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

Comment 21

[Boris Chiou [:boris]]

Attachment: Part 2: Change the types of direction and fillmode in StyleAnimation (v1)

Use scoped enum mozilla::dom::FillMode and mozilla::dom::PlaybackDirection,
instead of uint8_t in StyleAnimation.

Comment 22

[Boris Chiou [:boris]]

Attachment: Part 3: Change the types of direction and fillmode in AnimationTiming (v1)

Use mozilla::dom::FillMode and mozilla::dom::PlaybackDirection
in AnimationTiming.

Comment 23

[Boris Chiou [:boris]]

Attachment: Part 4: Add KeyframeEffectOptions (v1)

Add KeyframeEffectOptions in KeyframeEffect.webidl

Comment 24

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v3)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 25

[Boris Chiou [:boris]]

Attachment: Part 3: Change the types of direction and fillmode in AnimationTiming (v2)

Use mozilla::dom::FillMode and mozilla::dom::PlaybackDirection
in AnimationTiming.

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8686425 [details] [diff] [review]
Part 1: Remove NS_STYLE_ANIMATION_DIRECTION_XXX and NS_STYLE_ANIMATION_FILL_MODE_XXX (v1)

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

Hi, Cameron, could you please review my patch (part 1 ~ part 3)?

In part 1 ~ part 3, I try to replace the NS_STYLE_ANIMATION_DIRECTION_XXX and NS_STYLE_ANIMATION_FILL_MODE_XXX with the auto-generated scoped enums [1].
However, there are still two parts using "integers" for direction and fill mode:
1. IPC: direction in mozilla::layers::Animation [2]
2. nsCSSProps::kAnimationDirectionKTable and nsCSSProps::kAnimationFillModeKTable [3]
I tried to replace the direction in [2] with the auto-generated scoped enum, but I cannot include the auto-generated file (e.g. AnimationEffectReadOnlyBinding.h) which results in many compilation errors on Linux. (e.g. the brain damage of X11 header to FillMode::None, and some gcc #pragma push/pop errors)
Therefore, I keep them using integers and just static_cast between integers and scoped enums.

Thanks.

[1] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h?case=true&from=AnimationEffectReadOnlyBinding.h#32
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh?case=true&from=LayersMessages.ipdlh#199
[3] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp#704

Comment 27

[Cameron McCormack (:heycam)]

Comment on attachment 8686425 [details] [diff] [review]
Part 1: Remove NS_STYLE_ANIMATION_DIRECTION_XXX and NS_STYLE_ANIMATION_FILL_MODE_XXX (v1)

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

You will need to rebase over bug 1224464 if that lands first.

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8686556 [details] [diff] [review]
Part 2: Change the types of direction and fillmode in StyleAnimation (v1)

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

::: layout/style/nsStyleStruct.h
@@ +2147,5 @@
>    float mDuration;
>    float mDelay;
>    nsString mName; // empty string for 'none'
> +  dom::PlaybackDirection mDirection;
> +  dom::FillMode mFillMode;

I guess there's no good way to avoid this increasing the size of StyleAnimation objects?

Comment 29

[Cameron McCormack (:heycam)]

(In reply to Boris Chiou [:boris] from comment #26)
> 2. nsCSSProps::kAnimationDirectionKTable and
> nsCSSProps::kAnimationFillModeKTable [3]
> I tried to replace the direction in [2] with the auto-generated scoped enum,
> but I cannot include the auto-generated file (e.g.
> AnimationEffectReadOnlyBinding.h) which results in many compilation errors
> on Linux. (e.g. the brain damage of X11 header to FillMode::None, and some
> gcc #pragma push/pop errors)

I'm not sure about the #pragma issues, but at least for the X header defining None, you can just #undef None before #including "AnimationEffectReadOnlyBinding.h" at the top of nsCSSProps.cpp.  We do that for various Windows defines e.g. https://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.h#15

Comment 30

[Cameron McCormack (:heycam)]

Comment on attachment 8686564 [details] [diff] [review]
Part 3: Change the types of direction and fillmode in AnimationTiming (v2)

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

::: dom/animation/KeyframeEffect.h
@@ +56,3 @@
>  
> +  bool FillsForwards() const;
> +  bool FillsBackwards() const;

Does something prevent us from including KeyframeEffectReadOnlyBinding.h here in Keyframe.h, which means these can't be inline?  (I don't think it's critical these are inline, but we may as well keep them inline if we can.)

Comment 31

[Boris Chiou [:boris]]

(In reply to Cameron McCormack (:heycam) (away Nov 23 – Dec 4) from comment #28)
> Comment on attachment 8686556 [details] [diff] [review]
> Part 2: Change the types of direction and fillmode in StyleAnimation (v1)
> 
> Review of attachment 8686556 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsStyleStruct.h
> @@ +2147,5 @@
> >    float mDuration;
> >    float mDelay;
> >    nsString mName; // empty string for 'none'
> > +  dom::PlaybackDirection mDirection;
> > +  dom::FillMode mFillMode;
> 
> I guess there's no good way to avoid this increasing the size of
> StyleAnimation objects?

Yes. Unless we could customize the underlying type of auto-generated scoped enum. :)

Comment 32

[Boris Chiou [:boris]]

(In reply to Cameron McCormack (:heycam) (away Nov 23 – Dec 4) from comment #29)
> (In reply to Boris Chiou [:boris] from comment #26)
> > 2. nsCSSProps::kAnimationDirectionKTable and
> > nsCSSProps::kAnimationFillModeKTable [3]
> > I tried to replace the direction in [2] with the auto-generated scoped enum,
> > but I cannot include the auto-generated file (e.g.
> > AnimationEffectReadOnlyBinding.h) which results in many compilation errors
> > on Linux. (e.g. the brain damage of X11 header to FillMode::None, and some
> > gcc #pragma push/pop errors)
> 
> I'm not sure about the #pragma issues, but at least for the X header
> defining None, you can just #undef None before #including
> "AnimationEffectReadOnlyBinding.h" at the top of nsCSSProps.cpp.  We do that
> for various Windows defines e.g.
> https://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.h#15

"#undef None" can solve part of issues, but there are sill many #pragma issues while I am building gecko on Linux, and they look like not easy to solve. ;(

BTW, maybe we can file a bug to wrap the X header to prevent the definition problem in the future.

Comment 33

[Boris Chiou [:boris]]

(In reply to Cameron McCormack (:heycam) (away Nov 23 – Dec 4) from comment #30)
> Comment on attachment 8686564 [details] [diff] [review]
> Part 3: Change the types of direction and fillmode in AnimationTiming (v2)
> 
> Review of attachment 8686564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/KeyframeEffect.h
> @@ +56,3 @@
> >  
> > +  bool FillsForwards() const;
> > +  bool FillsBackwards() const;
> 
> Does something prevent us from including KeyframeEffectReadOnlyBinding.h
> here in Keyframe.h, which means these can't be inline?  (I don't think it's
> critical these are inline, but we may as well keep them inline if we can.)

The only reason is: X11 header cause a brain damage of FillMode::None. If it's a critical issue, I can undef "None" and then include the binding header. Or just wait for the fix of definition problem in the X header.

Comment 34

[Cameron McCormack (:heycam)]

No, feel free to make these methods non-inline as you've done.

Comment 35

[Boris Chiou [:boris]]

Attachment: Part 6: Test (v2)

Comment 36

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v4)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 37

[Brian Birtles (:birtles)]

Comment on attachment 8688381 [details] [diff] [review]
Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v4)

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

This looks good, but since we don't support iterationComposite, composite, or spacing yet, I think we could just hard-code those attributes to always return the same value. Then when we implement, say, 'composite', we can add it then?

I think that will be less confusing to other people looking at this code (since if they see mComposite they might assume it does something). It will also be easier to understand when we begin to implement these members since the reviewer can see how we are setting and storing the value. I think when we implement spacing, we'll probably decide to store the spacing type as an enum, then the paced property name as an nsCSSProperty (i.e. rather than a string for both).

I'd like to have another look at this with these simplifications made.

::: dom/animation/KeyframeEffect.h
@@ +215,3 @@
>    }
> +
> +  CompositeOperation Composite() const { return mComposite; }

e.g. just make this:

  CompositeOperation Composite() const { return CompositeOperation::Replace; }

(We'll need to import CompositeOperation, of course, rather than just a forwards declaration.)

Likewise for IterationComposite and Spacing?

@@ +327,5 @@
>  
>    AnimationTiming mTiming;
> +  IterationCompositeOperation mIterationComposite;
> +  CompositeOperation mComposite;
> +  nsString mSpacing;

I think we could drop all of these.

Comment 38

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #37)
> Comment on attachment 8688381 [details] [diff] [review]
> Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly
> constructor (v4)
> 
> Review of attachment 8688381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but since we don't support iterationComposite, composite,
> or spacing yet, I think we could just hard-code those attributes to always
> return the same value. Then when we implement, say, 'composite', we can add
> it then?
> 
> I think that will be less confusing to other people looking at this code
> (since if they see mComposite they might assume it does something). It will
> also be easier to understand when we begin to implement these members since
> the reviewer can see how we are setting and storing the value. I think when
> we implement spacing, we'll probably decide to store the spacing type as an
> enum, then the paced property name as an nsCSSProperty (i.e. rather than a
> string for both).
> 
> I'd like to have another look at this with these simplifications made.
> 
> ::: dom/animation/KeyframeEffect.h
> e.g. just make this:
> 
>   CompositeOperation Composite() const { return CompositeOperation::Replace;
> }
> 
> (We'll need to import CompositeOperation, of course, rather than just a
> forwards declaration.)
> 
> Likewise for IterationComposite and Spacing?

Sure. I will let these function always return default hard-core value. Thanks.

Comment 39

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #38)
> > (We'll need to import CompositeOperation, of course, rather than just a
> > forwards declaration.)
> > 
> > Likewise for IterationComposite and Spacing?
> 
> Sure. I will let these function always return default hard-core value.
> Thanks.


Btw, I want to move the implementation of compositeOperation and iterationCompositeOperation into cpp file to avoid including KeyframeEffectBinding.h which also includes AnimatinoEffectReadOnlyBinding.h where FillMode::None is defined, so I don't have to undef "None" in the header to pass the compilation on Linux.

Comment 40

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v5)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 41

[Boris Chiou [:boris]]

Attachment: Part 6: Test (v3)

Comment 42

[Brian Birtles (:birtles)]

Comment on attachment 8688793 [details] [diff] [review]
Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v5)

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

I think we need to adjust the way we handle invalid duration values. Apart from that, it looks good.

::: dom/animation/KeyframeEffect.cpp
@@ +553,5 @@
>  {
>    AnimationTiming animationTiming;
>  
> +  if (aOptions.IsKeyframeEffectOptions()){
> +    const KeyframeEffectOptions &opt = aOptions.GetAsKeyframeEffectOptions();

I think the & hugs the type here.

@@ +559,3 @@
>      animationTiming.mIterationDuration =
> +      opt.mDuration.IsUnrestrictedDouble() ?
> +      TimeDuration::FromMilliseconds(opt.mDuration.GetAsUnrestrictedDouble()) :

TimeDuration::FromMilliseconds will correctly handling infinite values, but I think an unrestricted double can be NaN, right? Can you add a test for NaN and check what we do? According to the spec, we should probably return NaN from animation.effect.timing.duration, but behave as if 0 was specified.

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

@@ +559,4 @@
>      animationTiming.mIterationDuration =
> +      opt.mDuration.IsUnrestrictedDouble() ?
> +      TimeDuration::FromMilliseconds(opt.mDuration.GetAsUnrestrictedDouble()) :
> +      TimeDuration(0); // e.g. auto

'i.e. auto'

Is this right? If we pass in "auto" (even as a default dictionary value), when we query animation.effect.timing.duration we should get back "auto" right. Likewise, if we pass in "cabbage", we should get back "cabbage".

@@ +568,5 @@
> +    if (aOptions.IsUnrestrictedDouble()) {
> +      animationTiming.mIterationDuration =
> +        TimeDuration::FromMilliseconds(aOptions.GetAsUnrestrictedDouble());
> +    } else {
> +      animationTiming.mIterationDuration = TimeDuration(0);

In both of these branches too, we should store the original duration value, but when we calculate the timing, treat it the duration as zero if it is NaN, or any string value. From the *computed* timing interface, we should return zero.

@@ +1617,3 @@
>    AnimationTiming timing = ConvertKeyframeEffectOptions(aOptions);
>  
> +  // Handle animaiton properties

'animation'

(I'm not sure we really need these comments.)

::: dom/animation/KeyframeEffect.h
@@ +214,5 @@
> +
> +  CompositeOperation Composite() const;
> +
> +  void GetSpacing(nsString& aRetVal) const {
> +    aRetVal = NS_LITERAL_STRING("distribute");

I think we can use aRetVal.AssignLiteral here?

@@ +220,5 @@
> +
> +  const AnimationTiming& Timing() const { return mTiming; }
> +
> +  AnimationTiming& Timing() { return mTiming; }
> +

I think it would be easier to read without all the extra whitespace between these methods.

Comment 43

[Brian Birtles (:birtles)]

Just looking at part 6, I see that you're only testing with getComputedTiming().

If you prefer not to store the original values for 'duration' since we only support getComputedTiming() that's fine. We can store those values once we implement AnimationEffectTimingReadOnly.

If that's the case, you can ignore some of more comments from comment 42 as long as you clearly document that this is a temporary measure. We still need to handle NaN values correctly, however.

Comment 44

[Brian Birtles (:birtles)]

Comment on attachment 8688796 [details] [diff] [review]
Part 6: Test (v3)

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

Looks good, but I'd like to see the additional tests.

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
@@ +464,5 @@
> +
> +var gKeyframeEffectOptionTests = [
> +  { desc:   "an empty KeyframeEffectOption",
> +    input:  {},
> +    output: {delay: 0,

I think 'expected' would be more clear that 'output'.

@@ +495,5 @@
> +             duration: 3000.5,
> +             direction: "normal",
> +             composite: "replace",
> +             iterationComposite: "replace",
> +             spacing: "distribute"} }

We should add tests for:

* Infinity
* -Infinity
* NaN
* -1
* { duration: Infinity }
* { duration: -Infinity }
* { duration: NaN }
* { duration: -1 }
* { duration: "cabbage" }
* { duration: "auto" }
* { iterations: Infinity }
* { iterations: -Infinity }
* { iterations: -1.0 }
* { iterations: NaN }

Comment 45

[Brian Birtles (:birtles)]

Comment on attachment 8688796 [details] [diff] [review]
Part 6: Test (v3)

Sorry, I should probably check the new tests. I didn't mean to r+ just yet.

Comment 46

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #43)
> Just looking at part 6, I see that you're only testing with
> getComputedTiming().
> 
> If you prefer not to store the original values for 'duration' since we only
> support getComputedTiming() that's fine. We can store those values once we
> implement AnimationEffectTimingReadOnly.
> 
> If that's the case, you can ignore some of more comments from comment 42 as
> long as you clearly document that this is a temporary measure. We still need
> to handle NaN values correctly, however.

Thanks for your review and examples. :)

I see. I think we can store the original value for 'duration' in struct AnimationTiming in this bug and make sure these cases can be handled correctly, so it would be easier to implement AnimationEffectTimingReadOnly in Bug 1214536.

Comment 47

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #42)
> @@ +559,3 @@
> >      animationTiming.mIterationDuration =
> > +      opt.mDuration.IsUnrestrictedDouble() ?
> > +      TimeDuration::FromMilliseconds(opt.mDuration.GetAsUnrestrictedDouble()) :
> 
> TimeDuration::FromMilliseconds will correctly handling infinite values, but
> I think an unrestricted double can be NaN, right? Can you add a test for NaN
> and check what we do? According to the spec, we should probably return NaN
> from animation.effect.timing.duration, but behave as if 0 was specified.

Yes, an unrestricted double can be NaN[1]. However, if we convert NaN into TimeDuration and then convert it back to a double value (by ToMilliseconds()), it becomes "-inf". This is not what we want, and looks like storing the original value is much better, so I should fix this part. Thanks for your reminder.

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h#121
    A float/double is NaN if all exponent bits are 1 and the significand contains at least one non-zero bit.

Comment 48

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #44)
> Comment on attachment 8688796 [details] [diff] [review]
> Part 6: Test (v3)
> 
> Review of attachment 8688796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I'd like to see the additional tests.
> 
> :::
> testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
> @@ +464,5 @@
> > +
> > +var gKeyframeEffectOptionTests = [
> > +  { desc:   "an empty KeyframeEffectOption",
> > +    input:  {},
> > +    output: {delay: 0,
> 
> I think 'expected' would be more clear that 'output'.
> 
> @@ +495,5 @@
> > +             duration: 3000.5,
> > +             direction: "normal",
> > +             composite: "replace",
> > +             iterationComposite: "replace",
> > +             spacing: "distribute"} }
> 
> We should add tests for:
> 
> * Infinity
> * -Infinity
> * NaN
> * -1
> * { duration: Infinity }
> * { duration: -Infinity }
> * { duration: NaN }
> * { duration: -1 }
> * { duration: "cabbage" }
> * { duration: "auto" }
> * { iterations: Infinity }
> * { iterations: -Infinity }
> * { iterations: -1.0 }
> * { iterations: NaN }

Hi Brian,
According to the spec [1], I want to make sure if my answers are correct (by effect.getComputedTiming().duration or .iterations)

* Infinity                  -> duration == Infinity
* -Infinity                 -> duration == 0 (should greater than or equal to zero)
* NaN                       -> duration == 0
* -1                        -> duration == 0
* { duration: Infinity }    -> duration == Infinity
* { duration: -Infinity }   -> duration == 0
* { duration: NaN }         -> duration == 0
* { duration: -1 }          -> duration == 0
* { duration: "cabbage" }   -> duration == 0
* { duration: "auto" }      -> duration == 0
* { iterations: Infinity }  -> iterations = Infinity
* { iterations: -Infinity } -> iterations = 0
* { iterations: -1.0 }      -> iterations = 0
* { iterations: NaN }       -> iterations = 0

[1] The iteration duration which is a "real number greater than or equal to zero (including positive infinity)"
[2] The animation effect’s iteration count property which is a "real number greater than or equal to zero (including positive infinity)"

Are they correct? Do we make them greater than or equal to zero in AnimationTiming::mDuration? Therefore, getComputedTiming) can get a valid "duration" value. I will store the original value however for AnimationEffectTimingReadOnly.

Thanks.

Comment 49

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris] from comment #48)
> * { iterations: -Infinity } -> iterations = 0
> * { iterations: -1.0 }      -> iterations = 0
> * { iterations: NaN }       -> iterations = 0

These should be 1.0.

The spec for getComputedTiming() says:

  "Although several of the attributes of the returned object are common to the
   AnimationEffectTimingReadOnly object returned by the timing attribute, the values
   of return returning object represent the values after applying the adjustments
   described by the AnimationEffectTiming interface as being “for the purpose of
   timing model calculations”."
  http://w3c.github.io/web-animations/#dom-animationeffectreadonly-getcomputedtiming

For iterations, it says:

  "Values less than zero and NaN values are treated as the value 1.0 for the
   purpose of timing model calculations."
  http://w3c.github.io/web-animations/#dom-animationeffecttiming-iterations

> Are they correct? Do we make them greater than or equal to zero in
> AnimationTiming::mDuration? Therefore, getComputedTiming) can get a valid
> "duration" value. I will store the original value however for
> AnimationEffectTimingReadOnly.

In the short-term you can do whatever is easiest. Long-term, you might decide to store the original value in mDuration, then, in GetComputedTiming(), detect bad values and clamp to 0.0.

I really wonder how important it is to store the original values, but as the spec currently stands, that's what we're supposed to do.

Comment 50

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #49)
> 
> In the short-term you can do whatever is easiest. Long-term, you might
> decide to store the original value in mDuration, then, in
> GetComputedTiming(), detect bad values and clamp to 0.0.
> 
> I really wonder how important it is to store the original values, but as the
> spec currently stands, that's what we're supposed to do.

Thanks. I see. I will handle this in another bug. In the short-term, I store both the original value (type: OwningUnrestrictedDoubleOrString) and "mDuration" (type: TimeDuration) in AnimationTiming, and handle those cases we mentioned in KeyframeEffectReadOnly::ConvertKeyframeEffectOptions(...) to make sure "mDuration" is correct.

Comment 51

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v6)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 52

[Boris Chiou [:boris]]

Attachment: Part 6: Test (v4)

Comment 53

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v6)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 54

[Boris Chiou [:boris]]

Attachment: Part 6: Test (v5)

Comment 55

[Boris Chiou [:boris]]

Attachment: Part 6: Test (v6)

Comment 56

[Brian Birtles (:birtles)]

Comment on attachment 8689337 [details] [diff] [review]
Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v6)

> /* static */ AnimationTiming
> KeyframeEffectReadOnly::ConvertKeyframeEffectOptions(
>-    const Optional<double>& aOptions)
>+    const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions)
> {
>   AnimationTiming animationTiming;
> 
>-  // The spec says to treat auto durations as 0 until a later version of
>-  // the spec says otherwise.  Bug 1215406 is for handling a
>-  // KeyframeEffectOptions object and not just an offset.
>-  if (aOptions.WasPassed()) {
>-    animationTiming.mIterationDuration =
>-      TimeDuration::FromMilliseconds(aOptions.Value());
>+  if (aOptions.IsKeyframeEffectOptions()) {
>+    const KeyframeEffectOptions& opt = aOptions.GetAsKeyframeEffectOptions();
>+    if (opt.mDuration.IsUnrestrictedDouble()) {
>+      const double& dur = opt.mDuration.GetAsUnrestrictedDouble();
>+      animationTiming.mIterationDuration =
>+        (IsNaN(dur) || dur < 0.0f) ? TimeDuration(0) :
>+        TimeDuration::FromMilliseconds(dur);
>+    } else {
>+      animationTiming.mIterationDuration = TimeDuration(0);
>+    }
>+    // Store the original value, so AnimationTimingEffectReadOnly
>+    // can get the original value easily.
>+    animationTiming.mRawDuration = opt.mDuration;
>+    animationTiming.mDelay = TimeDuration::FromMilliseconds(opt.mDelay);
>+    // FIXME: Covert mIterationCount to a valid value.
>+    // Bug 1214536 should revise this and keep original value, so
>+    // AnimationTimingEffectReadOnly can get the original iterations.
>+    animationTiming.mIterationCount =
>+      (IsNaN(opt.mIterations) || opt.mIterations < 0.0f) ? 1.0f : opt.mIterations;
>+    animationTiming.mDirection = opt.mDirection;
>+    animationTiming.mFillMode = opt.mFill;
>   } else {
>-    animationTiming.mIterationDuration = TimeDuration(0);
>+    double dur = 0.0f;
>+    if (aOptions.IsUnrestrictedDouble()) {
>+      dur = aOptions.GetAsUnrestrictedDouble();
>+      animationTiming.mIterationDuration =
>+        (IsNaN(dur) || dur < 0.0f) ? TimeDuration(0) :
>+        TimeDuration::FromMilliseconds(dur);
>+    } else {
>+      animationTiming.mIterationDuration = TimeDuration(0);
>+    }
>+    animationTiming.mRawDuration.SetAsUnrestrictedDouble() = dur;
>+    animationTiming.mDelay = TimeDuration(0);
>+    animationTiming.mIterationCount = 1.0f;
>+    animationTiming.mDirection = PlaybackDirection::Normal;
>+    animationTiming.mFillMode = FillMode::Auto;
>   }

TimeDuration initializes to zero so we can simplify this code somewhat.

Perhaps something like the following would reduce some of the duplicated code
(I haven't tried to compile this so you'll probably need to fix it):


  // Extract an iteration duration from an UnrestrictedDoubleOrXXX object.
  template <typename T>
  static TimeDuration
  GetIterationDuration(const T& aDuration) {
    // Always return the same object to benefit from return-value optimization.
    TimeDuration result;
    if (aDuration.IsUnrestrictedDouble()) {
      double durationMs = aDuration.GetAsUnrestrictedDouble();
      if (!IsNaN(durationMs) && durationMs >= 0.0) {
        result = TimeDuration::FromMilliseconds(durationMs);
      }
    }
    // else, aDuration should be zero

    return result;
  }

  /* static */ AnimationTiming
  KeyframeEffectReadOnly::ConvertKeyframeEffectOptions(
      const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions)
  {
    AnimationTiming animationTiming;

    if (aOptions.IsKeyframeEffectOptions()) {
      const KeyframeEffectOptions& opt = aOptions.GetAsKeyframeEffectOptions();
      animationTiming.mIterationDuration = GetIterationDuration(opt.mDuration);
      animationTiming.mDelay = TimeDuration::FromMilliseconds(opt.mDelay);
      // FIXME: Covert mIterationCount to a valid value.
      // Bug 1214536 should revise this and keep the original value, so
      // AnimationTimingEffectReadOnly can get the original iterations.
      animationTiming.mIterationCount = (IsNaN(opt.mIterations) ||
                                        opt.mIterations < 0.0f) ?
                                          1.0f :
                                          opt.mIterations;
      animationTiming.mDirection = opt.mDirection;
      animationTiming.mFillMode = opt.mFill;
    } else {
      animationTiming.mIterationDuration = GetIterationDuration(aOptions);
      animationTiming.mIterationCount = 1.0f;
      animationTiming.mDirection = PlaybackDirection::Normal;
      animationTiming.mFillMode = FillMode::Auto;
    }

    return animationTiming;
  }

I removed the assignment to mRawDuration since we're not using it in this bug.
I think we should add it when we are going to use it. We might find a better way
of doing this when we go to use it and it will just confuse people in the
meantime.

(For example, perhaps we could store *only* the OwningUnrestrictedDoubleOrString
value and do the conversion in GetComputedTimingAt, not
ConvertKeyframeEffectOptions, but we can discuss that when we come to implement
AnimationTimingReadOnly.)

If you add a constructor to AnimationTiming to fill in the default values of
mIterationCount etc. then I think you could make this even more simple.


>@@ -1576,17 +1612,17 @@ KeyframeEffectReadOnly::BuildAnimationPropertyList(
> /* static */ already_AddRefed<KeyframeEffectReadOnly>
> KeyframeEffectReadOnly::Constructor(
>     const GlobalObject& aGlobal,
>     Element* aTarget,
>     const Optional<JS::Handle<JSObject*>>& aFrames,
>-    const Optional<double>& aOptions,
>+    const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions,
>     ErrorResult& aRv)

I'm curious, when you have an optional union type do the WebIDL bindings no
longer use the Optional<> helper object but instead represent that through the
union type?

>diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
>index a126338..daf5ca1 100644
>--- a/dom/animation/KeyframeEffect.h
>+++ b/dom/animation/KeyframeEffect.h
> struct AnimationTiming
> {
>   TimeDuration mIterationDuration;
>   TimeDuration mDelay;
>   float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite
>   dom::PlaybackDirection mDirection;
>   dom::FillMode mFillMode;
>+  dom::OwningUnrestrictedDoubleOrString mRawDuration;

As mentioned above, I think we can leave this part of out this patch since we
don't seem to be using it, right?

>+  IterationCompositeOperation IterationComposite() const;
>+
>+  CompositeOperation Composite() const;
>+
>+  void GetSpacing(nsString& aRetVal) const {
>+    aRetVal.AssignLiteral("distribute");
>   }

I think we could remove the whitespace between these methods, but it's up to
you.


Sorry about so many rounds of feedback. Thank you very much for doing this. It's
getting very close. I will be away for the next few days but hopefully I can
find some time to check this along the way.

Comment 57

[Brian Birtles (:birtles)]

Comment on attachment 8689339 [details] [diff] [review]
Part 6: Test (v6)

>+// KeyframeEffectOptions
>+test(function(t) {
>+  var effect = new KeyframeEffectReadOnly(
>+    target, {left: ["10px", "20px"]});
>+  assert_equals(effect.composite, "replace", "default composite");
>+  assert_equals(effect.iterationComposite, "replace",
>+                "default iterationComposite");
>+  assert_equals(effect.spacing, "distribute",
>+                "default spacing");
>+}, "a KeyframeEffectReadOnly constructed without any " +
>+   "KeyframeEffectOptions object");

We should test the result of getComputedTiming() when no 'options' argument is provided since I'm pretty sure that exercises a different code path to the case where we pass an empty dictionary, '{}'.

>+var gKeyframeEffectOptionTests = [
>+  { desc:     "an empty KeyframeEffectOption",
>+    input:    {},
>+    expected: {delay: 0,
>+               fill: "auto",
>+               iterations: 1,
>+               duration: 0,
>+               direction: "normal"} },

This is not correct. The value for fill should be "none" in this case.

The spec is a bit hard to read here but see the definition of getComputedTiming():

  "fill – the auto value is replaced with the specific FillMode depending on the
   type of animation effect (see §5.9.1 The FillMode enumeration)."
  http://w3c.github.io/web-animations/#dom-animationeffectreadonly-getcomputedtiming

So we should be getting "none" in all cases. I don't think we have any tests for fill? We should test at least:

  input: { fill: 'forwards' } -> output: { fill: 'forwards' }
  input: { fill: 'auto' } -> output: { fill: 'none' }
  input: { } -> output: { fill: 'none' }

>+  { desc:     "an auto duration",
>+    input:    {duration: "cabbage"},
>+    expected: {delay: 0,
>+               fill: "auto",
>+               iterations: 1,
>+               duration: 0,
>+               direction: "normal"} },

Copy-paste bug here. (Should be "auto" not "cabbage".)

Comment 58

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) (away 20-23 Nov) from comment #56)
> (For example, perhaps we could store *only* the
> OwningUnrestrictedDoubleOrString
> value and do the conversion in GetComputedTimingAt, not
> ConvertKeyframeEffectOptions, but we can discuss that when we come to
> implement
> AnimationTimingReadOnly.)
> 
> If you add a constructor to AnimationTiming to fill in the default values of
> mIterationCount etc. then I think you could make this even more simple.

It's a good idea, I want to revise AnimationTiming in Bug 1214536, so it can store the original value and get a valid value by its API.

BTW, your function template works, so I can just use it. :)
> 
> 
> >@@ -1576,17 +1612,17 @@ KeyframeEffectReadOnly::BuildAnimationPropertyList(
> > /* static */ already_AddRefed<KeyframeEffectReadOnly>
> > KeyframeEffectReadOnly::Constructor(
> >     const GlobalObject& aGlobal,
> >     Element* aTarget,
> >     const Optional<JS::Handle<JSObject*>>& aFrames,
> >-    const Optional<double>& aOptions,
> >+    const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions,
> >     ErrorResult& aRv)
> 
> I'm curious, when you have an optional union type do the WebIDL bindings no
> longer use the Optional<> helper object but instead represent that through
> the
> union type?
>
I also feel strange when I saw the "Optional<>" disappears.
I used "./mach webidl-example KeyframeEffectReadOnly" to generate the KeyframeEffectReadOnly-example.h, and checked the constructor. Its type signature is:
  static already_AddRefed<KeyframeEffectReadOnly> Constructor(const GlobalObject& global, Element* target, const Optional<JS::Handle<JSObject*>>& frames, const UnrestrictedDoubleOrKeyframeEffectOptions& options, ErrorResult& aRv);

And then, I just copied it into our code.
I tested this by passing no 'options' argument, and aOptions.IsKeyframeEffectOptions() is true, so I guess it will pass a default KeyframeEffectOptions object.

Comment 59

[Boris Chiou [:boris]]

Attachment: Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v7)

Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor and other
related APIs.

Comment 60

[Boris Chiou [:boris]]

Attachment: Part 6: Test (v7)

Comment 61

[Brian Birtles (:birtles)]

Comment on attachment 8689892 [details] [diff] [review]
Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly constructor (v7)

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

Thanks for making those changes. Looks good.

Comment 62

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) (away 20-23 Nov) from comment #61)
> Comment on attachment 8689892 [details] [diff] [review]
> Part 5: Implement KeyframeEffectOptions in KeyframeEffectReadOnly
> constructor (v7)
> 
> Review of attachment 8689892 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for making those changes. Looks good.

Thanks for your review. :) I am waiting for the landing of Bug 1224464.

Comment 63

[Boris Chiou [:boris]]

Attachment: Part 1: Remove NS_STYLE_ANIMATION_DIRECTION_XXX and NS_STYLE_ANIMATION_FILL_MODE_XXX. (v2, carry heycam's r+)

Replace the definitions of
1. NS_STYLE_ANIMATION_DIRECTION_XXX with mozilla::dom::PlaybackDirection
2. NS_STYLE_ANIMATION_FILL_MODE_XXX with mozilla::dom::FillMode

So we can static_cast mozilla::dom::PlaybackDirection and mozilla::dom::FillMode
into integers directly.

Rebased.

Comment 64

[Boris Chiou [:boris]]

Attachment: Part 2: Change the types of direction and fillmode in StyleAnimation. (v2, carry heycam's r+)

Use scoped enum mozilla::dom::FillMode and mozilla::dom::PlaybackDirection,
instead of uint8_t in StyleAnimation.

Rebased.

Comment 65

[Boris Chiou [:boris]]

Attachment: Part 3: Change the types of direction and fillmode in AnimationTiming. (v3, carry heycam's r+)

Use mozilla::dom::FillMode and mozilla::dom::PlaybackDirection
in AnimationTiming.

Rebased.

Comment 66

[Boris Chiou [:boris]]

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

Comment 67

[Boris Chiou [:boris]]

Attachment: Part 1: Remove NS_STYLE_ANIMATION_DIRECTION_XXX and NS_STYLE_ANIMATION_FILL_MODE_XXX. (v3, carry heycam's r+)

Replace the definitions of
1. NS_STYLE_ANIMATION_DIRECTION_XXX with mozilla::dom::PlaybackDirection
2. NS_STYLE_ANIMATION_FILL_MODE_XXX with mozilla::dom::FillMode

So we can static_cast mozilla::dom::PlaybackDirection and mozilla::dom::FillMode
into integers directly.

Rebased and fix build errors.

Comment 68

[Boris Chiou [:boris]]

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

Comment 69

[Boris Chiou [:boris]]

Attachment: Part 6: Test (v8, carry birtles' r+)

Fix test failure in Windows 8 X64

Comment 70

[Boris Chiou [:boris]]

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

Comment 71

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/599f81b28729
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c444e04246
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d173ca3e99
https://hg.mozilla.org/integration/mozilla-inbound/rev/77f5b60efad7
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d884d1c84e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4050d9aaac1a

Comment 72

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/599f81b28729
https://hg.mozilla.org/mozilla-central/rev/00c444e04246
https://hg.mozilla.org/mozilla-central/rev/41d173ca3e99
https://hg.mozilla.org/mozilla-central/rev/77f5b60efad7
https://hg.mozilla.org/mozilla-central/rev/4d884d1c84e8
https://hg.mozilla.org/mozilla-central/rev/4050d9aaac1a