Closed Bug 1244586 Opened 8 years ago Closed 8 years ago

Implement KeyframeEffect constructor

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: motozawa, Assigned: ryo)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 11 obsolete files)

16.12 KB, patch
ryo
: review+
Details | Diff | Splinter Review
908 bytes, patch
ryo
: review+
Details | Diff | Splinter Review
5.56 KB, patch
ryo
: review+
Details | Diff | Splinter Review
      No description provided.
I added KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl and implemented it.
Attachment #8715106 - Flags: review?(bugs)
Attachment #8715106 - Flags: review?(bbirtles)
Depends on: 1226047
No longer need this entry when the constructor of KeyframeEffect was implemented.
Attachment #8715109 - Flags: review?(bbirtles)
I removed unnecessary spaces in dom/base/Element.cpp.
Attachment #8715131 - Flags: review?(bugs)
I removed unnecessary spaces in dom/base/Element.cpp.
Attachment #8715131 - Attachment is obsolete: true
Attachment #8715131 - Flags: review?(bugs)
Attachment #8715138 - Flags: review?(bugs)
Attachment #8715109 - Flags: review?(bbirtles) → review+
Comment on attachment 8715106 [details] [diff] [review]
Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl.

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

This is pretty close, but I'd like to have another look after removing the duplicated code from the Constructor methods.

::: dom/animation/KeyframeEffect.cpp
@@ +2073,5 @@
>      }
>    }
>    return false;
>  }
> +KeyframeEffect::KeyframeEffect(

Nit: There should be a blank line above this.

@@ +2078,5 @@
> +  nsIDocument* aDocument,
> +  Element* aTarget,
> +  nsCSSPseudoElements::Type aPseudoType,
> +  const TimingParams& aTiming)
> +  : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, aTiming)

This is hard to read. I think we can hang the parameters down the right and still fit within 80 characters. e.g.

  KeyframeEffect::KeyframeEffect(nsIDocument* aDocument,
                                 Element* aTarget,
                                 nsCSSPseudoElements::Type aPseudoType,
                                 const TimingParams& aTiming)
    : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, aTiming)

@@ +2091,5 @@
>                             JS::Handle<JSObject*> aGivenProto)
>  {
>    return KeyframeEffectBinding::Wrap(aCx, this, aGivenProto);
>  }
> +/* static */ already_AddRefed<KeyframeEffect>

Nit: Blank line above here too.

@@ +2119,5 @@
> +                       nsCSSPseudoElements::ePseudo_NotPseudoElement,
> +                       aTiming);
> +  effect->mProperties = Move(animationProperties);
> +  return effect.forget();
> +}

We should factor out a templated helper function to avoid duplicating code between KeyframeEffect::Constructor and KeyframeEffectReadOnly::Constructor. I think we can declare it as a private/protected function in KeyframeEffectReadOnly (since we need to access KeyframeEffectType::mProperties) and declare the specializations in the header--then we could keep the implementation in the .cpp file. (See [1] for some background to this.)

[1] https://isocpp.org/wiki/faq/templates#templates-defn-vs-decl

::: dom/webidl/KeyframeEffect.webidl
@@ +39,5 @@
>    // We use object instead of ComputedKeyframe so that we can put the
>    // property-value pairs on the object.
>    [Throws] sequence<object> getFrames();
>  };
>  // Bug 1211783 Implement KeyframeEffect constructor

Drop the comment here.
Attachment #8715106 - Flags: review?(bbirtles)
Added templated helper function for constructor of KeyframeEffect and KeyframeEffectReadOnly.

Removed an unnecessary comment.
Attachment #8715106 - Attachment is obsolete: true
Attachment #8715106 - Flags: review?(bugs)
Attachment #8715186 - Flags: review?(bugs)
Attachment #8715186 - Flags: review?(bbirtles)
Attachment #8715138 - Flags: review?(bugs) → review+
Comment on attachment 8715186 [details] [diff] [review]
Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v2

You should test also different values passed to 3rd param.
And would be great to test also passing odd values to 2nd param.
Something like 
new KeyframeEffect(target, { get left() { throw "hello"}}));


I think in that other bug about PseudoElements we use
(Element or CSSPseudoElement)? to pass an Animatable. But I guess you can't use that before the PseudoElement patch has landed.


r+ for the webidl, but create a followup bug or something to make sure we'll update the type of target argument to
include also PseudoElement.
Attachment #8715186 - Flags: review?(bugs) → review+
Comment on attachment 8715186 [details] [diff] [review]
Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v2

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

