368459 - Test for XML.prototype.normalize() in e4x/XML/13.4.4.26.js is wrong

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The test for E4X normalize() is wrong.  E4X normalize concatenates adjacent text nodes and removes empty text nodes.  The test, however, thinks that normalize() removes text nodes containing only XML whitespace (CR/LF/SP/HT).  (The only situation in which such nodes are "removed" is during the operation of the ToXML operator in MapInfoItemToXML, when a text node whose contents are all XML whitespace characters is left unmapped by returning null iff XML.ignoreWhiteSpace is true.)

The test basically needs to be rewritten to be correct -- I'll write up a new version and post the patch soon.

This issue was first brought up on <http://wiki.mozilla.org/E4X> three-ish months ago, where it's been sitting ever since because I haven't made the time to read it.  :-(  Sorry for the delay...

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch (for behavior and testcase)

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsxml.c&rev=3.139&mark=6584-6601,6643#6580

Sigh.  This was basically known to be a bug with the test, except that it was thought to be something which could have been an erratum.  :-\  I think we're past the point where it wouldn't have obviously been a testcase bug and might have been an erratum -- the wording has survived two editions of the E4X spec, and as written in the spec it acts exactly like the W3C DOM Node.normalize() method (a highly desirable property).

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 253062 [details] [diff] [review]
Patch (for behavior and testcase)

The tests did wag the dog here, and I thought that they might reflect enough real scripts to matter. But it's E4X -- what real scripts? :-P  Seriously, there are apps, esp. for Rhino, but I have no idea how to track 'em down.

Does Rhino need a change to match the test change?

/be

Comment 3

[Jeff Walden [:Waldo]]

I think so, yes, assuming these trims (the first one, and the totally superfluous second one) don't serve some other immediately obvious purpose:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/rhino/xmlimplsrc/org/mozilla/javascript/xmlimpl/XML.java&rev=1.21&mark=2465,2467#2443

I've CC'd David Caldwell, the person I believe to be most involved with E4X in Rhino, so hopefully he can follow up on it on the Rhino end.

Comment 4

[Jeff Walden [:Waldo]]

Patch checked in, on a surprisingly quicker time scale than I'd imagined when I filed the bug or even submitted the patch.  :-)

For those following along without reading patches, note that the modified test subsumes most of the original test's behavior, except that it tests for outputs the original test didn't expect to get but should have.  This catches the "normalize removes whitespace-only nodes" bug that the changes to jsxml.c fixed.

Comment 5

[David Caldwell]

There's a new Rhino E4X implementation near release status in which the implementation of normalize matches the spec (and hence the new test).

Comment 6

[Bob Clary [:bc] (inactive)]

update summary with e4x/XML/13.4.4.26.js to ease searching. Note 1.8.x will fail after the test is updated.

Comment 7

[Bob Clary [:bc] (inactive)]

verified fixed 20070217 1.9.0 windows/mac*/linux