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

[bugzilla-request-daemon at macosforge.org]

Lamar Goddard <lamargoddard at gmail.com> has asked  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 Lamar Goddard <lamargoddard at gmail.com>
(In reply to comment #12)
> +    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?

Yeah, I tried it with nullAtom, but it doesn't match and the "node" elem in the
test case also output the namespace.  I got the proper namespaceURI for xmlns
attributes from http://www.w3.org/TR/2006/REC-xml-names-20060816/#xmlReserved

> +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

I've made this change.

> +    // 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?

I tested without it and with using nullAtom instead of emptyAtom and both
failed so I think you're right in that the hash can't store nullAtom as a key.

> +    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.

I've added this where I thought it made sense.

> 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.

Ok, I've removed all the tabs.

> +    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?

I've made this change.

> 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!

I'm not sure how much overhead there is in copying a hash.  Each element should
have its own copy of the hash to pass on to its children after adding any
namespaces it defines.	In theory we could check startNode in WebCore::markup()
for being an element that either has a prefix/namespace or has an attribute
with a prefix/namespace not already in the hash before doing the copy.