Implement AnimationEffectTimingReadOnly for Web Animation API

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Animation
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: boris, Assigned: boris)

Tracking

({dev-doc-needed})

unspecified
mozilla46
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(URL)

Attachments

(9 attachments, 95 obsolete attachments)

3.44 KB, patch
boris
: review+
Details | Diff | Splinter Review
17.09 KB, patch
boris
: review+
Details | Diff | Splinter Review
15.91 KB, patch
boris
: review+
Details | Diff | Splinter Review
13.48 KB, patch
boris
: review+
Details | Diff | Splinter Review
20.96 KB, patch
boris
: review+
Details | Diff | Splinter Review
22.63 KB, patch
boris
: review+
Details | Diff | Splinter Review
1.58 KB, patch
boris
: review+
Details | Diff | Splinter Review
26.21 KB, patch
boris
: review+
Details | Diff | Splinter Review
4.09 KB, patch
boris
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
According to Web Animation spec, AnimationEffectReadOnly have two member:
interface AnimationEffectReadOnly
{
    readonly attribute AnimationEffectTimingReadOnly timing;
    ComputedTimingProperties getComputedTiming();
};

1. "ComputedTimingProperties" is being handled by Bug 1108055.
2. "AnimationEffectTimingReadOnly" is not yet implemented.

In this bug, we should add this new interface, AnimationEffectTimingReadOnly, and try to merge it into KeyframeEffectReadOnly.
(Assignee)

Updated

3 years ago
Assignee: nobody → boris.chiou
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

3 years ago
Attachment #8681875 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

3 years ago
Attachment #8681876 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

3 years ago
Depends on: 1215406
Comment hidden (obsolete)
(Assignee)

Updated

3 years ago
Attachment #8683590 - Attachment is obsolete: true
Attachment #8683591 - Attachment is obsolete: true
Attachment #8683592 - Attachment is obsolete: true
Attachment #8683593 - Attachment is obsolete: true
Comment hidden (obsolete)
Blocks: 1226047
Comment hidden (obsolete)
(Assignee)

Updated

3 years ago
Attachment #8688223 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8688231 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8690498 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8690621 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
Created attachment 8690654 [details] [diff] [review]
Part 1: Use unrestricted double for iterations (v4)

We want to store the original value from KeyframeEffectOptions whose
iterations is unrestricted double. Therefore, we can get the original value
of iterations by AnimationEffectTimingReadOnly.
(Assignee)

Updated

2 years ago
Attachment #8690629 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8690499 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Comment 16

2 years ago
Created attachment 8690694 [details] [diff] [review]
Part 3: Use OwingUnrestrictedDoubleOrString for duration (v2)

We store the original value of duration, so AnimationEffectReadOnly could get
it. Add some APIs to convert the original duraion into a valid real number, so
the Timing model can use it to calculate other values correctly.
(Assignee)

Updated

2 years ago
Attachment #8690671 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 17

2 years ago
Created attachment 8690695 [details] [diff] [review]
Part 4: Add AnimationEffectTimingReadOnly interface (v1)

Add AnimationEffectTimingReadOnly.webidl
(Assignee)

Comment 18

2 years ago
Created attachment 8691729 [details] [diff] [review]
Part 5: Implement TimingFromJS (v1)

Override TimingFromJS in KeyframeEffectReadOnly.
(Assignee)

Updated

2 years ago
Attachment #8690654 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8690659 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8690694 - Flags: review?(bbirtles)
(Assignee)

Comment 19

2 years ago
Created attachment 8691730 [details] [diff] [review]
Part 2: Store the original value of fill (v3)

FillMode could be 'auto', and we should treat it as 'none' in the timing model.
However, AnimationEffectTimingReadOnly should get its original value.

Fix typos.
Attachment #8691730 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8690659 - Attachment is obsolete: true
Attachment #8690659 - Flags: review?(bbirtles)
(Assignee)

Comment 20

