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

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied 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 13484: patch for fix and test case
http://bugs.webkit.org/attachment.cgi?id=13484&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks! This looks really good.

+    static const QualifiedName xmlnsAttr(nullAtom, "xmlns", xmlnsURI);
+    if (attr->name() == xmlnsAttr) {

Is it right that the QualifiedName for the "xmlns" attribute has a namespace?
I'd expect nullAtom for the namespace. Is there a test case that verifies that
this line of code is working properly?

+static bool shouldAddNamespaceAttr(const Attribute* attr,
HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces)
+static String addNamespace(const AtomicString& prefix, const AtomicString& ns,
HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces)
+static String addNamespaceElem(const Element *elem, HashMap<AtomicStringImpl*,
AtomicStringImpl*>* namespaces)
+static String addNamespaceAttr(const Attribute *attr,
HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces)

I still think that you should pass a reference here rather than a pointer (as I
suggested last time I reviewed a version of this patch). I see how it's
consistent to use a * since the namespaces hash is optional at the outermost
level

+    // Set pre to be emptyAtom if prefix is empty so that all empty
AtomicStrings will map to the same key in the hash.
+    AtomicString pre = prefix.isEmpty() ? emptyAtom : prefix;

I believe the behavior you're talking about here is already guaranteed by
AtomicString; the whole point of the class is that it merges all equal strings
into a single key. The only exception to this rule might be due to the
difference between "null" and "empty", so the code above might be useful if
you're trying to make the nullAtom be equal to the emptyAtom. Or maybe the hash
table can't handle nullAtom as a key? Did you test without this? Are you sure
you need that line of code?

+    AtomicString ns = elem->namespaceURI();

For lines of code like this one, you can actually make them slightly more
efficient and not do as much reference count churning if you make the local
variable "const AtomicString&". There are three caveats:

    1) you can't modify the value of the variable in that case
    2) you need to make sure that you do this only for functions that actually
return const AtomicString&, like namespaceURI() and getAttribute() and the like

    3) you have to be sure not to use the value if the object you got the
string from has been deleted

Anyway, not an important optimization but worth mentioning; I would have used
it where possible.

This patch has some tabs in it. We can't check a patch into Subversion in if it
has tab characters, so it makes work for whoever is landing your patch if you
use them. Please search for tab characters and make sure you're not using any
before posting your next patch.

+    if (namespaces)
+	 namespaceHash = *namespaces;

Since the markup function does not modify the passed-in namespaces hash, then I
think the namespaces parameter should be a const* parameter instead of a plain
old * parameter?

But I'm worried now that we're going to be doing a lot of hash table copying.
Is there any way to avoid that overhead while still keeping the same semantic.
I know, it's my fault for suggesting we use a hash table in the first place!

Everything looks good here.

I'm going to say review- just because of the tab characters, and also because
there are enough comments that you might want to make some revisions. However,
I don't think any of my comments besides the tab character one are mandatory
changes.