367994 - Centralize SVG presentation attribute mapping

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Currently we maintain the presentation attribute mapping at each SVG element implementation, in a overridden IsAttributeMapped.  These maps have had maintenance problems, and are the inverse of the mapping tables in the specification.

Moving all the mapping to nsSVGElement allows us to use the specification directly and should make for easier updates if we ever go to SVG 1.1+.

Comment 1

[tor]

Attachment: central SVG IsAttributeMapped

Comment 2

[tor]

Attachment: remove colorProfile atom

Comment 3

[tor]

Attachment: only lookup tag once, adjust naming

Comment 4

[tor]

Attachment: update to tip

Comment 5

[tor]

Attachment: missing atom changes in diff

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 253388 [details] [diff] [review]
missing atom changes in diff

>Index: content/base/src/nsGkAtomList.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGkAtomList.h,v
>retrieving revision 3.50
>diff -u -8 -p -r3.50 nsGkAtomList.h
>--- content/base/src/nsGkAtomList.h	30 Jan 2007 13:19:53 -0000	3.50
>+++ content/base/src/nsGkAtomList.h	30 Jan 2007 21:38:55 -0000
>@@ -903,19 +903,20 @@ GK_ATOM(arithmetic, "arithmetic")
> GK_ATOM(cm, "cm")
>-GK_ATOM(colorInterpolation, "color-interpolation")
>-GK_ATOM(colorInterpolationFilters, "color-interpolation-filters")
>-GK_ATOM(colorProfile, "color-profile")
>+GK_ATOM(color_interpolation,"color-interpolation")
>+GK_ATOM(color_interpolation_filters,"color-interpolation-filters")
>+GK_ATOM(color_profile,"color-profile")
>+GK_ATOM(color_rendering,"color-rendering")

Why move to underscores rather than camel case for some atom names? From my examination of this file, camel case is slightly more prevalent, though that's not much to go on. I do see that in layout\style we're mandated to use underscores. If underscores are the way to go then shouldn't you have a separate fix and make all SVG atom names consistent than picking some out?

+static PRBool
+CheckClass(nsIAtom *aTag, nsIAtom **aElements[])

Could you make the arguments const? I think I mean nsIAtom * const * in the second case as the elements are not modified.

