Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Unknown SVG Elements should be SVGElement, not SVGUnknownElement

RESOLVED FIXED in mozilla22

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla22
dev-doc-complete, site-compat
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 728863 [details] [diff] [review]
Part 1: Move nsIDOMSVGElement up to nsSVGElement
Attachment #728863 - Flags: review?(Ms2ger)
(Assignee)

Comment 2

4 years ago
Created attachment 728866 [details] [diff] [review]
Remove nsSVGUnknownElement
Attachment #728866 - Flags: review?(Ms2ger)
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
Attachment #728863 - Flags: review?(Ms2ger) → review+
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.
Attachment #728866 - Flags: review?(Ms2ger) → review+
(Assignee)

Comment 5

4 years ago
(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

4 years ago
With these patches, can you not nix the |'hasXPConnectImpls': True,| for SVGElement in Bindings.conf and the SVGElement bits in dom_quickstubs.qsconf?
(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.
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a86211079c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/889cee343aea
https://hg.mozilla.org/mozilla-central/rev/4a86211079c7
https://hg.mozilla.org/mozilla-central/rev/889cee343aea
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(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

4 years ago
Hmm.  Those are both abstract classes.  Presumably they should "override" Clone but with a pure virtual?
Override and must-override both, from the sounds of it.
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.
Attachment #731279 - Flags: review?(bzbarsky)

Comment 14

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

r=me
Attachment #731279 - Flags: review?(bzbarsky) → review+
Static-checking bustage fix pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62face029ee0
https://hg.mozilla.org/mozilla-central/rev/62face029ee0
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
Keywords: dev-doc-complete, site-compat
https://svgwg.org/svg2-draft/struct.html#InterfaceSVGUnknownElement defines the SVGUnknownElement interface. So I assume it should be added back, right?

Sebastian
Flags: needinfo?(dzbarsky)

Comment 19

2 years ago
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.
See Also: → bug 1239218
(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
Flags: needinfo?(dzbarsky)
You need to log in before you can comment on or make changes to this bug.