[webkit-reviews] review granted: [Bug 66423] libxml2 fragment parser loses prefix namespaces : [Attachment 105818] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 31 13:11:09 PDT 2011


Eric Seidel <eric at webkit.org> has granted Jeffrey Pfau <jeffrey at endrift.com>'s
request for review:
Bug 66423: libxml2 fragment parser loses prefix namespaces
https://bugs.webkit.org/show_bug.cgi?id=66423

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105818&action=review


The patch looks great.	I would perfer you landed an updated test if you have a
minute.

> LayoutTests/ChangeLog:1
> +2011-08-31  Jeffrey Pfau  <jeffrey at endrift.com>

apple.com?

> LayoutTests/fast/parser/innerhtml-with-prefixed-elements.xhtml:10
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +	<head>
> +		<title>Namespace chaining</title>
> +	</head>
> +	<body>
> +		<div id="d" xmlns:svg="http://www.w3.org/2000/svg">
> +			TEST FAILED
> +		</div>
> +		<script type="text/javascript"><![CDATA[
> +			var div = document.getElementById("d");

I think normally we use 4 space indent.

> LayoutTests/fast/parser/innerhtml-with-prefixed-elements.xhtml:11
> +			div.innerHTML = "<svg:svg width='20'
height='20'><svg:defs id='defs'><svg:text>TEST
FAILED</svg:text></svg:defs><svg:circle cx='10' cy='10' r='4'
id='c'/></svg:svg>";

Would be much better to use a 100x100 green square to show success (it's an old
convention from the w3c CSS working group test suite).

> Source/WebCore/ChangeLog:1
> +2011-08-31  Jeffrey Pfau  <jeffrey at endrift.com>

apple.com?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:614
> -    for (Element* element = elemStack.last(); !elemStack.isEmpty();
elemStack.removeLast()) {
> +    for (; !elemStack.isEmpty(); elemStack.removeLast()) {
> +	   Element* element = elemStack.last();

Wow.  I wonder how many other bad for loops like this we have.	Some day c++
may have a foreach. :)


More information about the webkit-reviews mailing list