[webkit-reviews] review granted: [Bug 5262] XMLSerializer drops Namespace information : [Attachment 13536] patch for fix and test case

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 5262: XMLSerializer drops Namespace information
http://bugs.webkit.org/show_bug.cgi?id=5262

Attachment 13536: patch for fix and test case
http://bugs.webkit.org/attachment.cgi?id=13536&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    // Set pre to be emptyAtom if prefix is empty so that all empty
AtomicStrings will map to the same key in the hash.

Comment should really mention null, because there are only two empty atomic
strings: null and empty.

The code below:

+    // Set pre to be emptyAtom if prefix is empty so that all empty
AtomicStrings will map to the same key in the hash.
+    const AtomicString& pre = prefix.isEmpty() ? emptyAtom : prefix;
+    AtomicString foundNS = namespaces.get(pre.impl());
+    if (foundNS.isEmpty() || (foundNS != ns)) {
+	 namespaces.set(pre.impl(), ns.impl());
+	 return " xmlns" + (!prefix.isEmpty() ? ":" + prefix : "") + "=\"" + ns
+ "\"";
+    }
+    return "";

seems more natural to me, written this way:

    if (ns.isEmpty())
	return "";
    // Use emptyAtom's impl() for both null and empty strings, since the
HashMap can't handle 0 as a key.
    AtomicStringImpl* pre = prefix.isEmpty() ? emptyAtom.impl() :
prefix.impl();
    AtomicStringImpl* foundNS = namespaces.get(pre.impl());
    if (foundNS != ns.impl()) {
	namespaces.set(pre.impl(), ns.impl());
	return " xmlns" + (!prefix.isEmpty() ? ":" + prefix : "") + "=\"" + ns
+ "\"";
    }
    return "";

Since the map is of AtomicStringImpl*, the code can work with those.

With the special case for ns, you can simplify callers of addNamespace -- they
don't need to check for the ns.isEmpty() special case.

But those changes are optional. r=me