389322 - [FIX]Enable JavaScript version 1.7+ for XBL

Status: RESOLVED FIXED

(Core :: XBL, enhancement)

Description

[Steve Krulewitz]

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

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please attach a testcase

Comment 2

[Steve Krulewitz]

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++) { 

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Aiko]

Attachment: Test JS 1.7 in various XBL parts

Is this testcase acceptable?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: What I have so far

This doesn't compile yet, and I need this tree.

Comment 7

[Brian Crowder]

Attachment: fix compile problems and add JS_SetVersion calls

bz:  This is a pretty minor update on what you had, is there more to do?

Comment 8

[Brian Crowder]

Attachment: one more compile issue

Comment 9

[Brian Crowder]

Nevermind, still have more build errors to chase.  More in a bit.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Ed Lee :Mardak]

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);
 }

Comment 12

[Ed Lee :Mardak]

(That works for me. At least I can use let in xbl. I can't quite get any of the xbl mochitests to run.)

Comment 13

[Brian Crowder]

I was hoping for something that trivial early on, but comment #10 here seems to suggest it is not adequate.

Comment 14

[Brendan Eich [:brendan]]

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

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

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?  ;)

Comment 16

[Brendan Eich [:brendan]]

(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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

Unless you meant to do the version-setting via JSAPI while continuing to use nsIScriptContext for the actual compilation, etc...  That might be doable.

Comment 19

[Brendan Eich [:brendan]]

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

Comment 20

[Robert Kaiser]

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 :(

Comment 21

[Mike Shaver (:shaver emeritus)]

Requesting blocking -- it's a painful developer confusion point, and should have an easy fix.

Comment 22

[Brian Crowder]

This bug needs a better owner than me, for now.  I'll pick it up soon, if I can.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't think this is blocking, but wanted to avoid the confusion and letting extension developers use the new cewl JS features.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Let's just do this

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 303473 [details] [diff] [review]
Let's just do this

Looks good to me.

Comment 26

[Brendan Eich [:brendan]]

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Followup bug fodder?

Bug 417871 filed.

Checked the patch in.

Comment 28

[Todd Whiteman]

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,"

Comment 29

[Boris Zbarsky [:bzbarsky]]

Doh.  Checked in a fix for that.