On Thu, 4 Jun 2020 15:44:49 GMT, Rony G. Flatscher <github.com+60214806+rony...@openjdk.org> wrote:
>> This PR adds a "compile" process instruction to FXML files with the optional >> PI data "true" (default) and "false". The >> PI data is turned into a boolean value using "Boolean.parseBoolean(String)". >> This makes it possible to inject the compile PI everywhere in a FXML file >> and turn on and off compilation of scripts if >> the scripting engine implements the javax.script.Compilable interface. The >> PR adds the ability for a fallback in case >> compilation of scripts fails, in which case a warning gets issued about this >> fact and evaluation of the script will be >> done without compilation. Because of the fallback scripts get compiled with >> this version by default. >> --------- >> ### Progress >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> - [ ] Change must be properly reviewed >> >> ### Issue >> * [JDK-8238080](https://bugs.openjdk.java.net/browse/JDK-8238080): >> FXMLLoader: if script engines implement >> javax.script.Compilable compile scripts >> >> >> ### Download >> `$ git fetch https://git.openjdk.java.net/jfx pull/192/head:pull/192` >> `$ git checkout pull/192` > > Rony G. Flatscher has updated the pull request with a new target base due to > a merge or a rebase. The pull request now > contains 27 commits: > - Updates to meet Kevin's comment in PR 192 as of May 27th. > - Merge remote-tracking branch 'upstream/master' into > scriptCompilablePIcompileFallback > - Reword the compile processing instructions (replace 'Hint' with 'Note:', > adjust text to context), remove spurious empty > line in script example code. > - Document the compile processing instruction for scripts. > - Add missing language processing instruction. > - Correct typo, replace tabs, remove trailing blanks. > - Make sure we test the default behaviour to compile script by leaving out > the compile PI. > - Revert temporary rename of test method. > - Correct ModuleLauncherTest (remove non-existing test), correct formatting. > - Always supply the script's filename in the error message first to further > ease spotting the location of script > exceptions. > - ... and 17 more: > https://git.openjdk.java.net/jfx/compare/6bd0e22d...7ef1bd68 I reviewed the implementation, and I think this is close to being ready, but I have a couple questions. I also still need to review the test and run it, but that will be later. I also noticed a few places with minor formatting issues -- mostly missing spaces and extra blank lines added to some files, but also `else {` when it should be `} else {`. I'll wait until the substantive part of the review is done to note them all (so if you can fix them ahead of time, that would be good). modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1571: > 1570: StringBuilder sb = new StringBuilder(); > 1571: char[] charBuffer = new char[4096]; > 1572: int n; Since you use the constant `4096` in three places (although it will be two once you fix the below bug), I recommend to either define a `final int` constant or else to use `charBuffer.length` as the size. modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1578: > 1577: } > 1578: } while (n == 4096); > 1579: script = sb.toString(); You can't assume that you are done if `Reader::read` returns less than what you asked for. You need to keep reading `while (n >= 0)`. modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1769: > 1768: try { > 1769: if (isCompiled) { > 1770: compiledScript.eval(localBindings); I think there may be other places you need to set isCompiled (it isn't set in the first couple of methods where you compile scripts). Can you check this? ------------- PR: https://git.openjdk.java.net/jfx/pull/192