This is good, but just a few tweaks to the templates.

::: dom/animation/KeyframeEffect.cpp
@@ +2061,5 @@
>  
>    return false;
>  }
>  
> +KeyframeEffect::KeyframeEffect(nsIDocument* aDocument,

We should add a block comment here to make the break between KeyframeEffectReadOnly and KeyframeEffect implementation. e.g.

//---------------------------------------------------------------------
//
// KeyframeEffect
//
//---------------------------------------------------------------------

@@ +2070,5 @@
> +  {
> +    MOZ_ASSERT(aTarget, "null animation target is not yet supported");
> +
> +    mTiming = new AnimationEffectTiming(aTiming);
> +  }

Remove indent from function body here, e.g.

KeyframeEffect::KeyframeEffect(nsIDocument* aDocument,
                               Element* aTarget,
                               nsCSSPseudoElements::Type aPseudoType,
                               const TimingParams& aTiming)
  : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, aTiming)
{
  MOZ_ASSERT(aTarget, "null animation target is not yet supported");
  mTiming = new AnimationEffectTiming(aTiming);
}

@@ +2087,5 @@
> +    JS::Handle<JSObject*> aFrames,
> +    const TimingParams& aTiming,
> +    ErrorResult& aRv)
> +{
> +  return ConstructKeyframeEffect<KeyframeEffect>(aGlobal, aTarget, aFrames, aTiming, aRv);

Wrap to < 80 chars please.

::: dom/animation/KeyframeEffect.h
@@ +195,1 @@
>                ErrorResult& aRv);

We should just make this call ConstructKeyframeEffect directly:

    return ConstructKeyframeEffect<KeyframeEffectReadOnly>(
      aGlobal, aTarget, aFrames,
      TimingParams::FromOptionsUnion(aOptions, aTarget), aRv);

@@ +364,5 @@
> +  ConstructKeyframeEffect(const GlobalObject& aGlobal,
> +                          Element* aTarget,
> +                          JS::Handle<JSObject*> aFrames,
> +                          const TimingParams& aTiming,
> +                          ErrorResult& aRv)

We typically put the data members at the end of the class so we should move this to just after the ~KeyframeEffectReadOnly destructor declaration.

More importantly, I think we can put the implementation in the cpp file (which is what I was trying to suggest in comment 5).

e.g. declaration:

  template <typename KeyframeEffectType>
  static already_AddRefed<KeyframeEffectType>
  ConstructKeyframeEffect(const GlobalObject& aGlobal,
                          Element* aTarget,
                          JS::Handle<JSObject*> aFrames,
                          const TimingParams& aTiming,
                          ErrorResult& aRv);


Then, after the declaration of KeyframeEffectReadOnly you need to explicitly declare the specific instantiations to avoid linker errors,

  template
  already_AddRefed<KeyframeEffectReadOnly>
  KeyframeEffectReadOnly::ConstructKeyframeEffect<>(const GlobalObject& aGlobal,
                                                    Element* aTarget,
                                                    JS::Handle<JSObject*> aFrames,
                                                    const TimingParams& aTiming,
                                                    ErrorResult& aRv);

  class KeyframeEffect;

  template
  already_AddRefed<KeyframeEffect>
  KeyframeEffectReadOnly::ConstructKeyframeEffect<>(const GlobalObject& aGlobal,
                                                    Element* aTarget,
                                                    JS::Handle<JSObject*> aFrames,
                                                    const TimingParams& aTiming,
                                                    ErrorResult& aRv);


Then in the .cpp file (after the ~KeyframeEffectReadOnly definition):

  template <class KeyframeEffectType>
  /* static */ already_AddRefed<KeyframeEffectType>
  KeyframeEffectReadOnly::ConstructKeyframeEffect(const GlobalObject& aGlobal,
                                                  Element* aTarget,
                                                  JS::Handle<JSObject*> aFrames,
                                                  const TimingParams& aTiming,
                                                  ErrorResult& aRv)
  {
    if (!aTarget) {
      // We don't support null targets yet.
      aRv.Throw(NS_ERROR_DOM_ANIM_NO_TARGET_ERR);
      return nullptr;
    }

    InfallibleTArray<AnimationProperty> animationProperties;
    BuildAnimationPropertyList(aGlobal.Context(), aTarget, aFrames,
                               animationProperties, aRv);

    if (aRv.Failed()) {
      return nullptr;
    }

    RefPtr<KeyframeEffectType> effect =
      new KeyframeEffectType(aTarget->OwnerDoc(), aTarget,
                             nsCSSPseudoElements::ePseudo_NotPseudoElement,
                             aTiming);
    effect->mProperties = Move(animationProperties);
    return effect.forget();
  }

