RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v3]

[Kevin Rushforth]

On Thu, 4 Jun 2020 15:44:49 GMT, Rony G. Flatscher <github.com+60214806+ronyfla at 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