Closed Bug 824327 Opened 7 years ago Closed 7 years ago

Convert direct subclasses of SVGElement to WebIDL

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(8 files, 7 obsolete files)

11.90 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
21.63 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
16.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
33.73 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
27.26 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
24.38 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #695429 - Flags: review?(bzbarsky)
Attached patch SVGDescElement (obsolete) — Splinter Review
Attachment #695430 - Flags: review?(bzbarsky)
Attached patch SVGTitleElement (obsolete) — Splinter Review
Attachment #695432 - Flags: review?(bzbarsky)
Attached patch SVGStopElement (obsolete) — Splinter Review
Attachment #695590 - Flags: review?(bzbarsky)
Attached patch SVGStyleElement (obsolete) — Splinter Review
Attachment #695592 - Flags: review?(bzbarsky)
Summary: Convert SVGDescElement, SVGTitleElement, SVGStyleElement, SVGStopElement to WebIDL → Convert SVGDescElement, SVGTitleElement, SVGStyleElement, SVGStopElement, SVGMetadataElement to WebIDL
Summary: Convert SVGDescElement, SVGTitleElement, SVGStyleElement, SVGStopElement, SVGMetadataElement to WebIDL → Convert SVGDescElement, SVGTitleElement, SVGStyleElement, SVGStopElement, SVGMetadataElement, SVGMPathElement to WebIDL
Attached patch SVGMetadataElement (obsolete) — Splinter Review
Attachment #695606 - Flags: review?(bzbarsky)
Summary: Convert SVGDescElement, SVGTitleElement, SVGStyleElement, SVGStopElement, SVGMetadataElement, SVGMPathElement to WebIDL → Convert direct subclasses of SVGElement to WebIDL
Attached patch SVGDescElementSplinter Review
Attachment #695430 - Attachment is obsolete: true
Attachment #695430 - Flags: review?(bzbarsky)
Attachment #695671 - Flags: review?(bzbarsky)
Attached patch SVGTitleElementSplinter Review
Attachment #695432 - Attachment is obsolete: true
Attachment #695432 - Flags: review?(bzbarsky)
Attachment #695672 - Flags: review?(bzbarsky)
Attached patch SVGStopElementSplinter Review
Attachment #695590 - Attachment is obsolete: true
Attachment #695590 - Flags: review?(bzbarsky)
Attachment #695673 - Flags: review?(bzbarsky)
Attached patch SVGStyleElementSplinter Review
Attachment #695592 - Attachment is obsolete: true
Attachment #695592 - Flags: review?(bzbarsky)
Attachment #695674 - Flags: review?(bzbarsky)
Attachment #695606 - Attachment is obsolete: true
Attachment #695606 - Flags: review?(bzbarsky)
Attachment #695676 - Flags: review?(bzbarsky)
Attached patch SVGMPathElementSplinter Review
Attachment #695678 - Flags: review?(bzbarsky)
Attached patch SVGScriptElement (obsolete) — Splinter Review
Attachment #695683 - Flags: review?(bzbarsky)
Attached patch SVGScriptElementSplinter Review
Attachment #695683 - Attachment is obsolete: true
Attachment #695683 - Flags: review?(bzbarsky)
Attachment #696244 - Flags: review?(bzbarsky)
Comment on attachment 695429 [details] [diff] [review]
Part 1: Add a namespaced version of NS_NewxxxSVGElement

Why this instead of keeping the NS_NewSVGWhateverElement functions but either use a different macro name (as this patch) that creates the namespaced version or do what Peter did for HTML and have the existing macro auto-detect the element concrete class?
Attachment #695429 - Attachment is obsolete: true
Attachment #695429 - Flags: review?(bzbarsky)
Comment on attachment 697665 [details] [diff] [review]
Part 1: Add a namespaced version of NS_NewxxxSVGElement

r=me
Attachment #697665 - Flags: review?(bzbarsky) → review+
Comment on attachment 695671 [details] [diff] [review]
SVGDescElement

>+#ifndef SVGDescElement_h

I think Peter and I have been using mozilla_dom_SVGDescElement_h

r=me with that and the NS_New thing updated.
Attachment #695671 - Flags: review?(bzbarsky) → review+
Comment on attachment 695671 [details] [diff] [review]
SVGDescElement

Oh and:

>+#include "SVGDescElement.h"

I'd prefer mozilla/dom/SVGDescElement.h.  Yes, that can matter on Windows, kinda.
Comment on attachment 695672 [details] [diff] [review]
SVGTitleElement

>+++ b/browser/base/content/browser.js

Probably better, for now, to set 'hasInstanceInterface': 'nsIDOMSVGTitleElement' in Bindings.conf for SVGTitleElement.  Especially because you missed a comm-central use equivalent to the one here...

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

mozilla/dom/SVGTitleElement.h

>+++ b/content/svg/content/src/SVGTitleElement.h
>+#ifndef SVGTitleElement_h

