15.92 KB, patch
|Details | Diff | Splinter Review|
1.30 KB, text/html
17.08 KB, patch
Mike Schroepfer: approval1.8b5+
|Details | Diff | Splinter Review|
From correspondence with David. /be ----- Brendan Eich wrote: > Thinking it through again, I see an earlier mistake that led to the > version-selection requirement that does us no good in hiding from old > browsers: again, the reason e4x=1 is required to enable E4X in full is to > avoid breaking the HTML "comment hiding hack". > > But any E4X comment literal usage is unlikely to be the first thing (excluding > whitespace) in a script tag. Good point! A script-hiding comment is useful only if it is the first token in the script. I suppose it is possible to construct examples where an XML literal begins with a comment that is the first literal. But in common usage, any such XML literal would be assigned to a variable first. I think it would be safe to disambiguate in this way.
Must-fix for 1.8b5/fx1.5 -- should be a small patch. /be
Created attachment 196746 [details] [diff] [review] bonus, optimize js_GetToken's line-dirtying a bit, by avoiding branch-tests This reduces JSOPTION_XML to an option enabling scanning of <!-- as an XML comment, instead of a "skip to end of line" marker for the old Netscape-2-era HTML comment hiding hack. That's it. :: and @ are scanned, #if JS_HAS_XML_SUPPORT and without any run-time option/version check. mrbkap, this required pushing your automagic JSOPTION_XML enable/disable code down into ParseXMLSource (which was a fix anyway, since it made 'l = new XMLList(...)' auto-enable, as well as 'x = new XML(...)'). This motivated an overdue (or just-in-time!) change to track an E4X spec-fix as it goes to ISO: ECMA-357 evaluates @a or *::b or * to undefined if the XML name is not found in the scope chain! See 11.1 3(a)(i). The change to throw a ReferenceError affects js.msg and js_FindXMLProperty's uses and definition. I had made this a strict warning upon discovering it when implementing ECMA-357. It's really a bug when you consider that, e.g., given no @b in the scope chain, @b = 42 fails (as it should), but @b not on the left-hand side of an assignment silently evaluates to undefined. Bob, the testsuite may not cover the weird old behavior -- can you adjust if it does, or add tests if it doesn't? Thanks. /be
Comment on attachment 196746 [details] [diff] [review] bonus, optimize js_GetToken's line-dirtying a bit, by avoiding branch-tests Argh, forgot to copy up new patch. /be
Created attachment 196747 [details] [diff] [review] the right patch
This might break sites that use the "comment hiding hack" in unusual ways. For example, if a single script tag contains two <!-- --> blocks, Firefox currently executes the scripts inside both blocks.
(In reply to comment #5) > This might break sites that use the "comment hiding hack" in unusual ways. For > example, if a single script tag contains two <!-- --> blocks, Firefox currently > executes the scripts inside both blocks. Do you know of any real-world examples? We could eliminate TSF_DIRTYINPUT and use only TSF_DIRTYLINE, but then valid E4X code such as var a_very_very_very_very_very_very_very_long_name = <!-- an XML comment that did not fit on the above line -->; would miscompile. /be
(In reply to comment #6) No, I have it! We just need to clear TSF_DIRTYINPUT after a --> that matches a non-E4X <!--, and we're back in the game! Ah, compatibility. Interdiff coming up to avoid whole-patch redo (for now). /be
Created attachment 196749 [details] [diff] [review] interdiff for last patch, as promised Just review with this applied in your head, or in your tree! /be
No, I don't know any real-world examples of sites that use the "comment hiding hack" in unusual ways.
Created attachment 196822 [details] [diff] [review] better patch, handle adjacent HTML comments closed by // -->
Comment on attachment 196822 [details] [diff] [review] better patch, handle adjacent HTML comments closed by // --> sr=shaver if you switch to setting oldopts from JS_SetOptions' return value, instead of directly peeking in cx.
Comment on attachment 196822 [details] [diff] [review] better patch, handle adjacent HTML comments closed by // --> >Index: jsxml.c > XML(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) XMLList needs the same treatment. r=mrbkap
mrbkap: thanks, sorry I doubted you without reading XMLList (but pushing the auto-enable code down to the common sub-subroutine would have been righteous even before this bug ;-). Fixed on trunk. /be
I'll attach a branch patch shortly. /be
(In reply to comment #14) testcase+ if _you check the test case in_, testcase? if you need work on it and need it to be checked in. I need to gomb and set up an area for browser only tests which do not have a place in the shell oriented test library before this can go it. Soonest.
Fixed on the 1.8 branch now. /be
Checking in regress-309242.js; /cvsroot/mozilla/js/tests/js1_6/Regress/regress-309242.js,v <-- regress-309242.js initial revision: 1.1 done
verified fixed 1.8.x and trunk.