Move SpecifiedTiming (or AnimationTiming) to a new file

RESOLVED FIXED in Firefox 48

Status

()

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

People

(Reporter: boris, Assigned: daisuke)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(URL)

Attachments

(3 attachments, 10 obsolete attachments)

15.96 KB, patch
Details | Diff | Splinter Review
27.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.30 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Bug 1214536 moved the timing struct, SpecifiedTiming(or AnimationTiming) into AnimationEffectTimingReadOnly.h, but this struct may be used by many other modules, and it's better to move it to a separate file to reduce the include dependencies.
This data structure is now named TimingParams.

More than moving the struct, the main reason I asked for this bug to be filed in bug 1214536 comment 137 was as follows:

> 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.

So actually the other change is converting the type of mDuration. I think probably Maybe<StickyTimeDuration> is the most appropriate type for this.

At the end of bug 1214536 comment 167 Boris said he would file another bug for renaming ComputedTiming to ComputedTimingParams but I guess he hasn't had a chance to do that yet so maybe we should that here as well.
(Assignee)

Updated

2 years ago
Assignee: nobody → daisuke
(Assignee)

Comment 4

2 years ago
Created attachment 8725960 [details] [diff] [review]
1237173-P1.patch

Part1 Move TimingParam struct to a new file.
Attachment #8725960 - Flags: review?(bbirtles)
(Assignee)

Comment 5

2 years ago
Created attachment 8725964 [details] [diff] [review]
1237173-P2.patch

Part2 Change type of duration to Maybe<StickyTimeDuration>
Attachment #8725964 - Flags: review?(bbirtles)
(Assignee)

Comment 6

2 years ago
Created attachment 8725966 [details] [diff] [review]
1237173-P3.patch

Part3 Change to 'auto' duration from NaN, negative number or string
Attachment #8725966 - Flags: review?(bbirtles)
Comment on attachment 8725960 [details] [diff] [review]
1237173-P1.patch

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

r=birtles with comments addressed

::: dom/animation/KeyframeEffect.h
@@ -42,2 @@
>  class OwningElementOrCSSPseudoElement;
> -class UnrestrictedDoubleOrKeyframeEffectOptions;

I think we still need these since they are referred to later in the file.

::: dom/animation/TimingParams.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et cindent: */

Apparently the mode line is now:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 sts=2 et sw=2 tw=80: */

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

::: dom/animation/TimingParams.h
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et cindent: */

This mode line should be:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 sts=2 et sw=2 tw=80: */
Attachment #8725960 - Flags: review?(bbirtles) → review+
Comment on attachment 8725960 [details] [diff] [review]
1237173-P1.patch

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

::: dom/animation/TimingParams.h
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_TimingParams_h
> +#define mozilla_TimingParams_h
> +

