<div dir="ltr">Hi all,<div><br></div><div>Could I have a couple of reviews for this patch, please?</div><div><br></div><div><a href="https://gist.github.com/rednaxelafx/6102608" target="_blank">https://gist.github.com/rednaxelafx/6102608</a><br>
</div><div><br></div><div style>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.</div>
<div style><br></div><div style>I'll walkthrough the patch below: (line numbers refer to sajs.patch, not sa.js)</div><div style><br></div><div style>Line 8-12:</div><div style><br></div><div style>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.</div>
<div style><br></div><div style>Line 20-42, 51-76:</div><div style><br></div><div style>When implementing a JavaAdapter for SA ScriptObject, the __get__ function calls the __has__ function:</div><div style> this.__has__(name)</div>
<div style>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.</div>
<div style><br></div><div style>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.</div>
<div style><br></div><div style>Line 45-48:</div><div style><br></div><div style>Removed trailing whitespace.</div><div style><br></div><div style>Line 84-85:</div><div style><br></div><div style>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.</div>
<div style><br></div><div style>Line 93-104:</div><div style><br></div><div style>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.</div>
<div style><br></div><div style>Line 113:</div><div><br></div><div style>This is the same change as purposed by Yunda back in April. [1] A missing fix from the NPG changes.</div><div style><br></div><div style>Line 121-122, 129-130:</div>
<div style><br></div><div style>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".</div>
<div style><br></div><div style>Line 138-144, 152-160:</div><div style><br></div><div style>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.</div>
<div style><br></div><div style>Line 168-172:</div><div style><br></div><div style>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.</div>
<div style><br></div><div style>That's all for this patch.</div><div style><br></div><div style>BTW, there is another patches pending review, too: JDK-8011979 [2]</div><div style><br></div><div style>Thanks,</div><div style>
Kris</div><div style><br></div><div style>[1]: <a href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009043.html">http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009043.html</a></div>
<div style>[2]: <a href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009149.html">http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009149.html</a></div></div>