Hi Kris,
All the JavaScript changes look good to me. Thanks for taking care of
rhino-vs-nashorn compatibility issues.
PS. I used to work in hotspot serviceability agent in the past. Now, I'm
with the nashorn team.
Thanks
-Sundar
On Monday 29 July 2013 01:23 PM, Krystal Mok wrote:
Hi all,
Could I have a couple of reviews for this patch, please?
https://gist.github.com/rednaxelafx/6102608
Switching the JavaScript engine from Rhino to Nashorn and the No
PermGen project caused a few issues that stopped sa.js from working.
This is a patch that tries to fix the issues that I've run into.
I'll walkthrough the patch below: (line numbers refer to sajs.patch,
not sa.js)
Line 8-12:
Since Nashorn implements ECMAScript 5.1, using JavaScript keywords as
the property name in the "dot" syntax is not a problem anymore. So I'm
un-commenting these lines.
Line 20-42, 51-76:
When implementing a JavaAdapter for SA ScriptObject, the __get__
function calls the __has__ function:
this.__has__(name)
which is working at the wrong level: __has__ is a special hook
function, and cannot be called via "this" this way. It'll trigger the
__call__ hook to find the "__has__" member, but the JavaAdapter here
does not override __call__, and then the lookup will fail.
Rather than going through the trouble of implementing the __call__
hook just for this purpose, I move the __has__ function up, and made
__get__ call __has__ directly instead. Now it won't trigger the
__call__ hook for the lookup, the things will work fine again.
Line 45-48:
Removed trailing whitespace.
Line 84-85:
This code used to work in Rhino, but apparently Nashorn doesn't
automatically convert a JavaScript Array into a Java array when doing
JS-to-Java interop, so this conversion has to be done manually.
Line 93-104:
I'm a little confused in this part. Nashorn exposes a "print" function
that's defined in jdk/nashorn/api/scripting/resources/engine.js, and
that it doesn't expose a corresponding "println" function. I'm not
sure if this code really worked when using Rhino...anyway, the
"println" function is missing, so "writeln = println" doesn't do
anything useful. I'm adding a shim here just in case either of "print"
or "println" functions are missing. This fixes an error when calling
"println" in line 87 of this patch.
Line 113:
This is the same change as purposed by Yunda back in April. [1] A
missing fix from the NPG changes.
Line 121-122, 129-130:
When specifying a method overload in Nashorn, the argument syntax it
takes for inner classes uses "." as the separator between the
enclosing class name and the inner class name, instead of "$" as in
the "binary name".
Line 138-144, 152-160:
Nashorn has stricter default behaviors than Rhino when overriding Java
methods. Where as Rhino defaults to a "do nothing" implementation,
Nashorn defaults to throwing UnsupportedOperationException for
unimplemented methods.
Line 168-172:
Before the fix, this code only replace the first occurrence of the
specified special characters, which happens to be not enough for newer
HotSpot types in C++ templates.
That's all for this patch.
BTW, there is another patches pending review, too: JDK-8011979 [2]
Thanks,
Kris
[1]:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009043.html
[2]:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009149.html
