Closed
Bug 846908
Opened 9 years ago
Closed 9 years ago
Move functions from nsISMILAnimationElement to SVGAnimationElement
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: dzbarsky, Assigned: dzbarsky)
Details
Attachments
(1 file)
107.30 KB,
patch
|
birtles
:
review+
|
Details | Diff | Splinter Review |
It is only implemented by SVGAnimationElement.
Attachment #720093 -
Flags: review?(jwatt)
Comment 1•9 years ago
|
||
I think it's best to call IsNodeOfType(nsINode::eANIMATION) rather than creating a new IsSVGAnimationElement method
Assignee | ||
Comment 2•9 years ago
|
||
(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•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
(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•9 years ago
|
||
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+
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe09ce5f9ed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cbe09ce5f9ed
Status: NEW → RESOLVED
Closed: 9 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.
Description
•