840417 - Unknown SVG Elements should be SVGElement, not SVGUnknownElement

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Move nsIDOMSVGElement up to nsSVGElement

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Remove nsSVGUnknownElement

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 728863 [details] [diff] [review]
Part 1: Move nsIDOMSVGElement up to nsSVGElement

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

\o/

::: content/svg/content/src/DOMSVGStringList.cpp
@@ +185,5 @@
>  DOMSVGStringList::InternalList()
>  {
>    if (mIsConditionalProcessingAttribute) {
> +    nsCOMPtr<dom::SVGTests> tests =
> +      do_QueryInterface(static_cast<nsIDOMSVGElement*>(mElement));

do_QueryObject should let you remove the cast, AIUI.

::: content/svg/content/src/SVGAElement.h
@@ +29,5 @@
>                                         already_AddRefed<nsINodeInfo> aNodeInfo));
>    virtual JSObject* WrapNode(JSContext *cx, JSObject *scope) MOZ_OVERRIDE;
>  
>  public:
>    // interfaces:

Nit: you removed this comment elsewhere.

::: content/svg/content/src/SVGAltGlyphElement.h
@@ +29,2 @@
>  
>    NS_DECL_ISUPPORTS_INHERITED

I wonder if we can remove this too (for classes with a single superclass).

::: content/svg/content/src/SVGClipPathElement.h
@@ +24,1 @@
>                                       public nsIDOMSVGUnitTypes

Are you planning to get rid of nsIDOMSVGUnitTypes?

::: content/svg/content/src/nsSVGElement.cpp
@@ +221,2 @@
>  // provided by Element:
>  //  NS_INTERFACE_MAP_ENTRY(nsIContent)

Kill this comment, please.