@@ +424,5 @@
> +              const UnrestrictedDoubleOrKeyframeEffectOptions& aOptions,
> +              ErrorResult& aRv)
> +  {
> +    return Constructor(aGlobal, aTarget, aFrames,
> +                       TimingParams::FromOptionsUnion(aOptions), aRv);

We should just call ConstructKeyframeEffect directly:

    return ConstructKeyframeEffect<KeyframeEffect>(
      aGlobal, aTarget, aFrames,
      TimingParams::FromOptionsUnion(aOptions, aTarget), aRv);

@@ +434,5 @@
> +  Constructor(const GlobalObject& aGlobal,
> +              Element* aTarget,
> +              JS::Handle<JSObject*> aFrames,
> +              const TimingParams& aTiming,
> +              ErrorResult& aRv);

We should just make this inline and call ConstructKeyframeEffect directly:

    return ConstructKeyframeEffect<KeyframeEffect>(aGlobal, aTarget, aFrames,
                                                   aTiming, aRv);

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
@@ +634,5 @@
> +                                  {left: ["10px", "20px"]});
> +
> +  assert_class_string(effect, "KeyframeEffect");
> +  assert_class_string(effect.timing, "AnimationEffectTiming");
> +})

We should add a test description (and semi-colon), e.g.

}, "KeyframeEffect constructor creates an AnimationEffectTiming timing object");
Attachment #8715186 - Flags: review?(bbirtles)
I added tests when values passed to KeyframeEffect constructor's 3rd param.
Added tests when odd values passed to KeyframeEffect constructor.
Put the implementation of ConstructKeyframeEffect in cpp file.
Attachment #8715186 - Attachment is obsolete: true
Attachment #8715700 - Flags: review?(bbirtles)
Attachment #8715700 - Attachment description: Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v3 → Part1 Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v3
Comment on attachment 8715109 [details] [diff] [review]
Part2 Remove entry in testing/web-platform/meta/web-animations/animatable/animate.html.ini.

># HG changeset patch
># User Ryo Motozawa <motozawa@mozilla-japan.org>
># Parent  b144b5af77a989ae942347224c382a6af3025eab
>Remove entry in testing/web-platform/meta/web-animations/animatable/animate.html.ini. r?birtles
>
>diff --git a/testing/web-platform/meta/web-animations/animatable/animate.html.ini b/testing/web-platform/meta/web-animations/animatable/animate.html.ini
>--- a/testing/web-platform/meta/web-animations/animatable/animate.html.ini
>+++ b/testing/web-platform/meta/web-animations/animatable/animate.html.ini
>@@ -1,10 +1,5 @@
> [animate.html]
>   type: testharness
>-  [Element.animate() creates an Animation object with a KeyframeEffect]
>-    expected: FAIL
>-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1211783
>-
>   [Element.animate() accepts a single-valued keyframe specification]
>     expected: FAIL
>     bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1216844
>-
Attachment #8715109 - Attachment description: Remove entry in testing/web-platform/meta/web-animations/animatable/animate.html.ini. → Part2 Remove entry in testing/web-platform/meta/web-animations/animatable/animate.html.ini.
Attachment #8715138 - Attachment description: Remove unnecessary spaces in dom/base/Element.cpp. v2 → Part3 Remove unnecessary spaces in dom/base/Element.cpp. v2
Comment on attachment 8715700 [details] [diff] [review]
Part1 Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v3

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

::: dom/animation/KeyframeEffect.cpp
@@ +600,5 @@
> +  if (!aTarget) {
> +    // We don't support null targets yet.
> +    aRv.Throw(NS_ERROR_DOM_ANIM_NO_TARGET_ERR);
> +    return nullptr;
> +  }

(You'll need to add the following lines here if bug 1239889 lands first:

+  if (!aTarget->GetCurrentDoc()) {
+    // Bug 1245748: We don't support targets that are not in a document yet.
+    aRv.Throw(NS_ERROR_DOM_ANIM_TARGET_NOT_IN_DOC_ERR);
+    return nullptr;
+  }

)

@@ +634,5 @@
> +KeyframeEffectReadOnly::ConstructKeyframeEffect<>(const GlobalObject& aGlobal,
> +                                                  Element* aTarget,
> +                                                  JS::Handle<JSObject*> aFrames,
> +                                                  const TimingParams& aTiming,
> +                                                  ErrorResult& aRv);