Oh, this needs to include Maybe.h, TimeStamp.h (// For TimeDuration), whatever bindings header file defines OwningUnrestrictedDoubleOrString, and whatever header file defines ComputedTimingFunction.

If you're defining a class/struct which has members that aren't pointer members, you typically need to include the headers for those types so that other places that include the header have everything they need.
Comment on attachment 8725964 [details] [diff] [review]
1237173-P2.patch

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

Looks good but I want to have another look with the following changes made.

::: dom/animation/AnimationEffectTiming.cpp
@@ +20,5 @@
>  
>  void
>  AnimationEffectTiming::SetDuration(const UnrestrictedDoubleOrString& aDuration)
>  {
> +  Maybe<StickyTimeDuration> newDuration; //Nothing

Nit: Space before 'Nothing' (personally, I'm not sure a comment is needed here)

@@ +23,5 @@
>  {
> +  Maybe<StickyTimeDuration> newDuration; //Nothing
> +  if (aDuration.IsUnrestrictedDouble()) {
> +    newDuration = Some(StickyTimeDuration::
> +                       FromMilliseconds(aDuration.GetAsUnrestrictedDouble()));

I don't think we normally break at ::

Perhaps:

    newDuration = Some(StickyTimeDuration::FromMilliseconds(
                         aDuration.GetAsUnrestrictedDouble()));

::: dom/animation/AnimationEffectTimingReadOnly.cpp
@@ +50,5 @@
>  }
>  
>  void
> +AnimationEffectTimingReadOnly::
> +GetDuration(OwningUnrestrictedDoubleOrString& aRetVal) const

We don't normally break after the ::

Instead you should wrap the arguments onto a new line. I've noticed a lot of people use 4 spaces when they can't line the arguments up with the (

e.g.

void
AnimationEffectTimingReadOnly::GetDuration(
    OwningUnrestrictedDoubleOrString& aRetVal) const

@@ +52,5 @@
>  void
> +AnimationEffectTimingReadOnly::
> +GetDuration(OwningUnrestrictedDoubleOrString& aRetVal) const
> +{
> +  if (mTiming.mDuration.isSome()) {

if (mTiming.mDuration) {

@@ +55,5 @@
> +{
> +  if (mTiming.mDuration.isSome()) {
> +    aRetVal.SetAsUnrestrictedDouble() = mTiming.mDuration->ToMilliseconds();
> +  } else {
> +    aRetVal.SetAsString() = NS_LITERAL_STRING("auto");

Nit: I think aRetVal.SetAsString().AssignLiteral("auto") might be fractionally faster since I think NS_LITERAL_STRING will still walk the string buffer to get its length. It's not a big deal here, however.

::: dom/animation/KeyframeEffect.cpp
@@ +241,1 @@
>        result.mDuration = StickyTimeDuration::FromMilliseconds(durationMs);

It seems like we convert from a StickyTimeDuration to a double then back to a StickyTimeDuration again. Why don't we just assign aTiming.mDuration to result.mDuration after checking that it is > 0?

e.g.

  if (aTiming.mDuration && aTiming.mDuration.ref() > zeroDuration) {
    result.mDuration = aTiming.mDuration.ref();
  }

::: dom/animation/KeyframeEffect.h
@@ +19,5 @@
>  #include "mozilla/StyleAnimationValue.h"
>  #include "mozilla/TimeStamp.h"
>  #include "mozilla/TimingParams.h"
>  #include "mozilla/dom/AnimationEffectReadOnly.h"
> +#include "mozilla/dom/AnimationEffectTimingReadOnly.h"

I thought you added this in part 1?

(Nit: Also, we don't need a blank line here.)

::: dom/animation/TimingParams.cpp
@@ +16,5 @@
>    , mFill(aRhs.mFill)
>  {
> +  if (aRhs.mDuration.IsUnrestrictedDouble()) {
> +    mDuration = Some(StickyTimeDuration::FromMilliseconds
> +                  (aRhs.mDuration.GetAsUnrestrictedDouble()));

This indentation is odd. The ( on the second line should probably be under the 'S' from StickyTimeDuration in the first line.

@@ +18,5 @@
> +  if (aRhs.mDuration.IsUnrestrictedDouble()) {
> +    mDuration = Some(StickyTimeDuration::FromMilliseconds
> +                  (aRhs.mDuration.GetAsUnrestrictedDouble()));
> +  } else {
> +    mDuration.reset();

This call to reset() isn't needed. mDuration will initialize to Nothing() by default.

@@ +76,5 @@
>  bool
>  TimingParams::operator==(const TimingParams& aOther) const
>  {
>    bool durationEqual;
> +  if (mDuration.isSome()) {

We can actually just write:

  if (mDuration) {

Since Maybe.h includes this line:

  explicit operator bool() const { return isSome(); }

But actually, I don't think we need any of this, right?

We can just drop |durationEqual| altogether and just use

  return mDuration == aOther.mDuration &&
         ...

right?

::: dom/animation/TimingParams.h
@@ +34,5 @@
>      const dom::UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
>      const Nullable<dom::ElementOrCSSPseudoElement>& aTarget);
>  
>    // The unitialized state of mDuration represents "auto".
> +  Maybe<StickyTimeDuration> mDuration;

We need to include mozilla/StickyTimeDuration.h in this file.

We should also update the comment to say something like :
// mDuration.isNothing() represents the "auto" value

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +583,5 @@
>      }
>  
>      TimingParams timing;
> +    timing.mDuration = Some(StickyTimeDuration::FromMilliseconds
> +                         (animation.duration().ToMilliseconds()));

When breaking after a function name, include the ( on the original line.

Also, we should indent the arguments accordingly:

    timing.mDuration = Some(StickyTimeDuration::FromMilliseconds(
                              animation.duration().ToMilliseconds()));

But more importantly, why are we going to milliseconds and back again? Is it because we have a TimeDuration but need a StickyTimeDuration? If so we can just write:

    timing.mDuration = Some(StickyTimeDuration(animation.duration()));

::: layout/style/nsAnimationManager.cpp
@@ +572,5 @@
>    {
>      TimingParams timing;
>  
> +    timing.mDuration = Some(StickyTimeDuration::
> +			    FromMilliseconds(aStyleAnimation.GetDuration()));

Nit: Wrapping here
Attachment #8725964 - Flags: review?(bbirtles)
Comment on attachment 8725966 [details] [diff] [review]
1237173-P3.patch

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

As discussed, we should probably throw when we are passed a negative value or a string other than "auto" but feel free to do that in another bug. I've yet to update the spec with that although I filed an issue[1].

[1] https://github.com/w3c/web-animations/issues/143

I'd like to see this after rebasing on the changes to part 2.

::: dom/animation/KeyframeEffect.cpp
@@ +236,5 @@
>    ComputedTiming result;
>  
>    if (aTiming.mDuration.isSome()) {
>      double durationMs = aTiming.mDuration->ToMilliseconds();
> +    result.mDuration = StickyTimeDuration::FromMilliseconds(durationMs);

I think this code is going to change after fixing part 2 anyway so I'd like to see it again, but I think we should add least add an assertion:

MOZ_ASSERT(aTiming.mDuration.ref() >= zeroDuration,
           "Iteration duration should be positive");

Or if we put this assertion before the if (...):

MOZ_ASSERT(!aTiming.mDuration || aTiming.mDuration.ref() >= zeroDuration, ...);

::: dom/animation/TimingParams.cpp
@@ +30,5 @@
> +  if (aDuration >= 0.0) {
> +    mDuration = Some(StickyTimeDuration::FromMilliseconds(aDuration));
> +  } else {
> +    mDuration.reset();
> +  }

mDuration defaults to Nothing() so we don't need this last branch.
Attachment #8725966 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #10)
> ::: dom/animation/TimingParams.cpp
> @@ +30,5 @@
> > +  if (aDuration >= 0.0) {
> > +    mDuration = Some(StickyTimeDuration::FromMilliseconds(aDuration));
> > +  } else {
> > +    mDuration.reset();
> > +  }
> 
> mDuration defaults to Nothing() so we don't need this last branch.

In that case, we can directly use emplace, that means we can avoid Some and Move, like this:

 mDuration.emplace(StickyTimeDuration::FromMilliseconds(aDuration));

Can't we?
(Assignee)

Comment 13

2 years ago
Created attachment 8727040 [details] [diff] [review]
1237173-P1-v2.patch

Update 1237173-P1.patch
Part1: Move TimingParam struct to a new file.
Attachment #8725960 - Attachment is obsolete: true
Attachment #8727040 - Flags: review?(bbirtles)
(Assignee)

Comment 14

2 years ago
Created attachment 8727041 [details] [diff] [review]
1237173-P2-v2.patch

Update 1237173-P2.patch
Change type of duration to Maybe<StickyTimeDuration>
Attachment #8725964 - Attachment is obsolete: true
Attachment #8727041 - Flags: review?(bbirtles)
(Assignee)

Comment 15

2 years ago
Created attachment 8727042 [details] [diff] [review]
1237173-P3-v2.patch

Modify 1237173-P3.patch to throw TypeError if duration is NaN, negative value or not 'auto' string.
Attachment #8725966 - Attachment is obsolete: true
Attachment #8727042 - Flags: review?(bbirtles)
(Assignee)

Comment 16

2 years ago
Created attachment 8727122 [details] [diff] [review]
1237173-P3-v3.patch

Forgot to add MOZ_ASSERT.
Also modified inside of KeyframeEffect constructor.
Attachment #8727042 - Attachment is obsolete: true
Attachment #8727042 - Flags: review?(bbirtles)
Attachment #8727122 - Flags: review?(bbirtles)
Comment on attachment 8727040 [details] [diff] [review]
1237173-P1-v2.patch

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

In general you don't need to re-request review if you already have r+. You only need to re-request review if you've made major changes or if there's something you're not certain about.

However, there are a couple of things still not addressed from the previous review (although one is probably is due to the fact that the Splinter review tool doesn't quote enough context so my earlier review comment wasn't clear).

r=birtles with comments addressed.

::: dom/animation/KeyframeEffect.h
@@ -37,5 @@
>  class AnimValuesStyleRule;
>  enum class CSSPseudoElementType : uint8_t;
>  
>  namespace dom {
> -class ElementOrCSSPseudoElement;

I think we still need this.

(I think when I pointed this out last time, the Splinter review tool didn't quote this line so it wasn't clear what I was talking about. MozReview is better at this.)

::: dom/animation/TimingParams.h
@@ +7,5 @@
> +#ifndef mozilla_TimingParams_h
> +#define mozilla_TimingParams_h
> +
> +#include "mozilla/TimeStamp.h" // for TimeDuration
> +#include "mozilla/Maybe.h"

My previous review comment was, "this needs to include Maybe.h, TimeStamp.h (// For TimeDuration), whatever bindings header file defines OwningUnrestrictedDoubleOrString, and whatever header file defines ComputedTimingFunction".

So this is still missing:

#include "mozilla/dom/UnionTypes.h" // For OwningUnrestrictedDoubleOrString
#include "mozilla/ComputedTimingFunction.h"

Also, I think we need Nullable too, and these should be in alphabetical order:

#include "mozilla/dom/Nullable.h"
#include "mozilla/dom/UnionTypes.h" // For OwningUnrestrictedDoubleOrString
#include "mozilla/ComputedTimingFunction.h"
#include "mozilla/Maybe.h"
#include "mozilla/TimeStamp.h" // for TimeDuration
Attachment #8727040 - Flags: review?(bbirtles) → review+
Comment on attachment 8727041 [details] [diff] [review]
1237173-P2-v2.patch

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

r=me with comments addressed.

::: dom/animation/TimingParams.h
@@ +41,5 @@
>      const dom::UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
>      const Nullable<dom::ElementOrCSSPseudoElement>& aTarget);
>  
>    // The unitialized state of mDuration represents "auto".
> +  // mDuration.isNothing() represents the "auto" value

We can drop the first line (The one that starts, "The uninitialized state...")

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +582,5 @@
>        continue;
>      }
>  
>      TimingParams timing;
> +    timing.mDuration = Some(StickyTimeDuration(animation.duration()));

Does it work if we just use timing.mDuration.emplace(animation.duration()); ?

There's a constructor for BaseTimeDuration[1] (the template-class that TimeDuration and StickyTimeDuration are specializations of) that is supposed to allow converting between StickyTimeDurations and TimeDurations so I *think* it should work.

[1] https://dxr.mozilla.org/mozilla-central/rev/46210f3ae07855edfe1383210d78433e38a6b564/mozglue/misc/TimeStamp.h#75

::: layout/style/nsAnimationManager.cpp
@@ +572,5 @@
>    {
>      TimingParams timing;
>  
> +    timing.mDuration = Some(StickyTimeDuration::FromMilliseconds(
> +                              aStyleAnimation.GetDuration()));

Likewise here, can we use emplace()?

Sorry for not pointing this out in the last review. I had forgotten about emplace() but Hiro reminded me about it. I think Some() is mostly used when you want to pass a Maybe<> to a function.

::: layout/style/nsTransitionManager.cpp
@@ +654,5 @@
>      reversePortion = valuePortion;
>    }
>  
>    TimingParams timing;
> +  timing.mDuration = Some(StickyTimeDuration::FromMilliseconds(duration));

Likewise here, we should use emplace() if we can.
Attachment #8727041 - Flags: review?(bbirtles) → review+
Comment on attachment 8727122 [details] [diff] [review]
1237173-P3-v3.patch

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

Looking good but I want to take another look with the following changes.

::: dom/animation/TimingParams.cpp
@@ +57,5 @@
>  template <class OptionsType>
>  static TimingParams
>  TimingParamsFromOptionsUnion(
>    const OptionsType& aOptions,
> +  const Nullable<dom::ElementOrCSSPseudoElement>& aTarget, ErrorResult& aRv)

Nit: 'ErrorResult& aRv' should be on a new line

@@ +92,5 @@
>  
>  /* static */ TimingParams
>  TimingParams::FromOptionsUnion(
>    const dom::UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
> +  const Nullable<dom::ElementOrCSSPseudoElement>& aTarget, ErrorResult& aRv)

Nit: ErrorResult& aRv should be on a new line

@@ +102,5 @@
> +/* static */ Maybe<StickyTimeDuration>
> +TimingParams::ParseDuration(DoubleOrString& aDuration, ErrorResult& aRv)
> +{
> +  if (aDuration.IsUnrestrictedDouble()) {
> +    if (aDuration.GetAsUnrestrictedDouble() >= 0)

if () needs a {

See [1], "Always brace controlled statements, even a single-line consequent of an if else else. This is redundant typically, but it avoids dangling else bugs, so it's safer at scale than fine-tuning."

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

@@ +106,5 @@
> +    if (aDuration.GetAsUnrestrictedDouble() >= 0)
> +      return Some(StickyTimeDuration::FromMilliseconds(
> +               aDuration.GetAsUnrestrictedDouble()));
> +  } else if (aDuration.GetAsString().EqualsLiteral("auto")) {
> +    return Maybe<StickyTimeDuration>();

I think we can make this a little more simple.

In C++, there is a special optimization called "return value optimization"[1] that means that, if you're returning a slightly complicated object, it can sometimes be faster if you return the same object from every code path since you can avoid one call to a copy-constructor. That's less important for classes that have move-constructors but it's still useful in this case, I think.

So, you can start the function off with the value you want to return from each code path:

  Maybe<StickyTimeDuration> result;

Since this initializes to Nothing(), which happens to be what you want to return in two of the code paths anyway, the rest of the function becomes much simpler. e.g.

  if (aDuration.IsUnrestrictedDouble()) {
    double durationInMs = aDuration.GetAsUnrestrictedDouble();
    if (durationInMs >= 0) {
      result.emplace(StickyTimeDuration::FromMilliseconds(durationInMs));
      return result;
    }
  } else if (aDuration.GetAsString().EqualsLiteral("auto")) {
    return result;
  }

  aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
  return result;

[1] https://en.wikipedia.org/wiki/Return_value_optimization

::: dom/animation/TimingParams.h
@@ +32,5 @@
>  {
>    TimingParams() = default;
>    TimingParams(const dom::AnimationEffectTimingProperties& aTimingProperties,
> +               const dom::Element* aTarget, ErrorResult& aRv);
> +  explicit TimingParams(double aDuration, ErrorResult& aRv);

Nit: You don't need explicit for a constructor that takes to arguments.

However, I've been thinking about this arrangement, and I feel it's a bit odd to have a constructor with an ErrorResult parameter. I think we should:

* Keep explicit TimingParams(double aDuration) and just make it assert that aDuration is >= 0.
* Make the constructor that takes a AnimationEffectTimingProperties object private, then add a FromTimingProperties static method that first calls ParseDuration then calls the private constructor. Then the private constructor can also assert that the duration is >= 0.

What do you think?

@@ +43,5 @@
> +    const Nullable<dom::ElementOrCSSPseudoElement>& aTarget, ErrorResult& aRv);
> +
> +  template <class DoubleOrString>
> +  static Maybe<StickyTimeDuration> ParseDuration(DoubleOrString& aDuration,
> +                                                 ErrorResult& aRv);

This probably needs a comment:

  // Range-checks and validates an UnrestrictedDoubleOrString or
  // OwningUnrestrictedDoubleOrString object and converts to a
  // StickyTimeDuration value or Nothing() if aDuration is "auto".
  // Caller must check aRv.Failed().

Also, declaring template methods in header files is kind of tricky because you can easily run into linker errors. I'm not sure why we're not hitting linker errors with this but it might just be that we're getting lucky due to the way the unified build works.

Typically, you have two options:

1) Put the implementation in the header file (simplest and most common).

2) Explicitly declare the specializations you will use (in this case UnrestrictedDoubleOrString and OwningUnrestrictedDoubleOrString).

Alternatively, you can actually declare two versions of the method and internally use a template function to share the implementation (that's what we do for FromOptionsUnion here).

For this, I'd probably suggest (1) is going to be easiest.

::: dom/webidl/AnimationEffectTiming.webidl
@@ +22,5 @@
>     //inherit attribute double                             iterationStart;
>     //Bug 1244640 - implement AnimationEffectTiming iterations
>     //inherit attribute unrestricted double                iterations;
> +  [SetterThrows]
> +  inherit attribute (unrestricted double or DOMString) duration;

Whenever you touch a webidl file, you need to get review from a DOM peer (e.g. smaug, bz or one of the people from here: [1])

So, after you've addressed my comments and updated the patch, please request additional review from a DOM peer.

[1] https://wiki.mozilla.org/Modules/Core#Document_Object_Model

::: testing/web-platform/tests/web-animations/animation-effect-timing/duration.html
@@ -15,1 @@
>  test(function(t) {

Nit: I think we should keep this blank line.

@@ +52,5 @@
> +  var div = createDiv(t);
> +  assert_throws({ name: 'TypeError' }, function() {
> +    div.animate({ opacity: [ 0, 1 ] }, -1);
> +  });
> +}, 'set negative duration in animate as simple attribute');

Nit: Here and below, replace 'as simple attribute' with 'using a duration parameter'

@@ +73,5 @@
> +  var div = createDiv(t);
> +  assert_throws({ name: 'TypeError' }, function() {
> +    div.animate({ opacity: [ 0, 1 ] }, { duration: -1 });
> +  });
> +}, 'set negative duration in animate as object');

Nit: Here and below replace 'as object' with 'using an options object'
Attachment #8727122 - Flags: review?(bbirtles)
(Assignee)

Comment 20

2 years ago
Created attachment 8727254 [details] [diff] [review]
Part1: Move TimingParam struct to a new file
(Assignee)

Updated

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

Comment 21

2 years ago
Created attachment 8727255 [details] [diff] [review]
Part2: Change type of duration to Maybe<StickyTimeDuration>
(Assignee)

Comment 22

2 years ago
Created attachment 8727256 [details] [diff] [review]
Part3: Throw TypeError if duration is NaN, negative value or not 'auto' string
Attachment #8727256 - Flags: review?(bbirtles)
(Assignee)

Updated

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

Updated

2 years ago
Attachment #8727122 - Attachment is obsolete: true
Comment on attachment 8727256 [details] [diff] [review]
Part3: Throw TypeError if duration is NaN, negative value or not 'auto' string

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

r=birtles with comments addressed

(Also, this needs review from a DOM peer.)

::: dom/animation/TimingParams.cpp
@@ +75,5 @@
> +    result.mIterationStart = timing.mIterationStart;
> +    result.mDirection = timing.mDirection;
> +    result.mFill = timing.mFill;
> +    result.mFunction = AnimationUtils::ParseEasing(
> +                         targetElement, timing.mEasing);

Nit: I'd probably just wrap this after =

    result.mFunction =
      AnimationUtils::ParseEasing(targetElement, timing.mEasing);

@@ +92,5 @@
>  
>  /* static */ TimingParams
>  TimingParams::FromOptionsUnion(
>    const dom::UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
> +  const Nullable<dom::ElementOrCSSPseudoElement>& aTarget, ErrorResult& aRv)

Nit: ErrorResult on a new line
Attachment #8727256 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 24

2 years ago
Created attachment 8727277 [details] [diff] [review]
Part3: Throw TypeError if duration is NaN, negative value or not 'auto' string
Attachment #8727277 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8727256 - Attachment is obsolete: true
(In reply to Daisuke Akatsuka from comment #24)
> Created attachment 8727277 [details] [diff] [review]
> Part3: Throw TypeError if duration is NaN, negative value or not 'auto'
> string

Olli: Just to clarify, this review request is for the WebIDL changes only.
(Assignee)

Comment 27

2 years ago
Created attachment 8728236 [details] [diff] [review]
Part3: Throw TypeError if duration is NaN, negative value or not 'auto' string

Fix bitrot.
Attachment #8728236 - Flags: review?(bugs)
(Assignee)

Updated

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

Comment 28

2 years ago
Created attachment 8728240 [details] [diff] [review]
Part2: Change type of duration to Maybe<StickyTimeDuration>

Fix bitrot.
(Assignee)

Updated

2 years ago
Attachment #8727255 - Attachment is obsolete: true
Comment on attachment 8728236 [details] [diff] [review]
Part3: Throw TypeError if duration is NaN, negative value or not 'auto' string

r+ for the .webidl
Attachment #8728236 - Flags: review?(bugs) → review+
I went to land this but it needed some non-trivial rebasing so I'm running it through try again before landing to at least make sure it builds cross-platform:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e52ab0692462
Status: NEW → ASSIGNED

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/75adfcb59f7b
https://hg.mozilla.org/mozilla-central/rev/8e41986d4724
https://hg.mozilla.org/mozilla-central/rev/006e29abfc8e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.