@@ +536,5 @@
>      }
>  
>      if (!foundMatch) {
>        // Check for conditional processing attributes
> +      nsCOMPtr<SVGTests> tests(do_QueryInterface(static_cast<nsIDOMSVGElement*>(this)));

nsCOMPtr<SVGTests> tests = do_QueryObject(this);

and a number of times below

@@ +1124,5 @@
>  NS_IMETHODIMP
>  nsSVGElement::GetViewportElement(nsIDOMSVGElement * *aViewportElement)
>  {
>    nsSVGElement* elem = GetViewportElement();
> +  nsCOMPtr<nsIDOMSVGElement> svgElem = static_cast<nsIDOMSVGElement*>(elem);

You shouldn't need this cast, I think

::: content/svg/content/src/nsSVGElement.h
@@ +293,5 @@
>    virtual nsIAtom* GetTransformListAttrName() const {
>      return nullptr;
>    }
>  
> +  virtual nsIDOMNode* AsDOMNode() { return this; }

Maybe add MOZ_FINAL

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 728866 [details] [diff] [review]
Remove nsSVGUnknownElement

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

::: content/svg/content/src/nsSVGElement.cpp
@@ +56,5 @@
>  //   nsSVGElement::GetAnimated{Length,Number,Integer}Values
>  // See bug 547964 for details:
>  PR_STATIC_ASSERT(sizeof(void*) == sizeof(nullptr));
>  
> +NS_IMPL_NS_NEW_SVG_ELEMENT()

I think this is going to cause warnings. Please unroll the macro?

::: content/svg/content/src/nsSVGElement.h
@@ +73,5 @@
>    virtual ~nsSVGElement(){}
>  
>  public:
> +
> +  virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;

Please add MOZ_MUST_OVERRIDE after the const here.

::: dom/base/nsDOMClassInfo.cpp
@@ +807,5 @@
>    // SVG document
>    NS_DEFINE_CLASSINFO_DATA(SVGDocument, nsDocumentSH,
>                             DOCUMENT_SCRIPTABLE_FLAGS)
>  
>    // SVG element classes

No more.

@@ -2201,5 @@
> -
> -  // XXX - the proto chain stuff is sort of hackish, because of the MI in
> -  // the SVG interfaces. I doubt that extending the proto on one interface
> -  // works properly on an element which inherits off multiple interfaces.
> -  // Tough luck. - bbaetz

Do we have badges for removing XXX comments that are over a decade old?

@@ +2195,5 @@
>      DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGDocument)
>      DOM_CLASSINFO_DOCUMENT_MAP_ENTRIES
>    DOM_CLASSINFO_MAP_END
>  
>    // SVG element classes

Not here either.

::: dom/base/nsDOMClassInfoClasses.h
@@ +119,5 @@
>  
>  // The SVG document
>  DOMCI_CLASS(SVGDocument)
>  
>  // SVG element classes

Or here.

Comment 5

[David Zbarsky (:dzbarsky)]

(In reply to :Ms2ger from comment #3)
> Comment on attachment 728863 [details] [diff] [review]
> Part 1: Move nsIDOMSVGElement up to nsSVGElement
> 
> Review of attachment 728863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> 
> I wonder if we can remove this too (for classes with a single superclass).
> 

Probably.  I actually did that originally, but it crashed on startup so I wanted to land this first.

Comment 6

[Boris Zbarsky [:bzbarsky]]

With these patches, can you not nix the |'hasXPConnectImpls': True,| for SVGElement in Bindings.conf and the SVGElement bits in dom_quickstubs.qsconf?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> With these patches, can you not nix the |'hasXPConnectImpls': True,| for
> SVGElement in Bindings.conf and the SVGElement bits in dom_quickstubs.qsconf?

Bug 854629.

Comment 8

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a86211079c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/889cee343aea

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a86211079c7
https://hg.mozilla.org/mozilla-central/rev/889cee343aea

Comment 10

[Joshua Cranmer [:jcranmer]]

(In reply to :Ms2ger from comment #4)
> ::: content/svg/content/src/nsSVGElement.h
> @@ +73,5 @@
> >    virtual ~nsSVGElement(){}
> >  
> >  public:
> > +
> > +  virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;
> 
> Please add MOZ_MUST_OVERRIDE after the const here.

I would like to personally thank you for breaking static checking builds just 5 days after I enabled them. At least SVGTransformableElement and SVGAnimationElement do not implement this method, so other this method doesn't satisfy MOZ_MUST_OVERRIDE or you have a bug on your ends.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm.  Those are both abstract classes.  Presumably they should "override" Clone but with a pure virtual?

Comment 12

[Jeff Walden [:Waldo]]

Override and must-override both, from the sounds of it.

Comment 13

[Joshua Cranmer [:jcranmer]]

Attachment: Fix static checking

This adds pure virtual MOZ_OVERRIDE methods to all the abstract classes (there are more than just those two) which satisfies the warning.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731279 [details] [diff] [review]
Fix static checking

r=me

Comment 15

[Joshua Cranmer [:jcranmer]]

Static-checking bustage fix pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62face029ee0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62face029ee0

Comment 17

[Kohei Yoshino [:kohei]]

I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

Comment 18

[Sebastian Zartner [:sebo]]

https://svgwg.org/svg2-draft/struct.html#InterfaceSVGUnknownElement defines the SVGUnknownElement interface. So I assume it should be added back, right?

Sebastian

Comment 19

[Boris Zbarsky [:bzbarsky]]

We could add the interface, I guess; we don't need the actual concrete class necessarily.

A bigger problem is that it's supposed to inherit from SVGGraphicsElement, but it's not clear why and how, since those elements don't get rendered, iirc, so having stuff like transforms and bounding boxes on them doesn't make sense.

Comment 20

[Sebastian Zartner [:sebo]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> A bigger problem is that it's supposed to inherit from SVGGraphicsElement,
> but it's not clear why and how, since those elements don't get rendered,
> iirc, so having stuff like transforms and bounding boxes on them doesn't
> make sense.

The reason for this is described in annotation 2 here:
https://svgwg.org/svg2-draft/struct.html#UnknownElement

That is:
"To allow fallbacks without the use of ‘switch’, and to align with the behavior of unknown elements in HTML."

I've created bug 1239218 to cover the reimplementation.

Sebastian