Closed Bug 846908 Opened 7 years ago Closed 7 years ago

Move functions from nsISMILAnimationElement to SVGAnimationElement

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
It is only implemented by SVGAnimationElement.
Attachment #720093 - Flags: review?(jwatt)
I think it's best to call IsNodeOfType(nsINode::eANIMATION) rather than creating a new IsSVGAnimationElement method
(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 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.
Attachment #720093 - Flags: review?(jwatt) → review?(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?
Flags: needinfo?(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.
Flags: needinfo?(dzbarsky)
(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 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' ?
Attachment #720093 - Flags: review?(birtles) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/cbe09ce5f9ed
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.