Closed Bug 389322 Opened 13 years ago Closed 12 years ago

[FIX]Enable JavaScript version 1.7+ for XBL

Categories

(Core :: XBL, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: skrulx, Assigned: bzbarsky)

References

()

Details

(Keywords: testcase)

Attachments

(1 file, 4 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: 

Right now it appears that the JavaScript in XBL is executed using JavaScript 1.6.  It would be nice to be able to use the stuff from 1.7 and 1.8 in XBL.

We could add an attribute to the implementation element to specify what version of javascript you want, or perhaps just make XBL always execute under the latest javascript version?  I did a search for "let" and "yield" over *.xml and didn't see any usage of variables with these names.


Reproducible: Always
Open up your js console and go to this url:

http://skrul.com/bug389322/test.xul

It uses the following XBL:

http://skrul.com/bug389322/test.xml

and the error console should say:

Error: missing ; after for-loop initializer
Source File: http://skrul.com/bug389322/test.xml
Line: 18, Column: 9
Source Code:
for (let i = 0; i < 3; i++) { 
Keywords: testcase
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Please actually attach the testcase to the bug.  First the XBL, then the file linking to it, pointing to the actual XBL attachment so that it can be run without downloading.  The testcase needs to test JS 1.7 features in all parts of XBL:

  * methods
  * properties
  * fields
  * event handlers
  * constructors/destructors

because they will in fact need pretty different fixes, from what I can see.  Once we have a testcase like that, I'd be happy to work on a fix.
Attached file Test JS 1.7 in various XBL parts (obsolete) —
Is this testcase acceptable?
That testcase is great.  I have this converted to a mochitest, and a partial patch.  I'm not sure when I'll have a chance to finish this, though, so if someone wants to pick it up, let me know.
Summary: Enable JavaScript 1.7+ for XBL → Enable JavaScript version 1.7+ for XBL
Attached patch What I have so far (obsolete) — Splinter Review
This doesn't compile yet, and I need this tree.
Keywords: helpwanted
bz:  This is a pretty minor update on what you had, is there more to do?
Assignee: nobody → crowder
Attachment #282333 - Attachment is obsolete: true
Attachment #285693 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch one more compile issue (obsolete) — Splinter Review
Attachment #286319 - Attachment is obsolete: true
Nevermind, still have more build errors to chase.  More in a bit.
Yeah.  There are three other things to do:

1)  Fix all the other consumers of this API to deal with passing in a version
2)  Just calling JS_SetVersion is sadly not the right thing to do.  See the
    existing nsJSContext code that deals with versions (in two places).  It does
    all sorts of E4X stuff as well, etc.  That mess should be factored out into
    a helper method or macro (or maybe helper object whose ctor sets the version
    and whose dtor restores the old version).  Restoring is important, by the
    way.
3)  Set the version to JSVERSION_LATEST right before we start compiling
    (and maybe installing?) the binding implementation, so that the version sets
    when we compile all those properties and methods are just fast no-ops.
    Restore when done with that.
Would just doing something like this work?

--- a/content/xbl/src/nsXBLDocumentInfo.cpp
+++ b/content/xbl/src/nsXBLDocumentInfo.cpp
@@ -265,12 +265,15 @@ nsXBLDocGlobalObject::SetContext(nsIScri
   nsresult rv;
   rv = aScriptContext->InitContext(nsnull);
   NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Script Language's InitContext failed");
   aScriptContext->DidInitializeContext();
   // and we set up our global manually
   mScriptContext = aScriptContext;
+
+  // Set to latest version of JavaScript
+  mScriptContext->SetDefaultLanguageVersion(JSVERSION_LATEST);
 }