2 years ago
Created attachment 8691854 [details] [diff] [review]
[WIP] Part 6: Test (v1)
For part 1, instead of adding AnimationTiming.AdjustedIterations(), why don't we just calculate the correct iteration value in GetComputedTimingAt? Unless there's a performance reason to do otherwise, I think it's simplest to just let GetComputedTimingAt be responsible for converting specified values into computed values.
Flags: needinfo?(boris.chiou)
Actually, never mind, I should follow up on the list to check whether this behavior is really necessary or not.
Flags: needinfo?(boris.chiou)
(In reply to Brian Birtles (:birtles) from comment #22)
> Actually, never mind, I should follow up on the list to check whether this
> behavior is really necessary or not.

https://lists.w3.org/Archives/Public/public-fx/2015OctDec/0038.html
Comment on attachment 8690654 [details] [diff] [review]
Part 1: Use unrestricted double for iterations (v4)

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

Explaining comment 21, what I have in mind is this:

::: dom/animation/KeyframeEffect.cpp
@@ +46,5 @@
>  {
>    // AnimationEffectTimingProperties
>    aRetVal.mDelay = aTiming.mDelay.ToMilliseconds();
>    aRetVal.mFill = aTiming.mFillMode;
> +  aRetVal.mIterations = aTiming.AdjustedIterations();

aRetVal.mIterations = aComputedTiming.mIterations;

@@ +176,5 @@
>    if (aLocalTime.IsNull()) {
>      return result;
>    }
>    const TimeDuration& localTime = aLocalTime.Value();
> +  const double iterations = aTiming.AdjustedIterations();

result.mIterations = IsNaN(mIterationCount) || mIterationCount < 0.0f ?
                     1.0f :
                     mIterationCount;

@@ +197,5 @@
>      // Note that infinity == floor(infinity) so this will also be true when we
>      // have finished an infinitely repeating animation of zero duration.
>      isEndOfFinalIteration =
> +      iterations != 0.0 &&
> +      iterations == floor(iterations);

s/iterations/result.mIterations/ here and through the rest of this function.

@@ +287,5 @@
>  
>  StickyTimeDuration
>  KeyframeEffectReadOnly::ActiveDuration(const AnimationTiming& aTiming)
>  {
> +  const double iterations = aTiming.AdjustedIterations();

This is a bit tricky, I guess we could pass in the adjusted iteration count as a separate parameter?

::: dom/animation/KeyframeEffect.h
@@ +48,5 @@
>   * Eventually this will represent all the input timing parameters specified
>   * by content but for now it encapsulates just the subset of those
>   * parameters passed to GetPositionInIteration.
>   */
>  struct AnimationTiming

I was thinking we should expand ComputedTiming and add the following member:

  double mIterations;

@@ +53,5 @@
>  {
>    TimeDuration mIterationDuration;
>    TimeDuration mDelay;
> +  // mIterationCount could be a real number, NaN, or +/-Infinity
> +  double mIterationCount;

I think we should rename this to mIterations but that can be done in a separate patch if it makes this patch unclear.

@@ +61,5 @@
> +  // Return a valid iteration count which is a real number
> +  // greater than or equal to 0 (including positive infinity).
> +  double AdjustedIterations() const {
> +    return (IsNaN(mIterationCount) || mIterationCount < 0.0f) ? 1.0f : mIterationCount;
> +  }

I think we should drop this function.

::: layout/base/nsDisplayList.cpp
@@ +381,5 @@
>                                StickyTimeDuration(timing.mDelay));
>    animation->initialCurrentTime() = aAnimation->GetCurrentTime().Value()
>                                      - timing.mDelay;
>    animation->duration() = timing.mIterationDuration;
> +  animation->iterationCount() = timing.AdjustedIterations();

We should add a call here to:

  ComputedTiming computedTiming = aAnimation->GetEffect()->GetComputedTiming();

And use computedTiming.mIterationCount;
What do you think of doing it this way?
Flags: needinfo?(boris.chiou)
(In reply to Brian Birtles (:birtles) from comment #24)
> @@ +287,5 @@
> >  
> >  StickyTimeDuration
> >  KeyframeEffectReadOnly::ActiveDuration(const AnimationTiming& aTiming)
> >  {
> > +  const double iterations = aTiming.AdjustedIterations();
> 
> This is a bit tricky, I guess we could pass in the adjusted iteration count
> as a separate parameter?

On further though, I think we could make ActiveDuration() just take two parameters: aIterations and aIterationDuration. These would be the "adjusted" values.
Maybe the original patch is ok, so long as we rename AdjustedIterations to ComputedIterations.
Comment on attachment 8690654 [details] [diff] [review]
Part 1: Use unrestricted double for iterations (v4)

Sorry for the back and forth on this one. Ideally, it would be nice if our internal model reflected the Web Animations model--i.e. you store your specified times and then you have a "compute timing" step and the result is that all the values are normalized.

However, I think the approach you have is ok, as long as we use the name "computed" rather than "adjusted" to reflect this step.

If you don't mind reworking this to match comment 24, I'd prefer that, but if you think this is better, I'm ok with it as long as you make the following changes.

>@@ -172,16 +172,17 @@ KeyframeEffectReadOnly::GetComputedTimingAt(
>   result.mActiveDuration = ActiveDuration(aTiming);
> 
>   // The default constructor for ComputedTiming sets all other members to
>   // values consistent with an animation that has not been sampled.
>   if (aLocalTime.IsNull()) {
>     return result;
>   }
>   const TimeDuration& localTime = aLocalTime.Value();
>+  const double iterations = aTiming.AdjustedIterations();

Perhaps call the local variable |computedIterations| ?

> StickyTimeDuration
> KeyframeEffectReadOnly::ActiveDuration(const AnimationTiming& aTiming)
> {
>-  if (aTiming.mIterationCount == mozilla::PositiveInfinity<float>()) {
>+  const double iterations = aTiming.AdjustedIterations();
>+  if (IsInfinite(iterations)) {

I think this method would actually be better taking aIterations and aIterationDuration members and asserting that they are sensible values, but this is ok for now.

>diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
>index 493db89..5966e00 100644
>--- a/dom/animation/KeyframeEffect.h
>+++ b/dom/animation/KeyframeEffect.h
>@@ -48,20 +48,27 @@ enum class CompositeOperation : uint32_t;
>  * Eventually this will represent all the input timing parameters specified
>  * by content but for now it encapsulates just the subset of those
>  * parameters passed to GetPositionInIteration.
>  */
> struct AnimationTiming
> {
>   TimeDuration mIterationDuration;
>   TimeDuration mDelay;
>-  float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite
>+  // mIterationCount could be a real number, NaN, or +/-Infinity
>+  double mIterationCount;

Let's call this mIterations and change the comment to something like:

    double mIterations; // Can be NaN, negative, +/-Infinity
                        // Use ComputedIterations() to get the normalized value

>   dom::PlaybackDirection mDirection;
>   dom::FillMode mFillMode;
> 
>+  // Return a valid iteration count which is a real number
>+  // greater than or equal to 0 (including positive infinity).
>+  double AdjustedIterations() const {
>+    return (IsNaN(mIterationCount) || mIterationCount < 0.0f) ? 1.0f : mIterationCount;
>+  }

Let's call this ComputedIterations();

>diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp
>index e1121c2..ee5e2a3 100644
>--- a/layout/base/nsDisplayList.cpp
>+++ b/layout/base/nsDisplayList.cpp
>@@ -377,17 +377,17 @@ AddAnimationForProperty(nsIFrame* aFrame, const AnimationProperty& aProperty,
>   Nullable<TimeDuration> startTime = aAnimation->GetCurrentOrPendingStartTime();
>   animation->startTime() = startTime.IsNull()
>                            ? TimeStamp()
>                            : aAnimation->AnimationTimeToTimeStamp(
>                               StickyTimeDuration(timing.mDelay));
>   animation->initialCurrentTime() = aAnimation->GetCurrentTime().Value()
>                                     - timing.mDelay;
>   animation->duration() = timing.mIterationDuration;
>-  animation->iterationCount() = timing.mIterationCount;
>+  animation->iterationCount() = timing.AdjustedIterations();

It would be nice to update the definition in gfx/layers/ipc/LayersMessages.ipdlh as well to use 'iterations' but we can do that in a separate bug.
Attachment #8690654 - Flags: review?(bbirtles) → review+
Comment on attachment 8691730 [details] [diff] [review]
Part 2: Store the original value of fill (v3)

Again, I'd prefer we added mFillMode to ComputedTiming and set it in GetComputedTimingAt. However, if you want to keep this approach, please rename AdjustedFill() to ComputedFill(). Also, we should rename mFillMode to mFill to match AnimationEffectTimingProperties.
Attachment #8691730 - Flags: review?(bbirtles) → review+
Comment on attachment 8690694 [details] [diff] [review]
Part 3: Use OwingUnrestrictedDoubleOrString for duration (v2)

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

r=birtles with the following changes. Again, I think I'd still prefer we do the computation in GetComputedTimingAt, but perhaps we can do that later as a separate step.

::: dom/animation/KeyframeEffect.cpp
@@ +24,5 @@
>  namespace mozilla {
>  
> +TimeDuration
> +AnimationTiming::AdjustedIterationDuration() const
> +{

Let's call this ComputedDuration().

Also, I think this should return a StickyTimeDuration since it can be infinity and will often be combined with other values.

@@ +178,5 @@
>                            const Nullable<TimeDuration>& aLocalTime,
>                            const AnimationTiming& aTiming)
>  {
>    const TimeDuration zeroDuration;
> +  const TimeDuration duration = aTiming.AdjustedIterationDuration();

I suspect you'll want to make both zeroDuration and duration have type StickyTimeDuration.

@@ +242,3 @@
>      iterationTime = isEndOfFinalIteration
> +                    ? StickyTimeDuration(duration)
> +                    : activeTime % duration;

You should be able to drop the StickyTimeDuration() here.

@@ +321,2 @@
>    }
> +  return StickyTimeDuration(duration.MultDouble(iterations));

You might be able to drop the StickTimeDuration wrapper here since duration should be a StickyTimeDuration.

@@ +585,5 @@
>      animationTiming.mFillMode = opt.mFill;
>    } else {
> +    double dur = aOptions.IsUnrestrictedDouble() ?
> +                   aOptions.GetAsUnrestrictedDouble() :
> +                   0.0f;

I think you should line up these last two lines like:

  double dur = aOptions.IsUnrestrictedDouble() ?
               aOptions.GetAsUnrestrictedDouble() :
               0.0;

::: dom/animation/KeyframeEffect.h
@@ +51,5 @@
>   * parameters passed to GetPositionInIteration.
>   */
>  struct AnimationTiming
>  {
> +  dom::OwningUnrestrictedDoubleOrString mIterationDuration;

Call it mDuration to match AnimationEffectTimingProperties?

@@ +62,5 @@
> +  // Return a valid iteration duration as a TimeDuration object.
> +  TimeDuration AdjustedIterationDuration() const;
> +  void SetIterationDuration(const double aDuration) {
> +    mIterationDuration.SetAsUnrestrictedDouble() = aDuration;
> +  }

s/AdjustedIterationDuration()/ComputedDuration()/
s/SetIterationDuration()/SetDuration()/

Also, no need for 'const double'. It's passed by-value.

It seems to me like it would be easier to read this if we grouped the methods together with the members they modify, e.g.

  // iterations
  double mIterations;
  double ComputedIterations() const {
    return (IsNaN(mIterationCount) || mIterationCount < 0.0f) ? 1.0f : mIterationCount;
  }

  // duration
  dom::OwningUnrestrictedDoubleOrString mDuration;
  StickyTimeDuration ComputedDuration() const;
  void SetDuration(double aDuration) {
    mDuration.SetAsUnrestrictedDouble() = aDuration;
  }

  // fill
  dom::FillMode mFill;
  dom::FillMode ComputedFill() const;
  bool FillsForwards() const;
  bool FillsBackwards() const;

@@ +90,5 @@
> +                 !mIterationDuration.IsString() &&
> +                 !aOther.mIterationDuration.IsUnrestrictedDouble() &&
> +                 !aOther.mIterationDuration.IsString();
> +    }
> +    return durEqual &&

Why not write this as:

    bool durationEqual;
    if (mDuration.IsUnrestrictedDouble()) {
      durationEqual = aOther.mDuration.IsUnrestrictedDouble() &&
                      (mDuration.GetAsUnrestrictedDouble() ==
                       aOther.mDuration.GetAsUnrestrictedDouble());
    } else if (mDuration.IsString()) {
      durationEqual = aOther.mDuration.IsString() &&
                      (mDuration.GetAsString() ==
                       aOther.mDuration.GetAsString());
    } else {
      // Check if both are uninitialized
      durationEqual = !aOther.mDuration.IsUnrestrictedDouble() &&
                      !aOther.mDuration.IsString();
    }

    return durationEqual &&
           mDelay == aOther.mDelay &&
           mIterationCount == aOther.mIterationCount &&
           mDirection == aOther.mDirection &&
           mFillMode == aOther.mFillMode;

Optionally, you could factor the first part out into a separate method, CompareUnrestrictedDoubleOrString.
Comment on attachment 8690654 [details] [diff] [review]
Part 1: Use unrestricted double for iterations (v4)

> struct AnimationTiming
> {
>   TimeDuration mIterationDuration;
>   TimeDuration mDelay;
>-  float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite
>+  // mIterationCount could be a real number, NaN, or +/-Infinity
>+  double mIterationCount;

Any reason to change this to double?
Attachment #8690694 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 32

2 years ago
(In reply to Brian Birtles (:birtles) from comment #31)
> Comment on attachment 8690654 [details] [diff] [review]
> Part 1: Use unrestricted double for iterations (v4)
> 
> > struct AnimationTiming
> > {
> >   TimeDuration mIterationDuration;
> >   TimeDuration mDelay;
> >-  float mIterationCount; // mozilla::PositiveInfinity<float>() means infinite
> >+  // mIterationCount could be a real number, NaN, or +/-Infinity
> >+  double mIterationCount;
> 
> Any reason to change this to double?

Just want to make sure mIterationCount has the same type as iterations in AnimationEffectTimingProperties (unresricted double).


BTW,
I will try to use your solution to make sure which one has clearer code. Thanks for your review.
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 33

2 years ago
(In reply to Brian Birtles (:birtles) from comment #25)
> What do you think of doing it this way?

Your solution is also good to me. I think the overhead is that we have to call GetEffect()->GetComputedTiming() in other places (for example: nsDisplayList.cpp) and I'm not sure the overhead of GetComputedTiming(). If it's fast, I think your solution is great (which keeps all computed values in one struct), and I am trying to revise them. Thanks.
(Assignee)

Comment 34

2 years ago
Created attachment 8692418 [details] [diff] [review]
Part 1: Use unrestricted double for iterations (v5)

We want to store the original value from KeyframeEffectOptions whose
iterations is unrestricted double. Therefore, we can get the original value
of iterations by AnimationEffectTimingReadOnly.

By the way, replace mIterationCount with mIterations.
Attachment #8692418 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8690654 - Attachment is obsolete: true
(Assignee)

Comment 35

2 years ago
Created attachment 8692419 [details] [diff] [review]
Part 2: Replace mIterationCount with mIterations (v1)

Replace mIterationCount with mIteraions in StyleAnimation and layers::Animation
Attachment #8692419 - Flags: review?(bbirtles)
(Assignee)

Comment 36

2 years ago
Created attachment 8692423 [details] [diff] [review]
Part 3: Store the original value of fill (v4)

FillMode could be 'auto', and we should treat it as 'none' in the timing model.
However, AnimationEffectTimingReadOnly should get its original value.

By the way, replace mFillMode with mFill.
Attachment #8692423 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8691730 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8690694 - Attachment is obsolete: true
(Assignee)

Comment 38

2 years ago
Created attachment 8692435 [details] [diff] [review]
Part 4: Use OwingUnrestrictedDoubleOrString for duration (v4)

We store the original value of duration in AnimationTiming, and add
computed duration in ComputedTiming, so both the Timing model and
AnimationEffectTimingReadOnly can get what they want.

By the way, replace mIterationDuration with mDuration.
Attachment #8692435 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8692424 - Attachment is obsolete: true
Attachment #8692424 - Flags: review?(bbirtles)
(Assignee)

Comment 39

2 years ago
Created attachment 8692436 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v1)

Add AnimationEffectTimingReadOnly.webidl
Attachment #8692436 - Flags: feedback?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8690695 - Attachment is obsolete: true
Attachment #8691729 - Attachment is obsolete: true
Attachment #8691854 - Attachment is obsolete: true
(Assignee)

Comment 40

2 years ago
Comment on attachment 8692418 [details] [diff] [review]
Part 1: Use unrestricted double for iterations (v5)

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

Sorry, I changed the design, so need your review again. Thanks.
(Assignee)

Comment 41

2 years ago
Created attachment 8692441 [details] [diff] [review]
Part 6: Implement TimingFromJS (v2)

Override TimingFromJS in KeyframeEffectReadOnly.
(Assignee)

Updated

2 years ago
Attachment #8692441 - Flags: review?(bbirtles)
Comment on attachment 8692418 [details] [diff] [review]
Part 1: Use unrestricted double for iterations (v5)

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

::: dom/animation/KeyframeEffect.h
@@ +88,5 @@
>    Nullable<double>    mProgress;
>    // Zero-based iteration index (meaningless if mProgress is null).
>    uint64_t            mCurrentIteration = 0;
> +  // Computed iterations
> +  double              mIterations = 1.0;

No need to say "computed", we're in the "ComputedTiming" struct.

Perhaps instead:

  // Unlike AnimationTiming::mIterations, this value is guaranteed to be in the
  // range [0, Infinity].

::: layout/style/nsTransitionManager.cpp
@@ +656,5 @@
>  
>    AnimationTiming timing;
>    timing.mIterationDuration = TimeDuration::FromMilliseconds(duration);
>    timing.mDelay = TimeDuration::FromMilliseconds(delay);
> +  timing.mIterations = 1.0;

As suggested in bug 1215406 comment 56, we should probably add a default constructor for AnimationTiming that sets these values to sensible defaults (probably using the dictionary defaults defined for AnimationEffectTimingProperties)--then we could simplify this code and ConvertKeyframeEffectOptions.

That's a separate patch/bug however.
Attachment #8692418 - Flags: review?(bbirtles) → review+
Comment on attachment 8692419 [details] [diff] [review]
Part 2: Replace mIterationCount with mIterations (v1)

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

I don't think we should change StyleAnimation since it maps to the animation-iteration-count property.

The changes to AsyncCompositionManager.cpp and LayersMessages.ipdlh look fine however.
Attachment #8692419 - Flags: review?(bbirtles)
Comment on attachment 8692423 [details] [diff] [review]
Part 3: Store the original value of fill (v4)

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

This doesn't seem right. If we are going to store dom::FillMode::Auto on AnimationTiming then AnimationTiming::FillsForwards() and FillBackwards() need to handle it.

It's probably better to move FillsForwards and FillsBackwards to ComputedTiming and, inside ComputedTiming::FillsForwards/Backwards(), assert that mFill is not Auto. We should also add a default value for mFill in ComputedTiming (probably none would be ok) so that when FillsFowards/FillsBackwards is called mFill is guaranteed to be set to a known value.
Attachment #8692423 - Flags: review?(bbirtles)
Comment on attachment 8692435 [details] [diff] [review]
Part 4: Use OwingUnrestrictedDoubleOrString for duration (v4)

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

r=birtles with comments addressed

::: dom/animation/KeyframeEffect.cpp
@@ +161,4 @@
>  
> +  // Always return the same object to benefit from return-value optimization.
> +  ComputedTiming result;
> +  if (aTiming.mDuration.IsUnrestrictedDouble()) {

Add a space before this last line otherwise it appears that the comment, "Always return..." applies to this line too.

@@ +163,5 @@
> +  ComputedTiming result;
> +  if (aTiming.mDuration.IsUnrestrictedDouble()) {
> +    double durationMs = aTiming.mDuration.GetAsUnrestrictedDouble();
> +    if (!IsNaN(durationMs) && durationMs >= 0.0f) {
> +      result.mDuration = TimeDuration::FromMilliseconds(durationMs);

Shouldn't this by StickyTimeDuration::FromMilliseconds?

@@ +176,1 @@
>               "Expecting iteration duration >= 0");

We can drop this assertion and the comment now since we ensure this is true in the lines above.

@@ +269,2 @@
>                 "In the active phase of a zero-duration animation?");
> +    double progress = result.mDuration == TimeDuration::Forever()

Nit: It might be slightly more natural to use StickyTimeDuration::Forever()?

@@ +313,2 @@
>    }
> +  return StickyTimeDuration(aComputedDuration.MultDouble(aComputedIterations));

Do we need the StickyTimeDuration() here? Doesn't MultDouble return the right kind of object for us?

@@ +583,5 @@
>      animationTiming.mDelay = TimeDuration(0);
>      animationTiming.mIterations = 1.0f;
>      animationTiming.mDirection = PlaybackDirection::Normal;
>      animationTiming.mFill = FillMode::None;
>    }

Again, if we give AnimationTiming a default constructor we can change this whole branch to just:

} else if (aOptions.IsUnrestrictedDouble()) {
  animationTiming.mDuration.SetAsUnrestrictedDouble() =
    aOptions.GetAsUnrestrictedDouble();
}

::: dom/animation/KeyframeEffect.h
@@ +107,5 @@
>    double              mIterations = 1.0;
>    // Computed fill
>    dom::FillMode       mFill;
> +  // Computed duration
> +  StickyTimeDuration  mDuration;

Once again, no need to repeat "computed" here. The type tells you that the duration is not "auto" so I think we can simply leave this without a comment.

@@ +271,5 @@
>    // Return the duration of the active interval for the given timing parameters.
>    static StickyTimeDuration
>    ActiveDuration(const AnimationTiming& aTiming,
> +                 double aComputedIterations,
> +                 const StickyTimeDuration& aComputedDuration);

I don't think we need the aTiming parameter any more, right?

(I also think it would be slightly more natural to put the duration parameter first.)

And perhaps we can update the comment too, e.g.:

  // Return the duration of the active interval for the given duration and
  // iteration count.
  ActiveDuration(const StickyTimeDuration& aIterationDuration,
                 double aIterationCount);

::: layout/style/nsTransitionManager.cpp
@@ +148,5 @@
>  
>    nsTransitionManager* manager = presContext->TransitionManager();
>    manager->QueueEvent(TransitionEventInfo(owningElement, owningPseudoType,
>                                            TransitionProperty(),
> +                                          mEffect->GetComputedTiming().mDuration,

I think this line may be over 80 characters long.
Attachment #8692435 - Flags: review?(bbirtles) → review+
Comment on attachment 8692436 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v1)

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

AnimationEffectTimingReadOnly is supposed to be a live object, i.e. should reflect the current values. We should just use AnimationEffectTimingReadOnly in place of AnimationTiming.
Attachment #8692436 - Flags: feedback?(bbirtles) → feedback-
Comment on attachment 8692441 [details] [diff] [review]
Part 6: Implement TimingFromJS (v2)

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

Based on my comments for part 5, this patch no longer makes sense.
Attachment #8692441 - Flags: review?(bbirtles)
(Assignee)

Comment 48

2 years ago
(In reply to Brian Birtles (:birtles) from comment 42
> ::: layout/style/nsTransitionManager.cpp
> As suggested in bug 1215406 comment 56, we should probably add a default
> constructor for AnimationTiming that sets these values to sensible defaults
> (probably using the dictionary defaults defined for
> AnimationEffectTimingProperties)--then we could simplify this code and
> ConvertKeyframeEffectOptions.
> 
> That's a separate patch/bug however.

OK. I will handle this later.
(Assignee)

Comment 49

2 years ago
(In reply to Brian Birtles (:birtles) from comment #44)
> Comment on attachment 8692423 [details] [diff] [review]
> Part 3: Store the original value of fill (v4)
> 
> Review of attachment 8692423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem right. If we are going to store dom::FillMode::Auto on
> AnimationTiming then AnimationTiming::FillsForwards() and FillBackwards()
> need to handle it.
> 
> It's probably better to move FillsForwards and FillsBackwards to
> ComputedTiming and, inside ComputedTiming::FillsForwards/Backwards(), assert
> that mFill is not Auto. We should also add a default value for mFill in
> ComputedTiming (probably none would be ok) so that when
> FillsFowards/FillsBackwards is called mFill is guaranteed to be set to a
> known value.

Oops, I didn't notice this part. Thanks for your rectification.
(Assignee)

Comment 50

2 years ago
Created attachment 8692732 [details] [diff] [review]
Part 1: Use unrestricted double for iterations (v6, carry birtles' r+)

We want to store the original value from KeyframeEffectOptions whose
iterations is unrestricted double. Therefore, we can get the original value
of iterations by AnimationEffectTimingReadOnly.

By the way, replace mIterationCount with mIterations.
Attachment #8692732 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8692418 - Attachment is obsolete: true
(Assignee)

Comment 51

2 years ago
Created attachment 8692735 [details] [diff] [review]
Part 2: Replace mIterationCount in layers::Animation (v2)

Replace mIterationCount with mIterations.
(Assignee)

Updated

2 years ago
Attachment #8692419 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8692735 - Flags: review?(bbirtles)
(Assignee)

Comment 52

2 years ago
Created attachment 8692738 [details] [diff] [review]
Part 3: Store the original value of fill (v5)

FillMode could be 'auto', and we should treat it as 'none' in the timing model.
However, AnimationEffectTimingReadOnly should get its original value.

By the way, replace mFillMode with mFill.
(Assignee)

Updated

2 years ago
Attachment #8692423 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8692738 - Flags: review?(bbirtles)
Attachment #8692735 - Flags: review?(bbirtles) → review+
Comment on attachment 8692738 [details] [diff] [review]
Part 3: Store the original value of fill (v5)

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

r=birtles with comments addressed

::: dom/animation/KeyframeEffect.cpp
@@ +26,5 @@
> +ComputedTiming::ComputedTiming()
> +  : mCurrentIteration(0)
> +  , mIterations(0.0)
> +  , mFill(dom::FillMode::None)
> +  , mPhase(AnimationPhase::Null)

Instead of adding a non-inline constructor, I think the member-initializer format is fine (and more likely to be inlined). Just add the initialization of mFill.

@@ +28,5 @@
> +  , mIterations(0.0)
> +  , mFill(dom::FillMode::None)
> +  , mPhase(AnimationPhase::Null)
> +{
> +  mProgress.SetNull();

This is not needed, Nullable initializes to a null value.

@@ +37,2 @@
>  {
> +  MOZ_ASSERT(mFill == dom::FillMode::Auto, "mFill couldn't be Auto.");

"mFill should not be Auto in ComputedTiming"

@@ +45,2 @@
>  {
> +  MOZ_ASSERT(mFill == dom::FillMode::Auto, "mFill couldn't be Auto.");

Ditto.

::: dom/animation/KeyframeEffect.h
@@ +91,5 @@
>    // Unlike AnimationTiming::mIterations, this value is guaranteed to be in the
>    // range [0, Infinity].
> +  double              mIterations;
> +
> +  // This is guaranteed to be None, Forwards, Backwards, or Both.

"This is the computed fill mode so it is number auto"
Attachment #8692738 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 54

2 years ago
(In reply to Brian Birtles (:birtles) from comment #53)
> Comment on attachment 8692738 [details] [diff] [review]
> Part 3: Store the original value of fill (v5)
> 
> Review of attachment 8692738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=birtles with comments addressed
> 
> ::: dom/animation/KeyframeEffect.cpp
> @@ +26,5 @@
> > +ComputedTiming::ComputedTiming()
> > +  : mCurrentIteration(0)
> > +  , mIterations(0.0)
> > +  , mFill(dom::FillMode::None)
> > +  , mPhase(AnimationPhase::Null)
> 
> Instead of adding a non-inline constructor, I think the member-initializer
> format is fine (and more likely to be inlined). Just add the initialization
> of mFill.

Actually, the reason is: I don't want to include the definition of dom::FillMode in KeyframeEffect.h to workaround some compilation errors on Linux. Therefore, add a non-inline constructor here.

> 
> @@ +28,5 @@
> > +  , mIterations(0.0)
> > +  , mFill(dom::FillMode::None)
> > +  , mPhase(AnimationPhase::Null)
> > +{
> > +  mProgress.SetNull();
> 
> This is not needed, Nullable initializes to a null value.

OK. I will remove this.
(In reply to Boris Chiou [:boris] from comment #54)
> > Instead of adding a non-inline constructor, I think the member-initializer
> > format is fine (and more likely to be inlined). Just add the initialization
> > of mFill.
> 
> Actually, the reason is: I don't want to include the definition of
> dom::FillMode in KeyframeEffect.h to workaround some compilation errors on
> Linux. Therefore, add a non-inline constructor here.

You can't just #undef None?
(Assignee)

Comment 56

2 years ago
(In reply to Brian Birtles (:birtles) from comment #55)
> (In reply to Boris Chiou [:boris] from comment #54)
> > > Instead of adding a non-inline constructor, I think the member-initializer
> > > format is fine (and more likely to be inlined). Just add the initialization
> > > of mFill.
> > 
> > Actually, the reason is: I don't want to include the definition of
> > dom::FillMode in KeyframeEffect.h to workaround some compilation errors on
> > Linux. Therefore, add a non-inline constructor here.
> 
> You can't just #undef None?

I can #undef None if it's necessary to include the definition in KeyframeEffect.h. I will upload a new patch soon to let you review again. Thanks.
(Assignee)

Comment 57

2 years ago
Created attachment 8692786 [details] [diff] [review]
Part 3: Store the original value of fill (v6)

FillMode could be 'auto', and we should treat it as 'none' in the timing model.
However, AnimationEffectTimingReadOnly should get its original value.

By the way, replace mFillMode with mFill.
(Assignee)

Updated

2 years ago
Attachment #8692738 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8692786 - Flags: review?(bbirtles)
Comment on attachment 8692786 [details] [diff] [review]
Part 3: Store the original value of fill (v6)

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

r=birtles with comments addressed

::: dom/animation/KeyframeEffect.h
@@ +24,5 @@
>  #include "mozilla/dom/Nullable.h"
>  
> +// Fix X11 header brain damage that conflicts with dom::FillMode::None
> +#undef None
> +#include "mozilla/dom/AnimationEffectReadOnlyBinding.h"

Just something like:

  // X11 has a #define for None
  #ifdef None
  #undef None
  #endif

would be fine :)

(Similar to what we have in Animation.h)

@@ +92,5 @@
>    // Unlike AnimationTiming::mIterations, this value is guaranteed to be in the
>    // range [0, Infinity].
>    double              mIterations = 1.0;
>  
> +  // This is the computed fill mode so it is number auto

Sorry, I made a mistake in my original comment. I meant "never auto" (not "number auto").

@@ +96,5 @@
> +  // This is the computed fill mode so it is number auto
> +  dom::FillMode       mFill = dom::FillMode::None;
> +  bool FillsForwards() const {
> +    MOZ_ASSERT(mFill == dom::FillMode::Auto,
> +               "mFill should not be Auto in ComputedTiming");

mFill != dom::FillMode::Auto

@@ +101,5 @@
> +    return mFill == dom::FillMode::Both ||
> +           mFill == dom::FillMode::Forwards;
> +  }
> +  bool FillsBackwards() const {
> +    MOZ_ASSERT(mFill == dom::FillMode::Auto,

Same here
Attachment #8692786 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 59

2 years ago
Created attachment 8692789 [details] [diff] [review]
Part 3: Store the original value of fill. (v7, carry birtles' r+)

FillMode could be 'auto', and we should treat it as 'none' in the timing model.
However, AnimationEffectTimingReadOnly should get its original value.

By the way, replace mFillMode with mFill.
Attachment #8692789 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8692786 - Attachment is obsolete: true
(Assignee)

Comment 60

2 years ago
Created attachment 8692790 [details] [diff] [review]
Part 4: Use OwingUnrestrictedDoubleOrString for duration. (v5, carry birtles' r+)

We store the original value of duration in AnimationTiming, and add
computed duration in ComputedTiming, so both the Timing model and
AnimationEffectTimingReadOnly can get what they want.

By the way, replace mIterationDuration with mDuration.
Attachment #8692790 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8692435 - Attachment is obsolete: true
(Assignee)

Comment 61

2 years ago
(In reply to Brian Birtles (:birtles) from comment #46)
> Comment on attachment 8692436 [details] [diff] [review]
> Part 5: Add AnimationEffectTimingReadOnly interface (v1)
> 
> Review of attachment 8692436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> AnimationEffectTimingReadOnly is supposed to be a live object, i.e. should
> reflect the current values. We should just use AnimationEffectTimingReadOnly
> in place of AnimationTiming.

Should I try to replace AnimationTiming with AnimationEffectTimingReadOnly directly or let AnimationEffectTimingReadOnly has a data member of AnimationTiming? I'm not sure whether we should keep AnimationTiming or not.
(Assignee)

Comment 62

2 years ago
(In reply to Brian Birtles (:birtles) from comment #42)
> As suggested in bug 1215406 comment 56, we should probably add a default
> constructor for AnimationTiming that sets these values to sensible defaults
> (probably using the dictionary defaults defined for
> AnimationEffectTimingProperties)--then we could simplify this code and
> ConvertKeyframeEffectOptions.
> 
> That's a separate patch/bug however.

If we replace AnimationTiming with AnimationEffectTimingReadOnly, we could add a constructor in AnimoationEffectTimingReadOnly to simplify these code.
(In reply to Boris Chiou [:boris] from comment #61)
> Should I try to replace AnimationTiming with AnimationEffectTimingReadOnly
> directly or let AnimationEffectTimingReadOnly has a data member of
> AnimationTiming? I'm not sure whether we should keep AnimationTiming or not.

At first I thought we should replace AnimationTiming with AnimationEffectTimingReadOnly. However, I think we create AnimationTiming objects on the compositor in order to pass to GetComputedTimingAt. It would be unfortunate to have to create a new heap object for every animation on the compositor for every tick just to calculate the current time.

So probably we want to keep a lightweight object AnimationTiming and pass that to GetComputedTimingAt.  Then AnimationEffectTimingReadOnly just needs to (non-owning) pointer back to its effect and return values from its effect.

The difficulty is orphaned objects. AnimationEffectTimingReadOnly objects should probably not keep their associated effect alive but script can keep a handle to one of these objects even after the Animation is garbage-collected. (Furthermore,  in future it should be possible to construct AnimationEffectTimingReadOnly objects without an effect so the same situation can arise there). There are probably a few different approaches:

1. Make AnimationEffectTimingReadOnly work in two different modes: one where it reads timing properties from an associated effect; and one, an orphan mode, where it uses local timing properties. This is what we did a lot in the SVG DOM and I think we decided it was too complex in the end. The advantage is we would only create the AnimationEffectTimingReadOnly object on-demand: most of the time it would not be needed.

2. Make AnimationEffectTimingReadOnly store all its timing properties locally (i.e. it has an mTiming member of type AnimationTiming) and when we call GetComputedTimingAt, we basically look up mTiming->mTiming.

There are probably a number of other ways. For some approaches we can probably use the WebIDL bindings to take care of tracking the tear-off object for us.
Just to outline the first approach a bit more, we'd basically make an timing tear-off object with a raw-pointer to an AnimationTiming object and an orphan flag.

When the timing object is attached to an effect, it will be passed a pointer to the effect's AnimationTiming object. When the effect is destroyed, it tells the tear-off object which allocates a new AnimationTiming object on the heap with a copy of the effect's AnimationTiming, updates it pointer to point to the new heap object, and set the orphan flag. When the tear-off object is destroyed, it checks the orphan flag, and if it is set, frees the memory for the heap-allocated timing object.

That's pretty much how we do the SVG DOM stuff but in this case it's quite simple because we already have the AnimationTiming object and the tear-off object is read-only.
Blocks: 1216842
(Assignee)

Comment 65

2 years ago
(In reply to Brian Birtles (:birtles) from comment #64)
> Just to outline the first approach a bit more, we'd basically make an timing
> tear-off object with a raw-pointer to an AnimationTiming object and an
> orphan flag.
> 
> When the timing object is attached to an effect, it will be passed a pointer
> to the effect's AnimationTiming object. When the effect is destroyed, it
> tells the tear-off object which allocates a new AnimationTiming object on
> the heap with a copy of the effect's AnimationTiming, updates it pointer to
> point to the new heap object, and set the orphan flag. When the tear-off
> object is destroyed, it checks the orphan flag, and if it is set, frees the
> memory for the heap-allocated timing object.
> 
> That's pretty much how we do the SVG DOM stuff but in this case it's quite
> simple because we already have the AnimationTiming object and the tear-off
> object is read-only.

Your explanation is helpful. However, I'm not familiar with SVG DOM stuffs,
so I want to make sure if I catch on this idea:
1. When someone calls KeyframeEffectReadOnly::Timing(), we new an AnimationEffectTimingReadOnly
   object which accesses the AnimationTiming object in KeyframeEffectReadOnly.
   (Now, the timing object is attached to an effect.)
2. KeyframeEffectReadOnly asks this AnimationEffectTimingReadOnly object to allocates a
   new AnimationTiming object on the heap in the destroctor (i.e. in ~KeyframeEffectReadOnly()).
   (Now, the timing object is an orphan.)

I'm not sure who should own this AnimationEffectTimingReadOnly object. When we create a new different effect, should it find an orphan timing object and let it to be attached?

Thanks.
There are a lot of ways you could do it. One way might be something like:

class KeyframeEffectReadOnly
{
  ~KeyframeEffectReadOnly() {
    if (mTiming) {
      mTiming.Orphan();
    }
  }

  RefPtr<AnimationEffectTimingReadOnly> Timing()
  {
    // (might need some const-cast / mutable annotations if Timing() is const)
    if (!mTiming) {
      mTiming = new AnimationEffectTimingReadOnly(mTimingParams);
    }
    return mTiming;
  }

  AnimationTiming mTimingParams;
  RefPtr<AnimationEffectTimingReadOnly> mTiming;
}

class AnimationEffectTimingReadOnly
{
public:
  explicit AnimationEffectTimingReadOnly(AnimationTiming& aTiming)
  : mTiming(&aTiming)
  {
  }

  void Orphan()
  {
    if (mLocalTiming && !mTiming) {
      mLocalTiming = new AnimationTiming(*mLocalTiming);
      mTiming = &mLocalTiming;
    }
  }

  // e.g.
  PlaybackDirection Direction() const {
    return mTiming->mDirection;
  }

protected:
  AnimationTiming* mTimingParams;
  nsAutoPtr<AnimationTiming> mLocalTiming;
}

You might be able to use something like [Constant, Cached] on the WebIDL and re-use that to track the mTiming object? I'm not sure.


Alternatively, you could create the tear-off object from the start, something like:

class KeyframeEffectReadOnly
{
  KeyframeEffectReadOnly()
  : mTiming(new AnimationEffectTimingReadOnly())
  {
  }

  RefPtr<AnimationEffectTimingReadOnly> Timing()
  {
    return mTiming;
  }

  RefPtr<AnimationEffectTimingReadOnly> mTiming;
}

class AnimationEffectTimingReadOnly
{
public:
  // e.g.
  PlaybackDirection Direction() const {
    return mTiming.mDirection;
  }

  AnimationTiming mTiming;
}

The second approach seems more simple. The main disadvantage is an extra allocation for every animation.

I think when we implement writeable timing objects, and replaceable timing objects, the second approach is going to be more simple. So I think perhaps the second approach is better here.
(Assignee)

Comment 67

2 years ago
(In reply to Brian Birtles (:birtles) from comment #66)
> The second approach seems more simple. The main disadvantage is an extra
> allocation for every animation.
> 
> I think when we implement writeable timing objects, and replaceable timing
> objects, the second approach is going to be more simple. So I think perhaps
> the second approach is better here.

Very clear :). Yes, second one is more intuitive. If the memory allocation is not critical, I would try to implement 2nd idea. Thanks.
(Assignee)

Comment 68

2 years ago
(In reply to Brian Birtles (:birtles) from comment #66)
> The second approach seems more simple. The main disadvantage is an extra
> allocation for every animation.
> 
> I think when we implement writeable timing objects, and replaceable timing
> objects, the second approach is going to be more simple. So I think perhaps
> the second approach is better here.


In the second approach, mTiming is a read-only object, but we still need to provide an API to reset the timing object (SetTiming(...)[1]). If we just wrap AnimationTiming by AnimationEffectTimingReadOnly, we can not revise it as before. I think We should use a workaround method to handle this case or remove the API.

[1]https://dxr.mozilla.org/mozilla-central/source/layout/style/nsAnimationManager.cpp#493
(In reply to Boris Chiou [:boris] (away 12/1~12/4) from comment #68)
> In the second approach, mTiming is a read-only object, but we still need to
> provide an API to reset the timing object (SetTiming(...)[1]). If we just
> wrap AnimationTiming by AnimationEffectTimingReadOnly, we can not revise it
> as before. I think We should use a workaround method to handle this case or
> remove the API.

I'm not sure I understand the concern.

I think the simplest way to organize this is as follows:

* KeyframeEffectReadOnly (or AnimationEffectReadOnly if you decide to do it there) *always* has a valid mTiming object. It is a non-nullable readonly attribute in the IDL so I think it makes sense to initialize mTiming to a new object when you create an AnimationEffectReadOnly object.
* KeyframeEffectReadOnly::Timing() always returns that object (we can use [Cached, Constant] in the WebIDL to enforce this, I believe)
* In nsAnimationManager etc., when you want to update the timing properties, call effect->Timing().mTiming = timingParams
* You could even implement the assignment operator or something similar so that when we copy timing from one effect to the other you can do:
  *oldEffect->Timing() = *newEffect->Timing()
(* You might find it easier to rename AnimationTiming to AnimationEffectTimingStruct? or something so that the names don't get too confusing?)
* You could also move some of the code for handling KeyframeEffectOptions to this object in order to simplify KeyframeEffectReadOnly.

For example, something like:

struct AnimationEffectTimingStruct
{
  // Technically, we should initialize mDuration to "auto", but that will mean
  // allocating an extra string buffer for every object, even if that value is
  // to be destroyed straight away.
  //
  // For now we know that every code path that generates an
  // AnimationEffectTimingStruct will also end up setting mDuration so we
  // can just leave it unitialized. In future, when we implement a constructor
  // AnimationEffectTimingReadOnly we might investigate other ways of storing
  // duration so that we don't need a string to represent "auto".
  // Perhaps we could even use the unitialized state to represent "auto"?
  dom::OwningUnrestrictedDoubleOrString mDuration;
  TimeDuration mIterationDuration;
  TimeDuration mDelay;
  double mIterations = 1.0;
  dom::PlaybackDirection mDirection = dom::PlaybackDirection::Normal;
  dom::FillMode mFill = dom::FillMode::Auto;

  bool operator==(const AnimationTiming& aOther) const { ... }
  bool operator!=(const AnimationTiming& aOther) const { ... }
};

class AnimationEffectTimingReadOnly
{
public:
  SetTiming(const AnimationEffectTimingProperties& aTiming);
  /* The following is not needed if we leave mTiming as public */
  SetTiming(const AnimationEffectTimingStruct& aTiming);

  AnimationEffectTimingReadOnly&
  operator= (const AnimationEffectTimingReadOnly& aOther);

  AnimationEffectTimingStruct mTiming;
};
(Assignee)

Comment 70

2 years ago
(In reply to Brian Birtles (:birtles) from comment #69)
> (In reply to Boris Chiou [:boris] (away 12/1~12/4) from comment #68)
> > In the second approach, mTiming is a read-only object, but we still need to
> > provide an API to reset the timing object (SetTiming(...)[1]). If we just
> > wrap AnimationTiming by AnimationEffectTimingReadOnly, we can not revise it
> > as before. I think We should use a workaround method to handle this case or
> > remove the API.
> 
> I'm not sure I understand the concern.
> 
> I think the simplest way to organize this is as follows:
> 
> * KeyframeEffectReadOnly (or AnimationEffectReadOnly if you decide to do it
> there) *always* has a valid mTiming object. It is a non-nullable readonly
> attribute in the IDL so I think it makes sense to initialize mTiming to a
> new object when you create an AnimationEffectReadOnly object.
> * KeyframeEffectReadOnly::Timing() always returns that object (we can use
> [Cached, Constant] in the WebIDL to enforce this, I believe)
> * In nsAnimationManager etc., when you want to update the timing properties,
> call effect->Timing().mTiming = timingParams

Sorry for replying lately. Thanks for your instructions step by step. It's really helpful.
I think my question is about this part: what does "readonly" mean in the webidl language. Could we revise the data member of this "readonly" attribute by any assignment? Thanks.


> * You could even implement the assignment operator or something similar so
> that when we copy timing from one effect to the other you can do:
>   *oldEffect->Timing() = *newEffect->Timing()
> (* You might find it easier to rename AnimationTiming to
> AnimationEffectTimingStruct? or something so that the names don't get too
> confusing?)
> * You could also move some of the code for handling KeyframeEffectOptions to
> this object in order to simplify KeyframeEffectReadOnly.
(In reply to Boris Chiou [:boris] (away 12/14~12/17) from comment #70)
> (In reply to Brian Birtles (:birtles) from comment #69)
> > * KeyframeEffectReadOnly (or AnimationEffectReadOnly if you decide to do it
> > there) *always* has a valid mTiming object. It is a non-nullable readonly
> > attribute in the IDL so I think it makes sense to initialize mTiming to a
> > new object when you create an AnimationEffectReadOnly object.
> > * KeyframeEffectReadOnly::Timing() always returns that object (we can use
> > [Cached, Constant] in the WebIDL to enforce this, I believe)
> > * In nsAnimationManager etc., when you want to update the timing properties,
> > call effect->Timing().mTiming = timingParams
> 
> Sorry for replying lately. Thanks for your instructions step by step. It's
> really helpful.
> I think my question is about this part: what does "readonly" mean in the
> webidl language. Could we revise the data member of this "readonly"
> attribute by any assignment? Thanks.

In WebIDL, readonly means you can't replace the attribute's value with a new object.

For example, if you have:

  interface A {
    readonly attribute B b;
  };

  interface B {
    attribute DOMString string;
  };

Then, if you have an object |a|, that implements interface A, you can do:

  a.b.string = "test"; // ok

However, you can't do:

  a.b = a.b; // TypeError exception thrown in strict mode

So in this case you can *never* do:

  effectA.timing = effectB.timing // TypeError!

But, if effectA is a KeyframeEffect object, then its 'timing' attribute will be a AnimationEffectTiming object so you *can* do:

  effectA.timing.duration = 3000; // Ok if 'timing' is an AnimationEffectTiming object
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8692436 - Attachment is obsolete: true
Attachment #8692441 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8699822 - Attachment is obsolete: true
(Assignee)

Comment 74

2 years ago
Created attachment 8699824 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v3)

Add AnimationEffectTimingReadOnly.webidl
(Assignee)

Updated

2 years ago
Attachment #8699823 - Attachment is obsolete: true
(Assignee)

Comment 75

2 years ago
Created attachment 8699878 [details] [diff] [review]
Part 4: Use OwingUnrestrictedDoubleOrString for duration. (v5, carry birtles' r+)

We store the original value of duration in AnimationTiming, and add
computed duration in ComputedTiming, so both the Timing model and
AnimationEffectTimingReadOnly can get what they want.

By the way, replace mIterationDuration with mDuration.

Rebased.
Attachment #8699878 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8692790 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8700466 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Comment 79

2 years ago
Created attachment 8700481 [details] [diff] [review]
Part 6: Add AnimationEffectTimingReadOnly cpp files (v3)

Add AnimationEffectTimingReadOnly.h/cpp and move AnimationTiming into
AnimatinoRffectTimingReadOnly.h.
Rename AnimationTiming as AnimationEffectTimingStruct.
(Assignee)

Updated

2 years ago
Attachment #8700471 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8700472 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8700482 - Attachment is obsolete: true
Attachment #8700484 - Attachment is obsolete: true
Attachment #8700486 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8700495 - Attachment is obsolete: true
(Assignee)

Comment 86

2 years ago
Created attachment 8700500 [details] [diff] [review]
Part 7: Replace AnimationTiming with AnimationEffectTimingStruct (v5)
(Assignee)

Updated

2 years ago
Attachment #8700497 - Attachment is obsolete: true
(Assignee)

Comment 87

2 years ago
Created attachment 8700501 [details] [diff] [review]
Part 8: Move ConvertKeyframeEffectOptions (v3)

Move ConvertKeyframeEffectOptions() from KeyframeEffectReadOnly into
AnimationEffectTimingReadOnly.
(Assignee)

Updated

2 years ago
Attachment #8700496 - Attachment is obsolete: true
(Assignee)

Comment 88

2 years ago
Created attachment 8700502 [details] [diff] [review]
Part 9: Implement Timing() in KeyframeEffectReadOnly (v2)

Replace AnimationEffectTimingStruct with AnimationEffectTimingReadOnly in
KeyframeEffectReadOnly and implement KeyframeEffectReadOnly::Timing().
(Assignee)

Comment 89

2 years ago
Created attachment 8701398 [details] [diff] [review]
[WIP] Part 10: Test (v1)
(Assignee)

Updated

2 years ago
Attachment #8699824 - Flags: feedback?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8700481 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8700500 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8700501 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8700502 - Flags: review?(bbirtles)
Comment on attachment 8699824 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v3)

Looks fine to me but obviously you need a DOM peer to review this.

Does this patch compile, however? Don't you need at least a stub implementation?
Attachment #8699824 - Flags: feedback?(bbirtles) → feedback+
(Assignee)

Comment 91

2 years ago
(In reply to Brian Birtles (:birtles) from comment #90)
> Comment on attachment 8699824 [details] [diff] [review]
> Part 5: Add AnimationEffectTimingReadOnly interface (v3)
> 
> Looks fine to me but obviously you need a DOM peer to review this.
> 
> Does this patch compile, however? Don't you need at least a stub
> implementation?

I put the other part in Part 9. I can move them back to this part and then send a review request to DOM peer. Thanks.
(In reply to Boris Chiou [:boris] from comment #91)
> (In reply to Brian Birtles (:birtles) from comment #90)
> > Comment on attachment 8699824 [details] [diff] [review]
> > Part 5: Add AnimationEffectTimingReadOnly interface (v3)
> > 
> > Looks fine to me but obviously you need a DOM peer to review this.
> > 
> > Does this patch compile, however? Don't you need at least a stub
> > implementation?
> 
> I put the other part in Part 9. I can move them back to this part and then
> send a review request to DOM peer. Thanks.

Ideally, you should be able to apply each patch in turn and be able to compile and pass all tests. That makes finding regressions a lot easier.
(Assignee)

Comment 93

2 years ago
Comment on attachment 8700502 [details] [diff] [review]
Part 9: Implement Timing() in KeyframeEffectReadOnly (v2)

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

I will move the changes in AnimationEffectReadOnly.h/cpp into part 5. Thanks.
Comment hidden (typo)
(Assignee)

Comment 95

2 years ago
Created attachment 8701676 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v4)

Add AnimationEffectTimingReadOnly.webidl

Move stuff implementation into this part.
(Assignee)

Updated

2 years ago
Attachment #8699824 - Attachment is obsolete: true
Comment on attachment 8701676 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v4)

>diff --git a/dom/animation/AnimationEffectReadOnly.cpp b/dom/animation/AnimationEffectReadOnly.cpp
>index 2363b31..673bc7d 100644
>--- a/dom/animation/AnimationEffectReadOnly.cpp
>+++ b/dom/animation/AnimationEffectReadOnly.cpp
>@@ -15,10 +15,16 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnimationEffectReadOnly, mParent)
> NS_IMPL_CYCLE_COLLECTING_ADDREF(AnimationEffectReadOnly)
> NS_IMPL_CYCLE_COLLECTING_RELEASE(AnimationEffectReadOnly)
> 
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnimationEffectReadOnly)
>   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>   NS_INTERFACE_MAP_ENTRY(nsISupports)
> NS_INTERFACE_MAP_END
> 
>+already_AddRefed<AnimationEffectTimingReadOnly>
>+AnimationEffectReadOnly::Timing() const
>+{
>+  return RefPtr<AnimationEffectTimingReadOnly>().forget();

I don't understand how this works. Where do we add the connection to KeyframeEffect's mTiming member?

Also, does this compile? Or are you going to rework all the other patches?
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 97

2 years ago
(In reply to Brian Birtles (:birtles) from comment #96)
> Comment on attachment 8701676 [details] [diff] [review]
> Part 5: Add AnimationEffectTimingReadOnly interface (v4)
> 
> >diff --git a/dom/animation/AnimationEffectReadOnly.cpp b/dom/animation/AnimationEffectReadOnly.cpp
> >index 2363b31..673bc7d 100644
> >--- a/dom/animation/AnimationEffectReadOnly.cpp
> >+++ b/dom/animation/AnimationEffectReadOnly.cpp
> >@@ -15,10 +15,16 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnimationEffectReadOnly, mParent)
> > NS_IMPL_CYCLE_COLLECTING_ADDREF(AnimationEffectReadOnly)
> > NS_IMPL_CYCLE_COLLECTING_RELEASE(AnimationEffectReadOnly)
> > 
> > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnimationEffectReadOnly)
> >   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> >   NS_INTERFACE_MAP_ENTRY(nsISupports)
> > NS_INTERFACE_MAP_END
> > 
> >+already_AddRefed<AnimationEffectTimingReadOnly>
> >+AnimationEffectReadOnly::Timing() const
> >+{
> >+  return RefPtr<AnimationEffectTimingReadOnly>().forget();
> 
> I don't understand how this works. Where do we add the connection to
> KeyframeEffect's mTiming member?
> 
> Also, does this compile? Or are you going to rework all the other patches?

OK. I rework all the other patches and make sure each patch can be compiled. Thanks.
Flags: needinfo?(boris.chiou)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8700481 - Attachment is obsolete: true
Attachment #8700481 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8700502 - Attachment is obsolete: true
Attachment #8701676 - Attachment is obsolete: true
Attachment #8700502 - Flags: review?(bbirtles)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8701721 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8700500 - Attachment is obsolete: true
Attachment #8700500 - Flags: review?(bbirtles)
(Assignee)

Comment 101

2 years ago
Created attachment 8701725 [details] [diff] [review]
Part 7: Move ConvertKeyframeEffectOptions (v4)

Move ConvertKeyframeEffectOptions() from KeyframeEffectReadOnly into
AnimationEffectTimingReadOnly.
(Assignee)

Updated

2 years ago
Attachment #8700501 - Attachment is obsolete: true
Attachment #8700501 - Flags: review?(bbirtles)
(Assignee)

Comment 102

2 years ago
Created attachment 8701726 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v7)

1. Add AnimationEffectTimingReadOnly.webidl.
2. Add AnimationEffectTimingReadOnly cpp files.
3. Replace AnimationTiming with AnimationEffectTimingReadOnly in
   KeyframeEffectReadOnly
(Assignee)

Updated

2 years ago
Attachment #8701723 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8701726 - Flags: review?(bbirtles)
(Assignee)

Comment 103

2 years ago
Comment on attachment 8701726 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v7)

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

Hi Smaug,
Could you please review the webidl files? Thanks
Attachment #8701726 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8701724 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8701724 - Flags: review?(bbirtles)
(Assignee)

Comment 104

2 years ago
Created attachment 8701730 [details] [diff] [review]
Part 6: Rename AnimationTiming as AnimationEffectTimingStruct (v7)
Attachment #8701730 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8701724 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8701725 - Flags: review?(bbirtles)
(Assignee)

Comment 105

2 years ago
Comment on attachment 8701726 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v7)

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

::: dom/animation/AnimationEffectReadOnly.cpp
@@ +23,5 @@
> +already_AddRefed<AnimationEffectTimingReadOnly>
> +AnimationEffectReadOnly::Timing() const
> +{
> +  RefPtr<AnimationEffectTimingReadOnly> temp; // empty object
> +  return temp.forget();

The return type generated by "./mach webidl-example" is "already_AddRefed<...>", not "RefPtr<...>", so I have to use RefPtr<...>().forget() to get the corresponding already_AddRefed<...>.
Comment on attachment 8701726 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v7)

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

I've only reviewed half of this so far. I'll pick up the rest later.

::: dom/animation/Animation.cpp
@@ +1165,5 @@
>      return StickyTimeDuration(0);
>    }
>  
> +  RefPtr<AnimationEffectTimingReadOnly> timing = mEffect->Timing();
> +  return timing->DelayAsDuration() + mEffect->GetComputedTiming().mActiveDuration;

So if we use Timing() like this we will end up doing an addref/release every time we want to access a timing property. That could add up and it seems unnecessary since we know that so long as the effect is alive, its timing object will be.

I wonder if we can add a shortcut method, like:

 virtual const AnimationTiming&
 AnimationEffectTimingReadOnly::TimingStruct() const = 0;

Then, at least when we're doing timing calculations we can access the timing properties without addref/release traffic?

In future we should move the timing properties to AnimationEffectTimingReadOnly so we can make the method non-virtual.

Let me think about it some more. If we mostly only access timing properties from inside KeyframeEffectReadOnly then we won't need this.

::: dom/animation/AnimationEffectReadOnly.h
@@ +30,5 @@
>    }
>  
>    nsISupports* GetParentObject() const { return mParent; }
>  
> +  virtual already_AddRefed<AnimationEffectTimingReadOnly> Timing() const;

Why can't you define this as pure virtual? i.e. = 0?

Do we ever try to instantiate one of these objects?

::: dom/animation/AnimationEffectTimingReadOnly.cpp
@@ +34,5 @@
> +}
> +
> +namespace dom {
> +
> +// Only needed for refcounted objects.

We don't need this comment

@@ +37,5 @@
> +
> +// Only needed for refcounted objects.
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnimationEffectTimingReadOnly, mParent)
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(AnimationEffectTimingReadOnly)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(AnimationEffectTimingReadOnly)

This would be easier to read with a blank line before and after the ADDREF/RELEASE pair.

@@ +59,5 @@
> +AnimationEffectTimingReadOnly&
> +AnimationEffectTimingReadOnly::operator=(const AnimationEffectTimingReadOnly& aRhs)
> +{
> +  MOZ_ASSERT(&aRhs != this);
> +  // Skip mParent bacause we don't implement it.

We can drop this comment.

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +14,5 @@
> +#include "mozilla/dom/UnionTypes.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +
> +// Fix X11 header brain damage that conflicts with dom::FillMode::None

Just the following would be fine:

  // X11 has a #define for None

@@ +26,5 @@
> +
> +struct AnimationTiming
> +{
> +  // FIXME: Initialize mDuration as an 'auto' string or
> +  //        use the unitialized state to represent "auto"?

What do you plan to do here?

@@ +28,5 @@
> +{
> +  // FIXME: Initialize mDuration as an 'auto' string or
> +  //        use the unitialized state to represent "auto"?
> +  dom::OwningUnrestrictedDoubleOrString mDuration;
> +  TimeDuration mDelay;

Perhaps add a comment like:

 TimeDuration mDelay; // Initializes to zero

@@ +50,5 @@
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(AnimationEffectTimingReadOnly)
> +
> +public:

No need for the extra public here. Also, we normally but the constructor before NS_DECL_* macros.

@@ +52,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(AnimationEffectTimingReadOnly)
> +
> +public:
> +  AnimationEffectTimingReadOnly() = default;
> +  AnimationEffectTimingReadOnly(const AnimationTiming& aTiming);

This needs the explicit keyword before the ctor.

@@ +64,5 @@
> +  JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +
> +  double Delay() const { return mTiming.mDelay.ToMilliseconds(); }
> +
> +  TimeDuration DelayAsDuration() const { return mTiming.mDelay; }

This file is hard to scan because there are so many one-liners. It would probably help to group the lines together.

e.g.

  nsISupports* GetParentObject() const { return mParent; }
  JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

  // delay
  double Delay() const { return mTiming.mDelay.ToMilliseconds(); }
  TimeDuration DelayAsDuration() const { return mTiming.mDelay; }

  // other timing parameters
  double EndDelay() const { return 0.0; }
  FillMode Fill() const { return mTiming.mFill; }
  double IterationStart() const { return 0.0; }
  double Iterations() const { return mTiming.mIterations; }
  void GetDuration(OwningUnrestrictedDoubleOrString& aRetVal) const
  {
    aRetVal = mTiming.mDuration;
  }
  PlaybackDirection Direction() const { return mTiming.mDirection; }
  void GetEasing(nsString& aRetVal) const { aRetVal.AssignLiteral("linear"); }

  const AnimationTiming& Timing() const { return mTiming; }

  AnimationEffectTimingReadOnly& operator=(const AnimationEffectTimingReadOnly& aRhs);
  bool operator==(const AnimationEffectTimingReadOnly& aRhs) const
  {
    return mTiming == aRhs.mTiming;
  }
  bool operator!=(const AnimationEffectTimingReadOnly& aRhs) const
  {
    return !operator==(aRhs);
  }

@@ +85,5 @@
> +  void GetEasing(nsString& aRetVal) const { aRetVal.AssignLiteral("linear"); }
> +
> +  const AnimationTiming& Timing() const { return mTiming; }
> +
> +  AnimationEffectTimingReadOnly& operator=(const AnimationEffectTimingReadOnly& aRhs);

This line is over 80 characters.

@@ +100,5 @@
> +protected:
> +  nsCOMPtr<nsISupports> mParent;
> +  AnimationTiming mTiming;
> +
> +};

Unnecessary blank line here.
(Assignee)

Comment 107

2 years ago
(In reply to Brian Birtles (:birtles) from comment #106)
> Comment on attachment 8701726 [details] [diff] [review]
> Part 5: Add AnimationEffectTimingReadOnly interface (v7)
> 
> Review of attachment 8701726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've only reviewed half of this so far. I'll pick up the rest later.

Really thanks for your review. Christmas is coming. :)

> 
> ::: dom/animation/Animation.cpp
> @@ +1165,5 @@
> >      return StickyTimeDuration(0);
> >    }
> >  
> > +  RefPtr<AnimationEffectTimingReadOnly> timing = mEffect->Timing();
> > +  return timing->DelayAsDuration() + mEffect->GetComputedTiming().mActiveDuration;
> 
> So if we use Timing() like this we will end up doing an addref/release every
> time we want to access a timing property. That could add up and it seems
> unnecessary since we know that so long as the effect is alive, its timing
> object will be.
> 
> I wonder if we can add a shortcut method, like:
> 
>  virtual const AnimationTiming&
>  AnimationEffectTimingReadOnly::TimingStruct() const = 0;
> 
> Then, at least when we're doing timing calculations we can access the timing
> properties without addref/release traffic?

Adding a function in AnimationEffectReadOnly/KeyframeEffectReadOnly to get AnimationTiming& directly is a good way to prevent adding/releasing ref count of RefPtr<AnimationEffectTimingReadOnly> if this object is still alive when we access AnimationTiming&. I can add it while updating this patch if you think it is OK.

For example:

const AnimationTiming&
KeyframeEffectReadOnly::TimingStruct() const
{
  return mTiming->Timing();
}

> 
> In future we should move the timing properties to
> AnimationEffectTimingReadOnly so we can make the method non-virtual.
> 
> Let me think about it some more. If we mostly only access timing properties
> from inside KeyframeEffectReadOnly then we won't need this.

In this bug, it looks like we only access (read or replace) timing properties from inside KeyframeEffectReadOnly.

> 
> ::: dom/animation/AnimationEffectReadOnly.h
> @@ +30,5 @@
> >    }
> >  
> >    nsISupports* GetParentObject() const { return mParent; }
> >  
> > +  virtual already_AddRefed<AnimationEffectTimingReadOnly> Timing() const;
> 
> Why can't you define this as pure virtual? i.e. = 0?
> 
> Do we ever try to instantiate one of these objects?

Sure. AnimationEffectReadOnly is not concrete, so we can define is as pure virtual.

> 
> @@ +26,5 @@
> > +
> > +struct AnimationTiming
> > +{
> > +  // FIXME: Initialize mDuration as an 'auto' string or
> > +  //        use the unitialized state to represent "auto"?
> 
> What do you plan to do here?

Actually, I prefer initialing mDuration as an "auto" string, so other developers can understand the code easily if the cost is not critical.

> @@ +52,5 @@
> > +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(AnimationEffectTimingReadOnly)
> > +
> > +public:
> > +  AnimationEffectTimingReadOnly() = default;
> > +  AnimationEffectTimingReadOnly(const AnimationTiming& aTiming);
> 
> This needs the explicit keyword before the ctor.

OK.

> 
> @@ +64,5 @@
> > +  JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> > +
> > +  double Delay() const { return mTiming.mDelay.ToMilliseconds(); }
> > +
> > +  TimeDuration DelayAsDuration() const { return mTiming.mDelay; }
> 
> This file is hard to scan because there are so many one-liners. It would
> probably help to group the lines together.
> 
> e.g.
> 
>   nsISupports* GetParentObject() const { return mParent; }
>   JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
> override;
> 
>   // delay
>   double Delay() const { return mTiming.mDelay.ToMilliseconds(); }
>   TimeDuration DelayAsDuration() const { return mTiming.mDelay; }
> 
>   // other timing parameters
>   double EndDelay() const { return 0.0; }
>   FillMode Fill() const { return mTiming.mFill; }
>   double IterationStart() const { return 0.0; }
>   double Iterations() const { return mTiming.mIterations; }
>   void GetDuration(OwningUnrestrictedDoubleOrString& aRetVal) const
>   {
>     aRetVal = mTiming.mDuration;
>   }
>   PlaybackDirection Direction() const { return mTiming.mDirection; }
>   void GetEasing(nsString& aRetVal) const { aRetVal.AssignLiteral("linear");
> }
> 
>   const AnimationTiming& Timing() const { return mTiming; }
> 
>   AnimationEffectTimingReadOnly& operator=(const
> AnimationEffectTimingReadOnly& aRhs);
>   bool operator==(const AnimationEffectTimingReadOnly& aRhs) const
>   {
>     return mTiming == aRhs.mTiming;
>   }
>   bool operator!=(const AnimationEffectTimingReadOnly& aRhs) const
>   {
>     return !operator==(aRhs);
>   }

Thanks for your example of the coding style. This is much better.
(In reply to Boris Chiou [:boris] from comment #107)
> Adding a function in AnimationEffectReadOnly/KeyframeEffectReadOnly to get
> AnimationTiming& directly is a good way to prevent adding/releasing ref
> count of RefPtr<AnimationEffectTimingReadOnly> if this object is still alive
> when we access AnimationTiming&.

Since we never replace Animation::mTiming, it is guaranteed to be alive as long as the Animation is alive.

> I can add it while updating this patch if
> you think it is OK.
> 
> For example:
> 
> const AnimationTiming&
> KeyframeEffectReadOnly::TimingStruct() const
> {
>   return mTiming->Timing();
> }

Yes, that's what I had in mind.

> > In future we should move the timing properties to
> > AnimationEffectTimingReadOnly so we can make the method non-virtual.
> > 
> > Let me think about it some more. If we mostly only access timing properties
> > from inside KeyframeEffectReadOnly then we won't need this.
> 
> In this bug, it looks like we only access (read or replace) timing
> properties from inside KeyframeEffectReadOnly.

I think we read it from Animation and from nsDisplayList so it sounds like we should add this shortcut.

> > @@ +26,5 @@
> > > +
> > > +struct AnimationTiming
> > > +{
> > > +  // FIXME: Initialize mDuration as an 'auto' string or
> > > +  //        use the unitialized state to represent "auto"?
> > 
> > What do you plan to do here?
> 
> Actually, I prefer initialing mDuration as an "auto" string, so other
> developers can understand the code easily if the cost is not critical.

I agree that it is easier if we only have one state representing "auto". Also, I expect that 99% of the time this member will be set. As long as we can avoid allocating a string buffer every time we create a new Animation I think this is ok. I'm not sure if we can avoid that, however.
(In reply to Brian Birtles (:birtles) from comment #108)
> (In reply to Boris Chiou [:boris] from comment #107)
> > I can add it while updating this patch if
> > you think it is OK.
> > 
> > For example:
> > 
> > const AnimationTiming&
> > KeyframeEffectReadOnly::TimingStruct() const
> > {
> >   return mTiming->Timing();
> > }
> 
> Yes, that's what I had in mind.

In fact, I think we should call this just Timing() and use BinaryName="TimingAsObject" or something like that for the one that returns the ref-counted object.
Comment on attachment 8701726 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v7)

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

Clearing review request since I want to see this again with the changes made.

Also, clearing the review request for the WebIDL changes since I think we want to change the WebIDL to add a BinaryName section.

::: dom/animation/KeyframeEffect.h
@@ +212,5 @@
>      aRetVal.AssignLiteral("distribute");
>    }
>  
> +  already_AddRefed<AnimationEffectTimingReadOnly> Timing() const override;
> +  void SetTiming(const RefPtr<AnimationEffectTimingReadOnly>& aTiming);

Can't we keep SetTiming as taking an AnimationTiming struct? This would allow nsAnimationManager etc. to avoid creating a new heap-allocated object every time they rebuild timing parameters.

We can define AnimationEffectTimingReadOnly has having an operator= that takes an AnimationTiming struct if necessary.

So I think we want to have something like this:

  const AnimationTiming& Timing() const;
  void SetTiming(const AnimationTiming& aTiming);
  already_AddRefed<AnimationEffectTimingReadOnly> TimingAsObject() const override;

I suspect Timing() doesn't need to be virtual since we normally have a reference to a KeyframeEffectReadOnly when we access this.
Attachment #8701726 - Flags: review?(bugs)
Attachment #8701726 - Flags: review?(bbirtles)
Comment on attachment 8701730 [details] [diff] [review]
Part 6: Rename AnimationTiming as AnimationEffectTimingStruct (v7)

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

This is fine, but I should probably look at it after rebasing on top of part 5.

Also, do you think this is necessary? I think AnimationEffectTimingStruct is more clear but it's a lot to type and remember. I wonder if the old name is actually better.
Attachment #8701730 - Flags: review?(bbirtles)
(Assignee)

Comment 112

2 years ago
(In reply to Brian Birtles (:birtles) from comment #111)
> Comment on attachment 8701730 [details] [diff] [review]
> Part 6: Rename AnimationTiming as AnimationEffectTimingStruct (v7)
> 
> Review of attachment 8701730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine, but I should probably look at it after rebasing on top of part
> 5.
> 
> Also, do you think this is necessary? I think AnimationEffectTimingStruct is
> more clear but it's a lot to type and remember. I wonder if the old name is
> actually better.

Either name is OK to me. We can understand this is a "struct", not a ref count interface by the new name easily. There are different timing types (such as AnimationEffectTimingReadOnly, AnimationEffectTiming, AnimationEffectTimingStruct), and all of them have the same prefix, so there relationship may be clear after we replace the name of AnimationTiming. Therefore, I upload this patch.
Comment on attachment 8701725 [details] [diff] [review]
Part 7: Move ConvertKeyframeEffectOptions (v4)

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

This looks fine, but I'm curious, why are we moving this? Is this blocking anything?

I'm just wondering if we should file a separate bug where we move the whole mTiming member over to AnimationEffectTimingReadOnly and drop all the virtual declarations on methods like Timing() (or TimingAsObject() if we call it).

::: dom/animation/AnimationEffectTimingReadOnly.cpp
@@ +88,5 @@
> +    animationTiming.mDuration.SetAsUnrestrictedDouble() = dur;
> +    animationTiming.mDelay = TimeDuration(0);
> +    animationTiming.mIterations = 1.0f;
> +    animationTiming.mDirection = PlaybackDirection::Normal;
> +    animationTiming.mFill = FillMode::None;

I think we can drop these last four lines since part 5 makes AnimationTiming (AnimationEffectTimingStruct) initialize these values for us.

It also makes the fill mode "auto" not "none" which is correct but we might need to update some tests or file a separate bug to translate "auto" to "none" in GetComputedTiming.
Attachment #8701725 - Flags: review?(bbirtles)
(Assignee)

Comment 114

2 years ago
(In reply to Brian Birtles (:birtles) from comment #113)
> Comment on attachment 8701725 [details] [diff] [review]
> Part 7: Move ConvertKeyframeEffectOptions (v4)
> 
> Review of attachment 8701725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, but I'm curious, why are we moving this? Is this blocking
> anything?

No. It doesn't block anyone. I just want to move all APIs about AnimationTiming into AnimationEffectTimingReadOnly.h

> 
> I'm just wondering if we should file a separate bug where we move the whole
> mTiming member over to AnimationEffectTimingReadOnly and drop all the
> virtual declarations on methods like Timing() (or TimingAsObject() if we
> call it).

Yap, if we want to make KeyframeEffectReadOnly simpler. We should think this more, however. If anyone has a good idea, we can try to do this.

> 
> ::: dom/animation/AnimationEffectTimingReadOnly.cpp
> @@ +88,5 @@
> > +    animationTiming.mDuration.SetAsUnrestrictedDouble() = dur;
> > +    animationTiming.mDelay = TimeDuration(0);
> > +    animationTiming.mIterations = 1.0f;
> > +    animationTiming.mDirection = PlaybackDirection::Normal;
> > +    animationTiming.mFill = FillMode::None;
> 
> I think we can drop these last four lines since part 5 makes AnimationTiming
> (AnimationEffectTimingStruct) initialize these values for us.

Sure.

> 
> It also makes the fill mode "auto" not "none" which is correct but we might
> need to update some tests or file a separate bug to translate "auto" to
> "none" in GetComputedTiming.

OK.
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8701726 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8701730 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8701725 - Attachment is obsolete: true
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Attachment #8701922 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8701931 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8701923 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8701934 - Attachment is obsolete: true
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8701398 - Attachment is obsolete: true
(Assignee)

Comment 123

2 years ago
Created attachment 8701956 [details] [diff] [review]
Part 8: Test (v2)
(Assignee)

Updated

2 years ago
Attachment #8701954 - Attachment is obsolete: true
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Attachment #8692732 - Attachment is obsolete: true
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Attachment #8692735 - Attachment is obsolete: true
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Attachment #8692789 - Attachment is obsolete: true
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Attachment #8699878 - Attachment is obsolete: true
(Assignee)

Comment 128

2 years ago
Created attachment 8702180 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v9)

1. Add AnimationEffectTimingReadOnly.webidl.
2. Add AnimationEffectTimingReadOnly cpp files.
3. Use AnimationEffectTimingReadOnly as KeyframeEffectReadOnly::mTiming.

Rebased.
(Assignee)

Updated

2 years ago
Attachment #8701932 - Attachment is obsolete: true
(Assignee)

Comment 129

2 years ago
Created attachment 8702181 [details] [diff] [review]
Part 6: Rename AnimationTiming as AnimationEffectTimingStruct (v10)

Rebased.
(Assignee)

Updated

2 years ago
Attachment #8701936 - Attachment is obsolete: true
(Assignee)

Comment 130

2 years ago
Created attachment 8702182 [details] [diff] [review]
Part 7: Move ConvertKeyframeEffectOptions (v5)

Move ConvertKeyframeEffectOptions() from KeyframeEffectReadOnly into
AnimationEffectTimingReadOnly.

Rebased.
(Assignee)

Updated

2 years ago
Attachment #8701924 - Attachment is obsolete: true
Comment on attachment 8702180 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v9)

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

Sorry, I've been away and I guess that has blocked your work. This is getting really close. A few overall comments:

1. This is a big patch. It can be difficult to write small patches that also compile and pass all tests when applied one-at-a-time, however it makes it a lot easier to review and a lot easier to bisect regressions. Right now this patch is hard to review because it does so many different things. There's no need to rewrite it now but in future it's best to try to make each patch the smallest possible change that can compile and pass all tests.

2. My biggest concern is probably this part from the definition of AnimationTiming in AnimationEffectTimingReadOnly.h:

  // FIXME: Initialize mDuration as an 'auto' string or
  //        use the unitialized state to represent "auto"?
  dom::OwningUnrestrictedDoubleOrString mDuration;

  I think we should probably just make this a Maybe<double> (or even a Maybe<TimeDuration> to avoid doing TimeDuration-to-double conversion on each call to GetComputedTimingAt) and treat null/empty as "auto". I think we can just make the spec say that any string other than "auto" gets turned into "auto". I'll run this by the Google guys.

  For now I'd like to avoid allocating a new string buffer for every new animation just to represent an intermediate state that will be immediately overridden.

::: dom/animation/AnimationEffectTimingReadOnly.cpp
@@ +46,5 @@
> +NS_INTERFACE_MAP_END
> +
> +AnimationEffectTimingReadOnly::AnimationEffectTimingReadOnly(
> +    const AnimationTiming& aTiming)
> +  : mTiming(aTiming)

Can we just put this in the header file?

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +43,5 @@
> +
> +
> +namespace dom {
> +
> +class AnimationEffectTimingReadOnly : public nsISupports,

Do we need nsISupports? Can't we just use native ref-counting?

e.g. see https://dxr.mozilla.org/mozilla-central/source/dom/base/DOMQuad.h

@@ +47,5 @@
> +class AnimationEffectTimingReadOnly : public nsISupports,
> +                                      public nsWrapperCache
> +{
> +public:
> +  explicit AnimationEffectTimingReadOnly() = default;

You only need 'explicit' when there is one parameter to the constructor so you should remove it from this line.

::: dom/animation/KeyframeEffect.cpp
@@ +107,5 @@
> +const AnimationTiming&
> +KeyframeEffectReadOnly::Timing() const
> +{
> +  return mTiming->Timing();
> +}

What would making this inline involve?

@@ +155,1 @@
>                                aRetVal);

Why don't you just write:

 GetComputedTimingDictionary(GetComputedTimingAt(currentTime, timing),
                             currentTime,
                             Timing(),
                             aRetVal);

@@ +293,5 @@
>  
> +ComputedTiming
> +KeyframeEffectReadOnly::GetComputedTiming(const AnimationTiming* aTiming) const
> +{
> +  return GetComputedTimingAt(GetLocalTime(), aTiming ? *aTiming : mTiming->Timing());

Again, we could just use Timing() here, instead of mTiming->Timing(). Then we can leave it in the header file too.

::: layout/style/nsAnimationManager.cpp
@@ +303,5 @@
> +{
> +  return mEffect ?
> +         std::max(TimeDuration(), mEffect->Timing().mDelay * -1) :
> +         TimeDuration();
> +}

I don't think we need to move this to the .cpp file anymore, right?

::: layout/style/nsTransitionManager.cpp
@@ +53,5 @@
>    // interval. However, it might be possible that we're behind on flushing
>    // causing us to get called *after* the animation interval. So, just in
>    // case, we override the fill mode to 'both' to ensure the progress
>    // is never null.
> +  AnimationTiming timingToUse = mTiming->Timing();

Just use Timing() here?
(Assignee)

Comment 132

2 years ago
(In reply to Brian Birtles (:birtles) from comment #131)
> Comment on attachment 8702180 [details] [diff] [review]
> Part 5: Add AnimationEffectTimingReadOnly interface (v9)
> 
> Review of attachment 8702180 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I've been away and I guess that has blocked your work. This is
> getting really close. A few overall comments:

Never mind. I was thinking how to do Bug 1174575 step by step recently, so this doesn't block my process.

> 
> 1. This is a big patch. It can be difficult to write small patches that also
> compile and pass all tests when applied one-at-a-time, however it makes it a
> lot easier to review and a lot easier to bisect regressions. Right now this
> patch is hard to review because it does so many different things. There's no
> need to rewrite it now but in future it's best to try to make each patch the
> smallest possible change that can compile and pass all tests.
> 

Yes. I agree. I will watch out for these details next time. Thanks for your reminder.

> 2. My biggest concern is probably this part from the definition of
> AnimationTiming in AnimationEffectTimingReadOnly.h:
> 
>   // FIXME: Initialize mDuration as an 'auto' string or
>   //        use the unitialized state to represent "auto"?
>   dom::OwningUnrestrictedDoubleOrString mDuration;
> 
>   I think we should probably just make this a Maybe<double> (or even a
> Maybe<TimeDuration> to avoid doing TimeDuration-to-double conversion on each
> call to GetComputedTimingAt) and treat null/empty as "auto". I think we can
> just make the spec say that any string other than "auto" gets turned into
> "auto". I'll run this by the Google guys.
> 
>   For now I'd like to avoid allocating a new string buffer for every new
> animation just to represent an intermediate state that will be immediately
> overridden.

OK. I will keep this declaration and use the uninitialized state to represent "auto", and keep an eye on handling the uninitialized state in the future.

> 
> ::: dom/animation/AnimationEffectTimingReadOnly.cpp
> @@ +46,5 @@
> > +NS_INTERFACE_MAP_END
> > +
> > +AnimationEffectTimingReadOnly::AnimationEffectTimingReadOnly(
> > +    const AnimationTiming& aTiming)
> > +  : mTiming(aTiming)
> 
> Can we just put this in the header file?

Sure.

> 
> ::: dom/animation/AnimationEffectTimingReadOnly.h
> @@ +43,5 @@
> > +
> > +
> > +namespace dom {
> > +
> > +class AnimationEffectTimingReadOnly : public nsISupports,
> 
> Do we need nsISupports? Can't we just use native ref-counting?
> 
> e.g. see https://dxr.mozilla.org/mozilla-central/source/dom/base/DOMQuad.h

I copied this from the auto-generated code by "mach webidl-example xxx". However, I can try not to use nsISuppoerts locally to make sure it also works and update this later.

> 
> @@ +47,5 @@
> > +class AnimationEffectTimingReadOnly : public nsISupports,
> > +                                      public nsWrapperCache
> > +{
> > +public:
> > +  explicit AnimationEffectTimingReadOnly() = default;
> 
> You only need 'explicit' when there is one parameter to the constructor so
> you should remove it from this line.

Oops. Yes. Thanks.

> 
> ::: dom/animation/KeyframeEffect.cpp
> @@ +107,5 @@
> > +const AnimationTiming&
> > +KeyframeEffectReadOnly::Timing() const
> > +{
> > +  return mTiming->Timing();
> > +}
> 
> What would making this inline involve?

If making this inline, we have to include AnimationEffectTimingReadOnly.h in the header, and I just tried to avoid including too many files in the header. However, I can make it inline in my next patch.

> 
> @@ +155,1 @@
> >                                aRetVal);
> 
> Why don't you just write:
> 
>  GetComputedTimingDictionary(GetComputedTimingAt(currentTime, timing),
>                              currentTime,
>                              Timing(),
>                              aRetVal);

"GetComputedTimingAt(currentTime, timing)" also need the timing object and I don't want to call this function twice. BTW, as you mentioned, I can call Timing() directly, instead of mTiming->Timing().
For example:
const AnimationTiming &timing = Timing();
GetComputedTimingDictionary(GetComputedTimingAt(currentTime, timing),
                            currentTime,
                            timing,
                            aRetVal);


> 
> @@ +293,5 @@
> >  
> > +ComputedTiming
> > +KeyframeEffectReadOnly::GetComputedTiming(const AnimationTiming* aTiming) const
> > +{
> > +  return GetComputedTimingAt(GetLocalTime(), aTiming ? *aTiming : mTiming->Timing());
> 
> Again, we could just use Timing() here, instead of mTiming->Timing(). Then
> we can leave it in the header file too.

OK. Thanks.

> 
> ::: layout/style/nsAnimationManager.cpp
> @@ +303,5 @@
> > +{
> > +  return mEffect ?
> > +         std::max(TimeDuration(), mEffect->Timing().mDelay * -1) :
> > +         TimeDuration();
> > +}
> 
> I don't think we need to move this to the .cpp file anymore, right?

Using ".mDelay" needs to know the definition of AnimationTiming, so I have to include AnimationEffectTimingReadOnly.h in nsAnimationManager.h, which adding one more dependency.

> 
> ::: layout/style/nsTransitionManager.cpp
> @@ +53,5 @@
> >    // interval. However, it might be possible that we're behind on flushing
> >    // causing us to get called *after* the animation interval. So, just in
> >    // case, we override the fill mode to 'both' to ensure the progress
> >    // is never null.
> > +  AnimationTiming timingToUse = mTiming->Timing();
> 
> Just use Timing() here?

Yes.


Thanks for your review.
(In reply to Boris Chiou [:boris] from comment #132)
> (In reply to Brian Birtles (:birtles) from comment #131)
> > Comment on attachment 8702180 [details] [diff] [review]
> > Part 5: Add AnimationEffectTimingReadOnly interface (v9)
> > 
> > Review of attachment 8702180 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sorry, I've been away and I guess that has blocked your work. This is
> > getting really close. A few overall comments:
> 
> Never mind. I was thinking how to do Bug 1174575 step by step recently, so
> this doesn't block my process.

Ok, great.

> > ::: dom/animation/AnimationEffectTimingReadOnly.h
> > @@ +43,5 @@
> > > +
> > > +
> > > +namespace dom {
> > > +
> > > +class AnimationEffectTimingReadOnly : public nsISupports,
> > 
> > Do we need nsISupports? Can't we just use native ref-counting?
> > 
> > e.g. see https://dxr.mozilla.org/mozilla-central/source/dom/base/DOMQuad.h
> 
> I copied this from the auto-generated code by "mach webidl-example xxx".
> However, I can try not to use nsISuppoerts locally to make sure it also
> works and update this later.

Please drop the nsISupports inheritance and do the ref-counting locally.

> > ::: dom/animation/KeyframeEffect.cpp
> > @@ +107,5 @@
> > > +const AnimationTiming&
> > > +KeyframeEffectReadOnly::Timing() const
> > > +{
> > > +  return mTiming->Timing();
> > > +}
> > 
> > What would making this inline involve?
> 
> If making this inline, we have to include AnimationEffectTimingReadOnly.h in
> the header, and I just tried to avoid including too many files in the
> header. However, I can make it inline in my next patch.

What's your next patch? Are you rewriting this patch or do you mean a separate bug?

> > Why don't you just write:
> > 
> >  GetComputedTimingDictionary(GetComputedTimingAt(currentTime, timing),
> >                              currentTime,
> >                              Timing(),
> >                              aRetVal);
> 
> "GetComputedTimingAt(currentTime, timing)" also need the timing object and I
> don't want to call this function twice. BTW, as you mentioned, I can call
> Timing() directly, instead of mTiming->Timing().
> For example:
> const AnimationTiming &timing = Timing();
> GetComputedTimingDictionary(GetComputedTimingAt(currentTime, timing),
>                             currentTime,
>                             timing,
>                             aRetVal);

If Timing() is inline in the header, and AnimationEffectTimingReadOnly::Timing() is also inline and just returning the member object then calling Timing() here is basically free so you can just call it twice. That would make the code easier to read I think.
(Assignee)

Comment 134

2 years ago
(In reply to Brian Birtles (:birtles) from comment #133)
> What's your next patch? Are you rewriting this patch or do you mean a
> separate bug?

I am rewriting this patch.

> If Timing() is inline in the header, and
> AnimationEffectTimingReadOnly::Timing() is also inline and just returning
> the member object then calling Timing() here is basically free so you can
> just call it twice. That would make the code easier to read I think.

OK. Thanks.
(Assignee)

Comment 135

2 years ago
Created attachment 8704016 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v10)

1. Add AnimationEffectTimingReadOnly.webidl.
2. Add AnimationEffectTimingReadOnly cpp files.
3. Use AnimationEffectTimingReadOnly as KeyframeEffectReadOnly::mTiming.
(Assignee)

Updated

2 years ago
Attachment #8702180 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8704016 - Flags: review?(bbirtles)
Hi Boris, I've been thinking about part 6 and what to name these things and I have a suggestion.

1. Rename "AnimationTiming" (the struct we use internally) to "SpecifiedTiming"
2. Rename the AnimationEffect::Timing() shortcut to SpecifiedTiming()
3. Drop the BinaryName annotations from part 3 and just let Timing() return the AnimationEffectReadOnly object

So you'd end up with an interface that looks like this (assuming we eventually move mTiming to AnimationEffectReadOnly):

class AnimationEffectReadOnly {
  // Specified timing
  const SpecifiedTiming& SpecifiedTiming() const { return mTiming->SpecifiedTiming(); }
  already_AddRefed<AnimationEffectTimingReadOnly> Timing() { return mTiming; }

  // Computed timing
  ComputedTiming GetComputedTiming() const;
  void GetComputedTimingAsDict(ComputedTimingProperties& aRetVal) const;
};

What do you think?

I won't make you change part 5 since it's already too complicated. I think we should just land it and then fix the issues one-by-one.
Comment on attachment 8704016 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface (v10)

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

r=birtles with comments addressed but I think we need to fix the following things soon after landing:

1. The renaming suggested in comment 136 (or whatever we agree is best there).
2. Moving SpecifiedTiming (or whatever we call it) to a separate file and simplifying the include dependencies introduced here.
3. Changing the type of mDuration to Maybe<TimeDuration> (and adjusting the spec so we're not required to store the original string).

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +25,5 @@
> +namespace mozilla {
> +
> +struct AnimationTiming
> +{
> +  // The unitialized state of mDuration represents "auto"

Do we actually make sure we never set the string "auto" here? If some code paths are setting the string "auto" and some code paths are leaving this uninitialized but both *mean* "auto" then operator== will return the wrong result.

Like I said elsewhere, I think we should just make this Maybe<double> (or, better still, Maybe<TimeDuration>) and then represent all strings including "auto" as a null/empty value. For now, I think we probably need to adjust operator== to make "uninitialized" and any string value compare equal.

@@ +37,5 @@
> +  bool operator!=(const AnimationTiming& aOther) const
> +  {
> +    return !(*this == aOther);
> +  }
> +};

Can you file a follow-up to move this to a separate file? (Possibly after renaming to SpecifiedTiming--see my earlier comments).

I think once we do that, and once we change mDuration to just Maybe<double> (or Maybe<TimeDuration>) we can tidy-up the include dependencies since a lot of places will only need to include SpecifiedTiming.

::: dom/animation/KeyframeEffect.cpp
@@ +114,4 @@
>  void
>  KeyframeEffectReadOnly::SetTiming(const AnimationTiming& aTiming)
>  {
> +  if (Timing() == aTiming) {

For what it's worth, I think it's probably better to use mTiming->Timing() here since this method is specifically about replacing mTiming's timing struct. In other methods, however, using Timing() is better.

::: dom/animation/KeyframeEffect.h
@@ +216,5 @@
>    void GetSpacing(nsString& aRetVal) const {
>      aRetVal.AssignLiteral("distribute");
>    }
>  
> +  const AnimationTiming& Timing() const { return mTiming->Timing(); }

Let's add a MOZ_ASSERT(mTiming) here.

::: layout/style/nsAnimationManager.h
@@ +176,5 @@
>  
>    // Returns the duration from the start of the animation's source effect's
>    // active interval to the point where the animation actually begins playback.
>    // This is zero unless the animation's source effect has a negative delay in
>    // which // case it is the absolute value of that delay.

Can we fix this comment at the same time (stray "//")?
Attachment #8704016 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 138

2 years ago
 (In reply to Brian Birtles (:birtles) from comment #136)
> Hi Boris, I've been thinking about part 6 and what to name these things and
> I have a suggestion.
> 
> 1. Rename "AnimationTiming" (the struct we use internally) to
> "SpecifiedTiming"
> 2. Rename the AnimationEffect::Timing() shortcut to SpecifiedTiming()
> 3. Drop the BinaryName annotations from part 3 and just let Timing() return
> the AnimationEffectReadOnly object

This is OK to me. Using another function name (instead of Timing()) for SpecifiedTiming looks better, so we can use Timing() for AnimationEffectReadOnly, as the spec says.

> 
> So you'd end up with an interface that looks like this (assuming we
> eventually move mTiming to AnimationEffectReadOnly):
> 
> class AnimationEffectReadOnly {
>   // Specified timing
>   const SpecifiedTiming& SpecifiedTiming() const { return
> mTiming->SpecifiedTiming(); }
>   already_AddRefed<AnimationEffectTimingReadOnly> Timing() { return mTiming;
> }
> 
>   // Computed timing
>   ComputedTiming GetComputedTiming() const;
>   void GetComputedTimingAsDict(ComputedTimingProperties& aRetVal) const;
> };
> 
> What do you think?
> 
> I won't make you change part 5 since it's already too complicated. I think
> we should just land it and then fix the issues one-by-one.

OK. I will add a new patch in this bug to fix them.
(Assignee)

Comment 139

2 years ago
(In reply to Boris Chiou [:boris] from comment #138)
> > 
> > I won't make you change part 5 since it's already too complicated. I think
> > we should just land it and then fix the issues one-by-one.
> 
> OK. I will add a new patch in this bug to fix them.

Sorry, I mean I will change the name in part 6.
(Assignee)

Comment 140

2 years ago
(In reply to Brian Birtles (:birtles) from comment #137)
> Comment on attachment 8704016 [details] [diff] [review]
> Part 5: Add AnimationEffectTimingReadOnly interface (v10)
> 
> Review of attachment 8704016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=birtles with comments addressed but I think we need to fix the following
> things soon after landing:
> 
> 1. The renaming suggested in comment 136 (or whatever we agree is best
> there).
> 2. Moving SpecifiedTiming (or whatever we call it) to a separate file and
> simplifying the include dependencies introduced here.
> 3. Changing the type of mDuration to Maybe<TimeDuration> (and adjusting the
> spec so we're not required to store the original string).
> 
> ::: dom/animation/AnimationEffectTimingReadOnly.h
> @@ +25,5 @@
> > +namespace mozilla {
> > +
> > +struct AnimationTiming
> > +{
> > +  // The unitialized state of mDuration represents "auto"
> 
> Do we actually make sure we never set the string "auto" here? If some code
> paths are setting the string "auto" and some code paths are leaving this
> uninitialized but both *mean* "auto" then operator== will return the wrong
> result.
> 
> Like I said elsewhere, I think we should just make this Maybe<double> (or,
> better still, Maybe<TimeDuration>) and then represent all strings including
> "auto" as a null/empty value. For now, I think we probably need to adjust
> operator== to make "uninitialized" and any string value compare equal.

OK. I will revise operator== for this case.

> 
> @@ +37,5 @@
> > +  bool operator!=(const AnimationTiming& aOther) const
> > +  {
> > +    return !(*this == aOther);
> > +  }
> > +};
> 
> Can you file a follow-up to move this to a separate file? (Possibly after
> renaming to SpecifiedTiming--see my earlier comments).
> 
> I think once we do that, and once we change mDuration to just Maybe<double>
> (or Maybe<TimeDuration>) we can tidy-up the include dependencies since a lot
> of places will only need to include SpecifiedTiming.

I agree. I can file a follow-up bug for this.

> 
> ::: dom/animation/KeyframeEffect.cpp
> @@ +114,4 @@
> >  void
> >  KeyframeEffectReadOnly::SetTiming(const AnimationTiming& aTiming)
> >  {
> > +  if (Timing() == aTiming) {
> 
> For what it's worth, I think it's probably better to use mTiming->Timing()
> here since this method is specifically about replacing mTiming's timing
> struct. In other methods, however, using Timing() is better.

OK.

> 
> ::: dom/animation/KeyframeEffect.h
> @@ +216,5 @@
> >    void GetSpacing(nsString& aRetVal) const {
> >      aRetVal.AssignLiteral("distribute");
> >    }
> >  
> > +  const AnimationTiming& Timing() const { return mTiming->Timing(); }
> 
> Let's add a MOZ_ASSERT(mTiming) here.

OK

> 
> ::: layout/style/nsAnimationManager.h
> @@ +176,5 @@
> >  
> >    // Returns the duration from the start of the animation's source effect's
> >    // active interval to the point where the animation actually begins playback.
> >    // This is zero unless the animation's source effect has a negative delay in
> >    // which // case it is the absolute value of that delay.
> 
> Can we fix this comment at the same time (stray "//")?

Sure
(Assignee)

Comment 141

2 years ago
Created attachment 8704463 [details] [diff] [review]
Part 6: Revise AnimationTiming::operator==
(Assignee)

Updated

2 years ago
Attachment #8701956 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8702181 - Attachment is obsolete: true
Attachment #8702182 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8704480 - Attachment is obsolete: true
(Assignee)

Comment 144

2 years ago
Created attachment 8704482 [details] [diff] [review]
Part 7: Rename AnimationTiming as SpecifiedTiming (v13)
(Assignee)

Updated

2 years ago
Attachment #8704481 - Attachment is obsolete: true
(Assignee)

Comment 145

2 years ago
Created attachment 8704484 [details] [diff] [review]
Part 8: Move ConvertKeyframeEffectOptions (v6)

Move ConvertKeyframeEffectOptions() from KeyframeEffectReadOnly into
AnimationEffectTimingReadOnly.
(Assignee)

Comment 146

2 years ago
Created attachment 8704485 [details] [diff] [review]
Part 9: Test (v2)
(Assignee)

Updated

2 years ago
Attachment #8704463 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8704482 - Flags: review?(bbirtles)
(Assignee)

Comment 147

2 years ago
I will send review requests to a DOM peer for part 5 and part 7 after getting r+ from Brian.
(Assignee)

Updated

2 years ago
Attachment #8704484 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Blocks: 1237173
(Assignee)

Comment 148

2 years ago
(In reply to Boris Chiou [:boris] from comment #140)
> > Can you file a follow-up to move this to a separate file? (Possibly after
> > renaming to SpecifiedTiming--see my earlier comments).
> > 
> > I think once we do that, and once we change mDuration to just Maybe<double>
> > (or Maybe<TimeDuration>) we can tidy-up the include dependencies since a lot
> > of places will only need to include SpecifiedTiming.
> 
> I agree. I can file a follow-up bug for this.

Filed Bug 1237173.
Comment on attachment 8704463 [details] [diff] [review]
Part 6: Revise AnimationTiming::operator==

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

::: dom/animation/AnimationEffectTimingReadOnly.cpp
@@ +20,3 @@
>    } else {
> +    // mDuration is a string or uninitialized,
> +    // so they are equal if aOther.mDuration is also a string or uninitizlized.

How about:

'We consider all string values and uninitialized values as meaning "auto". Since mDuration is either a string or uninitialized, we consider it equal if aOther.mDuration is also either a string or uninitialized.'

?

::: dom/animation/KeyframeEffect.cpp
@@ +113,5 @@
>  
>  void
>  KeyframeEffectReadOnly::SetTiming(const AnimationTiming& aTiming)
>  {
> +  if (mTiming->Timing() == aTiming) {

Not sure if you meant to make this part of patch 5? Anyway, I guess it doesn't really matter.

::: dom/animation/KeyframeEffect.h
@@ +220,5 @@
> +  const AnimationTiming& Timing() const
> +  {
> +    MOZ_ASSERT(mTiming);
> +    return mTiming->Timing();
> +  }

Again, maybe this was supposed to be part of patch 5?
Attachment #8704463 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 150

2 years ago
(In reply to Brian Birtles (:birtles) from comment #149)
> Comment on attachment 8704463 [details] [diff] [review]
> Part 6: Revise AnimationTiming::operator==
> 
> Review of attachment 8704463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/AnimationEffectTimingReadOnly.cpp
> @@ +20,3 @@
> >    } else {
> > +    // mDuration is a string or uninitialized,
> > +    // so they are equal if aOther.mDuration is also a string or uninitizlized.
> 
> How about:
> 
> 'We consider all string values and uninitialized values as meaning "auto".
> Since mDuration is either a string or uninitialized, we consider it equal if
> aOther.mDuration is also either a string or uninitialized.'
> 
> ?
> 
> ::: dom/animation/KeyframeEffect.cpp
> @@ +113,5 @@
> >  
> >  void
> >  KeyframeEffectReadOnly::SetTiming(const AnimationTiming& aTiming)
> >  {
> > +  if (mTiming->Timing() == aTiming) {
> 
> Not sure if you meant to make this part of patch 5? Anyway, I guess it
> doesn't really matter.
> 
> ::: dom/animation/KeyframeEffect.h
> @@ +220,5 @@
> > +  const AnimationTiming& Timing() const
> > +  {
> > +    MOZ_ASSERT(mTiming);
> > +    return mTiming->Timing();
> > +  }
> 
> Again, maybe this was supposed to be part of patch 5?

OK. I can merge them.
(Assignee)

Comment 151

2 years ago
(In reply to Brian Birtles (:birtles) from comment #149)
> ::: dom/animation/AnimationEffectTimingReadOnly.cpp
> @@ +20,3 @@
> >    } else {
> > +    // mDuration is a string or uninitialized,
> > +    // so they are equal if aOther.mDuration is also a string or uninitizlized.
> 
> How about:
> 
> 'We consider all string values and uninitialized values as meaning "auto".
> Since mDuration is either a string or uninitialized, we consider it equal if
> aOther.mDuration is also either a string or uninitialized.'
> 
> ?
> 

Sure!
(Assignee)

Comment 152

2 years ago
Created attachment 8704495 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface. (v11, carry birtles' r+)

1. Add AnimationEffectTimingReadOnly.webidl.
2. Add AnimationEffectTimingReadOnly cpp files.
3. Use AnimationEffectTimingReadOnly as KeyframeEffectReadOnly::mTiming.
Attachment #8704495 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8704016 - Attachment is obsolete: true
(Assignee)

Comment 153

2 years ago
Created attachment 8704496 [details] [diff] [review]
Part 6: Revise AnimationTiming::operator==. (v2, carry birtles' r+)
Attachment #8704496 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8704463 - Attachment is obsolete: true
Comment on attachment 8704482 [details] [diff] [review]
Part 7: Rename AnimationTiming as SpecifiedTiming (v13)

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

This seems mostly fine but I'd like to know what the specific compile errors are we get if we don't use 'struct SpecifiedTiming' and whether there's a better way to work around them than just littering the code with 'struct' here and there, e.g. by using a 'using' declaration or changing the naming scheme to avoid ambiguities.

::: dom/animation/KeyframeEffect.cpp
@@ +75,5 @@
>  KeyframeEffectReadOnly::KeyframeEffectReadOnly(
>    nsIDocument* aDocument,
>    Element* aTarget,
>    nsCSSPseudoElements::Type aPseudoType,
> +  const struct SpecifiedTiming& aTiming)

Do you need this 'struct' here? Are you hitting compile errors because SpecifiedTiming() is ambiguous now?

@@ +111,5 @@
>    return temp.forget();
>  }
>  
>  void
> +KeyframeEffectReadOnly::SetTiming(const struct SpecifiedTiming& aTiming)

Do you need 'struct' here?

@@ +1620,5 @@
>      aRv.Throw(NS_ERROR_DOM_ANIM_NO_TARGET_ERR);
>      return nullptr;
>    }
>  
> +  auto timing = ConvertKeyframeEffectOptions(aOptions);

I don't think this is necessary. We tend to be conservative about using auto when there's no obvious benefit.

::: layout/base/nsDisplayList.cpp
@@ +382,5 @@
>      aPending ?
>      aLayer->AddAnimationForNextTransaction() :
>      aLayer->AddAnimation();
>  
> +  const auto& timing = aAnimation->GetEffect()->SpecifiedTiming();

Again, I don't think we should use auto here.

::: layout/style/nsAnimationManager.cpp
@@ +325,5 @@
>      }
>      return result;
>    }
>  
> +  result = AnimationTimeToTimeStamp(aElapsedTime + mEffect->SpecifiedTiming().mDelay);

This looks to be over 80 characters. Can you please update your editor to indicate lines that are over 80 characters?

::: layout/style/nsTransitionManager.cpp
@@ +53,5 @@
>    // interval. However, it might be possible that we're behind on flushing
>    // causing us to get called *after* the animation interval. So, just in
>    // case, we override the fill mode to 'both' to ensure the progress
>    // is never null.
> +  auto timingToUse = SpecifiedTiming();

No need for auto here I think.
Comment on attachment 8704484 [details] [diff] [review]
Part 8: Move ConvertKeyframeEffectOptions (v6)

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

::: dom/animation/AnimationEffectTimingReadOnly.cpp
@@ +46,5 @@
> +/* static */ SpecifiedTiming
> +AnimationEffectTimingReadOnly::ConvertKeyframeEffectOptions(
> +    const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions)
> +{
> +  SpecifiedTiming animationTiming;

Just call this timing?

@@ +60,5 @@
> +  } else {
> +    animationTiming.mDuration.SetAsUnrestrictedDouble() =
> +      aOptions.IsUnrestrictedDouble() ?
> +      aOptions.GetAsUnrestrictedDouble() :
> +      0.0f;

I've just noticed that this is not actually right--if aOptions is uninitialized we should leave animationTiming.mDuration as uninitialized to represent the auto value... but we can fix that in a separate patch/bug. We should make this patch just about moving code.

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +77,5 @@
>    const SpecifiedTiming& Timing() const { return mTiming; }
>    void SetTiming(const SpecifiedTiming& aTiming) { mTiming = aTiming; }
>  
> +  // A helper function for converting KeyframeEffectOptions into
> +  // AnimationEffectTimingStruct

s/AnimationEffectTimingStruct/SpecifiedTiming/
Attachment #8704484 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 156

2 years ago
(In reply to Brian Birtles (:birtles) from comment #154)
> Comment on attachment 8704482 [details] [diff] [review]
> Part 7: Rename AnimationTiming as SpecifiedTiming (v13)
> 
> Review of attachment 8704482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems mostly fine but I'd like to know what the specific compile errors
> are we get if we don't use 'struct SpecifiedTiming' and whether there's a
> better way to work around them than just littering the code with 'struct'
> here and there, e.g. by using a 'using' declaration or changing the naming
> scheme to avoid ambiguities.

Maybe we can change the function name or the struct name to avoid this. You can check the error message below.

> 
> ::: dom/animation/KeyframeEffect.cpp
> @@ +75,5 @@
> >  KeyframeEffectReadOnly::KeyframeEffectReadOnly(
> >    nsIDocument* aDocument,
> >    Element* aTarget,
> >    nsCSSPseudoElements::Type aPseudoType,
> > +  const struct SpecifiedTiming& aTiming)
> 
> Do you need this 'struct' here? Are you hitting compile errors because
> SpecifiedTiming() is ambiguous now?

Yes. We use the same name for the member function and the timing struct, and I got some compilation errors which indicate I 
should add 'struct' keywords for them in KeyframeEffectReadOnly class.

For example:

KeyframeEffect.cpp:79:9: error: must use 'struct' tag to refer to type 'SpecifiedTiming' in this scope
   const SpecifiedTiming& aTiming)
         ^
         struct

KeyframeEffect.h:220:33: note: struct 'SpecifiedTiming' is hidden by a non-type declaration of 'SpecifiedTiming' here
   const SpecifiedTiming& SpecifiedTiming() const
                          ^

> 
> @@ +111,5 @@
> >    return temp.forget();
> >  }
> >  
> >  void
> > +KeyframeEffectReadOnly::SetTiming(const struct SpecifiedTiming& aTiming)
> 
> Do you need 'struct' here?
> 
> @@ +1620,5 @@
> >      aRv.Throw(NS_ERROR_DOM_ANIM_NO_TARGET_ERR);
> >      return nullptr;
> >    }
> >  
> > +  auto timing = ConvertKeyframeEffectOptions(aOptions);
> 
> I don't think this is necessary. We tend to be conservative about using auto
> when there's no obvious benefit.

OK. I will the struct name, instead of auto.
 
> ::: layout/style/nsAnimationManager.cpp
> @@ +325,5 @@
> >      }
> >      return result;
> >    }
> >  
> > +  result = AnimationTimeToTimeStamp(aElapsedTime + mEffect->SpecifiedTiming().mDelay);
> 
> This looks to be over 80 characters. Can you please update your editor to
> indicate lines that are over 80 characters?

Sorry, I use vimscript to replace these keywords. I will update my editor to indicate it.
(Assignee)

Comment 157

2 years ago
Created attachment 8704551 [details] [diff] [review]
Part 7: Rename AnimationTiming as SpecifiedTiming (v14)

Use KeyframeEffectReadOnly::GetSpecifiedTiming(), instead of
KeyframeEffectReadOnly::SpecifiedTiming() to avoid that struct 'SpecifiedTiming'
may be hidden by this function type in KeyframeEffecrtReadOnly class.
(Assignee)

Updated

2 years ago
Attachment #8704482 - Attachment is obsolete: true
Attachment #8704482 - Flags: review?(bbirtles)
(Assignee)

Comment 158

2 years ago
Comment on attachment 8704551 [details] [diff] [review]
Part 7: Rename AnimationTiming as SpecifiedTiming (v14)

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

Our coding style says: "Getters that never fail and never return null are named Foo(), while all other getters use GetFoo()."[1]. Although KeyframeEffectReadOnly::GetSpecifiedTiming returns a reference which couldn't be null, I want to prevent "struct SpecifiedTiming" from being hidden by the function name, so added a prefix, "Get".

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
Attachment #8704551 - Flags: review?(bbirtles)
(Assignee)

Comment 159

2 years ago
Created attachment 8704555 [details] [diff] [review]
Part 8: Move ConvertKeyframeEffectOptions. (v7, carry birtles' r+)

Move ConvertKeyframeEffectOptions() from KeyframeEffectReadOnly into
AnimationEffectTimingReadOnly.
Attachment #8704555 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8704484 - Attachment is obsolete: true
(Assignee)

Comment 160

2 years ago
Created attachment 8704559 [details] [diff] [review]
Part 9: Keep uninitialized state of KeyframeEffectOptions

Uninitialize the duration if KeyframeEffectOptions is uninitialized.
(Assignee)

Updated

2 years ago
Attachment #8704485 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8704559 - Flags: review?(bbirtles)
(Assignee)

Comment 161

2 years ago
(In reply to Brian Birtles (:birtles) from comment #155)

> I've just noticed that this is not actually right--if aOptions is
> uninitialized we should leave animationTiming.mDuration as uninitialized to
> represent the auto value... but we can fix that in a separate patch/bug. We
> should make this patch just about moving code.


BTW, is there any way to create an uninitialized aOptions by our platform test? I tried to create a KeyframeEffectReadOnly object without any KeyframeEffectOption[1], but always got a normal KeyframeEffectOption obeject[2] whose mDuration is an "auto" string.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/web-animations/keyframe-effect/constructor.html#455
[2] https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffect.cpp?from=KeyframeEffect.cpp#587
(Assignee)

Comment 162

2 years ago
(In reply to Boris Chiou [:boris] from comment #148)
> (In reply to Boris Chiou [:boris] from comment #140)
> > > Can you file a follow-up to move this to a separate file? (Possibly after
> > > renaming to SpecifiedTiming--see my earlier comments).
> > > 
> > > I think once we do that, and once we change mDuration to just Maybe<double>
> > > (or Maybe<TimeDuration>) we can tidy-up the include dependencies since a lot
> > > of places will only need to include SpecifiedTiming.
> > 
> > I agree. I can file a follow-up bug for this.
> 
> Filed Bug 1237173.

Sorry, I found we don't need to add so many inclusions in part 5.
These files includes KeyframeEffect.h, and KeyframeEffect.h also includes AnimationEffectTimingReadOnly.h already because we make this function, "const SpecifiedTiming& SpecifiedTiming()", inline.
I should update part 5 to remove these redundant inclusions after we finish all the reviews. Thanks.
(In reply to Boris Chiou [:boris] from comment #161)
> (In reply to Brian Birtles (:birtles) from comment #155)
> 
> > I've just noticed that this is not actually right--if aOptions is
> > uninitialized we should leave animationTiming.mDuration as uninitialized to
> > represent the auto value... but we can fix that in a separate patch/bug. We
> > should make this patch just about moving code.
> 
> 
> BTW, is there any way to create an uninitialized aOptions by our platform
> test? I tried to create a KeyframeEffectReadOnly object without any
> KeyframeEffectOption[1], but always got a normal KeyframeEffectOption
> obeject[2] whose mDuration is an "auto" string.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> web-animations/keyframe-effect/constructor.html#455
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffect.
> cpp?from=KeyframeEffect.cpp#587

Yeah, good point. According to WebIDL we should get a default dictionary when nothing is passed:

  "If the type of an argument is a dictionary type or a union type that has a dictionary type as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument MUST be specified as optional. Such arguments are always considered to have a default value of an empty dictionary, unless otherwise specified."
  (http://heycam.github.io/webidl/#idl-operations)
(In reply to Boris Chiou [:boris] from comment #158)
> Comment on attachment 8704551 [details] [diff] [review]
> Part 7: Rename AnimationTiming as SpecifiedTiming (v14)
> 
> Review of attachment 8704551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Our coding style says: "Getters that never fail and never return null are
> named Foo(), while all other getters use GetFoo()."[1]. Although
> KeyframeEffectReadOnly::GetSpecifiedTiming returns a reference which
> couldn't be null, I want to prevent "struct SpecifiedTiming" from being
> hidden by the function name, so added a prefix, "Get".

Yeah, I was thinking about this and I was thinking we should stick to the coding style here. I've been wondering if the following naming scheme works:

We already have:
  AnimationEffectTimingProperties (WebIDL dict)
  ComputedTimingProperties (WebIDL dict)
  
And:
  ComputedTiming (struct)

We would *like* to use AnimationEffectTiming for the specified-timing struct but there is already a WebIDL interface with that name. So how about we have:

  ComputedTiming -> ComputedTimingParams
  AnimationTiming/SpecifiedTiming -> TimingParams
  
What do you think?

That would mean we have something like this:
  
  class AnimationEffectTiming {
    Timing() -> returns AnimationEffectTiming (interface)
    SpecifiedTiming() -> returns TimingParams (struct)
    GetComputedTiming() -> returns ComputedTimingParams (struct)
    // ^ I guess we should rename GetComputedTiming to ComputedTiming ??
    
    // Also:
    GetComputedTimingAsDict() -> returns ComputedTimingProperties (dict)
  }

What do you think? If we do that we have to rename ComputedTiming and GetComputedTiming(At) which is a bigger change so maybe we should do this mass-renaming in another bug.
Comment on attachment 8704555 [details] [diff] [review]
Part 8: Move ConvertKeyframeEffectOptions. (v7, carry birtles' r+)

Sorry, I think when I reviewed this I assumed we'd discussed this change before so I didn't really think about if this is the right idea or not.

I think what we should do here is actually add a constructor to AnimationTiming/SpecifiedTiming/whatever-we-call-it that takes a const AnimationEffectTimingProperties&

Now, since KeyframeEffectOptions inherits from AnimationEffectTimingProperties we could write KeyframeEffectReadOnly::ConvertKeyframeEffectOptions as:

    /* static */ SpecifiedTiming
    KeyframeEffectReadOnly::ConvertKeyframeEffectOptions(
        const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions)
    {
      SpecifiedTiming timing;

      if (aOptions.IsKeyframeEffectOptions()) {
        timing = SpecifiedTiming(aOptions.GetAsKeyframeEffectOptions());
      } else {
        timing.mDuration.SetAsUnrestrictedDouble() =
          aOptions.GetAsUnrestrictedDouble();
      }
      
      return timing;
    }

I think that's more simple and makes more sense since the conversion from AnimationEffectTimingProperties to SpecifiedTiming belongs in SpecifiedTiming but conversion from UnrestrictedDoubleOrKeyframeEffectOptions doesn't.

(We could probably even take it a step further and define a constructor for SpecifiedTiming that takes a double and sets the rest of the values to their defaults using a delegated constructor. But that's something for much later once.)
Comment on attachment 8704495 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface. (v11, carry birtles' r+)

>diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
>index c920fac..c94940c 100644
>--- a/dom/animation/KeyframeEffect.h
>+++ b/dom/animation/KeyframeEffect.h
...
>   nsCOMPtr<Element> mTarget;
>   RefPtr<Animation> mAnimation;
> 
>-  AnimationTiming mTiming;
>+  RefPtr<AnimationEffectTimingReadOnly> mTiming;
>   nsCSSPseudoElements::Type mPseudoType;

I just realized we should probably use OwningNonNull[1] here. Then we could drop the assertion
from the Timing()/SpecifiedTiming() method.

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/base/OwningNonNull.h
(Assignee)

Comment 167

2 years ago
(In reply to Brian Birtles (:birtles) from comment #164) 
> Yeah, I was thinking about this and I was thinking we should stick to the
> coding style here. I've been wondering if the following naming scheme works:
> 
> We already have:
>   AnimationEffectTimingProperties (WebIDL dict)
>   ComputedTimingProperties (WebIDL dict)
>   
> And:
>   ComputedTiming (struct)
> 
> We would *like* to use AnimationEffectTiming for the specified-timing struct
> but there is already a WebIDL interface with that name. So how about we have:
> 
>   ComputedTiming -> ComputedTimingParams
>   AnimationTiming/SpecifiedTiming -> TimingParams
>   
> What do you think?
> 
> That would mean we have something like this:
>   
>   class AnimationEffectTiming {
>     Timing() -> returns AnimationEffectTiming (interface)
>     SpecifiedTiming() -> returns TimingParams (struct)
>     GetComputedTiming() -> returns ComputedTimingParams (struct)
>     // ^ I guess we should rename GetComputedTiming to ComputedTiming ??
>     
>     // Also:
>     GetComputedTimingAsDict() -> returns ComputedTimingProperties (dict)
>   }
> 
> What do you think? If we do that we have to rename ComputedTiming and
> GetComputedTiming(At) which is a bigger change so maybe we should do this
> mass-renaming in another bug.

OK. Let us rename AnimationTiming as TimingParams in this bug. I will create another bug to handle the renaming of ComputedTiming.
(Assignee)

Updated

2 years ago
Attachment #8704555 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8704559 - Flags: review?(bbirtles)
(Assignee)

Comment 168

2 years ago
(In reply to Brian Birtles (:birtles) from comment #166)
> Comment on attachment 8704495 [details] [diff] [review]
> Part 5: Add AnimationEffectTimingReadOnly interface. (v11, carry birtles' r+)
> 
> >diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
> >index c920fac..c94940c 100644
> >--- a/dom/animation/KeyframeEffect.h
> >+++ b/dom/animation/KeyframeEffect.h
> ...
> >   nsCOMPtr<Element> mTarget;
> >   RefPtr<Animation> mAnimation;
> > 
> >-  AnimationTiming mTiming;
> >+  RefPtr<AnimationEffectTimingReadOnly> mTiming;
> >   nsCSSPseudoElements::Type mPseudoType;
> 
> I just realized we should probably use OwningNonNull[1] here. Then we could
> drop the assertion
> from the Timing()/SpecifiedTiming() method.
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/xpcom/base/OwningNonNull.h

OK. It's a good idea. I think I should revise part 5 and also remove some redundant code, and then send a review request again. Thanks.
(Assignee)

Comment 169

2 years ago
Created attachment 8704985 [details] [diff] [review]
Part 3: Store the original value of fill. (v7, carry birtles' r+)

Rebased.
Attachment #8704985 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8702178 - Attachment is obsolete: true
(Assignee)

Comment 170

2 years ago
Created attachment 8704987 [details] [diff] [review]
Part 4: Use OwingUnrestrictedDoubleOrString for duration. (v5, carry birtles' r+)

Rebased.
Attachment #8704987 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8702179 - Attachment is obsolete: true
(Assignee)

Comment 171

2 years ago
Created attachment 8705004 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface. (v12)

1. Add AnimationEffectTimingReadOnly.webidl.
2. Add AnimationEffectTimingReadOnly cpp files.
3. Use AnimationEffectTimingReadOnly as KeyframeEffectReadOnly::mTiming.

This updated patch removes redundant inclusions and uses OwningNonNull<> as the
type of KeyframeEffectReadOnly::mTiming.
(Assignee)

Updated

2 years ago
Attachment #8704495 - Attachment is obsolete: true
(Assignee)

Comment 172

2 years ago
Created attachment 8705013 [details] [diff] [review]
Part 7: Rename AnimationTiming as TimingParams (v15)

1. struct AnimationTiming -> struct TimingParams
2. AnimationEffectReadOnly::TimingAsObject() -> AnimationEffectReadOnly::Timing()
3. KeyframeEffectReadOnly::Timing() -> KeyframeEffectReadOnly::SpecifiedTiming()
(Assignee)

Updated

2 years ago
Attachment #8704551 - Attachment is obsolete: true
Attachment #8704551 - Flags: review?(bbirtles)
(Assignee)

Comment 173

2 years ago
Comment on attachment 8705004 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface. (v12)

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

Hi Brian, I change the type of KeyframeEffectReadOnly::mTiming in this bug (OwingNonNull<>, instead of RefPtr<...>), so need your review again. Thanks.
Attachment #8705004 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
Attachment #8705013 - Flags: review?(bbirtles)
(Assignee)

Comment 174

2 years ago
Created attachment 8705046 [details] [diff] [review]
Part 8: Add an operator=() for TimingParams

Overload TimingParams::operator=() for AnimationEffectTimingProperties objects,
so we can assign an AnimationEffectTimingProperties/KeyframeEffectOptions object
to a TimingParams object.

We also keep the uninitialized state of KeyframeEffectOptions::mDuration while
converting a KeyframeEffectOptions object into a TimingParams object.
(Assignee)

Updated

2 years ago
Attachment #8704555 - Attachment is obsolete: true
Attachment #8704559 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705046 - Flags: review?(bbirtles)
(Assignee)

Comment 175

2 years ago
Created attachment 8705052 [details] [diff] [review]
Part 9: Test (v2)
(Assignee)

Updated

2 years ago
Attachment #8705052 - Flags: review?(bbirtles)
Comment on attachment 8705004 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface. (v12)

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

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +25,5 @@
> +namespace mozilla {
> +
> +struct AnimationTiming
> +{
> +  // The unitialized state of mDuration represents "auto"

Add a comment with a bug number where we will replace this with Maybe<TimeDuration> ?

::: dom/animation/KeyframeEffect.cpp
@@ +77,5 @@
>    nsCSSPseudoElements::Type aPseudoType,
>    const AnimationTiming& aTiming)
>    : AnimationEffectReadOnly(aDocument)
>    , mTarget(aTarget)
> +  , mTiming(*(new AnimationEffectTimingReadOnly(aTiming)))

I guess OwningNonNull doesn't have a ctor that takes a pointer because we don't want to assert in the ctor. It *does* have an assignment operator that takes a pointer, however, so maybe this would be more clear if we just drop mTiming from the initializer list and put it in the constructor body as:

  mTiming = new AnimationEffectTimingReadOnly(aTiming);

Right now this looks really confusing and anyone who is not familiar with OwningNonNull will assume this is a memory leak.

::: dom/animation/KeyframeEffect.h
@@ +331,5 @@
>  
>    nsCOMPtr<Element> mTarget;
>    RefPtr<Animation> mAnimation;
>  
> +  OwningNonNull<AnimationEffectTimingReadOnly> mTiming;

I guess we should add #include "mozilla/OwningNonNull.h" as well?

::: dom/webidl/AnimationEffectTimingReadOnly.webidl
@@ +10,5 @@
> + * liability, trademark and document use rules apply.
> + */
> +
> +[Func="nsDocument::IsWebAnimationsEnabled"]
> +interface AnimationEffectTimingReadOnly {

If you're adding a new interface, you need to edit dom/tests/mochitest/general/test_interfaces.html as well. I guess you haven't run this patch through try yet?
Attachment #8705004 - Flags: review?(bbirtles) → review+
Comment on attachment 8705013 [details] [diff] [review]
Part 7: Rename AnimationTiming as TimingParams (v15)

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

r=birtles with comments addressed

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +71,5 @@
>    }
>    PlaybackDirection Direction() const { return mTiming.mDirection; }
>    void GetEasing(nsString& aRetVal) const { aRetVal.AssignLiteral("linear"); }
>  
> +  const TimingParams& Timing() const { return mTiming; }

Can we call this TimingParams() or do we get ambiguities? If not, perhaps we can use AsTimingParams()?

@@ +72,5 @@
>    PlaybackDirection Direction() const { return mTiming.mDirection; }
>    void GetEasing(nsString& aRetVal) const { aRetVal.AssignLiteral("linear"); }
>  
> +  const TimingParams& Timing() const { return mTiming; }
> +  void SetTiming(const TimingParams& aTiming) { mTiming = aTiming; }

If we can rename Timing() to TimingParams(), we should change this to SetTimingParams().

::: dom/animation/KeyframeEffect.cpp
@@ +611,4 @@
>  KeyframeEffectReadOnly::ConvertKeyframeEffectOptions(
>      const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions)
>  {
> +  TimingParams animationTiming;

We should rename animationTiming to something else now, perhaps just 'timing'.

::: dom/animation/KeyframeEffect.h
@@ +215,5 @@
>    }
>  
> +  const TimingParams& SpecifiedTiming() const { return mTiming->Timing(); }
> +  void SetTiming(const TimingParams& aTiming);
> +  already_AddRefed<AnimationEffectTimingReadOnly> Timing() const override;

Nit: Can we group the two methods that deal with a TimingParams together, give them consistent names, and put them after the WebIDL method, e.g.

  already_AddRefed<AnimationEffectTimingReadOnly> Timing() const override;

  const TimingParams& SpecifiedTiming() const { return mTiming->Timing(); }
  void SetSpecifiedTiming(const TimingParams& aTiming);
Attachment #8705013 - Flags: review?(bbirtles) → review+
Comment on attachment 8705046 [details] [diff] [review]
Part 8: Add an operator=() for TimingParams

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

Nice.
Attachment #8705046 - Flags: review?(bbirtles) → review+
Comment on attachment 8705052 [details] [diff] [review]
Part 9: Test (v2)

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

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
@@ +450,5 @@
>  });
>  
>  
>  // KeyframeEffectOptions
> +// Tested by getComputedTiming() and timing attribute

Not sure we need this line.

@@ +470,5 @@
> +  assert_equals(ct.delay, 0, "computed delay");
> +  assert_equals(ct.fill, "none", "computed fill");
> +  assert_equals(ct.iterations, 1.0, "computed iterations");
> +  assert_equals(ct.duration, 0, "computed duration");
> +  assert_equals(ct.direction, "normal", "computed direction");

I think we should drop the getComputedTiming() part. We try to keep each test as simple and specific as possible. If we think getComputedTiming() needs testing, we should write a separate test for it.

Likewise for the rest of this file: I think we should move the getComputedTiming() tests to a separate file.

The idea is that getComputedTiming() is calculated from 'timing' so in the tests for the KeyframeEffectReadOnly constructor, we should simply test that 'timing' is correctly initialized. Then we can write separate tests that check that getComputedTiming() is calculated correctly from 'timing'.

Could you make a separate file for testing getComputedTiming()? Probably as keyframe-effect/getComputedTiming.html or something like that?
Attachment #8705052 - Flags: review?(bbirtles)
Comment hidden (typo)
(Assignee)

Comment 181

2 years ago
(In reply to Brian Birtles (:birtles) from comment #176)
> ::: dom/animation/AnimationEffectTimingReadOnly.h
> @@ +25,5 @@
> > +namespace mozilla {
> > +
> > +struct AnimationTiming
> > +{
> > +  // The unitialized state of mDuration represents "auto"
> 
> Add a comment with a bug number where we will replace this with
> Maybe<TimeDuration> ?


Sure. We can replace this with Maybe<TimeDuration> in Bug 1237173.

> 
> ::: dom/animation/KeyframeEffect.cpp
> @@ +77,5 @@
> >    nsCSSPseudoElements::Type aPseudoType,
> >    const AnimationTiming& aTiming)
> >    : AnimationEffectReadOnly(aDocument)
> >    , mTarget(aTarget)
> > +  , mTiming(*(new AnimationEffectTimingReadOnly(aTiming)))
> 
> I guess OwningNonNull doesn't have a ctor that takes a pointer because we
> don't want to assert in the ctor. It *does* have an assignment operator that
> takes a pointer, however, so maybe this would be more clear if we just drop
> mTiming from the initializer list and put it in the constructor body as:
> 
>   mTiming = new AnimationEffectTimingReadOnly(aTiming);
> 
> Right now this looks really confusing and anyone who is not familiar with
> OwningNonNull will assume this is a memory leak.

OK. I can move it into the constructor body.

> 
> ::: dom/animation/KeyframeEffect.h
> @@ +331,5 @@
> >  
> >    nsCOMPtr<Element> mTarget;
> >    RefPtr<Animation> mAnimation;
> >  
> > +  OwningNonNull<AnimationEffectTimingReadOnly> mTiming;
> 
> I guess we should add #include "mozilla/OwningNonNull.h" as well?

OK.

> 
> ::: dom/webidl/AnimationEffectTimingReadOnly.webidl
> @@ +10,5 @@
> > + * liability, trademark and document use rules apply.
> > + */
> > +
> > +[Func="nsDocument::IsWebAnimationsEnabled"]
> > +interface AnimationEffectTimingReadOnly {
> 
> If you're adding a new interface, you need to edit
> dom/tests/mochitest/general/test_interfaces.html as well. I guess you
> haven't run this patch through try yet?

OK. I haven't run this on try yet. Thanks for your reminder.
(Assignee)

Comment 182

2 years ago
(In reply to Brian Birtles (:birtles) from comment #177)
> ::: dom/animation/AnimationEffectTimingReadOnly.h
> @@ +71,5 @@
> >    }
> >    PlaybackDirection Direction() const { return mTiming.mDirection; }
> >    void GetEasing(nsString& aRetVal) const { aRetVal.AssignLiteral("linear"); }
> >  
> > +  const TimingParams& Timing() const { return mTiming; }
> 
> Can we call this TimingParams() or do we get ambiguities? If not, perhaps we
> can use AsTimingParams()?

I get ambiguities here, so I use AsTimingParams().

> 
> @@ +72,5 @@
> >    PlaybackDirection Direction() const { return mTiming.mDirection; }
> >    void GetEasing(nsString& aRetVal) const { aRetVal.AssignLiteral("linear"); }
> >  
> > +  const TimingParams& Timing() const { return mTiming; }
> > +  void SetTiming(const TimingParams& aTiming) { mTiming = aTiming; }
> 
> If we can rename Timing() to TimingParams(), we should change this to
> SetTimingParams().

I still rename this as SetTimingParams() because I think the newer name is better after we use to AsTimingParams() for the getter.

> 
> ::: dom/animation/KeyframeEffect.cpp
> @@ +611,4 @@
> >  KeyframeEffectReadOnly::ConvertKeyframeEffectOptions(
> >      const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions)
> >  {
> > +  TimingParams animationTiming;
> 
> We should rename animationTiming to something else now, perhaps just
> 'timing'.

I renamed this on Part 8, so I still keep it unchanged in this patch. Thanks.

> 
> ::: dom/animation/KeyframeEffect.h
> @@ +215,5 @@
> >    }
> >  
> > +  const TimingParams& SpecifiedTiming() const { return mTiming->Timing(); }
> > +  void SetTiming(const TimingParams& aTiming);
> > +  already_AddRefed<AnimationEffectTimingReadOnly> Timing() const override;
> 
> Nit: Can we group the two methods that deal with a TimingParams together,
> give them consistent names, and put them after the WebIDL method, e.g.
> 
>   already_AddRefed<AnimationEffectTimingReadOnly> Timing() const override;
> 
>   const TimingParams& SpecifiedTiming() const { return mTiming->Timing(); }
>   void SetSpecifiedTiming(const TimingParams& aTiming);

Sure.
(Assignee)

Comment 183

2 years ago
Created attachment 8705465 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface. (v13, carry birtles' r+)

1. Add AnimationEffectTimingReadOnly.webidl.
2. Add AnimationEffectTimingReadOnly cpp files.
3. Use AnimationEffectTimingReadOnly as KeyframeEffectReadOnly::mTiming.

Updated.
(Assignee)

Updated

2 years ago
Attachment #8705004 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8705465 - Flags: review+
(Assignee)

Comment 184

2 years ago
Created attachment 8705466 [details] [diff] [review]
Part 7: Rename AnimationTiming as TimingParams. (v16, carry birtles' r+)

1. struct AnimationTiming -> struct TimingParams
2. AnimationEffectReadOnly::TimingAsObject() -> AnimationEffectReadOnly::Timing()
3. KeyframeEffectReadOnly::Timing() -> KeyframeEffectReadOnly::SpecifiedTiming()

Updated.
Attachment #8705466 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8705013 - Attachment is obsolete: true
Comment hidden (typo)
(Assignee)

Comment 186

2 years ago
(In reply to Brian Birtles (:birtles) from comment #179)
> I think we should drop the getComputedTiming() part. We try to keep each
> test as simple and specific as possible. If we think getComputedTiming()
> needs testing, we should write a separate test for it.
> 
> Likewise for the rest of this file: I think we should move the
> getComputedTiming() tests to a separate file.
> 
> The idea is that getComputedTiming() is calculated from 'timing' so in the
> tests for the KeyframeEffectReadOnly constructor, we should simply test that
> 'timing' is correctly initialized. Then we can write separate tests that
> check that getComputedTiming() is calculated correctly from 'timing'.
> 
> Could you make a separate file for testing getComputedTiming()? Probably as
> keyframe-effect/getComputedTiming.html or something like that?

Sure. I agree.
(Assignee)

Updated

2 years ago
Attachment #8705465 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8705466 - Flags: review?(bugs)
(Assignee)

Comment 187

2 years ago
Hi, Olli,
Could you please take a look at the DOM changes(webidl files, test_interfaces.html, and other related files) in part 5 and part 7?

Part 5: Add a new interface, AnimationEffectTimingReadOnly
Part 7: Rename the binary name.

Thanks.
(Assignee)

Comment 188

2 years ago
Created attachment 8705501 [details] [diff] [review]
Part 9: Test (v3)
(Assignee)

Updated

2 years ago
Attachment #8705052 - Attachment is obsolete: true
(Assignee)

Comment 189

2 years ago
Comment on attachment 8705501 [details] [diff] [review]
Part 9: Test (v3)

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

I move the tests of getComputedTiming() into a separate file and use "effect.timing" to test KeyframeEffectOptions in constructor.html.
Attachment #8705501 - Flags: review?(bbirtles)
Comment on attachment 8705501 [details] [diff] [review]
Part 9: Test (v3)

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

::: testing/web-platform/tests/web-animations/keyframe-effect/getComputedTiming.html
@@ +11,5 @@
> +<div id="log"></div>
> +<div id="target"></div>
> +<style>
> +#target {
> +  border-style: solid;  /* so border-*-width values don't compute to 0 */

I don't think we need this?

@@ +191,5 @@
> +                  "computed iterations");
> +    assert_equals(ct.duration, stest.expected.duration, "computed duration");
> +    assert_equals(ct.direction, stest.expected.direction, "computed direction");
> +  }, "values of getComputedTiming() when a KeyframeEffectReadOnly is " +
> +      "constructed by " + stest.desc);

Nit: The quotes should probably line up here.
Attachment #8705501 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 191

2 years ago
(In reply to Brian Birtles (:birtles) from comment #190)
> Comment on attachment 8705501 [details] [diff] [review]
> Part 9: Test (v3)
> 
> Review of attachment 8705501 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> testing/web-platform/tests/web-animations/keyframe-effect/getComputedTiming.
> html
> @@ +11,5 @@
> > +<div id="log"></div>
> > +<div id="target"></div>
> > +<style>
> > +#target {
> > +  border-style: solid;  /* so border-*-width values don't compute to 0 */
> 
> I don't think we need this?

No. I just copy the setting from constructor.html. I can remove it.

> 
> @@ +191,5 @@
> > +                  "computed iterations");
> > +    assert_equals(ct.duration, stest.expected.duration, "computed duration");
> > +    assert_equals(ct.direction, stest.expected.direction, "computed direction");
> > +  }, "values of getComputedTiming() when a KeyframeEffectReadOnly is " +
> > +      "constructed by " + stest.desc);
> 
> Nit: The quotes should probably line up here.

OK
(Assignee)

Comment 192

2 years ago
Created attachment 8705510 [details] [diff] [review]
Part 9: Test. (v4, carry birtles' r+)
Attachment #8705510 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8705501 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8705465 - Flags: review?(bugs) → review+

Updated

2 years ago
Attachment #8705466 - Flags: review?(bugs) → review+
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8706189 - Flags: review?(bbirtles)
Comment hidden (obsolete)
(Assignee)

Comment 202

2 years ago
Created attachment 8706190 [details] [diff] [review]
Part 1: Use unrestricted double for iterations. (v7, carry birtles' r+)

We want to store the original value from KeyframeEffectOptions whose
iterations is unrestricted double. Therefore, we can get the original value
of iterations by AnimationEffectTimingReadOnly.

By the way, replace mIterationCount with mIterations.

Updated:
Fix compilation errors on B2G.
Error:
home/worker/workspace/gecko/dom/animation/KeyframeEffect.cpp:303:54: error:
ISO C++ says that these are ambiguous, even though the worst conversion for
the first is better than the worst conversion for the second: [-Werror]
Attachment #8706190 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8702176 - Attachment is obsolete: true
Attachment #8706189 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1238424
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Hi, part 3 failed to apply:

renamed 1214536 -> Bug-1214536---Part-3-Store-the-original-value-of-f.patch
applying Bug-1214536---Part-3-Store-the-original-value-of-f.patch
patching file dom/animation/KeyframeEffect.cpp
Hunk #1 FAILED at 0
1 out of 5 hunks FAILED -- saving rejects to file dom/animation/KeyframeEffect.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1214536---Part-3-Store-the-original-value-of-f.patch

could you take a look, thanks!
Flags: needinfo?(boris.chiou)
Keywords: checkin-needed
(Assignee)

Comment 206

2 years ago
Created attachment 8707713 [details] [diff] [review]
Part 3: Store the original value of fill. (v7, carry birtles' r+)

FillMode could be 'auto', and we should treat it as 'none' in the timing model.
However, AnimationEffectTimingReadOnly should get its original value.

By the way, replace mFillMode with mFill.
Attachment #8707713 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8704985 - Attachment is obsolete: true
(Assignee)

Comment 207

2 years ago
Created attachment 8707714 [details] [diff] [review]
Part 4: Use OwingUnrestrictedDoubleOrString for duration. (v5, carry birtles' r+)

We store the original value of duration in AnimationTiming, and add
computed duration in ComputedTiming, so both the Timing model and
AnimationEffectTimingReadOnly can get what they want.

By the way, replace mIterationDuration with mDuration.

Rebased.
Attachment #8707714 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8704987 - Attachment is obsolete: true
(Assignee)

Comment 208

2 years ago
Created attachment 8707716 [details] [diff] [review]
Part 5: Add AnimationEffectTimingReadOnly interface. (v13, carry birtles' r+)

1. Add AnimationEffectTimingReadOnly.webidl.
2. Add AnimationEffectTimingReadOnly cpp files.
3. Use AnimationEffectTimingReadOnly as KeyframeEffectReadOnly::mTiming.

Rebased.
Attachment #8707716 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8705465 - Attachment is obsolete: true
(Assignee)

Comment 209

2 years ago
Created attachment 8707717 [details] [diff] [review]
Part 6: Revise AnimationTiming::operator==. (v2, carry birtles' r+)

Rebased.
Attachment #8707717 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8704496 - Attachment is obsolete: true
(Assignee)

Comment 210

2 years ago
Created attachment 8707718 [details] [diff] [review]
Part 7: Rename AnimationTiming as TimingParams. (v16, carry birtles and smaug's r+)

1. struct AnimationTiming -> struct TimingParams
2. AnimationEffectReadOnly::TimingAsObject() -> AnimationEffectReadOnly::Timing()
3. KeyframeEffectReadOnly::Timing() -> KeyframeEffectReadOnly::SpecifiedTiming()

Rebased.
Attachment #8707718 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8705466 - Attachment is obsolete: true
(Assignee)

Comment 211

2 years ago
Created attachment 8707719 [details] [diff] [review]
Part 8: Add an operator=() for TimingParams. (carry birtles' r+)

Overload TimingParams::operator=() for AnimationEffectTimingProperties objects,
so we can assign an AnimationEffectTimingProperties/KeyframeEffectOption object
to a TimingParams object.

We also keep the uninitialized state of KeyframeEffectOptions::mDuration while
converting a KeyframeEffectOptions object into a TimingParams object.

Rebased.
Attachment #8707719 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8705046 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Flags: needinfo?(boris.chiou)
Keywords: checkin-needed
(Assignee)

Comment 212

2 years ago
(In reply to Carsten Book [:Tomcat] from comment #205)
> Hi, part 3 failed to apply:
> 
> renamed 1214536 -> Bug-1214536---Part-3-Store-the-original-value-of-f.patch
> applying Bug-1214536---Part-3-Store-the-original-value-of-f.patch
> patching file dom/animation/KeyframeEffect.cpp
> Hunk #1 FAILED at 0
> 1 out of 5 hunks FAILED -- saving rejects to file
> dom/animation/KeyframeEffect.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh
> Bug-1214536---Part-3-Store-the-original-value-of-f.patch
> 
> could you take a look, thanks!

Sure. I rebased my patches from Part 3 to Part 8. Thanks.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.