While the template definition can go in the .cpp file, the declaration that defines which specific instantiations we use should go in the header file (e.g. between the declaration of KeyframeEffectReadOnly and KeyframeEffect).

Specifically, these lines:

+template
+already_AddRefed<KeyframeEffectReadOnly>
+KeyframeEffectReadOnly::ConstructKeyframeEffect<>(const GlobalObject& aGlobal,
+                                                  Element* aTarget,
+                                                  JS::Handle<JSObject*> aFrames,
+                                                  const TimingParams& aTiming,
+                                                  ErrorResult& aRv);
+
+class KeyframeEffect;
+
+template
+already_AddRefed<KeyframeEffect>
+KeyframeEffectReadOnly::ConstructKeyframeEffect<>(const GlobalObject& aGlobal,
+                                                  Element* aTarget,
+                                                  JS::Handle<JSObject*> aFrames,
+                                                  const TimingParams& aTiming,
+                                                  ErrorResult& aRv);

@@ +1725,5 @@
>      ErrorResult& aRv)
>  {
> +  return ConstructKeyframeEffect<KeyframeEffectReadOnly>(
> +    aGlobal, aTarget, aFrames,
> +    TimingParams::FromOptionsUnion(aOptions), aRv);

We can just move this whole Constructor method to the header file now since it is so simple.

@@ +2110,5 @@
> +//---------------------------------------------------------------------
> +//
> +// KeyframeEffect
> +//
> +//---------------------------------------------------------------------

Nit: Add a blank line after this.

@@ +2118,5 @@
> +                               const TimingParams& aTiming)
> +  : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, aTiming)
> +{
> +  MOZ_ASSERT(aTarget, "null animation target is not yet supported");
> +  mTiming = new AnimationEffectTiming(aTiming);

So this means we'll end up creating a new AnimationEffectTimingReadOnly object on the heap (in the KeyframeEffectReadOnly constructor) only to delete it an re-create a new AnimationEffectTiming object on the heap. That seems unfortunate.

Instead, how about we add a protected constructor to KeyframeEffectReadOnly that takes an AnimationEffectTimingReadOnly* argument and then use delegated constructors[1] to call it from this constructor and the existing KeyframeEffectReadOnly constructor (passing 'new AnimationEffectTiming(...)' or 'new AnimationEffectTimingReadOnly(...)' as appropriate).

Delegated constructors a new feature in C++11 and are allowed in Mozilla code.[2]

[1] https://msdn.microsoft.com/en-au/library/dn387583.aspx?f=255&MSPPError=-2147217396#Anchor_2
[2] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

::: dom/animation/KeyframeEffect.h
@@ +417,5 @@
> +              const TimingParams& aTiming,
> +              ErrorResult& aRv)
> +  {
> +    return ConstructKeyframeEffect<KeyframeEffect>(aGlobal, aTarget, aFrames,
> +                                       aTiming, aRv);

Nit: We should line up aTiming under aGlobal here.

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
@@ +634,5 @@
> +                                  {left: ["10px", "20px"]});
> +
> +  assert_class_string(effect, "KeyframeEffect");
> +  assert_class_string(effect.timing, "AnimationEffectTiming");
> +}, "KeyframeEffect constructor creates an AnimationEffectTiming timing object when two parameters passed");

"KeyframeEffect constructor creates an AnimationEffectTiming timing object"

@@ +643,5 @@
> +                                  {offset: 0, left: "20px"});
> +
> +  assert_class_string(effect, "KeyframeEffect");
> +  assert_class_string(effect.timing, "AnimationEffectTiming");
> +}, "KeyframeEffect constructor creates an AnimationEffectTiming timing object when three parameters passed");

I'm not sure this test really makes sense. Why would the type of the effect or timing object differ based on the number of arguments?

Also, that third argument is incorrect. It's supposed to be a series of timing parameters, not a keyframe.

@@ +652,5 @@
> +
> +  assert_throws(test_error, function() {
> +    new KeyframeEffect(target, { get left() { throw test_error }})
> +  });
> +}, "KeyframeEffect constructor ");

"KeyframeEffect constructor propagates exceptions generated by accessing the options object"
Attachment #8715700 - Flags: review?(bbirtles)
I couldn't move instantiations from cpp file to header file because the compiler returned errors which says "explicit instantiation of undefined function template 'ConstructKeyframeEffect'".

