[webkit-reviews] review requested: [Bug 6638] Support Mozilla's
XPathEvaluator object. : [Attachment 8163] Address Darin's comments
bugzilla-request-daemon at opendarwin.org
bugzilla-request-daemon at opendarwin.org
Mon May 8 01:43:28 PDT 2006
Anders Carlsson <andersca at mac.com> has asked for review:
Bug 6638: Support Mozilla's XPathEvaluator object.
http://bugzilla.opendarwin.org/show_bug.cgi?id=6638
Attachment 8163: Address Darin's comments
http://bugzilla.opendarwin.org/attachment.cgi?id=8163&action=edit
------- Additional Comments from Anders Carlsson <andersca at mac.com>
(In reply to comment #15)
> (From update of attachment 8124 [edit])
> Many cases of * not next to the type. It should be Node*, not Node *.
>
I've (hopefully) fixed all of these.
> + } 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?
I've fixed this.
>
> + $type eq "float" or
> + $type eq "double") {
>
> + readonly attribute double numberValue
>
> Is double really allowed in IDL? Doesn't float in IDL mean double?
As Maciej said, double is allowed and the IDL for XPathResult uses double.
>
> + virtual JSObject* construct(ExecState* exec, const List &args) { return
> static_cast<JSObject *>(toJS(exec, new $interfaceName)); }
>
> Why the static_cast here?
because toJS returns a JSValue and construct takes a returns a JSObject.
>
> + 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.
Won't this be handled by the
} else if (code >= XPathExceptionOffset && code <= XPathExceptionMax) {
anyway?
>
> - 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.
>
Fixed
> + case Node::XPATH_NAMESPACE_NODE:
> + // FIXME: Create an XPath objective C wrapper
> + return nil;
>
> I'd like to see a bug number here.
Fixed
>
> + $(WebCore)/xpath
>
> Missing trailing \ character.
Fixed
>
> + 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").
Fixed (hopefully for all cases)
>
> +Element *XPathNamespace::ownerElement() const
> +{
> + // FIXME
> + return 0;
> +}
>
> Need more detail here. Also, is there a reason we can't fix this?
>
XPathNamespace isn't used currently (we don't support returning namespace
nodes), but I went ahead and fixed this to work according to the spec anyway.
> +#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.
>
I've renamed all the files in the impl directory.
> +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.
>
Fixed
> + 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.
Fixed
>
> + // 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.
It's not a bug. Node::lookupNamespaceURI shouldn't special-case the xml prefix.
The XPath spec says that XPathNSResolver::lookupNamespaceURI should though.
I've clarified this in the comment.
>
> m_node in XPathNSResolver needs to be a RefPtr.
Fixed.
>
> I think NodeSet is an awful name for a Vector of node pointers!
The name in the XPath spec is NodeSet, but I've changed this to NodeVector
anyway.
>
> Expression::optimize overwrites m_constantValue without deleting the old
value.
> Is it guaranteed to only be called once?
Turns out it wasn't called at all! I've fixed this to have it be called once
and I added an ASSERT to make sure this is the case.
>
> + 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.
Fixed.
>
> + 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.
Right, I've removed that.
>
> 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.
Fixed.
>
> + Parser(const Parser &rhs); // disabled
> + Parser &operator=(const Parser &rhs); // disabled
>
> Preferred style for this is to inherit from Noncopyable.
Fixed.
>
> Can Value go in its own header file, instead of with Expression?
>
Sure, fixed.
> + if (a.isNodeset())
> + node = a.toNodeSet()[0].get();
>
> If there are no nodes in the set, then this will crash.
Fixed.
>
> 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!
Fixed.
>
> + // extract 'en' out of 'en-us'
> + langNodeValue = langNodeValue.left(langNodeValue.find('-'));
>
> What does this do when langNodeValue doesn't have a "-".
Whoops. Fixed to make it work like the spec says.
>
> + LOG(XPath, "sum() expects <nodeset>\n");
>
> Don't use "\n" in LOG, since it already terminates lines. This is in all the
> LOG statements.
Fixed.
>
> + 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.
>
Fixed.
> What's the point of checking for nan and inf before calling floor? It already
> handles those cases properly.
Fixed.
>
> + return Value(double(round(arg(0)->evaluate().toNumber())));
>
> What's the point of the cast to double here? THe round function returns a
> double.
Fixed.
>
> + 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.
Fixed.
>
> 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.
>
I've renamed this to createFunction.
> + 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.
Fixed.
>
> + default:
> + LOG(XPath, "Unknown axis %s passed to Step::nodesInAxis",
> axisAsString(m_axis).ascii());
>
> Same comment about default here as elsewhere.
Fixed.
>
> +/* @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".
>
I've removed this function.
> +bool isValidContextNode(WebCore::Node *node);
>
> Need to eliminate excess WebCore:: prefixes like this one.
>
Fixed.
> 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.
I've fixed this by keeping around sets of the different object types allocated
by the parser. When the parse fails all the objects in the sets are delete.
It's a bit kludgy but it works.
>
> Don't use cell() on QChar. Instead use unicode(), which is more efficient.
>
Fixed.
> There are a lot of return statements with else statements after them. I think
> it's better style not to do that.
>
Fixed.
> +>>>>>>> .r14202
>
> Oops! Please remove that.
>
Fixed.
Phew. Thanks for the thorough review!
More information about the webkit-reviews
mailing list