Last Comment Bug 840417 - Unknown SVG Elements should be SVGElement, not SVGUnknownElement
: Unknown SVG Elements should be SVGElement, not SVGUnknownElement
Status: RESOLVED FIXED
: dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: David Zbarsky (:dzbarsky)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 825282
  Show dependency treegraph
 
Reported: 2013-02-11 22:43 PST by David Zbarsky (:dzbarsky)
Modified: 2016-01-12 22:26 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Move nsIDOMSVGElement up to nsSVGElement (161.80 KB, patch)
2013-03-25 00:44 PDT, David Zbarsky (:dzbarsky)
Ms2ger: review+
Details | Diff | Splinter Review
Remove nsSVGUnknownElement (9.84 KB, patch)
2013-03-25 00:53 PDT, David Zbarsky (:dzbarsky)
Ms2ger: review+
Details | Diff | Splinter Review
Fix static checking (4.15 KB, patch)
2013-03-29 13:34 PDT, Joshua Cranmer [:jcranmer]
bzbarsky: review+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2013-02-11 22:43:22 PST

    
Comment 1 David Zbarsky (:dzbarsky) 2013-03-25 00:44:55 PDT
Created attachment 728863 [details] [diff] [review]
Part 1: Move nsIDOMSVGElement up to nsSVGElement
Comment 2 David Zbarsky (:dzbarsky) 2013-03-25 00:53:13 PDT
Created attachment 728866 [details] [diff] [review]
Remove nsSVGUnknownElement
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2013-03-25 01:30:52 PDT
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 (⌚ UTC+1/+2) 2013-03-25 01:35:51 PDT
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) 2013-03-25 10:59:28 PDT
(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 [:bz] (still a bit busy) 2013-03-27 07:31:46 PDT
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 (⌚ UTC+1/+2) 2013-03-27 09:41:12 PDT
(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 10 Joshua Cranmer [:jcranmer] 2013-03-28 22:35:39 PDT
(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 [:bz] (still a bit busy) 2013-03-28 22:38:18 PDT
Hmm.  Those are both abstract classes.  Presumably they should "override" Clone but with a pure virtual?
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2013-03-29 12:52:54 PDT
Override and must-override both, from the sounds of it.
Comment 13 Joshua Cranmer [:jcranmer] 2013-03-29 13:34:58 PDT
Created attachment 731279 [details] [diff] [review]
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 [:bz] (still a bit busy) 2013-03-29 13:44:08 PDT
Comment on attachment 731279 [details] [diff] [review]
Fix static checking

r=me
Comment 15 Joshua Cranmer [:jcranmer] 2013-03-30 19:29:10 PDT
Static-checking bustage fix pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62face029ee0
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-04-01 11:01:47 PDT
https://hg.mozilla.org/mozilla-central/rev/62face029ee0
Comment 17 Kohei Yoshino [:kohei] 2013-05-16 11:59:37 PDT
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] 2016-01-12 14:31:14 PST
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 [:bz] (still a bit busy) 2016-01-12 18:25:14 PST
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] 2016-01-12 22:26:39 PST
(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

Note You need to log in before you can comment on or make changes to this bug.