I added a delegated constructor to KeyframeEffectReadOnly.
I fixed messages in a test.
Attachment #8715700 - Attachment is obsolete: true
Attachment #8716238 - Flags: review?(bbirtles)
(In reply to Ryo Motozawa [:ryo] from comment #12)
> Created attachment 8716238 [details] [diff] [review]
> Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v4
> 
> I couldn't move instantiations from cpp file to header file because the
> compiler returned errors which says "explicit instantiation of undefined
> function template 'ConstructKeyframeEffect'".

Oh, that's odd. It worked for me on Windows. Did you put it after the KeyframeEffectReadOnly class declaration?

I'll try to dig up my patches tomorrow and post them. I'm concerned that putting these instantiations in the .cpp file only works due to the unified build system and if we change the files in the unified build it will stop working.
 
> I added a delegated constructor to KeyframeEffectReadOnly.
> I fixed messages in a test.

Thanks.
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #13)
> (In reply to Ryo Motozawa [:ryo] from comment #12)
> > Created attachment 8716238 [details] [diff] [review]
> > Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v4
> > 
> > I couldn't move instantiations from cpp file to header file because the
> > compiler returned errors which says "explicit instantiation of undefined
> > function template 'ConstructKeyframeEffect'".
> 
> Oh, that's odd. It worked for me on Windows. Did you put it after the
> KeyframeEffectReadOnly class declaration?

I looked into it, and it turns out VC++ allows some things here it shouldn't. We could work around this but I think putting the explicit instantations in the .cpp file like you've done is better.
Comment on attachment 8716238 [details] [diff] [review]
Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v4

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

This looks great but I want to check if once more with the simplified constructor chain.

(You can see an example here: https://hg.mozilla.org/try/rev/247392566485)

::: dom/animation/KeyframeEffect.cpp
@@ +80,5 @@
> +{
> +  mTarget = aTarget;
> +  mPseudoType = aPseudoType;
> +  mInEffectOnLastAnimationTimingUpdate = false;
> +  MOZ_ASSERT(aTarget, "null animation target is not yet supported");

Rather than repeat this in each constructor, we should do it in the shared constructor (and use the member-initialization list).

Then this would become:

  KeyframeEffectReadOnly::KeyframeEffectReadOnly(
    nsIDocument* aDocument,
    Element* aTarget,
    nsCSSPseudoElements::Type aPseudoType,
    const TimingParams& aTiming)
    : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType,
                             new AnimationEffectTimingReadOnly(aTiming))
  {
  }

@@ +91,2 @@
>  {
> +  mTiming = aAnimEffectTiming;

We should move all the common initialization here and use the member-initialization list. e.g.

  KeyframeEffectReadOnly::KeyframeEffectReadOnly(
    nsIDocument* aDocument,
    Element* aTarget,
    nsCSSPseudoElements::Type aPseudoType,
    AnimationEffectTimingReadOnly* aTiming)
    : AnimationEffectReadOnly(aDocument)
    , mTarget(aTarget)
    , mTiming(*aTiming)
    , mPseudoType(aPseudoType)
    , mInEffectOnLastAnimationTimingUpdate(false)
  {
    MOZ_ASSERT(aTiming);
    MOZ_ASSERT(aTarget, "null animation target is not yet supported");
  }

@@ +607,5 @@
> +    // We don't support null targets yet.
> +    aRv.Throw(NS_ERROR_DOM_ANIM_NO_TARGET_ERR);
> +    return nullptr;
> +  }
> +

We'll need to merge in the changes from bug 1239889 here, i.e.

  if (!aTarget->GetCurrentDoc()) {
    // Bug 1245748: We don't support targets that are not in a document yet.
    aRv.Throw(NS_ERROR_DOM_ANIM_TARGET_NOT_IN_DOC_ERR);
    return nullptr;
  }

@@ +632,5 @@
> +                                                  JS::Handle<JSObject*> aFrames,
> +                                                  const TimingParams& aTiming,
> +                                                  ErrorResult& aRv);
> +
> +class KeyframeEffect;

We shouldn't need this line. (It's forward-declaring KeyframeEffect but by this point we should have already included KeyframeEffect.h which declares KeyframeEffect.)

@@ +2114,5 @@
> +{
> +  mTarget = aTarget;
> +  mPseudoType = aPseudoType;
> +  mInEffectOnLastAnimationTimingUpdate = false;
> +  MOZ_ASSERT(aTarget, "null animation target is not yet supported");

We should re-use the delegated constructor here:

  KeyframeEffect::KeyframeEffect(nsIDocument* aDocument,
                                Element* aTarget,
                                nsCSSPseudoElements::Type aPseudoType,
                                const TimingParams& aTiming)
    : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType,
                             new AnimationEffectTiming(aTiming))
  {
  }

::: dom/animation/KeyframeEffect.h
@@ +325,5 @@
>  
>    inline AnimationCollection* GetCollection() const;
>  
>  protected:
> +  KeyframeEffectReadOnly(nsIDocument* aDocument, AnimationEffectTimingReadOnly* aAnimEffectTiming);

This is > 80 chars long.

Also, we should do all the common initialization here so we should make this:

  KeyframeEffectReadOnly(nsIDocument* aDocument,
                         Element* aTarget,
                         nsCSSPseudoElements::Type aPseudoType,
                         AnimationEffectTimingReadOnly* aTiming);

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
@@ +642,5 @@
> +
> +  assert_throws(test_error, function() {
> +    new KeyframeEffect(target, { get left() { throw test_error }})
> +  });
> +}, "KeyframeEffect constructor propagates exceptions generated by accessing the options object");

