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

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Sun Mar 4 22:22:51 PST 2007

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 5262: XMLSerializer drops Namespace information

Attachment 13454: patch for fix and test case

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for tackling this.


+static bool shouldAddNamespaceElem(const Element* elem,
Vector<QualifiedName*>* namespaces)

Why the unused "namespaces" parameter?


+    const String prefix = elem->prefix();

We don't normally put const on local variables like this one. I know that some
people prefer to do this in their C++ code and there can be some modest
benefits, but I don't see any reason to do this with all the local variables in
your patch.


All the places you write:

    !!x && x.length()

you don't need to do it like that. This will do the same thing, more



This is a particularly inefficient way to check this:

+    if (attr->name().toString() == "xmlns") {

One good way to write this is:

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


+    if (attr->prefix() == "xmlns") {

Why it OK to check the prefix here and not the namespace?


+static DeprecatedString addNamespace(String prefix, const String ns,
Vector<QualifiedName*>* namespaces)

These parameters should be const String&, not String type.

The Vector should be a reference parameter, not a pointer parameter.

Please don't use DeprecatedString in the interface of new functions. This
should return a String.


+    prefix = !!prefix ? prefix : "";

Are you sure the above code is needed? I think everything would work fine
without it. Null strings and empty strings behave the same in most cases.


The * goes next to the type.

+	 QualifiedName *item = namespaces->at(i);


+static DeprecatedString startMarkup(const Node*, const Range*,
EAnnotateForInterchange, CSSMutableStyleDeclaration*, Vector<QualifiedName*>* =
+static DeprecatedString startMarkup(const Node *node, const Range *range,
EAnnotateForInterchange annotate, CSSMutableStyleDeclaration *defaultStyle,
Vector<QualifiedName*> *namespaces)

You should not have a separate declaration here. If you want to forward
declare, it goes at the top of the file. Otherwise, the definition is just
fine. Just put the = 0 after the namespaces vector.

+static DeprecatedString markup(Node*, bool, Vector<Node*>*,
Vector<QualifiedName*>* = 0);
+static DeprecatedString markup(Node* startNode, bool onlyIncludeChildren,
Vector<Node*>* nodes, Vector<QualifiedName*>* namespaces)

Same thing here.


+    Vector<QualifiedName*> my_namespaces;

We don't use underscore in our variable names, nor do we use "my" in variable

All the QualifiedName objects of the namespaces leak here. You need to do
something to delete them. A call to deleteAllValues would be one way to do it.


Are you sure the test case exercises all the code paths?

I think you're using the wrong data structure for the namespaces. It seems that
it should be a map from a prefix string to a namespace string. It's not a good
way to use QualifiedName. I'm thinking the type you want is either this:

    HashMap<AtomicString, AtomicString>

or this:

    HashMap<AtomicStringImpl*, AtomicStringImpl*>

Using a vector instead of a map is only good if there's a fixed limit on the
number of namespaces. Since there isn't, you should use a map.

More information about the webkit-reviews mailing list