[Webkit-unassigned] [Bug 5262] XMLSerializer drops Namespace information

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 7 16:22:52 PST 2007


lamargoddard at gmail.com changed:

           What    |Removed                     |Added
  Attachment #13484|0                           |1
        is obsolete|                            |
  Attachment #13536|                            |review?
               Flag|                            |

------- Comment #13 from lamargoddard at gmail.com  2007-03-07 16:22 PDT -------
Created an attachment (id=13536)
 --> (http://bugs.webkit.org/attachment.cgi?id=13536&action=view)
patch for fix and test case

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

Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

More information about the webkit-unassigned mailing list