>+ * Centralized presentation attribute mapping for SVG.  This allows us
>+ * to use the tables from Appendex M of the SVG specification
>+ * directly, instead of trying to maintain the inverse mapping.
>+ *
>+ *   http://www.w3.org/TR/SVG11/attindex.html
>+ *
>+ * If this starts to show up in performance profiles, bits can be
>+ * switched over to using hashtables.
>+ */
>+
>+NS_IMETHODIMP_(PRBool)
>+nsSVGElement::IsAttributeMapped(const nsIAtom* name) const
>+{
>+
>+  static AttribMap presentationAttrib[] = {

Could be const too I think.

>+  static nsIAtom **elementsAll[] = {

And nsIAtom * const * here?

+  for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(presentationAttrib); i++) {
+    if (name == *(presentationAttrib[i].mAtom))
+      attribClass = presentationAttrib[i].mClass;

break once you've got a match?

+  }

Comment 7

[tor]

Attachment: add consts and break

dbaron, could you review this from a style point of view?  I'm adding all the svg presentation attributes to the internal lists, including a number which we don't implement yet.  Will the style system cope with being sent svg style rules that it doesn't know about, or should I remove them from the mapping for now?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #7)
> dbaron, could you review this from a style point of view?  I'm adding all the
> svg presentation attributes to the internal lists, including a number which we
> don't implement yet.  Will the style system cope with being sent svg style
> rules that it doesn't know about, or should I remove them from the mapping for
> now?

Sorry, what do you mean by "being sent", "don't know about", and "don't implement yet"?

Comment 9

[tor]

(In reply to comment #8)
> Sorry, what do you mean by "being sent", "don't know about", and "don't
> implement yet"?

"being sent" ~= NS_NewCSSStyleRule(..., GetOwnerDoc()->CSSLoader()->GetParserFor()->ParseProperty(...))

"doesn't know about" = property not in nsCSSPropList, nsCSSParser, nsRuleNode, nsStyleStruct

"don't implement yet" = see above, plus functionality missing in layout/svg

Comment 10

[Robert Longson [:longsonr]]

I've just found that the http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-painting-marker-03-f.html
does not show markers because nsSVGGElement::IsAttributeMapped does not have sMarkersMap in it. I could do a one line patch that but if this patch fixes it anyway as I suspect it does should we wait for it to land instead?

Comment 11

[tor]

(In reply to comment #10)
> I've just found that the
> http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-painting-marker-03-f.html
> does not show markers because nsSVGGElement::IsAttributeMapped does not have
> sMarkersMap in it. I could do a one line patch that but if this patch fixes it
> anyway as I suspect it does should we wait for it to land instead?

It seems it might take some time to get reviews on this bug, so you should probably patch missing mappings as you see them.

Comment 12

[tor]

Attachment: update to tip

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 262812 [details] [diff] [review]
update to tip

Saying that an attribute is mapped but then not doing anything with it is harmless.  For what it's worth, given that SVG doesn't implement SetMappedAttribute, what you return from IsAttributeMapped doesn't actually affect anything outside of the SVG code.  If what you're asking is whether it's ok that you're sending them through to the CSS parser -- you'll probably end up with parser errors on the error console if you do so.

As far as the performance concerns, probably the biggest way to improve the performance would be to implement SetMappedAttribute (and make the objects unique per set of attributes specified, like HTML does).  I'm also concerned (if you don't do the above) that you don't cache the fact that UpdateContentStyleRule sometimes leaves mContentStyleRule null, so you'll call it repeatedly in that case.

If you implemented SetMappedAttribute like HTML and had a unique rule for every set of attributes, saying unimplemented attributes were mapped would lead to occasional reduction in the benefits of using unique rules if the unimplemented attributes varied.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #13)
> (From update of attachment 262812 [details] [diff] [review])
> Saying that an attribute is mapped but then not doing anything with it is
> harmless.  For what it's worth, given that SVG doesn't implement
> SetMappedAttribute, what you return from IsAttributeMapped doesn't actually
> affect anything outside of the SVG code.

Er, sorry, that's not true, since nsHTMLStyleSheet::HasAttributeDependentStyle matters.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 262812 [details] [diff] [review]
update to tip

r=dbaron; I couldn't make sense of Appendix M after a few minutes of reading.  (It looks like somebody turned a DTD into a table, but I'm not quite sure how.)  If you feel you need an SVG peer to look over the correspondence with it, it might not be a bad idea (not sure if longsonr did already), but it's ok without.

Sorry I took so long to get to this.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jwatt, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 17

[Robert Longson [:longsonr]]

Attachment: Bug 367994 - Map all SVG styles to all non-animation SVG elements r=emilio

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77efe6b04864
Map all SVG styles to all non-animation SVG elements r=emilio

Comment 19

[Cosmin Sabou [:CosminS]]

Backed out changeset 77efe6b04864 (Bug 367994) for causing SVGLength animation wpt unexpected passes.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&fromchange=77efe6b04864933c82f1602976a4e03560dd3cf9&tochange=4137803e786cc85a0439795d4d02eae237d3fbc7&test_paths=svg%2Fanimations&selectedTaskRun=Yu5sUBsETAKO9vDaOC7j4A.0

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=409009707&repo=autoland
https://treeherder.mozilla.org/logviewer?job_id=409011134&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/4137803e786cc85a0439795d4d02eae237d3fbc7

Comment 20

[Robert Longson [:longsonr]]

The rect has a font-size attribute. We now map that attribute so we pass the test.

Comment 21

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=56fc65f21f15726b635a0d35f3be04799f5ff07a

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b72e8e592e81
Map all SVG styles to all non-animation SVG elements r=emilio

Comment 23

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/b72e8e592e81