Closed Bug 824229 Opened 8 years ago Closed 8 years ago

Convert SVGGraphicsElement to WebIDL bindings

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(6 files, 2 obsolete files)

No description provided.
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.
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.
Summary: Conver SVGGraphicsElement to WebIDL bindings → Convert SVGGraphicsElement to WebIDL bindings
I checked our implementation, and SVGTests ends up being exposed on exactly the same elements.
Attachment #695281 - Flags: review?(bzbarsky)
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.
(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.
Attachment #695281 - Attachment is obsolete: true
Attachment #695281 - Flags: review?(bzbarsky)
FWIW marker elements are the great standout from DOMSVGTests, they don't support it.
SVG2 claims marker elements do support SVGTests.  Cameron, is this correct?
If marker elements are going to support DOMSVGTests then maybe you should move make it part of nsSVGElement.
Will title, desc and other non-graphical elements support DOMSVGTests?
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.
Cameron, if SVGDefinitonElement doesn't implement SVGTests, is there any point in it existing?
Probably not, then; I've mailed www-svg mentioning that too now.
Attachment #695941 - Flags: review?(bzbarsky)
Attachment #695942 - Flags: review?(bzbarsky)
Attachment #695943 - Flags: review?(bzbarsky)
Blocks: 825147
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.
Attachment #695125 - Flags: review?(bzbarsky) → review?(longsonr)
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....
Attachment #695941 - Flags: review?(bzbarsky) → review?(longsonr)
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 on attachment 695943 [details] [diff] [review]
Part 5: SVGGraphicsElement

Similar questions as for SVGTransformableElement.
Attachment #695943 - Flags: review?(bzbarsky) → review?(longsonr)
> Similar questions as for SVGTransformableElement.

Er, as SVGLocatableElement.
Attachment #695942 - Flags: review?(bzbarsky) → review?(longsonr)
(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 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.
Attachment #695944 - Flags: review?(bzbarsky) → review?(longsonr)
Attachment #695944 - Flags: review+
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"
>
Attachment #695944 - Flags: review?(longsonr) → review+
(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 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.
Attachment #695941 - Flags: review?(longsonr) → review+
(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.
Attachment #695942 - Flags: review?(longsonr) → review+
Comment on attachment 695943 [details] [diff] [review]
Part 5: SVGGraphicsElement

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

wrong order.
Attachment #695943 - Flags: review?(longsonr) → review+
Attachment #695125 - Flags: review?(longsonr) → review+
(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.
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?
(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.
(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.
I kept it on all the definition elements.  We can change that when the spec is fixed.
Attachment #696449 - Flags: review?(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?
I filed bug 825411 for our wrong behaviour.
Blocks: 825732
I removed SVGTests from mask, clipPath, and pattern.
Attachment #696449 - Attachment is obsolete: true
Attachment #696449 - Flags: review?(longsonr)
Attachment #697829 - Flags: review?(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.
Attachment #697829 - Flags: review?(longsonr) → review-
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.
(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.
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.
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 on attachment 697829 [details] [diff] [review]
Part 2: Move nsIDOMSVGTests up to SVGGraphicsElement

r=longsonr with comments addressed
Attachment #697829 - Flags: review- → review+
(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.
(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.
We didn't want clipPath to implement SVGTests. We can fix it in bug 826858 though.
It doesn't.  I moved everything _except_ SVGTests out of SVGGraphicsElement.
Ahh, I misread the comment. That's just right then :-)
Depends on: 837450
Depends on: 861188
You need to log in before you can comment on or make changes to this bug.