846908 - Move functions from nsISMILAnimationElement to SVGAnimationElement

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

It is only implemented by SVGAnimationElement.

Comment 1

[Robert Longson [:longsonr]]

I think it's best to call IsNodeOfType(nsINode::eANIMATION) rather than creating a new IsSVGAnimationElement method

Comment 2

[David Zbarsky (:dzbarsky)]

(In reply to Robert Longson from comment #1)
> I think it's best to call IsNodeOfType(nsINode::eANIMATION) rather than
> creating a new IsSVGAnimationElement method

You're right; I didn't know that existed.

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 720093 [details] [diff] [review]
Patch

Moving the review request to Brian since I'm not familiar with the future SMIL/animation plans, or whether there is a good reason to leave things as they are.

Comment 4

[Brian Birtles (:birtles)]

What's the reason for this? The reason for nsISMILAnimationElement was that we originally implemented SMIL so that it was independent of SVG. It's true it's not needed, but is it causing problems by being there?

I'm expecting we'll rewrite pretty much all that code when we implement Web Animations so I wonder if we should just wait until then?

Comment 5

[David Zbarsky (:dzbarsky)]

It's not causing any problems; it's just general cleanup that makes those elements a little smaller.  If you think we'll need this interface for Web Animations we should just WONTFIX this bug.

Comment 6

[Brian Birtles (:birtles)]

(In reply to David Zbarsky (:dzbarsky) from comment #5)
> It's not causing any problems; it's just general cleanup that makes those
> elements a little smaller.  If you think we'll need this interface for Web
> Animations we should just WONTFIX this bug.

I'm still going through it. It's probably ok to land since it does clean things up but I want to just check it over once more.

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 720093 [details] [diff] [review]
Patch

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

I think this looks good. It certainly makes things simpler.

My main concern is that we might have to reintroduce this sort of thing later but that's probably easy enough to do.

Thanks David!

::: content/base/public/Element.h
@@ +895,5 @@
> +                       Tag() == nsGkAtoms::animateTransform ||
> +                       Tag() == nsGkAtoms::animateMotion ||
> +                       Tag() == nsGkAtoms::set);
> +  }
> +

As Robert points out in comment 1, you can probably use IsNodeOfType(nsINode::eANIMATION) here.

::: content/smil/nsSMILAnimationController.h
@@ +25,5 @@
> +class SVGAnimationElement;
> +}
> +}
> +
> +typedef mozilla::dom::SVGAnimationElement SVGAnimationElement;

This may be fine but personally I prefer not putting namespace typedefs and 'using' etc. in header files since it somewhat undermines the purpose of using namespaces.

I think others don't care though so it's up to you.

@@ +132,2 @@
>      nsSMILMilestone                              mMilestone;
>    };

Can you fix the spacing here so the two members line up?

::: content/smil/nsSMILTimedElement.h
@@ +539,1 @@
>                                                       // owner

Please fix the spacing of the comment.

::: content/svg/content/src/Makefile.in
@@ +155,2 @@
>  	$(NULL)
>  

What about this patch makes this necessary?

::: content/svg/content/src/SVGAnimateMotionElement.h
@@ -38,5 @@
>  
>    // nsIDOMNode specializations
>    virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;
>  
> -  // nsISMILAnimationElement

We're removing these comments everywhere. Do you think it makes sense to label these blocks of methods 'SVGAnimationElement' ?

Comment 8

[Brian Birtles (:birtles)]

Oops, didn't realise splinter cut off so much context.

(In reply to Brian Birtles (:birtles) from comment #7)
> ::: content/base/public/Element.h
> @@ +895,5 @@
> > +                       Tag() == nsGkAtoms::animateTransform ||
> > +                       Tag() == nsGkAtoms::animateMotion ||
> > +                       Tag() == nsGkAtoms::set);
> > +  }
> > +
> 
> As Robert points out in comment 1, you can probably use
> IsNodeOfType(nsINode::eANIMATION) here.

This is about the IsSVGAnimationElement method.

> 
> ::: content/svg/content/src/Makefile.in
> @@ +155,2 @@
> >  	$(NULL)
> >  
> 
> What about this patch makes this necessary?

This is about the addition of nsSVGClass.h, nsSVGElement.h, SVGStringList.h.

Comment 9

[David Zbarsky (:dzbarsky)]

(In reply to Brian Birtles (:birtles) from comment #7)
> Comment on attachment 720093 [details] [diff] [review]
> Patch
> 
> Review of attachment 720093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks good. It certainly makes things simpler.
> 
> My main concern is that we might have to reintroduce this sort of thing
> later but that's probably easy enough to do.
> 
> Thanks David!
> 
> ::: content/base/public/Element.h
> @@ +895,5 @@
> > +                       Tag() == nsGkAtoms::animateTransform ||
> > +                       Tag() == nsGkAtoms::animateMotion ||
> > +                       Tag() == nsGkAtoms::set);
> > +  }
> > +
> 
> As Robert points out in comment 1, you can probably use
> IsNodeOfType(nsINode::eANIMATION) here.
> 
> ::: content/smil/nsSMILAnimationController.h
> @@ +25,5 @@
> > +class SVGAnimationElement;
> > +}
> > +}
> > +
> > +typedef mozilla::dom::SVGAnimationElement SVGAnimationElement;
> 
> This may be fine but personally I prefer not putting namespace typedefs and
> 'using' etc. in header files since it somewhat undermines the purpose of
> using namespaces.
> 
> I think others don't care though so it's up to you.

I changed it since it sometimes causes build errors on gcc.

> >  
> 
> What about this patch makes this necessary?
> 

I think including the exported SVGAnimationElement header means those headers also have to be exported.  I don't remember the exact reason, but it fixed build errors.

> ::: content/svg/content/src/SVGAnimateMotionElement.h
> @@ -38,5 @@
> >  
> >    // nsIDOMNode specializations
> >    virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;
> >  
> > -  // nsISMILAnimationElement
> 
> We're removing these comments everywhere. Do you think it makes sense to
> label these blocks of methods 'SVGAnimationElement' ?

Yup, I'll do that.

Comment 10

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe09ce5f9ed

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbe09ce5f9ed