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

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=5262


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #13454|review?                     |review-
               Flag|                            |




------- Comment #10 from darin at apple.com  2007-03-04 22:22 PDT -------
(From update of attachment 13454)
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.


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