824229 - Convert SVGGraphicsElement to WebIDL bindings

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: SVGSVGElement should be a subclass of SVGGraphicsElement

Comment 2

[Robert Longson [:longsonr]]

SVGSVGElement is not transformable (no NS_DECL_NSIDOMSVGTRANSFORMABLE) but SVGGraphicsElement is, that's why it is the way it is. Of course we could implement some SVGLocatable class and inherit SVGGraphics and SVGSVGELement from both.

There is a proposal to make SVGSVGElement transformable for SVG 2. Cameron would know more about the state of that.

Comment 3

[Cameron McCormack (:heycam)]

Indeed there was a plan to do this, and the spec has been updated to make SVGSVGElement have this inheritance chain:

  SVGSVGElement -> SVGGraphicsElement -> SVGTransformableElement (where the
    `transform` IDL attribute lives) -> SVGLocatableElement -> SVGElement -> Element

There are probably other parts of the spec that haven't been updated to really require <svg transform=""> to work, but that will be done at some point.


By the way, do we handle the 'transform' attribute applying to SVG elements, or do we still just have a transform="" attribute?  SVG 2 is in the process of moving towards just using the property everywhere (and with transform="" as its presentation attribute) too.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Move nsIDOMSVGTests up to SVGGraphicsElement

I checked our implementation, and SVGTests ends up being exposed on exactly the same elements.

Comment 5

[Robert Longson [:longsonr]]

nsSVGAnimateElement's base is nsSVGAnimationElement which has a base of nsSVGElement (no SVGGraphicsElement there) - fortunately bug 619469 added tests for animation elements so if you break this we'll know about it.

nsSVGFilter isn't an nsSVGGraphicsElement either, nor are gradients, masks or patterns (or <svg> elements - modulo one of your other bugs)

DOMSVGTests has a reftest in layout/reftests/svg/conditions-06.svg, but that doesn't cover filters, masks, gradients, patterns or svg elements, it would be best if you're still planning to change things around if there was a test or tests that covered the missing elements. A mochitest may be better here as most of these element's aren't directly displayable.

Comment 6

[David Zbarsky (:dzbarsky)]

