Convert subclasses of SVGGraphicsElement to WebIDL

RESOLVED FIXED in mozilla21

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments)

13.52 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
17.25 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
22.74 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
13.19 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
39.92 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
23.63 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
18.20 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
19.70 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
27.35 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
22.51 KB, patch
dzbarsky
: checkin+
Details | Diff | Splinter Review
21.16 KB, patch
Details | Diff | Splinter Review
5.11 KB, patch
Details | Diff | Splinter Review
19.29 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

5 years ago
Depends on: 824229
(Assignee)

Comment 1

5 years ago
Created attachment 696234 [details] [diff] [review]
SVGDefsElement
Attachment #696234 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

5 years ago
Created attachment 696237 [details] [diff] [review]
SVGSwitchElement
Attachment #696237 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 696247 [details] [diff] [review]
SVGForeignObjectElement
Attachment #696247 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

5 years ago
Created attachment 696249 [details] [diff] [review]
SVGGElement
Attachment #696249 - Flags: review?(bzbarsky)

Updated

5 years ago
Blocks: 825282
(Assignee)

Comment 5

5 years ago
Created attachment 696589 [details] [diff] [review]
SVGImageElement
Attachment #696589 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

5 years ago
Created attachment 696590 [details] [diff] [review]
SVGRectElement
Attachment #696590 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

5 years ago
Created attachment 696591 [details] [diff] [review]
SVGCircleElement
(Assignee)

Comment 8

5 years ago
Created attachment 696592 [details] [diff] [review]
SVGEllipseElement
Attachment #696592 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #696591 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

5 years ago
Created attachment 696593 [details] [diff] [review]
SVGPolylineElement and SVGPolygonElement
Attachment #696593 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

5 years ago
Created attachment 696858 [details] [diff] [review]
SVGLineelement
Attachment #696858 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

5 years ago
Created attachment 696948 [details] [diff] [review]
SVGAElement
Attachment #696948 - Flags: review?(bzbarsky)
Comment on attachment 696234 [details] [diff] [review]
SVGDefsElement

r=me modulo the include guard thing and MOZ_OVERRIDE on WrapNode.
Attachment #696234 - Flags: review?(bzbarsky) → review+
Comment on attachment 696237 [details] [diff] [review]
SVGSwitchElement

Usual comments here MOZ_OVERRIDE and the include guard.

r=me with that
Attachment #696237 - Flags: review?(bzbarsky) → review+
Comment on attachment 696247 [details] [diff] [review]
SVGForeignObjectElement

With the MOZ_OVERRIDE thing and the include guard thing, r=me
Attachment #696247 - Flags: review?(bzbarsky) → review+
Comment on attachment 696249 [details] [diff] [review]
SVGGElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.
Attachment #696249 - Flags: review?(bzbarsky) → review+
Comment on attachment 696589 [details] [diff] [review]
SVGImageElement

>+++ b/dom/webidl/SVGImageElement.webidl

You need to put the nsIImageLoadingContent stuff in here.  See the patches in bug 825527 for discussion; you should just be able to copy/paste from HTMLImageElement.webidl and have things Just Work, since SVGimageElement inherits from nsImageLoadingContent.  Though you will want to add nsIImageLoadingContent to the classinfo for SVGImageElement.

Also, this patch as is breaks page info for SVG images because that does "instanceof SVGImageElement".  You need to add a 'hasInstanceInterface': 'nsIDOMSVGImageElement' in Bindings.conf.

And the usual MOZ_OVERRIDE and include guard nits.

r=me with the above fixed, though I'd like to see either the updated patch or better yet the interdiff.
Attachment #696589 - Flags: review?(bzbarsky) → review+
Comment on attachment 696590 [details] [diff] [review]
SVGRectElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.
Attachment #696590 - Flags: review?(bzbarsky) → review+
Comment on attachment 696591 [details] [diff] [review]
SVGCircleElement

