Closed Bug 824898 Opened 7 years ago Closed 7 years ago

Convert SVG animation elements to WebIDL

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attachment #695929 - Flags: review?(bzbarsky)
Attached patch Convert to WebIDL bindings (obsolete) — Splinter Review
Attachment #696816 - Flags: review?(bzbarsky)
SVGAnimationElement should be concrete: false.
Attachment #696816 - Attachment is obsolete: true
Attachment #696816 - Flags: review?(bzbarsky)
Attachment #696817 - Flags: review?(bzbarsky)
Comment on attachment 695929 [details] [diff] [review]
Rename files and move classes into namespaces

Probably better to not lose the SVGWhateverElementBase typedefs on subclasses of SVGAnimationElement.  If you do want to lose them, please get review from an SVG peer on that.

>+++ b/content/svg/content/src/SVGAnimateElement.cpp
>+#include "SVGAnimateElement.h"

mozilla/dom/SVGAnimateElement.h

>+++ b/content/svg/content/src/SVGAnimateMotionElement.cpp
>+#include "SVGAnimateMotionElement.h"

mozilla/dom/SVGAnimateMotionElement.h

>+++ b/content/svg/content/src/SVGAnimateTransformElement.cpp
>+#include "SVGAnimateTransformElement.h"

mozilla/dom/SVGAnimateTransformElement.h

>+++ b/content/svg/content/src/SVGAnimationElement.cpp
>+#include "SVGAnimationElement.h"

mozilla/dom/SVGAnimationElement.h

>+++ b/content/svg/content/src/SVGSetElement.cpp
>+#include "SVGSetElement.h"

mozilla/dom/SVGSetElement.h

r=me with the above addressed
Attachment #695929 - Flags: review?(bzbarsky) → review+
Comment on attachment 696817 [details] [diff] [review]
Convert to WebIDL bindings

>+++ b/content/svg/content/src/SVGAnimateElement.h
>+  virtual JSObject* WrapNode(JSContext *aCx, JSObject *aScope, bool *aTriedToWrap);

MOZ_OVERRIDE.  Same elsewhere in this patch.

>+++ b/content/svg/content/src/SVGAnimationElement.cpp
> SVGAnimationElement::GetTargetElement(nsIDOMSVGElement** aTarget)
>+  NS_IF_ADDREF(*aTarget = targetSVG);

targetSVG.forget(aTarget);

>+SVGAnimationElement::GetTargetElement()
>+  nsCOMPtr<nsSVGElement> elem = do_QueryInterface(target);

Why is this a sane thing to do?  Does nsSVGElement have an IID?

Seems like it would be better to check target->IsSVG() and if so cast to nsSVGElement*, else return null.

>+++ b/dom/bindings/Bindings.conf
>+'SVGAnimationElement': {
>+    'resultNotAddRefed': ['targetElement'],
>+    'conrete': False

'concrete'.  I wonder if we can catch typos like that somehow...

r=me with the above fixed (esp. the bogus QI bit).
Attachment #696817 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/7601aad6045f
https://hg.mozilla.org/mozilla-central/rev/28cb4a10444e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.