(That works for me. At least I can use let in xbl. I can't quite get any of the xbl mochitests to run.)
I was hoping for something that trivial early on, but comment #10 here seems to suggest it is not adequate.
Something's wrong if too much code is required...

/be
Edward's suggestion would work if you don't care about using JS 1.7 in fields and event handlers.  That is, it'll work for properties, methods, constructors, and destructors.  That's assuming that the version only affects compile-time.  If it affects runtime, then other things would also be affected.

This should have been clear from running the test in my patch.  Edward, please feel free to contact me about your issues with running mochitest; it should Just Work.

> Something's wrong if too much code is required...

The main wrongness is twofold: not all of nsIScriptContext takes versions (so we need an API change to do this, hence need to update all callers) and the existing version stuff in nsJSContext was copy/pasted all over, so needs to either be copy/pasted to the new code or refactored properly so it can be sanely reused.

I suppose an additional "wrongness" is that this is going through nsIScriptContext to start with, eh?  ;)
(In reply to comment #15)
> Edward's suggestion would work if you don't care about using JS 1.7 in fields
> and event handlers.  That is, it'll work for properties, methods, constructors,
> and destructors.  That's assuming that the version only affects compile-time. 
> If it affects runtime, then other things would also be affected.

Version is saved at compile-time in executable objects, and selected from their private data when they are run. So you don't need version params for any but the Compile* methods.

> This should have been clear from running the test in my patch.  Edward, please
> feel free to contact me about your issues with running mochitest; it should
> Just Work.

What are the test results with the latest patch?

> > Something's wrong if too much code is required...
> 
> The main wrongness is twofold: not all of nsIScriptContext takes versions

Only the Compile and Evaluate (not the Execute or Call, etc.) APIs need version parameters.

> I suppose an additional "wrongness" is that this is going through
> nsIScriptContext to start with, eh?  ;)

Could use bare JS API from XBL for sure. Why not?

/be
> Only the Compile and Evaluate (not the Execute or Call, etc.) APIs need version
> parameters.

Well, CompileEventHandler doesn't have a version parameter...  Nor does CompileFunction.  Those are the API changes I made in my first-cut patch.

> Could use bare JS API from XBL for sure. Why not?

It's an even bigger change than fixing nsIScriptContext to deal with versions for everything as needed, to be honest.  Especially if we want to get all the security stuff that nsIScriptContext does for us right...

Unless you meant to do the version-setting via JSAPI while continuing to use nsIScriptContext for the actual compilation, etc...  That might be doable.
Yes, bare JS API where needed, followup bugs for later.

For HTML, we Ecma folk think it's likely that event handlers, like type-less or unversioned scripts, will need to be JS1-ish forever (ES3). Even with the default scripting language content header / meta http-equiv thingie that HTML 4 spec'ed, which no one uses. Not sure this matters for XBL.

/be
Hrm, it would be cool if we'd get XBL to use the same JS version as chrome. We currently can't port Firefox code (that sometimes uses JS 1.7) to SeaMonkey prefs because we load script via loadSubScript() from an XBL handler :(
Requesting blocking -- it's a painful developer confusion point, and should have an easy fix.
Flags: blocking1.9?
This bug needs a better owner than me, for now.  I'll pick it up soon, if I can.
Assignee: crowder → nobody
Status: ASSIGNED → NEW
Don't think this is blocking, but wanted to avoid the confusion and letting extension developers use the new cewl JS features.
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Assignee: nobody → bzbarsky
Attachment #286322 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #303473 - Flags: superreview?(jst)
Attachment #303473 - Flags: review?(brendan)
Keywords: helpwanted
Summary: Enable JavaScript version 1.7+ for XBL → [FIX]Enable JavaScript version 1.7+ for XBL
Comment on attachment 303473 [details] [diff] [review]
Let's just do this

Looks good to me.
Attachment #303473 - Flags: superreview?(jst) → superreview+
Comment on attachment 303473 [details] [diff] [review]
Let's just do this

I think I steered mhammond in a bad direction, or (memory fails) perhaps the code already was going that way when he came along: instead of |= JSVERSION_HAS_XML in callers, requiring this ugly recovery of the XML option from the version, it might be better to allow JSVERSION_HAS_XML to be included in a version passed to JS_SetVersion. Followup bug fodder?

r=me on the patch, anyway -- thanks!

/be
Attachment #303473 - Flags: review?(brendan)
Attachment #303473 - Flags: review+
Attachment #303473 - Flags: approval1.9+
> Followup bug fodder?

Bug 417871 filed.

Checked the patch in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Typo in this patch breaks the building of the python dom extension.

"extensions/python/dom/src/nsPyContext.cpp", line 478.

The ";" in the first patch hunk for this file should have been a comma:

"+                                 PRUint32 aVersion;"

"+                                 PRUint32 aVersion,"
Doh.  Checked in a fix for that.
Depends on: 419765
Depends on: 1121323
You need to log in before you can comment on or make changes to this bug.