Summary: | <tref> element not implemented | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | ||||||||||||
Priority: | P4 | ||||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2006-01-08 02:22:53 PST
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.
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.
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.
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.
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.
Created attachment 7820 [details]
Tref tests + changed test results
See description :)
Cheers,
Rob.
Comment on attachment 7807 [details]
Better patch yet
Fabulous. r=me
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. |