Nit: We can wrap this line like so:

  }, "KeyframeEffect constructor propagates exceptions generated by accessing"
     + " the options object");
Attachment #8716238 - Flags: review?(bbirtles)
I can't apply changes because there are no macro definition 'NS_ERROR_DOM_ANIM_TARGET_NOT_IN_DOC_ERR'.
I was also wondering if we should change:

  KeyframeEffectReadOnly(nsIDocument* aDocument,
                         Element* aTarget,
                         nsCSSPseudoElements::Type aPseudoType,
                         AnimationEffectTimingReadOnly* aTiming);

to:

  KeyframeEffectReadOnly(nsIDocument* aDocument,
                         Element* aTarget,
                         nsCSSPseudoElements::Type aPseudoType,
                         OwningNonNull<AnimationEffectTimingReadOnly>& aTiming);

That's probably more robust. What do you think?
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #18)
> I was also wondering if we should change:
> 
>   KeyframeEffectReadOnly(nsIDocument* aDocument,
>                          Element* aTarget,
>                          nsCSSPseudoElements::Type aPseudoType,
>                          AnimationEffectTimingReadOnly* aTiming);
> 
> to:
> 
>   KeyframeEffectReadOnly(nsIDocument* aDocument,
>                          Element* aTarget,
>                          nsCSSPseudoElements::Type aPseudoType,
>                          OwningNonNull<AnimationEffectTimingReadOnly>&
> aTiming);
> 
> That's probably more robust. What do you think?

This declaration does not match the call such as 'KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, new AnimationEffectTimingReadOnly(aTiming))'.
How I call it correctly from delegating constructor?
(In reply to Ryo Motozawa [:ryo] from comment #19)
> (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #18)
> > I was also wondering if we should change:
> > 
> >   KeyframeEffectReadOnly(nsIDocument* aDocument,
> >                          Element* aTarget,
> >                          nsCSSPseudoElements::Type aPseudoType,
> >                          AnimationEffectTimingReadOnly* aTiming);
> > 
> > to:
> > 
> >   KeyframeEffectReadOnly(nsIDocument* aDocument,
> >                          Element* aTarget,
> >                          nsCSSPseudoElements::Type aPseudoType,
> >                          OwningNonNull<AnimationEffectTimingReadOnly>&
> > aTiming);
> > 
> > That's probably more robust. What do you think?
> 
> This declaration does not match the call such as
> 'KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, new
> AnimationEffectTimingReadOnly(aTiming))'.
> How I call it correctly from delegating constructor?

You'd have to do something like 'OwningNonNull(*(new AnimationEffectTimingReadOnly(aTiming)))' I think.
I fixed constructor definition and implementation.
Attachment #8716854 - Flags: review?(bbirtles)
Comment on attachment 8716854 [details] [diff] [review]
Part1 Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v5

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

Thank you! This looks good.

I was wondering if it might be neater to avoid passing an OwningNonNull. Please see my suggestion below. If it doesn't make sense then what you have here is ok but please discuss with Hiro since I will be away tomorrow (Tuesday).

Thanks again!

::: dom/animation/KeyframeEffect.cpp
@@ +76,5 @@
>    Element* aTarget,
>    nsCSSPseudoElements::Type aPseudoType,
>    const TimingParams& aTiming)
> +  : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType,
> +                           OwningNonNull<AnimationEffectTimingReadOnly>(*(new AnimationEffectTimingReadOnly(aTiming))))

It looks like OwningNonNull is quite difficult to use.