(In reply to Robert Longson from comment #5)
> nsSVGAnimateElement's base is nsSVGAnimationElement which has a base of
> nsSVGElement (no SVGGraphicsElement there) - fortunately bug 619469 added
> tests for animation elements so if you break this we'll know about it.

nsSVGAnimationElement still inherits from DOMSVGTests

> nsSVGFilter isn't an nsSVGGraphicsElement either, nor are gradients, masks
> or patterns (or <svg> elements - modulo one of your other bugs)

Ah, you're right.  I'll make an nsSVGDefinitionElement class that will inherit from DOMSVGTests.

Comment 7

[Robert Longson [:longsonr]]

FWIW marker elements are the great standout from DOMSVGTests, they don't support it.

Comment 8

[David Zbarsky (:dzbarsky)]

SVG2 claims marker elements do support SVGTests.  Cameron, is this correct?

Comment 9

[Robert Longson [:longsonr]]

If marker elements are going to support DOMSVGTests then maybe you should move make it part of nsSVGElement.

Comment 10

[Robert Longson [:longsonr]]

Will title, desc and other non-graphical elements support DOMSVGTests?

Comment 11

[Cameron McCormack (:heycam)]

I think I might have made a mistake in having `SVGDefinitionElement implements SVGTests` in the IDL -- SVG 1.1 says that the conditional processing attributes have no effect on mask, clipPath and pattern (but says nothing of other definition elements).

I think we *do* want to have SVGMarkerElement implement SVGTests, however, for the case where we have a <marker> child element (new in SVG 2):

  <path d="...">
    <marker position="50%" systemLanguage="..."> ... </marker>
  </path>

So what I think we should be doing is not having SVGDefinitionElement implement SVGTests, but for individual definition elements where it makes sense (like SVGMarkerElement) implement it.  I'll raise an issue on the spec.

Comment 12

[David Zbarsky (:dzbarsky)]

Cameron, if SVGDefinitonElement doesn't implement SVGTests, is there any point in it existing?

Comment 13

[Cameron McCormack (:heycam)]

Probably not, then; I've mailed www-svg mentioning that too now.

Comment 14

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: SVGLocatableElement

Comment 15

[David Zbarsky (:dzbarsky)]

Attachment: Part 4: SVGTransformableElement

Comment 16

[David Zbarsky (:dzbarsky)]

Attachment: Part 5: SVGGraphicsElement

Comment 17

[David Zbarsky (:dzbarsky)]

Attachment: Part 6: Move SVGGraphicsElement to mozilla::dom

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 695125 [details] [diff] [review]
Part 1: SVGSVGElement should be a subclass of SVGGraphicsElement

This is really something an SVG peer would be better for.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 695941 [details] [diff] [review]
Part 3: SVGLocatableElement

And here.

In general, if it's not involving pretty binding-specific code, probably better to ask the module owners, not me....

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 695941 [details] [diff] [review]
Part 3: SVGLocatableElement

Though one comment: you should mark this 'concrete':False in a followup patch where you add something inheriting from it, assuming it is in fact not concrete.  And if it's not concrete you don't need the WrapNode override either.  Or are there actually SVG elements whose proto is SVGLocatableElement?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 695943 [details] [diff] [review]
Part 5: SVGGraphicsElement

Similar questions as for SVGTransformableElement.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Similar questions as for SVGTransformableElement.

Er, as SVGLocatableElement.

Comment 23

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #21)
> Comment on attachment 695943 [details] [diff] [review]
> Part 5: SVGGraphicsElement
> 
> Similar questions as for SVGTransformableElement.

None of these are concrete.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 695944 [details] [diff] [review]
Part 6: Move SVGGraphicsElement to mozilla::dom

>+class SVGGraphicsElement : public SVGTransformableElement,
>+                          public DOMSVGTests

Please fix the indent.

r=me in general, but the non-mechanical change in nsSVGFilterElement::IsAttributeMapped should get ok from someone who knows that code.

Comment 25

[Robert Longson [:longsonr]]

Comment on attachment 695944 [details] [diff] [review]
Part 6: Move SVGGraphicsElement to mozilla::dom


I don't see why we're losing nsSVGGraphicElementBase (although you'd want to rename it to SVGGraphicElementBase). The others that you've kept have certainly made this patch easier. I'd rather it stayed unless there's some reason to nuke it that I'm unaware of.

> 
> nsChangeHint
>-nsSVGGraphicElement::GetAttributeChangeHint(const nsIAtom* aAttribute,
>-                                            int32_t aModType) const
>+SVGGraphicsElement::GetAttributeChangeHint(const nsIAtom* aAttribute,
>+                                          int32_t aModType) const

Nit: One more space please.

> gfxMatrix
>-nsSVGGraphicElement::PrependLocalTransformsTo(const gfxMatrix &aMatrix,
>-                                              TransformTypes aWhich) const
>+SVGGraphicsElement::PrependLocalTransformsTo(const gfxMatrix &aMatrix,
>+                                            TransformTypes aWhich) const

and here.

> 
>-#ifndef __NS_SVGGRAPHICELEMENT_H__
>-#define __NS_SVGGRAPHICELEMENT_H__
>+#ifndef SVGGraphicsElement_h
>+#define SVGGraphicsElement_h

I've not seen mixed case before in the mozilla codebase, is this what we're using now all over? Elsewhere in SVG content where we've got mozilla namespaced headers (e.g. SVGLengthList) we've used MOZILLA_SVGLENGTHLIST_H__ If we're gradually converting all code is this way I don't mind, if not I'd prefer upper case with MOZILLA to start so at least we get consistency in the svg codebase.

>+class SVGGraphicsElement : public SVGTransformableElement,
>+                          public DOMSVGTests

One more space please.

> #ifndef NS_SVGAELEMENT_H_
> #define NS_SVGAELEMENT_H_
> 
> #include "Link.h"
> #include "nsIDOMSVGAElement.h"
> #include "nsIDOMSVGURIReference.h"
> #include "nsILink.h"
>-#include "nsSVGGraphicElement.h"
>+#include "SVGGraphicsElement.h"
> #include "nsSVGString.h"

Nit: Should go at the end now it begins with S

> 
> #ifndef __NS_SVGFOREIGNOBJECTELEMENT_H__
> #define __NS_SVGFOREIGNOBJECTELEMENT_H__
> 
> #include "nsIDOMSVGForeignObjectElem.h"
>-#include "nsSVGGraphicElement.h"
>+#include "SVGGraphicsElement.h"

Probably should be first as the subclass.

> #ifndef __NS_SVGMARKERELEMENT_H__
> #define __NS_SVGMARKERELEMENT_H__
> 
> #include "gfxMatrix.h"
> #include "nsIDOMSVGFitToViewBox.h"
> #include "nsIDOMSVGMarkerElement.h"
> #include "nsSVGAngle.h"
> #include "nsSVGEnum.h"
>-#include "nsSVGGraphicElement.h"
>+#include "SVGGraphicsElement.h"

Keep case insensivite alpha order please.

> #include "nsSVGLength2.h"
> #include "nsSVGViewBox.h"
> #include "SVGAnimatedPreserveAspectRatio.h"
> #include "mozilla/Attributes.h"
> 
>diff --git a/layout/svg/nsSVGPathGeometryFrame.cpp b/layout/svg/nsSVGPathGeometryFrame.cpp
>--- a/layout/svg/nsSVGPathGeometryFrame.cpp
>+++ b/layout/svg/nsSVGPathGeometryFrame.cpp
>@@ -9,17 +9,17 @@
> // Keep others in (case-insensitive) order:
> #include "gfxContext.h"
> #include "gfxPlatform.h"
> #include "gfxSVGGlyphs.h"
> #include "nsDisplayList.h"
> #include "nsGkAtoms.h"
> #include "nsRenderingContext.h"
> #include "nsSVGEffects.h"
>-#include "nsSVGGraphicElement.h"
>+#include "SVGGraphicsElement.h"

ditto.

> #include "nsSVGIntegrationUtils.h"
> #include "nsSVGMarkerFrame.h"
> #include "nsSVGPathGeometryElement.h"
> #include "nsSVGUtils.h"
> #include "SVGAnimatedTransformList.h"
> 

>--- a/layout/svg/nsSVGTextFrame.cpp
>+++ b/layout/svg/nsSVGTextFrame.cpp
>@@ -7,17 +7,17 @@
> #include "nsSVGTextFrame.h"
> 
> // Keep others in (case-insensitive) order:
> #include "nsGkAtoms.h"
> #include "nsIDOMSVGRect.h"
> #include "nsIDOMSVGTextElement.h"
> #include "nsISVGGlyphFragmentNode.h"
> #include "nsSVGGlyphFrame.h"
>-#include "nsSVGGraphicElement.h"
>+#include "SVGGraphicsElement.h"

ditto.

> #include "nsSVGIntegrationUtils.h"
> #include "nsSVGPathElement.h"
> #include "nsSVGTextPathFrame.h"
> #include "nsSVGUtils.h"
> #include "SVGLengthList.h"
>

Comment 26

[David Zbarsky (:dzbarsky)]

(In reply to Robert Longson from comment #25)
> Comment on attachment 695944 [details] [diff] [review]
> Part 6: Move SVGGraphicsElement to mozilla::dom
> 
> 
> I don't see why we're losing nsSVGGraphicElementBase (although you'd want to
> rename it to SVGGraphicElementBase). The others that you've kept have
> certainly made this patch easier. I'd rather it stayed unless there's some
> reason to nuke it that I'm unaware of.

OK, I can put it back.

> 
> I've not seen mixed case before in the mozilla codebase, is this what we're
> using now all over? Elsewhere in SVG content where we've got mozilla
> namespaced headers (e.g. SVGLengthList) we've used MOZILLA_SVGLENGTHLIST_H__
> If we're gradually converting all code is this way I don't mind, if not I'd
> prefer upper case with MOZILLA to start so at least we get consistency in
> the svg codebase.

I think this is what we're moving to for new stuff.
https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLDivElement.h#5

Comment 27

[Robert Longson [:longsonr]]

Comment on attachment 695941 [details] [diff] [review]
Part 3: SVGLocatableElement

>+
>+#include "SVGLocatableElement.h"
>+#include "DOMSVGMatrix.h"
>+#include "SVGContentUtils.h"
>+#include "nsISVGChildFrame.h"
>+#include "SVGContentUtils.h"
>+#include "nsSVGUtils.h"
>+#include "nsSVGSVGElement.h"
>+#include "nsIFrame.h"
>+#include "nsSVGRect.h"
>+#include "mozilla/dom/SVGLocatableElementBinding.h"

Could we have case insensitive order for everything except the first item please?

>+
>+#ifndef SVGLocatableElement_h
>+#define SVGLocatableElement_h

See previous review.

Comment 28

[Robert Longson [:longsonr]]

(In reply to David Zbarsky (:dzbarsky) from comment #26)
> I think this is what we're moving to for new stuff.
> https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> HTMLDivElement.h#5

OK, leave that as it is then.

Comment 29

[Robert Longson [:longsonr]]

Comment on attachment 695943 [details] [diff] [review]
Part 5: SVGGraphicsElement

> 
> #include "nsIDOMSVGTests.h"
> #include "nsStringFwd.h"
> #include "SVGStringList.h"
>+#include "nsCOMPtr.h"

wrong order.

Comment 30

[David Zbarsky (:dzbarsky)]

(In reply to Robert Longson from comment #25)
> Comment on attachment 695944 [details] [diff] [review]
> Part 6: Move SVGGraphicsElement to mozilla::dom
> 
> 
> I don't see why we're losing nsSVGGraphicElementBase (although you'd want to
> rename it to SVGGraphicElementBase). The others that you've kept have
> certainly made this patch easier. I'd rather it stayed unless there's some
> reason to nuke it that I'm unaware of.
> 

SVGTransformableElement is shorter than SVGGraphicsElementBase and I was changing those lines anyway.  Also I found a bug by removing it (nsSVGFilterElement::IsAttributeMapped wasn't calling SVGGraphicsElement::IsAttributeMapped).  If you really feel strongly about it I can put it back.

Comment 31

[David Zbarsky (:dzbarsky)]

Cameron,

We currently expose SVGTests on the following definition elements:
clipPath, filter, gradient, pattern, mask, symbol.

This seems to contradict SVG 1.1.  What do you think we should do here?

Comment 32

[Robert Longson [:longsonr]]

(In reply to David Zbarsky (:dzbarsky) from comment #30)
> 
> SVGTransformableElement is shorter than SVGGraphicsElementBase and I was
> changing those lines anyway.  Also I found a bug by removing it
> (nsSVGFilterElement::IsAttributeMapped wasn't calling
> SVGGraphicsElement::IsAttributeMapped).  If you really feel strongly about
> it I can put it back.

I'd prefer it to be put back please. If SVG 2 changes again then it will make updates simpler.

Comment 33

[Cameron McCormack (:heycam)]

(In reply to David Zbarsky (:dzbarsky) from comment #31)
> We currently expose SVGTests on the following definition elements:
> clipPath, filter, gradient, pattern, mask, symbol.
> 
> This seems to contradict SVG 1.1.  What do you think we should do here?

I just tested clipPath, filter and mask, and it seems that these currently do affect whether you can reference the elements, but they causing the referencing element not to render at all.  This differs from WebKit, where the referencing element still renders but the effect is not applied.  Opera follows the spec and applies the effect regardless of the conditional processing attribute.

I'll follow up on www-svg about whether we want to keep this behaviour in the spec.  For now, I think we should keep SVGTests implemented on these definition elements.

Comment 34

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Move nsIDOMSVGTests up to SVGGraphicsElement

I kept it on all the definition elements.  We can change that when the spec is fixed.

Comment 35

[Robert Longson [:longsonr]]

I'm think the spec will get changed not to have nsIDOMSVGTests on clipPaths etc. I don't think it makes sense to do that so I'd rather assume the spec will change and implement it as a follow up if it does not.

If you've testcases where we stop rendering because a conditional processing test fails on a clipPath or filter or mask can you raise that as a new bug please and include the testcases you have?

Comment 36

[Cameron McCormack (:heycam)]

I filed bug 825411 for our wrong behaviour.

Comment 37

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Move nsIDOMSVGTests up to SVGGraphicsElement

I removed SVGTests from mask, clipPath, and pattern.

Comment 38

[Robert Longson [:longsonr]]

Comment on attachment 697829 [details] [diff] [review]
Part 2: Move nsIDOMSVGTests up to SVGGraphicsElement

The trouble is that clipPath and filter both derive from SVGGraphicsElement but unfortunately neither should implement DOMSVGTests. That's rather a killer isn't it?

I'm afraid I'm going to have to r- this because of that.

> nsresult
> nsSVGAElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttr,
>                          bool aNotify)
> {
>-  nsresult rv = nsSVGAElementBase::UnsetAttr(aNameSpaceID, aAttr, aNotify);
>+  nsresult rv = nsSVGElement::UnsetAttr(aNameSpaceID, aAttr, aNotify);

Why?

>--- a/content/svg/content/src/nsSVGMarkerElement.cpp
>+++ b/content/svg/content/src/nsSVGMarkerElement.cpp
>@@ -257,17 +257,17 @@ nsSVGMarkerElement::UnsetAttr(int32_t aN
>                               bool aNotify)
> {
>   if (aNamespaceID == kNameSpaceID_None) {
>     if (aName == nsGkAtoms::orient) {
>       mOrientType.SetBaseValue(SVG_MARKER_ORIENT_ANGLE);
>     }
>   }
> 
>-  return nsSVGMarkerElementBase::UnsetAttr(aNamespaceID, aName, aNotify);
>+  return nsSVGElement::UnsetAttr(aNamespaceID, aName, aNotify);
> }

Ditto.

Comment 39

[Robert Longson [:longsonr]]

nsSVGFilterElement shouldn't derive from nsSVGGraphicsElement as filterElement is not transformable. (Can you fix that here or raise a follow up please?)

clipPaths are transformable, however so that's the main problem and I think that sinks this approach.

Comment 40

[David Zbarsky (:dzbarsky)]

(In reply to Robert Longson from comment #39)
> nsSVGFilterElement shouldn't derive from nsSVGGraphicsElement as
> filterElement is not transformable. (Can you fix that here or raise a follow
> up please?)
> 
> clipPaths are transformable, however so that's the main problem and I think
> that sinks this approach.

I'm going to keep our current behavior and land the rest of the patches so that I'm not blocked on this.  Filed bug 826858.

Comment 41

[Robert Longson [:longsonr]]

Part 4 and 5 of this patch set can come to our rescue here, can't they? If clipPath derives directly from SVGTransformable then we're sorted.

Comment 42

[Robert Longson [:longsonr]]

So if you fix the nits in comment 38 and make nsSVGFilterElement derive from nsSVGElement and nsSVGClipPathElement derive from SVGTransformable that gets you an r+ from me on this lot and you can land it modulo passing try.

Comment 43

[Robert Longson [:longsonr]]

Comment on attachment 697829 [details] [diff] [review]
Part 2: Move nsIDOMSVGTests up to SVGGraphicsElement

r=longsonr with comments addressed

Comment 44

[David Zbarsky (:dzbarsky)]

(In reply to Robert Longson from comment #38)
> Comment on attachment 697829 [details] [diff] [review]
> Part 2: Move nsIDOMSVGTests up to SVGGraphicsElement
> 
> The trouble is that clipPath and filter both derive from SVGGraphicsElement
> but unfortunately neither should implement DOMSVGTests. That's rather a
> killer isn't it?
> 
> I'm afraid I'm going to have to r- this because of that.
> 
> > nsresult
> > nsSVGAElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttr,
> >                          bool aNotify)
> > {
> >-  nsresult rv = nsSVGAElementBase::UnsetAttr(aNameSpaceID, aAttr, aNotify);
> >+  nsresult rv = nsSVGElement::UnsetAttr(aNameSpaceID, aAttr, aNotify);
> 
> Why?

The compiler was giving an error about UnsetAttr being ambiguous because DOMSVGTests has an UnsetAttr with a different signature.


> Ditto.

Same thing.

Comment 45

[Robert Longson [:longsonr]]

(In reply to David Zbarsky (:dzbarsky) from comment #44)
> 
> The compiler was giving an error about UnsetAttr being ambiguous because
> DOMSVGTests has an UnsetAttr with a different signature.
> 

OK, I guess I'll have to live with you changing this then.

Comment 46

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d5717bc07bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/0515cea84af1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82e982042c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9681d77595
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cfc54a3465b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b08c0ff70b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/257a5c4fd629

I ended up pushing all the functionality of SVGGraphicsElement (except for SVGTests) up to SVGTransformableElement so that nsSVGClipPathElement would inherit it.

Comment 47

[Robert Longson [:longsonr]]

We didn't want clipPath to implement SVGTests. We can fix it in bug 826858 though.

Comment 48

[David Zbarsky (:dzbarsky)]

It doesn't.  I moved everything _except_ SVGTests out of SVGGraphicsElement.

Comment 49

[Robert Longson [:longsonr]]

Ahh, I misread the comment. That's just right then :-)

Comment 50

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0d5717bc07bd
https://hg.mozilla.org/mozilla-central/rev/0515cea84af1
https://hg.mozilla.org/mozilla-central/rev/d82e982042c8
https://hg.mozilla.org/mozilla-central/rev/9c9681d77595
https://hg.mozilla.org/mozilla-central/rev/7cfc54a3465b
https://hg.mozilla.org/mozilla-central/rev/3b08c0ff70b1
https://hg.mozilla.org/mozilla-central/rev/257a5c4fd629