[webkit-reviews] review denied: [Bug 16833] Acid3 expects createElementNS to throw exception for localname = ":div" : [Attachment 18416] full patch, including test cases

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 13 09:04:15 PST 2008

Darin Adler <darin at apple.com> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 16833: Acid3 expects createElementNS to throw exception for localname =

Attachment 18416: full patch, including test cases

------- Additional Comments from Darin Adler <darin at apple.com>
Looks like great work!

 3 function quoteString(s) {

Please format JavaScript code so that function braces are on a new line, as in
our C code.

 36	shouldThrowWithCode("document.createAttributeNS(" +
quoteString(namespace) + ", " + quoteString(name) + ")", code);

Can you use the standard shouldThrow instead of writing your own?

 39 // We use numbers instead of names, because it's slightly easier to read

I think the names are clearer. I found the numbers confusing when looking at
the test output. If you find the long qualified names and full exception
strings too wordy maybe you could still write your own harness, but make it so
only the short part of the names show up: "NAMESPACE_ERR". That's definitely
clearer than "5".

 49 shouldBeEqualToString("document.createElementNS(null,
undefined).toString()", "[object Element]");

I normally just use shouldBe here and put two sets of quotes around the
expected string. Is this style better?

 703	 // FIXME: We define these more than once in WebCore, we could use an
XMLNSNames.cpp someday

While this is true, I personally don't think it's helpful to have a FIXME for

Also, I don't think we want to add a file named XMLNSNames.cpp. I'd suggest we
add these XML namespace constants to XMLNames.h.

 704	 static const AtomicString xmlnsURI = "http://www.w3.org/2000/xmlns/";

I think it's confusing to have this named xmlnsURL and another one named
xmlNamespaceURI meaning something different. The new one should probably be

 701 static bool checkForNamespaceError(const QualifiedName& qName)

A function that returns a bool and is named "check" confused me. I couldn't
figure out whether true meant "had an error" or "OK". It would be better to
name this something like hasNamespaceError.

 710	 if (qName.prefix() == emptyAtom) // createElementNS(null, ":div")
 714	 if (!qName.prefix().isEmpty() && qName.namespaceURI().isNull()) //
createElementNS(null, "html:div")

One of these checks uses isEmpty() and the other uses emptyAtom. Why the

 744	 // FIXME: the element factories should be fixed to not ignore

This comment is not clear enough. The comment should state what the factories
should do with the prefix rather than just saying "not ignore".

 746	 // Then function can stop taking ExceptionCode& as well.

What function? You mean setPrefix? Please name the function and also say why.

 757 PassRefPtr<Element> Document::createElementNS(const String& _namespaceURI,
const String& qualifiedName, ExceptionCode& ec)

Given the other changes you made, I'm surprised you didn't take the spurious
underscore off of _namespaceURI here.

 761	     // qualifiedName.isEmpty() also hits this path (not
 762	     //
 763	     // and http://www.w3.org/Bugs/Public/show_bug.cgi?id=525

I don't think this is a helpful comment. It's a great code change! But we
already have a test case, and I don't really think we need a pointer to that
bug. If we do want that bug URL somewhere, I suggest putting it in the test
case rather than putting it in two places in the code.

 3447	  // Spec: DOM Level 2 Core:  createAttributeNS(null, "xmlns") should
 3448	  // http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-DocCrAttrN
 3449	  if (qName.localName() == "xmlns" && qName.namespaceURI() !=
"http://www.w3.org/2000/xmlns/") {

You left an "S" off the URL here, so the URL didn't work. Please fix that.

I also don't think the example in the comment is clear enough since it's a
specific case involving null and doesn't match the much broader test in the
code, although the code does match the spec. It'd probably be better to just
omit the example.

The code you added is only for xmlns, but the section you site in the DOM Level
2 document also mentions a couple other rules that don't seem to be implemented
here. The one for xml and the one about a prefix without a namespace. The test
cases seem to cover these, but I don't see why it works. Shouldn't you use the
whole hasNamespaceError function here?

Even though everything looks good, there are enough comments here that I'll say

More information about the webkit-reviews mailing list