[Webkit-unassigned] [Bug 16833] Acid3 expects createElementNS to throw exception for localname = ":div"

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


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


darin at apple.com changed:

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




------- Comment #8 from darin at apple.com  2008-01-13 09:04 PDT -------
(From update of attachment 18416)
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
this.

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

 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
difference?

 744     // FIXME: the element factories should be fixed to not ignore
qName.prefix()

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
NAMESPACE_ERR), see:
 762         //
dom/html/level1/core/hc_documentinvalidcharacterexceptioncreateattribute1.html
 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
throw
 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
review-.


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



More information about the webkit-unassigned mailing list