[webkit-reviews] review denied: [Bug 6638] Support Mozilla's XPathEvaluator object. : [Attachment 8124] New iteration

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 6638: Support Mozilla's XPathEvaluator object.
http://bugzilla.opendarwin.org/show_bug.cgi?id=6638

Attachment 8124: New iteration
http://bugzilla.opendarwin.org/attachment.cgi?id=8124&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Many cases of * not next to the type. It should be Node*, not Node *.

+  } elsif ($type eq "XPathEvaluator") {
+      return 0;
+  } elsif ($type eq "XPathNSResolver") {
+      return 0;
+  } elsif ($type eq "XPathResult") {
+      return 0;

+  } elsif ($type eq "XPathNSResolver" or
+	    $type eq "XPathResult") {
+    $implIncludes{"JS$type.h"} = 1;
+    return "to$type($value)";

+  } elsif ($type eq "XPathExpression" or
+	    $type eq "XPathNSResolver" or
+	    $type eq "XPathResult") {
+    $implIncludes{"JS$type.h"} = 1;
+    return "toJS(exec, $value)";

Why are these all separate and not sharing code with other cases?

+	    $type eq "float" or 
+	    $type eq "double") {

+	 readonly attribute double	    numberValue

Is double really allowed in IDL? Doesn't float in IDL mean double?

+    virtual JSObject* construct(ExecState* exec, const List &args) { return
static_cast<JSObject *>(toJS(exec, new $interfaceName)); }

Why the static_cast here?

+    int nameIndex = code - INVALID_EXPRESSION_ERR;

This can underflow if the code is a large negative number. Should write this so
it works with any value.

-  const char* name = code < nameTableSize ? nameTable[code] : 0;
+  if (!name)
+      name = code < nameTableSize ? nameTable[code] : 0;

I don't like the way this works. If we need a separate nameIndex then lets do
that in all cases. The use of a null name as a special flag does not seem like
a good way to do things.

+	 case Node::XPATH_NAMESPACE_NODE:
+	     // FIXME: Create an XPath objective C wrapper
+	     return nil;

I'd like to see a bug number here.

+    $(WebCore)/xpath

Missing trailing \ character.

+    PassRefPtr<XPathExpression> createExpression(const String& expression,
+						  XPathNSResolver* resolver,
+						  ExceptionCode& ec);

Should leave out names like "resolver" and "ec" that are completely redundant
with the type name. It's our preferred style and I think it makes things easier
to read (especially for "ec").

+Element *XPathNamespace::ownerElement() const
+{
+    // FIXME
+    return 0;
+}

Need more detail here. Also, is there a reason we can't fix this?

+#include "impl/expression.h"
+#include "impl/util.h"

XCode, at least, uses a flat namespace for all headers. So short names like
expression.h and util.h are not really compatible; directories don't really
provide a namespace. Our filenames will need to include the prefix XPath.

+PassRefPtr<XPathExpression> XPathExpression::createExpression(const String&
expression, XPathNSResolver* resolver, ExceptionCode& ec)
+{
+    XPathExpression* expr = new XPathExpression;
+    
+    Expression::evaluationContext().resolver = resolver;
+    if (!expr->m_statement.parse(expression.deprecatedString(), ec)) {
+	 delete expr;
+	 return 0;
+    }
+    
+    return expr;
+}

Needs to use a RefPtr for the expr local variable. It's bad form to call
functions with an expr object floating like this, and it's bad form to do a
delete on a shared object.

+    XPathResult* result = new
XPathResult(static_cast<EventTargetNode*>(eventTarget),
+					   m_statement.evaluate(contextNode));
+    
+    if (type != XPathResult::ANY_TYPE) {
+	 result->convertTo(type, ec);
+
+	 if (ec)
+	     return 0;
+    }
+
+    return result;

The result here should be a RefPtr to avoid a storage leak in the case where
convertTo fails.

+    // Apparently Node::lookupNamespaceURI doesn't do this.
+    if (prefix == "xml")
+	 return "http://www.w3.org/XML/1998/namespace";

Is this a bug in lookupNamespaceURI or not? I don't like leaving the comment
like this. I'd like to be clearer on whether this is correct or not.

m_node in XPathNSResolver needs to be a RefPtr.

I think NodeSet is an awful name for a Vector of node pointers!

Expression::optimize overwrites m_constantValue without deleting the old value.
Is it guaranteed to only be called once?

+    explicit Value(Node *value);
+    explicit Value(const NodeSet& value);
+    explicit Value(bool value);
+    explicit Value(double value);
+    explicit Value(const String& value);

Why does the Value constructor need to be marked explicit? It doesn't seem
harmful to have an implicit conversion to a Value, and we could remove tons of
explicit calls to the Value constructor.

+	     if (rightVal == 0.0 || rightVal == -0.0)

The way floating point works, both of these expressions are the same. Negative
0 and positive 0 return the same thing. But also, why have this check? Floating
point correctly produces infinities and NaN's as needed for cases like this.
THere's no need for our code to do a redunddant check.

In NumericOp::doEvaluate:

+	 default:
+	     assert(0);
+	 return Value();

If you leave out the default case from the switch statement, gcc will warn if
one of the enum cases is not handled. So you should do that.

+    Parser(const Parser &rhs); 		 // disabled
+    Parser &operator=(const Parser &rhs);	 // disabled

Preferred style for this is to inherit from Noncopyable.

Can Value go in its own header file, instead of with Expression?

+	 if (a.isNodeset())
+	     node = a.toNodeSet()[0].get();

If there are no nodes in the set, then this will crash.

Patch doesn't include the NodeSet class!

+    return Value(node->localName().deprecatedString());

Value now takes a String, so this is going to convert to DeprecatedString and
then back!

+    // extract 'en' out of 'en-us'
+    langNodeValue = langNodeValue.left(langNodeValue.find('-'));

What does this do when langNodeValue doesn't have a "-".

+	 LOG(XPath, "sum() expects <nodeset>\n");

Don't use "\n" in LOG, since it already terminates lines. This is in all the
LOG statements.

+    const double num = arg(0)->evaluate().toNumber();
+
+    if (isnan(num) || isinf(num)) {
+	 return Value(num);
+    }
+
+    return Value(floor(num));

Excessive use of const on local variable here and elsewhere.

What's the point of checking for nan and inf before calling floor? It already
handles those cases properly.

+    return Value(double(round(arg(0)->evaluate().toNumber())));

What's the point of the cast to double here? THe round function returns a
double.

+    static FunctionMapping functions[] = {

Need to be static const, not just static.

+    static const unsigned int numFunctions = sizeof(functions) /
sizeof(functions[ 0 ]);

No need for static here. I believe with static we get a global variable rather
than a compile time constant.

Since we're using objects that are allocated and returned, I think we need to
be very careful and come up with a calling convention that makes it clear that
objects are allocated.

+Function *FunctionLibrary::getFunction(const char *name, const
Vector<Expression*> &args) const

There's nothing about the name of this that makes it clear to me that it
allocates its results and you are responsible for deleting it. Should talk to
Maciej about the memory allocation strategy.

+	     Node *n = context->firstChild();
+	     while (n) {
+		 nodes.append(n);
+		 n = n->nextSibling();
+	     }

+	     Node *n = context->parentNode();
+	     while (n) {
+		 nodes.append(n);
+		 n = n->parentNode();
+	     }
+	     return nodes;

+	     Node *n = context->nextSibling();
+	     while (n) {
+		 nodes.append(n);
+		 n = n->nextSibling();
+	     }

I prefer for statements for this sort of thing. Also, with a for statement you
would not need the outer braces.

+	 default:
+	     LOG(XPath, "Unknown axis %s passed to Step::nodesInAxis",
axisAsString(m_axis).ascii());

Same comment about default here as elsewhere.

+/* @return all ancestor nodes of the given node, in document order.
+ */
+NodeSet getChildrenRecursively(Node *node);

This function is a bad idea. There's no need to use a recursive algorithm to
build the list of all the children in order because we have enough pointers to
walk the tree without that. Instead clients can write a simple loop using
traverseNextNode and avoid building a NodeSet entirely. Also, it says "all
ancestor nodes" but I assume it means "all desecendant nodes".

+bool isValidContextNode(WebCore::Node *node);

Need to eliminate excess WebCore:: prefixes like this one.

XPathGrammar.y needs a strategy to delete the nodes if there's a parse failure.
You can't just use new inside a Yacc grammar. See the JavaScript grammar and
the CSS grammar for examples of how to do this.

Don't use cell() on QChar. Instead use unicode(), which is more efficient.

There are a lot of return statements with else statements after them. I think
it's better style not to do that.

+>>>>>>> .r14202

Oops! Please remove that.