And mozilla_dom_SVGTitleElement_h

>+  virtual JSObject* WrapNode(JSContext *aCx, JSObject *aScope, bool *aTriedToWrap);

MOZ_OVERRIDE.  I missed that on SVGDescElement, so add it there too?

r=me with those nits.
Attachment #695672 - Flags: review?(bzbarsky) → review+
Comment on attachment 695673 [details] [diff] [review]
SVGStopElement

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

mozilla/dom/SVGStopElement.h

>+// sSVGElement methods

Drop that first 's' too?

>+++ b/content/svg/content/src/SVGStopElement.h
>+#ifndef SVGStopElement_h

mozilla_dom_SVGStopElement_h

r=me
Attachment #695673 - Flags: review?(bzbarsky) → review+
Comment on attachment 695674 [details] [diff] [review]
SVGStyleElement

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

mozilla/dom/SVGStyleElement.h

>+++ b/content/svg/content/src/SVGStyleElement.h
>+#ifndef SVGStyleElement_h

mozilla_dom_SVGStyleElement_h

How about we assume those two comments for all these patches and I stop making them?

>+++ b/dom/webidl/SVGStyleElement.webidl
>+  attribute DOMString xmlspace; // Spec claims this should be on SVGElement
>+  attribute DOMString type;
>+  attribute DOMString media;
>+  attribute DOMString title;

These should all be [SetterThrows].  You can use the XPCOM getters fine, but the setters will need to look somewhat like SetHTMLAttr (the version that takes an ErrorResult).

Maybe we should hoist an equivalent of SetHTMLAttr on Element...

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

With the usual bits about the include guard and the #include in the cpp, r=me.
Attachment #695676 - Flags: review?(bzbarsky) → review+
Comment on attachment 695678 [details] [diff] [review]
SVGMPathElement

With the usual bits about the include guard and the #include in the cpp, r=me.
Attachment #695678 - Flags: review?(bzbarsky) → review+
Comment on attachment 696244 [details] [diff] [review]
SVGScriptElement

>+++ b/content/html/content/test/reflect.js

Peter has convinced me that we should do the explicit list thing here... Would you mind doing that?

It looks like the SVGScriptElement.h was created via hg add, not hg cp.  I'd prefer hg cp.

Usual comments about the include guard and #include line.

Usual comment about MOZ_OVERRIDE on WrapNode (which I think I forgot on some earlier patches).

>+++ b/dom/webidl/SVGScriptElement.webidl
>+  attribute DOMString type;
>+  attribute DOMString crossOrigin;

Both of these need to be [SetterThrows]; see comments above for SVGStyleElement.

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

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

::: dom/webidl/SVGMPathElement.webidl
@@ +10,5 @@
> + * liability, trademark and document use rules apply.
> + */
> +
> +interface SVGMPathElement : SVGElement {
> +};

Hrm, this used to be called SVGMpathElement (lower case p), no? If so, you'll probably need to add something to a test_interfaces.html
(In reply to :Ms2ger from comment #26)
> Comment on attachment 695678 [details] [diff] [review]
> SVGMPathElement
> 
> Review of attachment 695678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/SVGMPathElement.webidl
> @@ +10,5 @@
> > + * liability, trademark and document use rules apply.
> > + */
> > +
> > +interface SVGMPathElement : SVGElement {
> > +};
> 
> Hrm, this used to be called SVGMpathElement (lower case p), no? If so,
> you'll probably need to add something to a test_interfaces.html

I think XPConnect still exposes this interface as SVGMpathElement
(In reply to Boris Zbarsky (:bz) from comment #25)
> Comment on attachment 696244 [details] [diff] [review]
> SVGScriptElement
> 
> >+++ b/content/html/content/test/reflect.js
> 
> Peter has convinced me that we should do the explicit list thing here...
> Would you mind doing that?
> 

reflect.js is only used for HTML elements and the SVG script element, so I think the easiest solution may be to convert HTMLScriptElement and land it at the same time as SVGScriptElement.
(In reply to David Zbarsky (:dzbarsky) from comment #27)
> > Hrm, this used to be called SVGMpathElement (lower case p), no? If so,
> > you'll probably need to add something to a test_interfaces.html
> 
> I think XPConnect still exposes this interface as SVGMpathElement

It's SVGMPathElement in the spec, so I think it should be exposed as such.
> so I think the easiest solution may be to convert HTMLScriptElement and land it at the
> same time as SVGScriptElement.

Makes sense to me.

> It's SVGMPathElement in the spec, so I think it should be exposed as such.

Yes, that's what this patch does.  So we have Components.interfaces.nsIDOMSVGMpathElement but window.SVGMPathElement.

The failing test is testing the code that does instanceof SVGTitleElement.  I'm fairly surprised that didn't work right, since the landed patch did have the hasInstanceInterface bit in it...
Pushed SVGScriptElement
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d028fec8318
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.