Bug 6427 – <tref> element not implemented

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

<tref> element not implemented
http://www.w3.org/TR/SVG/text.html#TRefElement

Should be quite simple.

[Rob Buis]

Created attachment 7757 [details]
Possible fix.

Possible fix. Only does local references. I am up for suggestions
on what test case(s) to add.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 7757 [details]
Possible fix.

A couple comments:

1.  This violates the style guidelines:
+    if (SVGURIReference::parseMappedAttribute(attr)) return;
(the return should be on its own line)

+    DeprecatedString ref = String(href()->baseVal()).deprecatedString();

is unnecessarily complicated.  If you really need a deprecatedString, I believe you can ask the AtomicString directly.

If you're going to ignore the exception you might call this variable "ignored"
+        ExceptionCode ec = 0;
unless there is already a version of createTextNode which ignores exceptions.

New code should have the * close to the type not the variable name.

Generally we try not to use closeRenderer, but rather respond to dynamic updates.  In this case we should update the text children whenever the href property changes.  Probably a call to setTextContent would work well for that.

Are you sure you should return RenderInline and not RenderSVGText?  I'd have to go stare at both to decide myself.

I think there are enough minor issues here to prevent landing.

[Rob Buis]

Created attachment 7773 [details]
Better patch

This patch addresses some of the issues. I do not really see
how to prevent the deprecatedString and I think using RenderInline
is correct.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 7773 [details]
Better patch

attach() is still the wrong way to do this.  As this won't handle dynamic updates.  I think this would be "ok" to land as is, but it's better to just fix it right the first time.

Also, ideally your patch would contain a diff of the test results you've changed, including possibly a dynamicly updating test to show that you got href changes working correctly (which is what my previous attach() complaint is about).

Other than that, this looks great.  Wonderful work as always Rob.

[Rob Buis]

Created attachment 7807 [details]
Better patch yet

This patch should fix the problem with using attach(). I think since
handling of attributes is not guarenteed, we need to check for
the referenced text both in attributeChanged and parseMappedAttribute.
I chose for a private helper method, I have no idea whether a non-member(hidden)
method is better, but I'll probably soon know.
Finally the patch lacks testcases, I just commit the patch to find out whether the
code part is correct now.
Cheers,

Rob.

[Rob Buis]

Created attachment 7820 [details]
Tref tests + changed test results

See description  :)
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 7807 [details]
Better patch yet

Fabulous.  r=me

[Eric Seidel (no email)]

I landed.  However I did not update all of the pixel results, given that I am landing from an intel machine and we have outstanding issues wrt pixel results on mac intel boxes.