I'm away tomorrow, so can you ask Hiro to help with this? I think what you did in v4 of this patch is good but a little bit unsafe (since we pass around a pointer to a ref-counted object with a zero refcount so we will leak if we are not careful).

I wonder if we can:
* make the parameter to the delegated constructor an already_AddRefed parameter,
* pass RefPtr(new AnimationEffectTimingReadOnly(aTiming)).forget(),
* then just assign from aTiming to mTiming in the member initializer list.

Then if the already_AddRefed got destroyed without being assigned we would get a failed assertion so it would be safer.

Alternatively, we could just pass around a RefPtr reference and call forget() before assigning in the constructor.

@@ +84,5 @@
> +KeyframeEffectReadOnly::KeyframeEffectReadOnly(
> +  nsIDocument* aDocument,
> +  Element* aTarget,
> +  nsCSSPseudoElements::Type aPseudoType,
> +  OwningNonNull<AnimationEffectTimingReadOnly> aTiming)

i.e. I'm suggesting we could make this already_AddRefed<AnimationEffectTimingReadOnly>&& here.

@@ +89,3 @@
>    : AnimationEffectReadOnly(aDocument)
>    , mTarget(aTarget)
> +  , mTiming(*aTiming)

And this would just become mTiming(aTiming).
Attachment #8716854 - Flags: review?(bbirtles) → review+
I modified KeyframeEffect constructor.
Attachment #8716854 - Attachment is obsolete: true
Attachment #8717307 - Flags: review+
Attachment #8716238 - Attachment is obsolete: true
Fixed constructor of KeyframeEffectReadOnly and KeyframeEffect because previous patch will conflict with Bug 1174575 patch.
Attachment #8717307 - Attachment is obsolete: true
Attachment #8717368 - Flags: review?(bugs)
Comment on attachment 8717368 [details] [diff] [review]
Part1 Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v7

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

::: dom/animation/KeyframeEffect.cpp
@@ +84,5 @@
> +KeyframeEffectReadOnly::KeyframeEffectReadOnly(
> +  nsIDocument* aDocument,
> +  Element* aTarget,
> +  nsCSSPseudoElements::Type aPseudoType,
> +  already_AddRefed<AnimationEffectTimingReadOnly>&& aTiming)

Just a note:
If we use OwningNonNull<>&& here, we can pass aTiming to mTiming without Move().
Comment on attachment 8717368 [details] [diff] [review]
Part1 Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v7


>+KeyframeEffectReadOnly::KeyframeEffectReadOnly(
>+  nsIDocument* aDocument,
>+  Element* aTarget,
>+  nsCSSPseudoElements::Type aPseudoType,
>+  already_AddRefed<AnimationEffectTimingReadOnly>&& aTiming)
So it is not clear to me what this 
already_AddRefed<AnimationEffectTimingReadOnly>&& buys us here.
If the argument isn't assigned to some RefPtr variable, we leak anyhow, whether or not
we're dealing with already_AddRefed<AnimationEffectTimingReadOnly>&& or AnimationEffectTimingReadOnly*
And if you do that, the code calling this constructor becomes easier to read.
So, I'd prefer AnimationEffectTimingReadOnly* here-



>+template <class KeyframeEffectType>
>+/* static */ already_AddRefed<KeyframeEffectType>
>+KeyframeEffectReadOnly::ConstructKeyframeEffect(const GlobalObject& aGlobal,
>+                                                const Nullable<ElementOrCSSPseudoElement>& aTarget,
>+                                                JS::Handle<JSObject*> aFrames,
>+                                                const TimingParams& aTiming,
>+                                                ErrorResult& aRv)
Why does this need to be templated?
We don't really seem to use KeyframeEffectType
...


>+  RefPtr<KeyframeEffect> effect =
>+    new KeyframeEffect(targetElement->OwnerDoc(), targetElement,
>+                               pseudoType, aTiming);
>+  effect->mProperties = Move(animationProperties);
>+  return effect.forget();
... the method always creates a KeyframeEffect, even if KeyframeEffectType was KeyframeEffectReadOnly.
I assume the method is supposed to create different kinds of objects.
Or am I missing something here?


Do we not have a test that a constructor in JS actually creates the right type of object?


