[webkit-reviews] review denied: [Bug 5262] XMLSerializer drops Namespace information : [Attachment 13454] 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 13454: patch for fix and test case
http://bugs.webkit.org/attachment.cgi?id=13454&action=edit

------- 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
efficiently:

    !x.isEmpty()

--------

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.
Vector<QualifiedName*>&.

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*>* =
0);
+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
names.

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.