r=me with MOZ_OVERRIDE and the include guard thing fixed.
Attachment #696591 - Flags: review?(bzbarsky) → review+
Er, I guess for circle you already have the MOZ_OVERRIDE, so nevermind me!
Comment on attachment 696592 [details] [diff] [review]
SVGEllipseElement

r=me
Attachment #696592 - Flags: review?(bzbarsky) → review+
Comment on attachment 696593 [details] [diff] [review]
SVGPolylineElement and SVGPolygonElement

>+++ b/content/svg/content/src/nsSVGPolyElement.cpp
>+nsSVGPolyElement::Points()
>+  nsRefPtr<DOMSVGPointList> points = DOMSVGPointList::GetDOMWrapper(key, this, false).get();

This leaks.  Please drop the .get() on the end.


>+nsSVGPolyElement::AnimatedPoints()
>+  nsRefPtr<DOMSVGPointList> points = DOMSVGPointList::GetDOMWrapper(key, this, false).get();

Likewise, and third arg to GetDOMWrapper here should be true.  I guess we have no test coverage for this, eh?  ;)

r=me with those fixed.
Attachment #696593 - Flags: review?(bzbarsky) → review+
Comment on attachment 696858 [details] [diff] [review]
SVGLineelement

The ellipse changes are perhaps meant to be in the ellipse patch?

r=me with that fixed.
Attachment #696858 - Flags: review?(bzbarsky) → review+
Comment on attachment 696948 [details] [diff] [review]
SVGAElement

I'm surprised this passed tests.  It's missing a WrapNode implementation that would instantiate the right binding.  Pelase fix that and add some tests?

Also, this needs a hasInstanceInterface because we have code in browser.js that does instanceof SVGAElement.
Attachment #696948 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 24

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #23)
> >+nsSVGPolyElement::AnimatedPoints()
> >+  nsRefPtr<DOMSVGPointList> points = DOMSVGPointList::GetDOMWrapper(key, this, false).get();

> Likewise, and third arg to GetDOMWrapper here should be true.  I guess we have no test coverage for this, eh?  ;)


> Comment on attachment 696948 [details] [diff] [review]
> SVGAElement
> 
> I'm surprised this passed tests.  It's missing a WrapNode implementation
> that would instantiate the right binding.  Pelase fix that and add some
> tests?
> 

Both of these actually failed tests, I just forgot to upload fixed versions.
(Assignee)

Comment 25

5 years ago
Created attachment 698364 [details] [diff] [review]
SVGImageElement interdiff
Attachment #698364 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #696234 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696237 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696247 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696249 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696589 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696590 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696591 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696592 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696593 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #696858 - Flags: checkin+
(Assignee)

Comment 29

5 years ago
Created attachment 698462 [details] [diff] [review]
SVGAElement
Attachment #698462 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Summary: Convert subclasses of SVGGraphicsElement → Convert subclasses of SVGGraphicsElement to WebIDL
Version: unspecified → Trunk
(Assignee)

Comment 30

5 years ago
SVGAElement needs hasInstanceInterface
Comment on attachment 698364 [details] [diff] [review]
SVGImageElement interdiff

Fix the comment in nsIImageLoadingContent.idl to point to here too, not just HTMLImageElement, and r=me.

Though maybe we should put all this gunk on a separate [NoInterfaceObject] non-concrete interface (called ImageLoadingContent, say) and just have HTMLImageElement and SVGImageElement implement it?  Either way for this part, though that would make keeping things in sync simpler.

r=me
Attachment #698364 - Flags: review?(bzbarsky) → review+
Comment on attachment 698462 [details] [diff] [review]
SVGAElement

>+                              public mozilla::dom::Link

Just Link.

You still need a hasInstanceInterface here.

r=me if you add that.
Attachment #698462 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/7f39d4e9a70a
https://hg.mozilla.org/mozilla-central/rev/d36a97058e39
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.