>+  // More generalized version for Animatable.animate.
>+  // Not exposed to content.
>+  static already_AddRefed<KeyframeEffect>
>+  inline Constructor(const GlobalObject& aGlobal,
>+              const Nullable<ElementOrCSSPseudoElement>& aTarget,
>+              JS::Handle<JSObject*> aFrames,
>+              const TimingParams& aTiming,
>+              ErrorResult& aRv)
You could align params here and elsewhere
Attachment #8717368 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (high review load) from comment #26)
> Comment on attachment 8717368 [details] [diff] [review]
> Part1 Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v7
> 
> 
> >+KeyframeEffectReadOnly::KeyframeEffectReadOnly(
> >+  nsIDocument* aDocument,
> >+  Element* aTarget,
> >+  nsCSSPseudoElements::Type aPseudoType,
> >+  already_AddRefed<AnimationEffectTimingReadOnly>&& aTiming)
> So it is not clear to me what this 
> already_AddRefed<AnimationEffectTimingReadOnly>&& buys us here.
> If the argument isn't assigned to some RefPtr variable, we leak anyhow,
> whether or not
> we're dealing with already_AddRefed<AnimationEffectTimingReadOnly>&& or
> AnimationEffectTimingReadOnly*
> And if you do that, the code calling this constructor becomes easier to read.
> So, I'd prefer AnimationEffectTimingReadOnly* here-

Yeah, good call. I got worried because when I pushed this to try I saw leaks (that turned out to be unrelated) and I was looking for some way to make sure we could make this more robust.

Motozawa-san, sorry for the extra work I caused you here.

> >+template <class KeyframeEffectType>
> >+/* static */ already_AddRefed<KeyframeEffectType>
> >+KeyframeEffectReadOnly::ConstructKeyframeEffect(const GlobalObject& aGlobal,
> >+                                                const Nullable<ElementOrCSSPseudoElement>& aTarget,
> >+                                                JS::Handle<JSObject*> aFrames,
> >+                                                const TimingParams& aTiming,
> >+                                                ErrorResult& aRv)
> Why does this need to be templated?
> We don't really seem to use KeyframeEffectType

We should be returning different types: either KeyframeEffect or KeyframeEffectReadOnly.

> >+  RefPtr<KeyframeEffect> effect =
> >+    new KeyframeEffect(targetElement->OwnerDoc(), targetElement,
> >+                               pseudoType, aTiming);
> >+  effect->mProperties = Move(animationProperties);
> >+  return effect.forget();
> ... the method always creates a KeyframeEffect, even if KeyframeEffectType
> was KeyframeEffectReadOnly.
> I assume the method is supposed to create different kinds of objects.
> Or am I missing something here?

Yeah, this seems to be a bug introduced between v6 and v7 of the patch.

> Do we not have a test that a constructor in JS actually creates the right
> type of object?

I guess not. This patch adds a test for the specific case of KeyframeEffect, but we should at least add one for KeyframeEffectReadOnly. Better still, we should add an idlharness.js test and pass the result of each constructor to it. I believe that would cover it.
(In reply to Ryo Motozawa [:ryo] from comment #24)
> Created attachment 8717368 [details] [diff] [review]
> Part1 Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v7
> 
> Fixed constructor of KeyframeEffectReadOnly and KeyframeEffect because
> previous patch will conflict with Bug 1174575 patch.

Thank you for rebasing! I guess that was a lot of work. I was hoping we could land this before bug 1174575, but my review feedback probably held this up.
I fixed mistakes in template function.
Fixed constructor to use AnimationEffectTimingReadOnly*.
Attachment #8717368 - Attachment is obsolete: true
Attachment #8717737 - Flags: review?(bugs)
Comment on attachment 8717737 [details] [diff] [review]
Part1 Add KeyframeEffect constructor in dom/webidl/KeyframeEffect.webidl. v8




>+  if (!targetElement->GetCurrentDoc()) {
You want GetComposedDoc() here. Though, this code just moved so, feel free to not fix in this bug.

>+  RefPtr<KeyframeEffectType> effect =
>+    new KeyframeEffectType(targetElement->OwnerDoc(), targetElement,
>+                               pseudoType, aTiming);
align the last two params

>+// Explicit instantiations
oh, this is to avoid some linker issue. Please add a comment about that.
Attachment #8717737 - Flags: review?(bugs) → review+
Modified KeyframeEffect.cpp according to comment #30.
Attachment #8717737 - Attachment is obsolete: true
Attachment #8718675 - Flags: review+
I fixed commit message.
Attachment #8718690 - Flags: review+
Attachment #8715109 - Attachment is obsolete: true
Attachment #8715138 - Attachment is obsolete: true
Hello sheriffs,
Could you please land patches in bug 1211783, bug 1226047 and this in a push at once?
Please be careful these dependencies.
Thank you,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=692bd370ea37
Keywords: checkin-needed
Assignee: nobody → motoryo1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: