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
