Closed Bug 1174575 Opened 9 years ago Closed 9 years ago

Implement a (CSS)PseudoElement interface

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox41 --- affected
firefox47 --- fixed

People

(Reporter: birtles, Assigned: boris)

References

Details

Attachments

(6 files, 27 obsolete files)

Part 1: Define CSSPseudoElement interface. (v6, carry birtles' and smaug's r+)
8.88 KB, patch
boris
: review+
Details | Diff | Splinter Review
Part 3: Implement KeyframeEffectReadOnly::GetTarget(). (v6, carry birtles' r+)
9.96 KB, patch
boris
: review+
Details | Diff | Splinter Review
Part 2: Replace Element in KeyframeEffectReadOnly WebIDL. (v7, carry birtles' and smaug's r+)
15.54 KB, patch
boris
: review+
Details | Diff | Splinter Review
Part 4: Support CSSPseudoElement for TimingParams. (v3, carry birtles' r+)
2.76 KB, patch
boris
: review+
Details | Diff | Splinter Review
Part 5: Support pseudo-element type in StyleAnimation. (v3, carry birtles' r+)
17.07 KB, patch
boris
: review+
Details | Diff | Splinter Review
Part 6: Implement KeyframeEffectReadOnly Constructor for CSSPseudoElement. (v7, carry birtles' r+)
14.88 KB, patch
boris
: review+
Details | Diff | Splinter Review 
Bug 1150810 provides a means for getting (more-or-less) all the animations in a document including animations that target pseudo-elements.

For those animations that target pseudo-elements we need something to return as the animation's target.

e.g. If we have:

  div::before {
    content: "Note ";
    animation: anim 10s;
  }
  @keyframes anim {
    to { margin-left: 20px; }
  }

And we do:

  var anim = document.timeline.getAnimations()[0];
  console.log(anim.effect.target);

What should we return?

The spec says we should return a PseudoElement object but there are two competing definitions of that interface.

A) http://dev.w3.org/csswg/css-pseudo/#CSSPseudoElement-interface:

    interface CSSPseudoElement
    {
      readonly attribute DOMString type;
      readonly attribute CSSStyleDeclaration style;
    };

    CSSPseudoElement implements EventTarget;
    
    interface CSSPseudoElementList
    {
      readonly attribute unsigned long length;
      CSSPseudoElement item(unsigned long index);
      CSSPseudoElement getByType(DOMString type);
                       // replies null if no pseudo-element exists for
                       //     the requested type
    };
    
    partial interface Window {
      CSSPseudoElementList getPseudoElements(Element elt, DOMString type);
    };
    
B) http://dev.w3.org/csswg/cssom/#the-pseudoelement-interface:
    
    interface PseudoElement
    {
    };

    PseudoElement implements GetStyleUtils;
    
    partial interface Element {
      PseudoElement? pseudo(DOMString pseudoElt);
    };

Of the two, B is more recently updated.

I asked which one to implement and was told "neither".[1]

I suggest we could do a minimum implementation based on the following:
    
    interface CSSPseudoElement
    {
      readonly attribute DOMString type;
    };
    
That should be enough to test with. If neither A nor B progresses by the time we come to ship we could push for a resolution on at least the interface name and just ship empty objects (i.e. hide the type attribute) for the time being.

The alternative is to change Web Animations to have both 'target' and 'targetPseudo' attributes but that seems really backwards and we'd have to change a lot of the rest of the API (e.g. all methods that take a target parameter would need to be adjusted).

[1] https://lists.w3.org/Archives/Public/www-style/2014Nov/0043.html
After finishing bugs about computing timing dictionary (ex. Bug 1108055), I will check this bug.
Assignee: nobody → boris.chiou
Blocks: 1206420
I think we probably want to have the parent element too, e.g.

    interface CSSPseudoElement
    {
      readonly attribute DOMString type;
      readonly attribute Element parentElement;
    };

Where 'type' is either '::before' or '::after'.

In terms of implementation, I was thinking we could store a weak reference as a property on the element. The reason is that once all references from script are dropped, we don't need to keep the object alive.

I suppose the CSSPseudoElement needs to keep an owning reference back to the parent element and needs to interface with the cycle collector.
Blocks: 1234403
No longer blocks: 1206420
Status: NEW → ASSIGNED
Hi Brian,

The things I want to do in this bug:
1. Define and implement (CSS)PseduoElement interface
  * Maybe we can use the definition from Comment 2.
  * For example: If we define a CSS pseudo element, 'p::after {...}', the parentElement is 'p' and the type is '::after'.
2. Replace Element with Animatable in KeyframeEffectReadOnly
  * Now, only KeyframeEffectReadOnly uses this new interface. (i.e. (CSS)PseudoElement)
  * Store Animatable, instead of Element, in KeyframeEffectReadOnly. ex. nsCOMPtr<Animatable> mTarget;

PS. We have a follow-up bug to support pseudo elements for document.getAnimations() already. (Bug 1234403)

However, I have some questions about Animatable.

The spec says:
1. Element implements Animatable [1]
2. (CSS)PseudoElement implements Animatable [2]
(Assuming that we use the definition of (CSS)PseudoElement in Comment 2.)
We use "implements", instead of inheritance, to define the relationships of them.

The first argument of the KeyframeEffectReadOnly constuctor is "Animatable" which is [NoInterfaceObject] [3], and I think Animatable, Element, (CSS)PseudoElement are definitely different interface/classes, right? It looks like Animatable object is neither a real DOM element nor a PseudoElement in accordance with the spec. Therefore, I wonder how does KeyframeEffectReadOnly constructor know whether this Animatable object is an Element or a PseudoEmenent? Or do I have to implement a special base class for both Element and PseudoElement for this case?
Thanks.

[1] https://w3c.github.io/web-animations/#extensions-to-the-element-interface
[2] https://w3c.github.io/web-animations/#extensions-to-the-pseudoelement-interface
[3] https://w3c.github.io/web-animations/#animatable
> and I think Animatable, Element, (CSS)PseudoElement are definitely different interface/classes, right?

Animatable is not a class at all, and there are no Animatable objects.  It's an interface in IDL, but that's basically an IDL bug; it's really a mixin not an interface.  There are objects that _are_ Animatable, but they're all either Element or CSSPseudoElement.

Anyway, your real question is about what the bindings generator will do with this setup.  What it will do is first complain that you need a descriptor for Animatable, and then when you add one it will basically take the nativeType you define in that descriptor (defaulting to mozilla::dom::Animatable) and try to use QueryInterface on the thing stored in the JS object to get that type out.  Which means you would have to have Element and CSSPseudoElement both QI to this new thing.

So in practice you would need a special base class for both Element and PseudoElement, which is a huge pain, actually, especially if you want to not add a word of vtable pointer to Element.

And then your constructor would have to use QI or some other means of interrogating an Animatable* to tell which one it is.

So what I propose instead is that you make the KeyframeEffectReadOnly constructor take "(Element or CSSPseudoElement)" and avoid all those complications.  And file a followup bug (cc me) on making the binding generator automatically convert this sort of IDL, where a mixin is being used as an arg type, into a union type.
(In reply to Boris Chiou [:boris] from comment #3)
> Hi Brian,
> 
> The things I want to do in this bug:
> 1. Define and implement (CSS)PseduoElement interface
>   * Maybe we can use the definition from Comment 2.
>   * For example: If we define a CSS pseudo element, 'p::after {...}', the
> parentElement is 'p' and the type is '::after'.
> 2. Replace Element with Animatable in KeyframeEffectReadOnly
>   * Now, only KeyframeEffectReadOnly uses this new interface. (i.e.
> (CSS)PseudoElement)
>   * Store Animatable, instead of Element, in KeyframeEffectReadOnly. ex.
> nsCOMPtr<Animatable> mTarget;

I'm not sure if that works--or at least it's not enough. You need to be able to be able to ensure that when we have two animations on the same pseudo-element, that they end up returning the same CSSPseudoElement object. Basically, we need a way of going from an Element & pseudo-type pair, to a single CSSPseudoElement object.

What I had in mind is to use a property on the parent element to store this. You can look at, for example, EffectSet, to see how to store properties on elements.

Internally, I don't know if we need to use CSSPseudoElement. We might want to do that in the future, but it's a big job and I think it might be simpler to just continue storing an Element:PseudoType pair initially. We might want to a wrapper to represent this pair at some point, however. So I was thinking CSSPseudoElement would be purely for external use.

Suppose we define CSSPseudoElement as something like:

  // namespace mozilla::dom
  class CSSPseudoElement : public nsWrapperCache {
    RefPtr<Element> mParentElement;
    nsCSSPseudoElements::Type mPseudoType
  };

mParentElement needs to be an owning reference since it script is holding on to the pseudo-element, it needs to continue to be able to refer to the parent element.

Going the other direction, from element to pseudo-element, I think we want a weak reference. If this is a purely external interface created on-demand, then when all references from script to the pseudo are dropped, we can drop the CSSPseudoElement object.

You'll probably want a DOM peer to advise on this but I imagine you could add a helper function (perhaps as a static method on CSSPseudoElement) to get the CSSPseudoElement object given an Element:PseudoType pair. It would call GetProperty, do the necessary casting, and return an already_AddRefed<> pointer. If the property doesn't exist, it would create it at the same time.

In terms of clearing up the property, I guess we could make the dtor for CSSPseudoElement call DeleteProperty on its mParentElement.
(In reply to Boris Zbarsky [:bz] from comment #4) 
> So what I propose instead is that you make the KeyframeEffectReadOnly
> constructor take "(Element or CSSPseudoElement)" and avoid all those
> complications.  And file a followup bug (cc me) on making the binding
> generator automatically convert this sort of IDL, where a mixin is being
> used as an arg type, into a union type.

Thanks for explaining my question, bz. I believe this idea is much easier to implement.
I'd like to make this argument as (Element or CSSPseudoElement) in this bug
and then file a followup bug to automatically convert this sort of IDL.
(In reply to Brian Birtles (:birtles) from comment #5)
> I'm not sure if that works--or at least it's not enough. You need to be able
> to be able to ensure that when we have two animations on the same
> pseudo-element, that they end up returning the same CSSPseudoElement object.
> Basically, we need a way of going from an Element & pseudo-type pair, to a
> single CSSPseudoElement object.
> 
> What I had in mind is to use a property on the parent element to store this.
> You can look at, for example, EffectSet, to see how to store properties on
> elements.
> 
> Internally, I don't know if we need to use CSSPseudoElement. We might want
> to do that in the future, but it's a big job and I think it might be simpler
> to just continue storing an Element:PseudoType pair initially. We might want
> to a wrapper to represent this pair at some point, however. So I was
> thinking CSSPseudoElement would be purely for external use.

OK. I agree. Keep using Element & pseudo-type pair in KeyframeEffecrReadOnly and
CSSPseudoElement for external usage is a good idea to simplify this bug.

> 
> Suppose we define CSSPseudoElement as something like:
> 
>   // namespace mozilla::dom
>   class CSSPseudoElement : public nsWrapperCache {
>     RefPtr<Element> mParentElement;
>     nsCSSPseudoElements::Type mPseudoType
>   };
> 
> mParentElement needs to be an owning reference since it script is holding on
> to the pseudo-element, it needs to continue to be able to refer to the
> parent element.
> 
> Going the other direction, from element to pseudo-element, I think we want a
> weak reference. If this is a purely external interface created on-demand,
> then when all references from script to the pseudo are dropped, we can drop
> the CSSPseudoElement object.
> 
> You'll probably want a DOM peer to advise on this but I imagine you could
> add a helper function (perhaps as a static method on CSSPseudoElement) to
> get the CSSPseudoElement object given an Element:PseudoType pair. It would
> call GetProperty, do the necessary casting, and return an already_AddRefed<>
> pointer. If the property doesn't exist, it would create it at the same time.
> 
> In terms of clearing up the property, I guess we could make the dtor for
> CSSPseudoElement call DeleteProperty on its mParentElement.

For example:
According to your idea, this graph comes into my mind:

 (Pseudo)            (parent)          (pseudo)
|---------|  strong  |-----|  strong  |--------|
|p::before| -------> |  p  | <------- |p::after|
|         | <------- |     | -------> |        |
|---------|   weak   |-----|   weak   |--------|

When we create a new pseudo element (p::before/p::after), we could set a property
whose value is a weak pointer to p::before/p::after in the parent element, p.
I should write a version for this and then ask a DOM peer for some advice.

Thanks for your advice. :)
See Also: → 1241783
Blocks: 1241784
Attachment #8710339 - Attachment is obsolete: true
Attachment #8710340 - Attachment is obsolete: true
Attachment #8710914 - Attachment is obsolete: true
Attachment #8711533 - Attachment is obsolete: true
Attachment #8711575 - Attachment is obsolete: true
Comment on attachment 8711596 [details] [diff] [review]
Part 3: Implement KeyframeEffectReadOnly::GetTarget() (v3)

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

Hi Olli,

I think you might want to check the implementation of CSSPseudoElement.
In Part 1, I define the interface of CSSPseudoElement (which is a purely external interface now), and according to Brian's suggestion in comment #5, I use a strong reference from a CSSPseudoElement to an Element because the script is holding on to the pseudo-element and it needs to continue to be able to refer to its parent element. In the other direction, I use a raw pointer from an Element to a CSSPseudoElement because when all references from script to the pseudo are dropped, we can drop the CSSPseudoElement object. I'd like to know is there any problem I didn't think of if using SetProperty()/GetProperty().

Script --(strong)--> CSSPseudoElement  --(strong)--> Element
                                       <--(weak)----

I use a static function in CSSPseudoElement to get from Element:PseudoType pair or to create a CSSPseuedoElement object on-demand. Do you have any concern or advice for this?

BTW, we don't have an official spec for CSSPseudoElement know, as Brian mentioned in Description, so could we push this equivalent definition to our code base?
Thank you.
Attachment #8711596 - Flags: feedback?(bugs)
Comment on attachment 8711596 [details] [diff] [review]
Part 3: Implement KeyframeEffectReadOnly::GetTarget() (v3)


>-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CSSPseudoElement)
>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(CSSPseudoElement, mParentElement)
> 
> NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(CSSPseudoElement, AddRef)
> NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(CSSPseudoElement, Release)
> 
>+CSSPseudoElement::CSSPseudoElement(Element* aElement,
>+                                   nsCSSPseudoElements::Type aType)
>+  : mParentElement(aElement)
>+  , mPseudoType(aType)
>+{
>+}
>+
>+CSSPseudoElement::~CSSPseudoElement()
>+{
>+  mParentElement->DeleteProperty(GetCSSPseudoElementPropertyAtom(mPseudoType));
So unlink may have run before this and then mParentElement points to null.
You need to remove the property _also_ during unlink before mParentElement.
And before accessing mParentElement in dtor, you need to null check it.

I think with that, this should be ok.
Attachment #8711596 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #18)
> >+CSSPseudoElement::~CSSPseudoElement()
> >+{
> >+  mParentElement->DeleteProperty(GetCSSPseudoElementPropertyAtom(mPseudoType));
> So unlink may have run before this and then mParentElement points to null.
> You need to remove the property _also_ during unlink before mParentElement.
> And before accessing mParentElement in dtor, you need to null check it.
> 
> I think with that, this should be ok.

Thanks, Olli. I should also delete this property in UnbindFromTree and add a null check here.
That I don't know. Depends on whether PseudoElement should "work" even after the relevant element has been unbound. It probably should, in which case doing anything in UnbindFromTree isn't needed.
Also, from performance point of view would be nice if nothing was added to UnbindFromTree.
(In reply to Olli Pettay [:smaug] from comment #20)
> That I don't know. Depends on whether PseudoElement should "work" even after
> the relevant element has been unbound. It probably should, in which case
> doing anything in UnbindFromTree isn't needed.
> Also, from performance point of view would be nice if nothing was added to
> UnbindFromTree.

Got it. Looks like only need to remove the property also during unlink before mParentElement (i.e. in NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(FragmentOrElement) block), as you mentioned.
Thank you for explanation.
Or could we rely on nsNodeUtils::LastRelease removing whatever properties there still are?
The property here is after all something which doesn't affect to cycle collection.
(In reply to Olli Pettay [:smaug] from comment #23)
> Or could we rely on nsNodeUtils::LastRelease removing whatever properties
> there still are?
> The property here is after all something which doesn't affect to cycle
> collection.

Looks like that nsNodeUtils::LastRelease checks if there is any property and then deletes them [1], and we use nsNodeUtils::LastRelease(this) while releasing FragmentOrElement object [2]. Therefore, I think you are right. We could rely on it to remove all the properties and I don't have to do anything for this. ONLY need to add a null check in part 3. Thank you!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsNodeUtils.cpp#321
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/FragmentOrElement.cpp#1985
Attachment #8710912 - Attachment is obsolete: true
Attachment #8710913 - Attachment is obsolete: true
Attachment #8711596 - Attachment is obsolete: true
Attachment #8711597 - Attachment is obsolete: true
Hi, Brian,

I think these patches are ready to review, but I'd like to move the test cases into bug 1234403 (Implement getAnimations for pseudo) because I have not idea to get an effect that targets a pseudo element before finishing doucument.getAnimations() for pseudo elements.

BTW, is it possible to push the equivalent interface of CSSPseudoElement into the web-animation spec because there is no reference to this interface for developers right now?

Thank you.
(In reply to Boris Chiou [:boris] from comment #29)
> I think these patches are ready to review, but I'd like to move the test
> cases into bug 1234403 (Implement getAnimations for pseudo) because I have
> not idea to get an effect that targets a pseudo element before finishing
> doucument.getAnimations() for pseudo elements.

Yeah, good point. I think you're right. Let's leave the tests until then.

> BTW, is it possible to push the equivalent interface of CSSPseudoElement
> into the web-animation spec because there is no reference to this interface
> for developers right now?

I don't think we should put that interface in web animations. It's already defined in two specs so we shouldn't add a third. Since we're not planning on shipping this interface to regular Web content yet (the urgent need here is DevTools) I think it's ok if the specs are still in flux. If we get to the point where we're ready to ship, we can push the CSSWG to agree on a minimal interface for this. For now we should just point to the two different specs with a comment explaining the situation.
Hi David,

Do you have any concern if we change the type of "enum nsCSSPseudoElement::Type"[1] to "scoped enum"? Many functions use this type as an argument and we can forward declaration it easily if we use scoped enum. I can file a bug to handle this. Thanks.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPseudoElements.h#56
Flags: needinfo?(dbaron)
No concerns.  That sounds great.  Please remove "ePseudo_" from all the names since they'll now be qualified with the enum name (which might need to be renamed and/or moved out of the nsCSSPseudoElements class to make things make sense.
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] ⌚️UTC+13 (busy, returning 8 February) from comment #32)
> No concerns.  That sounds great.  Please remove "ePseudo_" from all the
> names since they'll now be qualified with the enum name (which might need to
> be renamed and/or moved out of the nsCSSPseudoElements class to make things
> make sense.

Thanks. Filed Bug 1244049.
Attachment #8713436 - Flags: review?(bbirtles)
Attachment #8713438 - Flags: review?(bbirtles)
Attachment #8713440 - Flags: review?(bbirtles)
Attachment #8713441 - Flags: review?(bbirtles)
Comment on attachment 8713436 [details] [diff] [review]
Part 1: Define CSSPseudoElement interface (v3)

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

::: dom/animation/CSSPseudoElement.cpp
@@ +24,5 @@
> +void
> +CSSPseudoElement::GetAnimations(nsTArray<RefPtr<Animation>>& aRetVal)
> +{
> +  // Bug 1234403: Implement this API.
> +  MOZ_ASSERT(true);

What is this? Do you mean MOZ_ASSERT(false) maybe? Or perhaps NS_NOTREACHED?

@@ +35,5 @@
> +    const UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
> +    ErrorResult& aError)
> +{
> +  // Bug 1241784: Implement this API.
> +  MOZ_ASSERT(true);

Likewise here.

Also, the signature of this should match Element::Animate (where aFrames is not const since it is passed by value).

::: dom/animation/CSSPseudoElement.h
@@ +29,5 @@
> +protected:
> +  virtual ~CSSPseudoElement() = default;
> +
> +public:
> +  CSSPseudoElement* GetParentObject() const { return nullptr; }

This doesn't seem right. I think you need to return the associated global here.

::: dom/webidl/CSSPseudoElement.webidl
@@ +5,5 @@
> + *
> + * The origin of this IDL file is
> + * https://drafts.csswg.org/css-pseudo/#CSSPseudoElement-interface
> + * https://drafts.csswg.org/cssom/#pseudoelement
> + * https://w3c.github.io/web-animations/#extensions-to-the-pseudoelement-interface

Move this last line to above the "implements Animatable" line.

@@ +7,5 @@
> + * https://drafts.csswg.org/css-pseudo/#CSSPseudoElement-interface
> + * https://drafts.csswg.org/cssom/#pseudoelement
> + * https://w3c.github.io/web-animations/#extensions-to-the-pseudoelement-interface
> + *
> + * Copyright © 2015 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W

Is this missing a "3C" at the end?

@@ +13,5 @@
> + */
> +
> +// We don't have an official spec for CSSPseudoElement.
> +// Both draft specs (in cssom and css-pseudo) are not updated for a long time,
> +// so we need to provide this equivalent definition.

// Both CSSOM and CSS Pseudo-Elements 4 provide contradictory definitions for
// this interface.
// What we implement here is a minimal subset of the two definitions which we
// ship behind a pref until the specification issues have been resolved.
Attachment #8713436 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #34)
> > +  CSSPseudoElement* GetParentObject() const { return nullptr; }
> 
> This doesn't seem right. I think you need to return the associated global
> here.

Do you mean the nsIGlobalObject from mParentElement?
e.g. mParentElement->GetOwnerGlobal()
(In reply to Boris Chiou [:boris] from comment #35)
> (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #34)
> > > +  CSSPseudoElement* GetParentObject() const { return nullptr; }
> > 
> > This doesn't seem right. I think you need to return the associated global
> > here.
> 
> Do you mean the nsIGlobalObject from mParentElement?
> e.g. mParentElement->GetOwnerGlobal()

See point 2 from https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class

If you're going to add this in a later patch in the same series, please add a comment to that effect.
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #36)
> (In reply to Boris Chiou [:boris] from comment #35)
> > (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #34)
> > > > +  CSSPseudoElement* GetParentObject() const { return nullptr; }
> > > 
> > > This doesn't seem right. I think you need to return the associated global
> > > here.
> > 
> > Do you mean the nsIGlobalObject from mParentElement?
> > e.g. mParentElement->GetOwnerGlobal()
> 
> See point 2 from
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Adding_WebIDL_bindings_to_a_class
> 
> If you're going to add this in a later patch in the same series, please add
> a comment to that effect.

Thanks for your reference. 
Eventually, the GetParentObject chain will get us to a Window and I think CSSPseudoElement and its parent Element should go to the same Window. I'd like to return the value of mParentElement->GetParentObject() in a later patch and add a comment here.
Comment on attachment 8713438 [details] [diff] [review]
Part 2: Replace Element in KeyframeEffectReadOnly WebIDL (v3)

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

r=birtles with comments addressed but obviously this needs DOM peer review

::: dom/animation/KeyframeEffect.cpp
@@ +1681,5 @@
>      aRv.Throw(NS_ERROR_DOM_ANIM_NO_TARGET_ERR);
>      return nullptr;
>    }
>  
> +  Element& target = aTarget.Value().GetAsElement();

Nit: I think targetElement would be more clear here.

@@ +1707,5 @@
> +  // targets a pseudo-element so this should never be called when
> +  // mPseudoType is not 'none' (see bug 1174575).
> +  MOZ_ASSERT(mPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement,
> +            "Requesting the target of a KeyframeEffect that targets a"
> +            " pseudo-element is not yet supported.");

Indentation here seems to be off-by-one.

::: dom/base/nsDOMMutationObserver.cpp
@@ +388,5 @@
>      return;
>    }
>  
> +  mozilla::dom::Element* animationTarget;
> +  nsCSSPseudoElements::Type pType;

s/pType/pseudoType/

::: dom/webidl/KeyframeEffect.webidl
@@ +22,5 @@
>  };
>  
> +// For the constructor we use (Element or CSSPseudoElement)? for the first
> +// argument since we cannot convert a mixin into a union type automatically,
> +// so define a union type directly to simplify the implementation.

Nit: We don't need the last line of this comment. (When you say we 'define' a union type it sounds like we're adding a typedef, but we're not, so we shouldn't say that.)

@@ +30,4 @@
>               object? frames,
>               optional (unrestricted double or KeyframeEffectOptions) options)]
>  interface KeyframeEffectReadOnly : AnimationEffectReadOnly {
> +  readonly attribute (Element or CSSPseudoElement)?  target;

The spec uses 'Animatable' here. We should either use Animatable here or add a comment saying why we don't (like above).
Attachment #8713438 - Flags: review?(bbirtles) → review+
Comment on attachment 8713440 [details] [diff] [review]
Part 3: Implement KeyframeEffectReadOnly::GetTarget() (v4)

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

Looks good but I'd like to take one last check with comments addressed. Also, I think you should get smaug to look over this too.

::: dom/animation/CSSPseudoElement.cpp
@@ +18,5 @@
>  
> +CSSPseudoElement::CSSPseudoElement(Element* aElement,
> +                                   nsCSSPseudoElements::Type aType)
> +  : mParentElement(aElement)
> +  , mPseudoType(aType)

There's a mismatch between the pseudo types allowed here and and the types allowed by nsCSSPseudoElements::GetPseudoAtom, as used by GetType. For example, aType could be ePseudo_NotPseudoElement here, but GetPseudoAtom doesn't allow that.

We should probably add a MOZ_ASSERT that aType < nsCSSPseudoElements::ePseudo_PseudoElementCount and document that restriction in the header file. We should *also* add an assertion in GetType that the return type of GetPseudoAtom is non-null.

@@ +61,5 @@
> +                                      nsCSSPseudoElements::Type aType)
> +{
> +  nsIAtom* propName = CSSPseudoElement::GetCSSPseudoElementPropertyAtom(aType);
> +  RefPtr<CSSPseudoElement> pseudo(
> +    static_cast<CSSPseudoElement*>(aElement->GetProperty(propName)));

I think you should use assignment here since it's more clear in light of C++'s "most vexing parse".

@@ +92,5 @@
> +      return nsGkAtoms::cssPseudoElementAfterProperty;
> +
> +    default:
> +      NS_NOTREACHED("Should not try to get CSSPseudoElement "
> +                    "other than :before or :after");

If we're only supporting ::before or ::after, we should probably assert that in the constructor.

Also, note that the canonical notation of pseudo-elements has TWO colons, e.g. "::before". The single-colon syntax is just a backwards compatibility requirement.

::: dom/animation/CSSPseudoElement.h
@@ +37,5 @@
>    virtual JSObject* WrapObject(JSContext* aCx,
>                                 JS::Handle<JSObject*> aGivenProto) override;
>  
> +  void GetType(nsString& aRetVal) const {
> +    aRetVal = nsCSSPseudoElements::GetPseudoAtom(mPseudoType)->GetUTF16String();

As mentioned later, we should also add something like:

  MOZ_ASSERT(nsCSSPseudoElements::GetPseudoAtom(mPseudoType),
             "All pseudo-types allowed by this class should have a"
             " corresponding atom");

Also, I think we can just do:

  nsCSSPseudoElements::GetPseudoAtom(mPseudoType)->ToString(aRetVal);

I think this would allow us to re-use the string buffer (or at very least avoid iterating over the buffer to get its length).

@@ +41,5 @@
> +    aRetVal = nsCSSPseudoElements::GetPseudoAtom(mPseudoType)->GetUTF16String();
> +  }
> +  already_AddRefed<Element> ParentElement() const {
> +    RefPtr<Element> ret(mParentElement);
> +    return ret.forget();

'ret' is a bit weird. I think we normally use 'retval' (or sometimes retVal--just use whichever is more common). I'm pretty sure 'rv' is normally only used when the type is nsresult.

@@ +53,5 @@
>              ErrorResult& aError);
> +
> +  // This function will try to get a valid CSSPseudoElement from an
> +  // Element:PseudoType pair first. If there is no CSSPseudoElement linked to
> +  // this Element:PseudoType pair, it will create a new one.

"Given an element:pseudoType pair, returns the CSSPseudoElement stored as a property on |aElement|. If there is no CSSPseudoElement for the specified pseudo-type on element, a new CSSPseudoElement will be created and stored on the element."

@@ +63,5 @@
> +
> +  static nsIAtom*
> +  GetCSSPseudoElementPropertyAtom(nsCSSPseudoElements::Type aType);
> +
> +private:

No need for the extra 'private' here.

@@ +64,5 @@
> +  static nsIAtom*
> +  GetCSSPseudoElementPropertyAtom(nsCSSPseudoElements::Type aType);
> +
> +private:
> +  // mParentElement needs to be an owning reference since the script is holding

"... since, if script is holding ... "

@@ +65,5 @@
> +  GetCSSPseudoElementPropertyAtom(nsCSSPseudoElements::Type aType);
> +
> +private:
> +  // mParentElement needs to be an owning reference since the script is holding
> +  // on to the pseudo-element, and it needs to continue to be able to refer to

"... on to the pseudo-element, it needs to ..."

::: dom/animation/KeyframeEffect.cpp
@@ +1714,5 @@
> +      aRv.SetValue().SetAsElement() = mTarget;
> +      break;
> +
> +    default:
> +      MOZ_ASSERT(true, "Unsupport pseudo type for web animation");

I'm pretty sure you don't mean MOZ_ASSERT(true) here. Perhaps NS_NOTREACHED?
Also, the message should probably be something like "Animation of unsupported pseudo-type"
Attachment #8713440 - Flags: review?(bbirtles)
Comment on attachment 8713440 [details] [diff] [review]
Part 3: Implement KeyframeEffectReadOnly::GetTarget() (v4)

A few extra comments.

>diff --git a/dom/animation/CSSPseudoElement.h b/dom/animation/CSSPseudoElement.h
>index a6152ca..90ddef1 100644
...
> public:
>   CSSPseudoElement* GetParentObject() const { return nullptr; }

I believe this needs to return the parent object of mParentElement.

>+private:
>+  // mParentElement needs to be an owning reference since the script is holding
>+  // on to the pseudo-element, and it needs to continue to be able to refer to
>+  // the parent element.
>+  RefPtr<Element> mParentElement;

I wonder if this should be NonNull.
Comment on attachment 8713441 [details] [diff] [review]
Part 4: Implement KeyframeEffectReadOnly Constructor for CSSPseudoElement (v3)

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

::: dom/animation/KeyframeEffect.cpp
@@ +1676,5 @@
>      const TimingParams& aTiming,
>      ErrorResult& aRv)
>  {
> +  if (aTarget.IsNull() ||
> +      (!aTarget.Value().IsElement() && !aTarget.Value().IsCSSPseudoElement())) {

Does this second condition ever evaluate true? I think undefined will become null, right?

@@ +1689,5 @@
> +  if (aTarget.Value().IsElement()) {
> +    target = &aTarget.Value().GetAsElement();
> +  } else {
> +    target = aTarget.Value().GetAsCSSPseudoElement().ParentElement();
> +    pseudoType = aTarget.Value().GetAsCSSPseudoElement().GetType();

This seems a little clumsy, but ok.

@@ +1696,2 @@
>    InfallibleTArray<AnimationProperty> animationProperties;
> +  BuildAnimationPropertyList(aGlobal.Context(), target, aFrames,

It's not clear *why* passing the parent content element of a pseudo-element to BuildAnimationPropertyList is ok here. We need to go through how target is used and document at each point that for pseudo-elements we use the parent element.
Attachment #8713441 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #40)
> >+  RefPtr<Element> mParentElement;
> 
> I wonder if this should be NonNull.

Yes. It should be NonNull<>, so we could avoid redundant MOZ_ASSERTs.
(In reply to Boris Chiou [:boris] from comment #42)
> (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #40)
> > >+  RefPtr<Element> mParentElement;
> > 
> > I wonder if this should be NonNull.
> 
> Yes. It should be NonNull<>, so we could avoid redundant MOZ_ASSERTs.

I'm thinking the unlink of Element may have run before the destructor of CSSPseudoElement. If we use OwningNonNull<Element> and we want to call mParentElement->DeleteProperty() in the destructor, mParentElement might be set to null already, so OwningNonNull will be asserted[1]. If we just use Refptr<Element>, we can skip this case to avoid MOZ_ASSERT in [1].

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/base/OwningNonNull.h#56
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #41)
> > +  if (aTarget.IsNull() ||
> > +      (!aTarget.Value().IsElement() && !aTarget.Value().IsCSSPseudoElement())) {
> 
> Does this second condition ever evaluate true? I think undefined will become
> null, right?

Yes. aTarget couldn't be in the uninitialized state if we call it from JS side, but if someone call this internally (C++ side), we have to make sure it return nullptr.

> 
> @@ +1696,2 @@
> >    InfallibleTArray<AnimationProperty> animationProperties;
> > +  BuildAnimationPropertyList(aGlobal.Context(), target, aFrames,
> 
> It's not clear *why* passing the parent content element of a pseudo-element
> to BuildAnimationPropertyList is ok here. We need to go through how target
> is used and document at each point that for pseudo-elements we use the
> parent element.

OK. I have to check them. Thanks.
Attachment #8713436 - Attachment is obsolete: true
Use (Element or CSSPseudoElement)? as the first arguement of constructor and
the type of target.
Attachment #8714243 - Flags: review+
Attachment #8713438 - Attachment is obsolete: true
Implement GetTarget() and functions of CSSPseudoElement.
We use a strong reference from CSSPseudoElement to Element and
a weak(raw pointer) reference from Element to CSSPseudoElement.
Attachment #8713440 - Attachment is obsolete: true
Create CSSPseudoElement.webidl, CSSPseudoElement.h, and CSSPseudoElement.cpp.
Attachment #8714242 - Attachment is obsolete: true
Attachment #8714246 - Flags: review?(bugs)
Attachment #8714246 - Flags: review?(bbirtles)
Attachment #8714243 - Flags: review?(bugs)
Attachment #8714244 - Flags: review?(bbirtles)
(In reply to Boris Chiou [:boris] from comment #44)
> (In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #41)
> > > +  if (aTarget.IsNull() ||
> > > +      (!aTarget.Value().IsElement() && !aTarget.Value().IsCSSPseudoElement())) {
> > 
> > Does this second condition ever evaluate true? I think undefined will become
> > null, right?
> 
> Yes. aTarget couldn't be in the uninitialized state if we call it from JS
> side, but if someone call this internally (C++ side), we have to make sure
> it return nullptr.

In that case just assert that Value() is initialized after the early return.
Comment on attachment 8714246 [details] [diff] [review]
Part 1: Define CSSPseudoElement interface (v5)

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

::: dom/animation/CSSPseudoElement.h
@@ +32,5 @@
> +public:
> +  ParentObject GetParentObject() const {
> +    // This will be implemented in later patch.
> +    return ParentObject(nullptr, nullptr);
> +  }

Curious, why did you switch to ParentObject here instead of nsISupports*?
Attachment #8714246 - Flags: review?(bbirtles) → review+
Comment on attachment 8714243 [details] [diff] [review]
Part 2: Replace Element in KeyframeEffectReadOnly WebIDL. (v4, carry birtles' r+)

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

::: dom/webidl/KeyframeEffect.webidl
@@ +29,5 @@
>               object? frames,
>               optional (unrestricted double or KeyframeEffectOptions) options)]
>  interface KeyframeEffectReadOnly : AnimationEffectReadOnly {
> +  // For the type of target, we use (Element or CSSPseudoElement)?,
> +  // instead of Animatable?, because of the same reason of the constructor.

// As with the constructor, we use (Element or CSSPseudoElement)? for the type
// of |target| instead of Animatable?
Attachment #8714243 - Flags: review?(bugs) → review+
Attachment #8714243 - Flags: review+ → review?(bugs)
Comment on attachment 8714244 [details] [diff] [review]
Part 3: Implement KeyframeEffectReadOnly::GetTarget() (v5)

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

r=birtles with comments addressed

::: dom/animation/CSSPseudoElement.cpp
@@ +23,5 @@
> +{
> +  MOZ_ASSERT(aElement);
> +  MOZ_ASSERT(aType == nsCSSPseudoElements::ePseudo_after ||
> +             aType == nsCSSPseudoElements::ePseudo_before,
> +             "Unexpected Pseudo Type");

We should document this restriction in the header file (i.e. only ::before and ::after are supported).

@@ +77,5 @@
> +  }
> +
> +  // CSSPseudoElement is a purely external interface created on-demand, and
> +  // when all references from script to the pseudo are dropped, we can drop the
> +  // CSSPseudoElement object, so use a raw pointer from Element to

Perhaps instead of "raw pointer", a "non-owning reference" would be more clear (since you can still use a raw pointer as an owning reference).

::: dom/animation/CSSPseudoElement.h
@@ +71,5 @@
> +
> +  static nsIAtom*
> +  GetCSSPseudoElementPropertyAtom(nsCSSPseudoElements::Type aType);
> +
> +

Nit: I think just one blank line would be enough here

::: dom/animation/KeyframeEffect.cpp
@@ +1710,5 @@
> +        CSSPseudoElement::GetCSSPseudoElement(mTarget, mPseudoType);
> +      break;
> +
> +    case nsCSSPseudoElements::ePseudo_NotPseudoElement:
> +      aRv.SetValue().SetAsElement() = mTarget;

Does this work if mTarget is nullptr? I think currently mTarget is never nullptr, but eventually it will be able to be null (probably in bug 1067769). If this *doesn't* work when mTarget is null we should add a comment saying that (or simply test for !mTarget here).
Attachment #8714244 - Flags: review?(bbirtles) → review+
Comment on attachment 8714243 [details] [diff] [review]
Part 2: Replace Element in KeyframeEffectReadOnly WebIDL. (v4, carry birtles' r+)

r+ to the .webidl assuming the bug bz requested has been filed.
Attachment #8714243 - Flags: review?(bugs) → review+
Comment on attachment 8714246 [details] [diff] [review]
Part 1: Define CSSPseudoElement interface (v5)

>
>+  ParentObject GetParentObject() const {
Nit, { should be on its own line.

>+// Both CSSOM and CSS Pseudo-Elements 4 provide contradictory definitions for
>+// this interface.
Bah, I sure hope there are spec bugs filed.

>+// What we implement here is a minimal subset of the two definitions which we
>+// ship behind a pref until the specification issues have been resolved.
>+[Func="nsDocument::IsWebAnimationsEnabled"]
>+interface CSSPseudoElement {
>+  readonly attribute DOMString type;
>+  readonly attribute Element parentElement;
>+};

So we probably should put this interface behind some other pref, since we may want to enable other animation stuff sooner, and
this not so often needed pseudoelement handling later once the spec situation isn't totally messy.
Or can we at least have something to ensure that we don't accidentally expose totally not-spec'ed-properly PseudoElement to the web?
So, please add a comment near http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2533
"// Is support for the Web Animations API enabled?" to explain that before enabling by default, make sure also PseudoElement interface has been spec'ed
properly. With that, r+ to the webidl. (I would prefer making all PseudoElement handling to be behind some separate pref, but I can live with a comment in all.js)

I've rarely seen as broken or messy situation in specs, so better be careful here.
Attachment #8714246 - Flags: review?(bugs) → review+
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #49)
> In that case just assert that Value() is initialized after the early return.

OK. Using assert would be better.
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #50)
> > +  ParentObject GetParentObject() const {
> > +    // This will be implemented in later patch.
> > +    return ParentObject(nullptr, nullptr);
> > +  }
> 
> Curious, why did you switch to ParentObject here instead of nsISupports*?
Just want to use the same return type as nsINode::GetParentObject() [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsINode.h#392
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #52)
> > +    case nsCSSPseudoElements::ePseudo_NotPseudoElement:
> > +      aRv.SetValue().SetAsElement() = mTarget;
> 
> Does this work if mTarget is nullptr? I think currently mTarget is never
> nullptr, but eventually it will be able to be null (probably in bug
> 1067769). If this *doesn't* work when mTarget is null we should add a
> comment saying that (or simply test for !mTarget here).

I think this works, but may confuse people. If we have a null mTarget, I think we should call aRv.SetNull() directly. The constructor, KeyframeEffectReadOnly::KeyframeEffectReadOnly [1], has an assertion to check aTarget, and there is no assignment to mTarget in the current implementation, so it is never nullptr, as you mentioned. However, adding an assertion here is OK and we shouldn't return a null Element object here now.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffect.cpp#84
(In reply to Boris Chiou [:boris] from comment #57)
> I think this works, but may confuse people. If we have a null mTarget, I
> think we should call aRv.SetNull() directly. The constructor,
> KeyframeEffectReadOnly::KeyframeEffectReadOnly [1], has an assertion to
> check aTarget, and there is no assignment to mTarget in the current
> implementation, so it is never nullptr, as you mentioned. However, adding an
> assertion here is OK and we shouldn't return a null Element object here now.

Sorry. I'd like to revise here. Add this in the beginning of GetTarget():

if (!mTarget) {
  aRv.SetNull();
  return;
}

Thanks.
(In reply to Olli Pettay [:smaug] from comment #53)
> r+ to the .webidl assuming the bug bz requested has been filed.

I forgot to add the bug number in the comment. I should update this patch. Thanks.
(In reply to Olli Pettay [:smaug] from comment #54)
> >+// Both CSSOM and CSS Pseudo-Elements 4 provide contradictory definitions for
> >+// this interface.
> Bah, I sure hope there are spec bugs filed.

Hi, Brian, do you know how to file the related bugs or do something for this? I have no idea how to handle this.

> Or can we at least have something to ensure that we don't accidentally
> expose totally not-spec'ed-properly PseudoElement to the web?
> So, please add a comment near
> http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js#2533
> "// Is support for the Web Animations API enabled?" to explain that before
> enabling by default, make sure also PseudoElement interface has been spec'ed
> properly. With that, r+ to the webidl. (I would prefer making all
> PseudoElement handling to be behind some separate pref, but I can live with
> a comment in all.js)
> 
> I've rarely seen as broken or messy situation in specs, so better be careful
> here.

Thanks, Olli. I prefer to add this comment in all.js.
Create CSSPseudoElement.webidl, CSSPseudoElement.h, and CSSPseudoElement.cpp.

Updated: address reviewers' comments.
Attachment #8714678 - Flags: review+
Attachment #8714246 - Attachment is obsolete: true
Use (Element or CSSPseudoElement)? as the first arguement of constructor and
the type of target.

Updated: rebase and address reviewers' comments.

Sorry, Brian. I need your review for this patch again because I have to revise
the APIs of TimingParams after rebasing the code.
Attachment #8714680 - Flags: review?(bbirtles)
Attachment #8714680 - Flags: review+
Attachment #8714243 - Attachment is obsolete: true
Implement GetTarget() and functions of CSSPseudoElement.
We use a strong reference from CSSPseudoElement to Element and a non-owning
reference from Element to CSSPseudoElement.

Updated: address Brian's comments.
Attachment #8714681 - Flags: review+
Attachment #8714244 - Attachment is obsolete: true
(In reply to Boris Chiou [:boris]  from comment #61)
> (In reply to Olli Pettay [:smaug] from comment #54)
> > >+// Both CSSOM and CSS Pseudo-Elements 4 provide contradictory definitions for
> > >+// this interface.
> > Bah, I sure hope there are spec bugs filed.
> 
> Hi, Brian, do you know how to file the related bugs or do something for
> this? I have no idea how to handle this.

It's a known problem that I've mailed www-style about before. I've told the editors of the relevant specs too and dbaron added issues to the corresponding specs last December so I think this is sufficiently reported.
Comment on attachment 8714680 [details] [diff] [review]
Part 2: Replace Element in KeyframeEffectReadOnly WebIDL. (v5, carry smaug's r+)

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

::: dom/animation/AnimationEffectTimingReadOnly.cpp
@@ +36,5 @@
>    if (aOptions.IsUnrestrictedDouble()) {
>      return TimingParams(aOptions.GetAsUnrestrictedDouble());
>    } else {
>      MOZ_ASSERT(aOptions.IsKeyframeEffectOptions());
> +    MOZ_ASSERT(!aTarget.IsNull() && aTarget.Value().IsElement());

Why is this a valid assertion? Is it because we don't support CSSPseudoElement::Animate yet? If so, we should add a comment with the bug number and a message to the assertion.

@@ +53,5 @@
>    } else {
>      MOZ_ASSERT(aOptions.IsKeyframeAnimationOptions());
> +    MOZ_ASSERT(!aTarget.IsNull() && aTarget.Value().IsElement());
> +    return TimingParams(aOptions.GetAsKeyframeAnimationOptions(),
> +                        &aTarget.Value().GetAsElement());

Now that the two versions of FromOptionsUnion are getting more complex (and will likely get more complex still once we support CSSPseudoElement::Animate) I wonder if we can combine them. What do you think about doing something like:

template <class OptionsType>
const dom::AnimationEffectTimingProperties&
GetTimingProperties(const OptionsType& aOptions);

template <>
const dom::AnimationEffectTimingProperties&
GetTimingProperties(const dom::UnrestrictedDoubleOrKeyframeEffectOptions&
                      aOptions)
{
  MOZ_ASSERT(aOptions.IsKeyframeEffectOptions());
  return aOptions.GetAsKeyframeEffectOptions();
}

template <>
const dom::AnimationEffectTimingProperties&
GetTimingProperties(const dom::UnrestrictedDoubleOrKeyframeAnimationOptions&
                      aOptions)
{
  MOZ_ASSERT(aOptions.IsKeyframeAnimationOptions());
  return aOptions.GetAsKeyframeAnimationOptions();
}

template <class OptionsType>
TimingParams
TimingParamsFromOptionsUnion(const OptionsType& aOptions,
                             const dom::Element* aTarget)
{
  if (aOptions.IsUnrestrictedDouble()) {
    return TimingParams(aOptions.GetAsUnrestrictedDouble());
  } else {
    return TimingParams(GetTimingProperties(aOptions), aTarget);
  }
}

/* static */ TimingParams
TimingParams::FromOptionsUnion(
  const dom::UnrestrictedDoubleOrKeyframeEffectOptions& aOptions,
  const dom::Element* aTarget)
{
  return TimingParamsFromOptionsUnion(aOptions, aTarget);
}

/* static */ TimingParams
TimingParams::FromOptionsUnion(
  const dom::UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
  const dom::Element* aTarget)
{
  return TimingParamsFromOptionsUnion(aOptions, aTarget);
}

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +36,5 @@
>  {
>    TimingParams() = default;
>    explicit TimingParams(
>      const dom::AnimationEffectTimingProperties& aTimingProperties,
>      const dom::Element* aTarget);

Can you drop the 'explicit' from this (since this constructor now has *two* parameters) while you're touching this file? Thanks.
Attachment #8714680 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #66)
> > +    MOZ_ASSERT(!aTarget.IsNull() && aTarget.Value().IsElement());
> 
> Why is this a valid assertion? Is it because we don't support
> CSSPseudoElement::Animate yet? If so, we should add a comment with the bug
> number and a message to the assertion.

Yes. we don't support CSSPseudoElement::Animate yet here, so I add this check. I will add a comment with the bug number.

> Now that the two versions of FromOptionsUnion are getting more complex (and
> will likely get more complex still once we support
> CSSPseudoElement::Animate) I wonder if we can combine them. What do you
> think about doing something like:
> 
> template <class OptionsType>
> const dom::AnimationEffectTimingProperties&
> GetTimingProperties(const OptionsType& aOptions);
> 
> ...

OK. Function template is more flexible. Let's try it.

> >    explicit TimingParams(
> >      const dom::AnimationEffectTimingProperties& aTimingProperties,
> >      const dom::Element* aTarget);
> 
> Can you drop the 'explicit' from this (since this constructor now has *two*
> parameters) while you're touching this file? Thanks.

Sure.
Attachment #8714680 - Attachment is obsolete: true
Attachment #8715185 - Attachment is obsolete: true
Attachment #8713441 - Attachment is obsolete: true
Attachment #8716203 - Attachment is obsolete: true
Attachment #8716198 - Flags: review?(bbirtles)
Attachment #8716199 - Flags: review?(bbirtles)
Attachment #8716206 - Flags: review?(bbirtles)
Comment on attachment 8716198 [details] [diff] [review]
Part 4: Support CSSPseudoElement for TimingParams

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

r=birtles with the leak fixed and bug filed

::: dom/animation/AnimationEffectTimingReadOnly.cpp
@@ +64,5 @@
> +    // If aTarget is a pseudo element, we pass its parent element because
> +    // TimingParams only needs its owner doc to parse easing and both pseudo
> +    // element and its parent element should have the same owner doc.
> +    dom::Element* targetElement = nullptr;
> +    if (!aTarget.IsNull()) {

This is really odd that we need a target element in order to parse a timing function. Can you file a bug to fix that (blocking web-animations) and add a comment/warning here?

@@ +70,5 @@
> +      MOZ_ASSERT(target.IsElement() || target.IsCSSPseudoElement(),
> +                 "Uninitialized target");
> +      targetElement = target.IsElement() ?
> +                      &target.GetAsElement() :
> +                      target.GetAsCSSPseudoElement().ParentElement().take();

This looks like a memory leak. Calling take() on an already_AddRefed is going to return the already add-refed pointer. If we assign that to a raw pointer (targetElement) we'll never release it.

We should just make targetElement a RefPtr, I think.
Attachment #8716198 - Flags: review?(bbirtles) → review+
Comment on attachment 8716199 [details] [diff] [review]
Part 5: Support pseudo type in StyleAnimation

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

r=birtles with comments addressed

::: layout/style/StyleAnimationValue.h
@@ +135,5 @@
>     * @param aProperty       The property whose value we're computing.
>     * @param aTargetElement  The content node to which our computed value is
>     *                        applicable.
> +   * @param aPseudoType     The psuedo type which is used to get the correct
> +   *                        style context for computing

Spelling: 'The pseudo type'

This sentence seems incomplete. Perhaps just "The type of pseudo-element to which the computed value is applicable."

We should also update the description of 'aTargetElement' to say 'The content node to which our computed value is applicable. For pseudo-elements, this is the parent element to which the pseudo is attached, not the generated content node.'
Attachment #8716199 - Flags: review?(bbirtles) → review+
Comment on attachment 8716206 [details] [diff] [review]
Part 6: Implement KeyframeEffectReadOnly Constructor for CSSPseudoElement (v5)

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

::: dom/animation/KeyframeEffect.cpp
@@ +1142,5 @@
>   * into a ComputedTimingFunction value in the corresponding KeyframeValueEntry
>   * objects.
>   *
>   * @param aTarget The target of the animation.
> + * @param aPsuedoType The pseudo type of the target if it is a psuedo element.

Spelling: aPseudoType
Spelling: pseudo

@@ +1160,5 @@
>    nsCSSPropertySet propertiesWithToValue;   // Those with a defined 100% value.
>  
>    for (OffsetIndexedKeyframe& keyframe : aKeyframes) {
>      float offset = float(keyframe.mKeyframeDict.mOffset.Value());
> +    // ParseEasing use element's owner doc, so if it is a pseudo element,

Nit: uses

@@ +1427,5 @@
>      aRv.Throw(NS_ERROR_FAILURE);
>      return;
>    }
>  
> +  // ParseEasing use element's owner doc, so if it is a pseudo element,

uses
Attachment #8716206 - Flags: review?(bbirtles) → review+
Attachment #8716198 - Attachment is obsolete: true
Attachment #8716199 - Attachment is obsolete: true
Use (Element or CSSPseudoElement)? as the first arguement of constructor and
the type of target.

Rebased.
Attachment #8716570 - Flags: review+
Attachment #8716197 - Attachment is obsolete: true
Attachment #8716206 - Attachment is obsolete: true
Attachment #8716550 - Attachment is obsolete: true
Keywords: checkin-needed
Hi Part 5 fails to apply:

adding 1174575 to series file
renamed 1174575 -> Bug-1174575---Part-5-Support-pseudo-type-in-StyleA.patch
applying Bug-1174575---Part-5-Support-pseudo-type-in-StyleA.patch
patching file layout/style/StyleAnimationValue.cpp
Hunk #2 FAILED at 2572
1 out of 2 hunks FAILED -- saving rejects to file layout/style/StyleAnimationValue.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1174575---Part-5-Support-pseudo-type-in-StyleA.patch
Flags: needinfo?(boris.chiou)
Keywords: checkin-needed
Add one more argument, nsCSSPseudoElement::Type, for
StyleAnimation::ComputeValue and StyleAnimation::ComputeValues

Rebased.
Attachment #8717406 - Flags: review+
Attachment #8716551 - Attachment is obsolete: true
Let KeyframeEffectReadOnly::Constructor support both Element and
CSSPseudoElement as the target.

Rebased.
Attachment #8717407 - Flags: review+
Attachment #8716571 - Attachment is obsolete: true
Rebase part 5 and part 6.
Flags: needinfo?(boris.chiou)
Keywords: checkin-needed
Depends on: 1248105
Blocks: 1249230
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: