[webkit-reviews] review granted: [Bug 79586] xmlserializer strips xlink from xlink:html svg image tag : [Attachment 133659] Patch

Sun Mar 25 04:46:38 PDT 2012

Nikolas Zimmermann <zimmermann at kde.org> has granted Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 79586: xmlserializer strips xlink from xlink:html svg image tag
https://bugs.webkit.org/show_bug.cgi?id=79586

Attachment 133659: Patch
https://bugs.webkit.org/attachment.cgi?id=133659&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133659&action=review

Looks good, r=me, with comments:

>> Source/WebCore/svg/SVGURIReference.cpp:36
>> +	    attr->setPrefix("xlink");
> 
> We must have xlink in an AtomicString somewhere already.  It seems strange to
mutate attr here.  This function is generally about updating |this| to take
|attr| into account.

Correct, it's XLinkNames::hrefAttr.

> LayoutTests/svg/custom/xlink-prefix-in-attributes.html:1
> +<html xmlns="http://www.w3.org/1999/xhtml">

I would have constructed a regular .svg testcase for this, and inject code,
that serializes the document to a String, and put that as textContent of a
<text> element.
You can either follow my suggestion, and rework this, or keep your current test
and fix the comments below:

> LayoutTests/svg/custom/xlink-prefix-in-attributes.html:9
> +	   var root = document.getElementById("svg");

Quite confusing, I'd name this differently, this suggests its an SVGSVGElement.

how about "var target = document.getElementById("target")".

> LayoutTests/svg/custom/xlink-prefix-in-attributes.html:15
> +	   var svgEl = document.createElementNS(svgns, "svg");

s/svgEl/svgElement/ or root, or svg, but no abbreviations.

> LayoutTests/svg/custom/xlink-prefix-in-attributes.html:17
> +	   svgEl.setAttributeNS(null, "svg", svgns);

What's the purpose of this?

> LayoutTests/svg/custom/xlink-prefix-in-attributes.html:19
> +	   svgEl.setAttributeNS(null, "width", 200);

, "200"

> LayoutTests/svg/custom/xlink-prefix-in-attributes.html:22
> +	   var imgElem = document.createElementNS(svgns, "image");

s/imgElem/image/

> LayoutTests/svg/custom/xlink-prefix-in-attributes.html:23
> +	   imgElem.setAttributeNS(null, "width" ,20);

, "20" (space is misplaced).