209271 - Dodgy SetCaseSensitive() call in nsDOMCSSAttrDeclaration.cpp

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P3)

Description

[Hixie (not reading bugmail)]

nsDOMCSSAttrDeclaration.cpp contains the following code:

 // look up our namespace.  If we're XHTML, we need to be case-sensitive
 // Otherwise, we should not be
 (*aCSSParser)->SetCaseSensitive(nodeInfo->NamespaceEquals(kNameSpaceID_XHTML));

This seems wrong. We should be case sensitive in any namespace _other_ than
non-XML HTML, not _just_ case sensitive in the case of XHTML.

Note that IMHO, if the code is relying on some other constant (like, this code
only gets called for XHTML and HTML, but not, e.g., SVG) then a comment should
be added to this effect.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

This code is indeed relying on the fact that it's only called for HTML.  That
said, this is a bit safer...

Of course all this only matters when you have a node that can no longer reach
its ownerDocument (so that it's not getting a CSS parser via the loader), which
is kinda hard to arrange... ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126105 [details] [diff] [review]
Patch

The other option is to assert that in fact
mContent->IsContentOfType(nsIContent::eHTML)

Comment 3

[Boris Zbarsky [:bzbarsky]]

.

Comment 4

[Boris Zbarsky [:bzbarsky]]

fixed.

Comment 5

[Hixie (not reading bugmail)]

+  // If we are not HTML, we need to be case-sensitive.  Otherwise, Look up our
+  // namespace.  If we're XHTML, we need to be case-sensitive Otherwise, we
+  // should not be

That's still a confusing comment, IMHO. (It requires that you realise that XHTML
is also HTML.)

What about something like "If we are XML, we are case sensitive on all
attributes. Otherwise, we're HTML, and we should only be case sensitive on those
that the HTML spec says are [CS]."

But yeah, whatever. It's only a comment in a rarely used piece of code. :-)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> (It requires that you realise that XHTML is also HTML.)

If you don't know that about nsIContent objects in Mozilla, you probably should
not be touching this code.  ;)

Comment 7

[Brendan Eich [:brendan]]

> What about something like "If we are XML, we are case sensitive on all
> attributes. Otherwise, we're HTML, and we should only be case sensitive on those

Since you're picking comment nits: "... we should be case sensitive only on
those ...."

/be