[jfx11u] Integrated: 8273732: Clarify review policies for clean backports in JavaFX update releases

[Kevin Rushforth]

On Thu, 16 Sep 2021 12:27:00 GMT, Kevin Rushforth  wrote:

> Clean backport of update to `CONTRIBUTING.md` to clarify that clean backports 
> of approved fixes can be integrated without further review.

This pull request has now been integrated.

Changeset: 896089eb
Author:Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx11u/commit/896089ebe7764d0b9503c394b5d4ed2a639e9b4a
Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod

8273732: Clarify review policies for clean backports in JavaFX update releases

Backport-of: 549bfef95e4c2e690eb83523a3bf5a8344c072f6

-

PR: https://git.openjdk.java.net/jfx11u/pull/53

[jfx11u] Integrated: 8273732: Clarify review policies for clean backports in JavaFX update releases

[Kevin Rushforth]

Clean backport of update to `CONTRIBUTING.md` to clarify that clean backports 
of approved fixes can be integrated without further review.

-

Commit messages:
 - 8273732: Clarify review policies for clean backports in JavaFX update 
releases

Changes: https://git.openjdk.java.net/jfx11u/pull/53/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx11u=53=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8273732
  Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx11u/pull/53.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx11u pull/53/head:pull/53

PR: https://git.openjdk.java.net/jfx11u/pull/53

[jfx17u] Integrated: 8273732: Clarify review policies for clean backports in JavaFX update releases

[Kevin Rushforth]

On Wed, 15 Sep 2021 22:05:26 GMT, Kevin Rushforth  wrote:

> Added a paragraph indicating that a review of a clean backport to an update 
> release is optional, if the bug in question has been approved for inclusion 
> into the release.

This pull request has now been integrated.

Changeset: 549bfef9
Author:Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx17u/commit/549bfef95e4c2e690eb83523a3bf5a8344c072f6
Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod

8273732: Clarify review policies for clean backports in JavaFX update releases

Reviewed-by: prr, pbansal, jvos

-

PR: https://git.openjdk.java.net/jfx17u/pull/9

Re: [jfx17u] RFR: 8273732: Clarify review policies for clean backports in JavaFX update releases [v2]

[Johan Vos]

On Wed, 15 Sep 2021 23:06:39 GMT, Kevin Rushforth  wrote:

>> Added a paragraph indicating that a review of a clean backport to an update 
>> release is optional, if the bug in question has been approved for inclusion 
>> into the release.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Minor wording update

This clarifies it indeed.

-

Marked as reviewed by jvos (Reviewer).

PR: https://git.openjdk.java.net/jfx17u/pull/9

Re: [jfx17u] RFR: 8273732: Clarify review policies for clean backports in JavaFX update releases [v2]

[Pankaj Bansal]

On Wed, 15 Sep 2021 23:06:39 GMT, Kevin Rushforth  wrote:

>> Added a paragraph indicating that a review of a clean backport to an update 
>> release is optional, if the bug in question has been approved for inclusion 
>> into the release.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Minor wording update

Marked as reviewed by pbansal (Committer).

-

PR: https://git.openjdk.java.net/jfx17u/pull/9

Re: [jfx17u] RFR: 8273732: Clarify review policies for clean backports in JavaFX update releases [v2]

[Phil Race]

On Wed, 15 Sep 2021 23:06:39 GMT, Kevin Rushforth  wrote:

>> Added a paragraph indicating that a review of a clean backport to an update 
>> release is optional, if the bug in question has been approved for inclusion 
>> into the release.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Minor wording update

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx17u/pull/9

Re: [jfx17u] RFR: 8273732: Clarify review policies for clean backports in JavaFX update releases [v2]

[Kevin Rushforth]

> Added a paragraph indicating that a review of a clean backport to an update 
> release is optional, if the bug in question has been approved for inclusion 
> into the release.

Kevin Rushforth has updated the pull request incrementally with one additional 
commit since the last revision:

  Minor wording update

-

Changes:
  - all: https://git.openjdk.java.net/jfx17u/pull/9/files
  - new: https://git.openjdk.java.net/jfx17u/pull/9/files/8bc1bc69..af930d70

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx17u=9=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx17u=9=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx17u/pull/9.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/9/head:pull/9

PR: https://git.openjdk.java.net/jfx17u/pull/9

[jfx17u] RFR: 8273732: Clarify review policies for clean backports in JavaFX update releases

[Kevin Rushforth]

Added a paragraph indicating that a review of a clean backport to an update 
release is optional, if the bug in question has been approved for inclusion 
into the release.

-

Commit messages:
 - 8273732: Clarify review policies for clean backports in JavaFX update 
releases

Changes: https://git.openjdk.java.net/jfx17u/pull/9/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u=9=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8273732
  Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx17u/pull/9.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/9/head:pull/9

PR: https://git.openjdk.java.net/jfx17u/pull/9

Re: Eclipse: any way to checkout a PR for review?

[Nir Lisker]

A Pull Request is a GitHub feature, it doesn't exist in Git generally, so
EGit (I assume that's what you also use) is oblivious to PRs. Maybe there
is a special plugin for GitHubs or PRs.

Also, adding a remote is a one-time operation. Most contributors are
recurring, so for me reviewing amounts to a right click > pull on the
remote and a double click on the branch to check it out. I don't see any
inconvenience.

On Wed, Jul 28, 2021 at 12:45 PM Jeanette Winzenburg <
faste...@swingempire.de> wrote:

>
> Zitat von Nir Lisker :
>
> > I'm not really sure what you mean. If you pull from the remote that the
> PR
> > is on and checkout the remote branch, is it not good enough for a review?
> >
>
> yeah, that's what I meant - without having been too clear - with
> fork-the-fork :) What I had hoped for was some hidden eclipse magic to
> directly access a PR as branch (from jfx) - something like the Pull
> requests tab in GitHub Desktop. Which under the hood adds a new remote
> to the fork that created the pr. And that's what I ended up with: add
> that remote manually and checkout the branch.
>
> Thanks for your input!
>
> -- Jeanette
>
>
>
> > On Tue, Jul 27, 2021 at 2:16 PM Jeanette Winzenburg <
> faste...@swingempire.de>
> > wrote:
> >
> >>
> >> when reviewing a PR with only a few files changed, I simply create a
> >> local branch and c the changes (*cough, pretty sure there's a better
> >> way, but then that's the most simple ;).
> >>
> >> With changes to many files (like f.i.
> >> https://github.com/openjdk/jfx/pull/569) that still would be doable,
> >> but rather cumbersome - so looking for something like "gh pr checkout
> >> 569" (not that I ever tried that, just copied from the doc :) inside
> >> Eclipse.
> >>
> >> An alternative might be to fork-the-fork .. ?
> >>
> >> -- Jeanette
> >>
> >>
>
>
>
>

Re: Eclipse: any way to checkout a PR for review?

[Jeanette Winzenburg]

Zitat von Nir Lisker :


I'm not really sure what you mean. If you pull from the remote that the PR
is on and checkout the remote branch, is it not good enough for a review?



yeah, that's what I meant - without having been too clear - with  
fork-the-fork :) What I had hoped for was some hidden eclipse magic to  
directly access a PR as branch (from jfx) - something like the Pull  
requests tab in GitHub Desktop. Which under the hood adds a new remote  
to the fork that created the pr. And that's what I ended up with: add  
that remote manually and checkout the branch.


Thanks for your input!

-- Jeanette




On Tue, Jul 27, 2021 at 2:16 PM Jeanette Winzenburg 
wrote:



when reviewing a PR with only a few files changed, I simply create a
local branch and c the changes (*cough, pretty sure there's a better
way, but then that's the most simple ;).

With changes to many files (like f.i.
https://github.com/openjdk/jfx/pull/569) that still would be doable,
but rather cumbersome - so looking for something like "gh pr checkout
569" (not that I ever tried that, just copied from the doc :) inside
Eclipse.

An alternative might be to fork-the-fork .. ?

-- Jeanette

Re: Eclipse: any way to checkout a PR for review?

[Nir Lisker]

I'm not really sure what you mean. If you pull from the remote that the PR
is on and checkout the remote branch, is it not good enough for a review?

On Tue, Jul 27, 2021 at 2:16 PM Jeanette Winzenburg 
wrote:

>
> when reviewing a PR with only a few files changed, I simply create a
> local branch and c the changes (*cough, pretty sure there's a better
> way, but then that's the most simple ;).
>
> With changes to many files (like f.i.
> https://github.com/openjdk/jfx/pull/569) that still would be doable,
> but rather cumbersome - so looking for something like "gh pr checkout
> 569" (not that I ever tried that, just copied from the doc :) inside
> Eclipse.
>
> An alternative might be to fork-the-fork .. ?
>
> -- Jeanette
>
>

Re: Eclipse: any way to checkout a PR for review?

[Jeanette Winzenburg]

ahh .. yeah, that looks doable - had hoped to get away without  
installing git and frickle with commandlines ;)


thanks

Zitat von Kevin Rushforth :


I do something like this:

git fetch upstream pull/569/head:pr_569

Then you can checkout the "pr_569" branch. I typically will then  
merge in the current upstream/master to test. If you don't have an  
"upstream" remote you can instead:


git fetch https://github.com/openjdk/jfx pull/569/head:pr_569

-- Kevin

On 7/27/2021 4:15 AM, Jeanette Winzenburg wrote:


when reviewing a PR with only a few files changed, I simply create  
a local branch and c the changes (*cough, pretty sure there's a  
better way, but then that's the most simple ;).


With changes to many files (like f.i.  
https://github.com/openjdk/jfx/pull/569) that still would be  
doable, but rather cumbersome - so looking for something like "gh  
pr checkout 569" (not that I ever tried that, just copied from the  
doc :) inside Eclipse.


An alternative might be to fork-the-fork .. ?

-- Jeanette

Re: Eclipse: any way to checkout a PR for review?

[Kevin Rushforth]

I do something like this:

git fetch upstream pull/569/head:pr_569

Then you can checkout the "pr_569" branch. I typically will then merge 
in the current upstream/master to test. If you don't have an "upstream" 
remote you can instead:


git fetch https://github.com/openjdk/jfx pull/569/head:pr_569

-- Kevin

On 7/27/2021 4:15 AM, Jeanette Winzenburg wrote:


when reviewing a PR with only a few files changed, I simply create a 
local branch and c the changes (*cough, pretty sure there's a better 
way, but then that's the most simple ;).


With changes to many files (like f.i. 
https://github.com/openjdk/jfx/pull/569) that still would be doable, 
but rather cumbersome - so looking for something like "gh pr checkout 
569" (not that I ever tried that, just copied from the doc :) inside 
Eclipse.


An alternative might be to fork-the-fork .. ?

-- Jeanette

Eclipse: any way to checkout a PR for review?

[Jeanette Winzenburg]

when reviewing a PR with only a few files changed, I simply create a  
local branch and c the changes (*cough, pretty sure there's a better  
way, but then that's the most simple ;).


With changes to many files (like f.i.  
https://github.com/openjdk/jfx/pull/569) that still would be doable,  
but rather cumbersome - so looking for something like "gh pr checkout  
569" (not that I ever tried that, just copied from the doc :) inside  
Eclipse.


An alternative might be to fork-the-fork .. ?

-- Jeanette

Re: Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLL

[Kevin Rushforth]

This is waiting on me. I need to review the updated API docs and CSR 
test, and then create a draft CSR issue.


-- Kevin


On 5/23/2020 11:30 AM, Rony G Flatscher wrote:

Hi Kevin,

is there anything I need to do? What are the next steps I should look for?

—-rony

Rony G. Flatscher (mobil/e)


Am 12.05.2020 um 19:23 schrieb Rony G. Flatscher :

Hi Kevin,

in the meantime I have tried to come up with a formulation for the "Introduction to 
FXML"
specification about the new compile processing instruction which is brief and 
complete. While being
there I fixed a typo in the document and added a missing language processing 
instruction to an
existing script example, hope that was o.k. as I have not filed an explicit bug 
for it.

Also closed the PR 129 and 187, removed the WIP from PR 192, and supplied the 
CSR draft as a comment.

If there is anything else I should do, please let me know.

Cheers,

---rony



On 11.05.2020 14:12, Rony G. Flatscher wrote:
Hi Kevin,


On 09.05.2020 17:16, Kevin Rushforth wrote:
I'm finally getting back to this. I took a look at 
https://github.com/openjdk/jfx/pull/192 and I
like that as the direction for this enhancement.

The initial CSR you have is a good start.

Thank you!


Next steps are:

1. Update the "Introduction to FXML" specification (see my comment in the PR)
2. Update PR 192 with the draft CSR as a comment, modifying it to include the 
above additions to
"Introduction to FXML"
3. Remove WIP from the title

You can then close the other two PRs (129 and 187).

will do (may take a little bit).

Cheers

---rony



On 4/28/2020 6:15 AM, Rony G. Flatscher wrote:

Hi Kevin,

what should be the next steps?

Should I remove "WIP" from the title in 
<https://github.com/openjdk/jfx/pull/192> and add the CSR
draft text of my last e-mail as a "CSR" comment with PR # 192, thereby 
requesting it to be added
to <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>?

Please advise.

TIA,

---rony

P.S.: This is the RFE:

  * RFE (2020-01-24): 
<https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>

These are the three versions (all with appropriate unit tests) that I came up 
with
chronologically to implement the RFE, would prefer the latest version (PR # 
192):

  * Compile if Compilable implemented (2020-02-28): 
<https://github.com/openjdk/jfx/pull/129>
  * Compile if compile PI and Compilable is implemented (2020-04-11):
<https://github.com/openjdk/jfx/pull/187>
  * Compile with fallback, if Compilable is implemented, compile PI for 
fine-grained control
(2020-04-14): <https://github.com/openjdk/jfx/pull/192>


On 22.04.2020 20:01, Rony G. Flatscher wrote:

Hi Kevin,

as I am not able to file a CSR with the issue you suggested to come up with a 
draft, so here it goes:

Summary
===
Have javafx.fxml.FXMLLoader compile FXML scripts before evaluating them, if 
the script engine
implements the javax.script.Compilable interface to speed up execution. In 
case compilation
throws a javax.script.ScriptException fall back to evaluating the 
uncompiled script. Allow
control of script compilation with a "compile" PI for FXML files.

Problem
===
javafx.fxml.FXMLLoader is able to execute scripts in Java script languages
(javax.script.ScriptEngine implementations) referred to or embedded in a 
FXML file.

If a script engine implements the javax.script.Compilable interface, then 
such scripts could be
compiled and the resulting javax.script.CompiledScript could be executed 
instead using its
eval() methods.

Evaluating the javax.script.CompiledScript objects may help speed up the 
execution of script
invocations, especially for scripts defined for event attributes in FXML 
elements (e.g. like
onMouseMove) which may be repetitively invoked and evaluated.

Solution

Before evaluating the script code test whether the 
javax.script.ScriptEngine implements
javax.script.Compilable. If so, compile the script to a 
javax.script.CompiledScript object first
and then use its eval() method to evaluate the script, otherwise continue 
to use the
javax.script.ScriptEngine's eval() method instead. Should compilation of a 
script yield
(unexpectedly) a javax.script.ScriptException then fall back to using the
javax.script.ScriptEngine's eval() method. A new process instruction 
"compile" allows control of
the compilation of scripts ("true" sets compilation on, "false" to set 
compilation off) in FXML
files.

Specification
=
If a javax.script.ScriptEngine implements the javax.script.Compilable 
interface, then use its
compile() method to compile the script to a javax.script.CompiledScript 
object and use its
eval() method to run the script. In the case that the compilation throws 
(unexpectedly) a
 

Re: Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLL

[Rony G Flatscher]

Script could be 
>>>>> executed instead using its
>>>>>eval() methods.
>>>>> 
>>>>>Evaluating the javax.script.CompiledScript objects may help speed up 
>>>>> the execution of script
>>>>>invocations, especially for scripts defined for event attributes in 
>>>>> FXML elements (e.g. like
>>>>>onMouseMove) which may be repetitively invoked and evaluated.
>>>>> 
>>>>>Solution 
>>>>>
>>>>>Before evaluating the script code test whether the 
>>>>> javax.script.ScriptEngine implements
>>>>>javax.script.Compilable. If so, compile the script to a 
>>>>> javax.script.CompiledScript object first
>>>>>and then use its eval() method to evaluate the script, otherwise 
>>>>> continue to use the
>>>>>javax.script.ScriptEngine's eval() method instead. Should compilation 
>>>>> of a script yield
>>>>>(unexpectedly) a javax.script.ScriptException then fall back to using 
>>>>> the
>>>>>javax.script.ScriptEngine's eval() method. A new process instruction 
>>>>> "compile" allows control of
>>>>>the compilation of scripts ("true" sets compilation on, "false" to set 
>>>>> compilation off) in FXML
>>>>>files.
>>>>> 
>>>>>Specification
>>>>>=
>>>>>If a javax.script.ScriptEngine implements the javax.script.Compilable 
>>>>> interface, then use its
>>>>>compile() method to compile the script to a 
>>>>> javax.script.CompiledScript object and use its
>>>>>eval() method to run the script. In the case that the compilation 
>>>>> throws (unexpectedly) a
>>>>>javax.script.ScriptException log a warning and fall back to using the
>>>>>javax.script.ScriptEngine's eval() method instead.
>>>>>To allow setting this feature off and on while processing the FXML 
>>>>> file a "compile" process
>>>>>instruction ("" or "") gets defined 
>>>>> that allows to turn
>>>>>compilation off and on throughout a FXML file.
>>>>> 
>>>>> Having never seen a real CSR I hope that this matches what is expected 
>>>>> and is helpful for
>>>>> assessment. If not please advise (got the name of these fields from [1]).
>>>>> 
>>>>> ---
>>>>> 
>>>>> Also added brief information about the respective test units (what they 
>>>>> test and yield) in the WIP  [2].
>>>>> 
>>>>> ---rony
>>>>> 
>>>>> [1] "CSR-FAQ": <https://wiki.openjdk.java.net/display/csr/CSR+FAQs>
>>>>> 
>>>>> [2] "WIP: Script compilable+compile PI+fallback: 8238080: FXMLLoader: if 
>>>>> script engines implement
>>>>> javax.script.Compilable compile scripts #192":  
>>>>> <https://github.com/openjdk/jfx/pull/192>
>>>>> 
>>>>> 
>>>>> On 20.04.2020 14:58, Rony G. Flatscher wrote:
>>>>>> There is a new WIP at <https://github.com/openjdk/jfx/pull/192>:
>>>>>> 
>>>>>>This WIP 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. It
>>>>>>extends PR 187 #187.
>>>>>> 
>>>>>>To further ease spotting scripts that cause a ScriptException a 
>>>>>> message in the form of
>>>>>>"filename: caused ScriptException" gets added to the exception 
>>>>>> handling in either of the three
>>>>>>locations: an error message, a stack trace or a wrap-up into a 
>>>>>> RuntimeException (having three
>>>>>>different kinds of reporting ScriptExceptions may be questioned, 
>>>>>> however none of these tear down
>>>>>>the FXML GUI).
>>>>>> 
>>>>>>

Re: Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLL

[Rony G. Flatscher]

>> javax.script.Compilable. If so, compile the script to a 
>>>> javax.script.CompiledScript object first
>>>> and then use its eval() method to evaluate the script, otherwise 
>>>> continue to use the
>>>> javax.script.ScriptEngine's eval() method instead. Should compilation 
>>>> of a script yield
>>>> (unexpectedly) a javax.script.ScriptException then fall back to using 
>>>> the
>>>> javax.script.ScriptEngine's eval() method. A new process instruction 
>>>> "compile" allows control of
>>>> the compilation of scripts ("true" sets compilation on, "false" to set 
>>>> compilation off) in FXML
>>>> files.
>>>>
>>>> Specification
>>>> =
>>>> If a javax.script.ScriptEngine implements the javax.script.Compilable 
>>>> interface, then use its
>>>> compile() method to compile the script to a 
>>>> javax.script.CompiledScript object and use its
>>>> eval() method to run the script. In the case that the compilation 
>>>> throws (unexpectedly) a
>>>> javax.script.ScriptException log a warning and fall back to using the
>>>> javax.script.ScriptEngine's eval() method instead.
>>>> To allow setting this feature off and on while processing the FXML 
>>>> file a "compile" process
>>>> instruction ("" or "") gets defined 
>>>> that allows to turn
>>>> compilation off and on throughout a FXML file.
>>>>
>>>> Having never seen a real CSR I hope that this matches what is expected and 
>>>> is helpful for
>>>> assessment. If not please advise (got the name of these fields from [1]).
>>>>
>>>> ---
>>>>
>>>> Also added brief information about the respective test units (what they 
>>>> test and yield) in the WIP  [2].
>>>>
>>>> ---rony
>>>>
>>>> [1] "CSR-FAQ": <https://wiki.openjdk.java.net/display/csr/CSR+FAQs>
>>>>
>>>> [2] "WIP: Script compilable+compile PI+fallback: 8238080: FXMLLoader: if 
>>>> script engines implement
>>>> javax.script.Compilable compile scripts #192":  
>>>> <https://github.com/openjdk/jfx/pull/192>
>>>>
>>>>
>>>> On 20.04.2020 14:58, Rony G. Flatscher wrote:
>>>>> There is a new WIP at <https://github.com/openjdk/jfx/pull/192>:
>>>>>
>>>>> This WIP 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. It
>>>>> extends PR 187 #187.
>>>>>
>>>>> To further ease spotting scripts that cause a ScriptException a 
>>>>> message in the form of
>>>>> "filename: caused ScriptException" gets added to the exception 
>>>>> handling in either of the three
>>>>> locations: an error message, a stack trace or a wrap-up into a 
>>>>> RuntimeException (having three
>>>>> different kinds of reporting ScriptExceptions may be questioned, 
>>>>> however none of these tear down
>>>>> the FXML GUI).
>>>>>
>>>>> This WIP comes with proper test units as well. As per Kevin's suggestion 
>>>>> a warning gets logged
>>>>> whenever a script cannot be compiled and the fallback gets used.
>>>>>
>>>>> It is suggested to use this WIP as it includes the compilation by default 
>>>>> with a safe fallback to
>>>>> evaluate the uncompiled script, if compilation (unexpectedly) fails.
>>>>>
>>>>> Again, any feedback, discussion welcome!
>>>>>
>>>>> ---rony
>>>>>
>>>>> P.S.: In the log history there is a commit message "Make message more 
>>>>> pregnant.", it should have
>>>>> read "Make messages more terse." instead|.||
>>>>> |
>>>>>
>>>>>
>>>>> On 17.04.2020 19:37, Rony G. Flatscher wrote:
>>>>>>

Re: Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLL

[Rony G. Flatscher]

le the script to a javax.script.CompiledScript 
>>> object and use its
>>> eval() method to run the script. In the case that the compilation 
>>> throws (unexpectedly) a
>>> javax.script.ScriptException log a warning and fall back to using the
>>> javax.script.ScriptEngine's eval() method instead.
>>> To allow setting this feature off and on while processing the FXML file 
>>> a "compile" process
>>> instruction ("" or "") gets defined 
>>> that allows to turn
>>> compilation off and on throughout a FXML file.
>>>
>>> Having never seen a real CSR I hope that this matches what is expected and 
>>> is helpful for
>>> assessment. If not please advise (got the name of these fields from [1]).
>>>
>>> ---
>>>
>>> Also added brief information about the respective test units (what they 
>>> test and yield) in the WIP  [2].
>>>
>>> ---rony
>>>
>>> [1] "CSR-FAQ": <https://wiki.openjdk.java.net/display/csr/CSR+FAQs>
>>>
>>> [2] "WIP: Script compilable+compile PI+fallback: 8238080: FXMLLoader: if 
>>> script engines implement
>>> javax.script.Compilable compile scripts #192":  
>>> <https://github.com/openjdk/jfx/pull/192>
>>>
>>>
>>> On 20.04.2020 14:58, Rony G. Flatscher wrote:
>>>> There is a new WIP at <https://github.com/openjdk/jfx/pull/192>:
>>>>
>>>> This WIP 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. It
>>>> extends PR 187 #187.
>>>>
>>>> To further ease spotting scripts that cause a ScriptException a 
>>>> message in the form of
>>>> "filename: caused ScriptException" gets added to the exception 
>>>> handling in either of the three
>>>> locations: an error message, a stack trace or a wrap-up into a 
>>>> RuntimeException (having three
>>>> different kinds of reporting ScriptExceptions may be questioned, 
>>>> however none of these tear down
>>>> the FXML GUI).
>>>>
>>>> This WIP comes with proper test units as well. As per Kevin's suggestion a 
>>>> warning gets logged
>>>> whenever a script cannot be compiled and the fallback gets used.
>>>>
>>>> It is suggested to use this WIP as it includes the compilation by default 
>>>> with a safe fallback to
>>>> evaluate the uncompiled script, if compilation (unexpectedly) fails.
>>>>
>>>> Again, any feedback, discussion welcome!
>>>>
>>>> ---rony
>>>>
>>>> P.S.: In the log history there is a commit message "Make message more 
>>>> pregnant.", it should have
>>>> read "Make messages more terse." instead|.||
>>>> |
>>>>
>>>>
>>>> On 17.04.2020 19:37, Rony G. Flatscher wrote:
>>>>> There is a new WIP at <https://github.com/openjdk/jfx/pull/187> which 
>>>>> adds a compile PI (process
>>>>> instruction) for turning on and off script compilation if the script 
>>>>> engine implements the
>>>>> Compilable interface.
>>>>>
>>>>> By default compilation is off (no compilation), such that one needs to 
>>>>> add a compile PI
>>>>> ("") at the top to activate this feature. Supplying "true" 
>>>>> (default) or "false" as the PI
>>>>> data turns this feature on and off.
>>>>>
>>>>> The WIP comes with adapted test units that test "compile on" for an 
>>>>> entire fxml file, "compile off",
>>>>> alternating using "compile on and off", and alternating using "compile 
>>>>> off and on". This will test
>>>>> all variants of applying the compile PI for all categories of scripts.
>>>>>
>>>>> Any feedback appreciated!
>>>>>
>>>>> ---rony
>>>>>
>>>>> P.S.: FXML files that contain unknown PIs do not cause a runtime error by 
>>>>> FXMLLoader, 

Re: Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLL

[Kevin Rushforth]

 at<https://github.com/openjdk/jfx/pull/192>:

 This WIP 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. It
 extends PR 187 #187.

 To further ease spotting scripts that cause a ScriptException a message in 
the form of
 "filename: caused ScriptException" gets added to the exception handling in 
either of the three
 locations: an error message, a stack trace or a wrap-up into a 
RuntimeException (having three
 different kinds of reporting ScriptExceptions may be questioned, however 
none of these tear down
 the FXML GUI).

This WIP comes with proper test units as well. As per Kevin's suggestion a 
warning gets logged
whenever a script cannot be compiled and the fallback gets used.

It is suggested to use this WIP as it includes the compilation by default with 
a safe fallback to
evaluate the uncompiled script, if compilation (unexpectedly) fails.

Again, any feedback, discussion welcome!

---rony

P.S.: In the log history there is a commit message "Make message more 
pregnant.", it should have
read "Make messages more terse." instead|.||
|


On 17.04.2020 19:37, Rony G. Flatscher wrote:

There is a new WIP at<https://github.com/openjdk/jfx/pull/187>  which adds a 
compile PI (process
instruction) for turning on and off script compilation if the script engine 
implements the
Compilable interface.

By default compilation is off (no compilation), such that one needs to add a 
compile PI
("") at the top to activate this feature. Supplying "true" (default) or 
"false" as the PI
data turns this feature on and off.

The WIP comes with adapted test units that test "compile on" for an entire fxml file, 
"compile off",
alternating using "compile on and off", and alternating using "compile off and 
on". This will test
all variants of applying the compile PI for all categories of scripts.

Any feedback appreciated!

---rony

P.S.: FXML files that contain unknown PIs do not cause a runtime error by 
FXMLLoader, they just get
ignored. Therefore one could apply the compile PI to FXML files that are used 
in older JavaFX runtimes.

P.P.S.: In the next days I will also add Kevin's idea in a separate version 
that will have a
fallback solution in case a compilation is (unexpectedly) not successful, 
reverting to
(interpretative) evaluation/execution of the script. In that version it is 
planned to have
compilation on by default as in the case of a compilation failure there will be 
a safe backup solution.


On 14.04.2020 19:52, Kevin Rushforth wrote:

Yes, I agree that enough time has gone by. Go ahead with your proposal. I would 
wait a bit to
create the CSR until the review is far enough along to know which direction we 
intend to go.

Unless there is a real concern about possible regressions if scripts are 
compiled by default, I
think "enabled by default" is the way to go. Your argument that such script 
engines are broken
seems reasonable, since this only applies to script engines that implement 
javax.script.Compilable
in the first place. We still might want to add way to turn compilation off for 
individual scripts.
One other thing to consider is that if compilation fails, it might make sense 
to log a warning and
fall back to the existing interpreted mode.

Does anyone else have any concerns with this?

-- Kevin


On 4/14/2020 9:48 AM, Rony G. Flatscher wrote:

Hi there,

as there was probably enough time that has passed by I would intend to create a 
CSR in the next days
with the PR as per Kevin's suggestion.

(For the case that this feature should not be active by default, the CSR will 
suggest to define a
new "compile" PI in the form  (default, if no PI data 
given: true), which
is independent of the existence of a language PI (this way it becomes also 
possible to allow
compilation of external scripts denoted with the script-element, which do not 
need a page language
to be set as the file's extension allows the appropriate script engine to be 
loaded and used for
execution). A compile-PI would allow for turning compilation of scripts on by 
just adding the PI
 or   to FXML files (and  to turn 
off), which seems to
be simple and self-documentary. In general employing such compile PIs allows 
for setting compilation
of scripts on and off throughout an FXML file.)

---rony


On 04.04.2020 18:03, Rony G. Flatscher wrote:


Hi Kevin,

On 03.04.2020 01:21, Kevin Rushforth wrote:

I see that you updated the PR and sent it for review.

Before we formally review it in the PR, let's finish the discussion as to 
whether this is a useful
feature, and if so, what form this feature should take.

  From my point of view, this does seem like a useful feature

Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoade

[Rony G. Flatscher]

t and evaluation of the script will be 
>> done without
>> compilation. Because of the fallback scripts get compiled with this 
>> version by default. It
>> extends PR 187 #187.
>>
>> To further ease spotting scripts that cause a ScriptException a message 
>> in the form of
>> "filename: caused ScriptException" gets added to the exception handling 
>> in either of the three
>> locations: an error message, a stack trace or a wrap-up into a 
>> RuntimeException (having three
>> different kinds of reporting ScriptExceptions may be questioned, however 
>> none of these tear down
>> the FXML GUI).
>>
>> This WIP comes with proper test units as well. As per Kevin's suggestion a 
>> warning gets logged
>> whenever a script cannot be compiled and the fallback gets used.
>>
>> It is suggested to use this WIP as it includes the compilation by default 
>> with a safe fallback to
>> evaluate the uncompiled script, if compilation (unexpectedly) fails.
>>
>> Again, any feedback, discussion welcome!
>>
>> ---rony
>>
>> P.S.: In the log history there is a commit message "Make message more 
>> pregnant.", it should have
>> read "Make messages more terse." instead|.||
>> |
>>
>>
>> On 17.04.2020 19:37, Rony G. Flatscher wrote:
>>> There is a new WIP at <https://github.com/openjdk/jfx/pull/187> which adds 
>>> a compile PI (process
>>> instruction) for turning on and off script compilation if the script engine 
>>> implements the
>>> Compilable interface.
>>>
>>> By default compilation is off (no compilation), such that one needs to add 
>>> a compile PI
>>> ("") at the top to activate this feature. Supplying "true" 
>>> (default) or "false" as the PI
>>> data turns this feature on and off.
>>>
>>> The WIP comes with adapted test units that test "compile on" for an entire 
>>> fxml file, "compile off",
>>> alternating using "compile on and off", and alternating using "compile off 
>>> and on". This will test
>>> all variants of applying the compile PI for all categories of scripts.
>>>
>>> Any feedback appreciated!
>>>
>>> ---rony
>>>
>>> P.S.: FXML files that contain unknown PIs do not cause a runtime error by 
>>> FXMLLoader, they just get
>>> ignored. Therefore one could apply the compile PI to FXML files that are 
>>> used in older JavaFX runtimes.
>>>
>>> P.P.S.: In the next days I will also add Kevin's idea in a separate version 
>>> that will have a
>>> fallback solution in case a compilation is (unexpectedly) not successful, 
>>> reverting to
>>> (interpretative) evaluation/execution of the script. In that version it is 
>>> planned to have
>>> compilation on by default as in the case of a compilation failure there 
>>> will be a safe backup solution.
>>>
>>>
>>> On 14.04.2020 19:52, Kevin Rushforth wrote:
>>>> Yes, I agree that enough time has gone by. Go ahead with your proposal. I 
>>>> would wait a bit to
>>>> create the CSR until the review is far enough along to know which 
>>>> direction we intend to go.
>>>>
>>>> Unless there is a real concern about possible regressions if scripts are 
>>>> compiled by default, I
>>>> think "enabled by default" is the way to go. Your argument that such 
>>>> script engines are broken
>>>> seems reasonable, since this only applies to script engines that implement 
>>>> javax.script.Compilable
>>>> in the first place. We still might want to add way to turn compilation off 
>>>> for individual scripts.
>>>> One other thing to consider is that if compilation fails, it might make 
>>>> sense to log a warning and
>>>> fall back to the existing interpreted mode.
>>>>
>>>> Does anyone else have any concerns with this?
>>>>
>>>> -- Kevin
>>>>
>>>>
>>>> On 4/14/2020 9:48 AM, Rony G. Flatscher wrote:
>>>>> Hi there,
>>>>>
>>>>> as there was probably enough time that has passed by I would intend to 
>>>>> create a CSR in the next days
>>>>> with the PR as per Kevin's suggestion.
>>>>>
>>>>> (For the case that this feature should not be 

An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engin

[Rony G. Flatscher]

compilation if the script engine 
>> implements the
>> Compilable interface.
>>
>> By default compilation is off (no compilation), such that one needs to add a 
>> compile PI
>> ("") at the top to activate this feature. Supplying "true" 
>> (default) or "false" as the PI
>> data turns this feature on and off.
>>
>> The WIP comes with adapted test units that test "compile on" for an entire 
>> fxml file, "compile off",
>> alternating using "compile on and off", and alternating using "compile off 
>> and on". This will test
>> all variants of applying the compile PI for all categories of scripts.
>>
>> Any feedback appreciated!
>>
>> ---rony
>>
>> P.S.: FXML files that contain unknown PIs do not cause a runtime error by 
>> FXMLLoader, they just get
>> ignored. Therefore one could apply the compile PI to FXML files that are 
>> used in older JavaFX runtimes.
>>
>> P.P.S.: In the next days I will also add Kevin's idea in a separate version 
>> that will have a
>> fallback solution in case a compilation is (unexpectedly) not successful, 
>> reverting to
>> (interpretative) evaluation/execution of the script. In that version it is 
>> planned to have
>> compilation on by default as in the case of a compilation failure there will 
>> be a safe backup solution.
>>
>>
>> On 14.04.2020 19:52, Kevin Rushforth wrote:
>>> Yes, I agree that enough time has gone by. Go ahead with your proposal. I 
>>> would wait a bit to
>>> create the CSR until the review is far enough along to know which direction 
>>> we intend to go.
>>>
>>> Unless there is a real concern about possible regressions if scripts are 
>>> compiled by default, I
>>> think "enabled by default" is the way to go. Your argument that such script 
>>> engines are broken
>>> seems reasonable, since this only applies to script engines that implement 
>>> javax.script.Compilable
>>> in the first place. We still might want to add way to turn compilation off 
>>> for individual scripts.
>>> One other thing to consider is that if compilation fails, it might make 
>>> sense to log a warning and
>>> fall back to the existing interpreted mode.
>>>
>>> Does anyone else have any concerns with this?
>>>
>>> -- Kevin
>>>
>>>
>>> On 4/14/2020 9:48 AM, Rony G. Flatscher wrote:
>>>> Hi there,
>>>>
>>>> as there was probably enough time that has passed by I would intend to 
>>>> create a CSR in the next days
>>>> with the PR as per Kevin's suggestion.
>>>>
>>>> (For the case that this feature should not be active by default, the CSR 
>>>> will suggest to define a
>>>> new "compile" PI in the form  (default, if no PI 
>>>> data given: true), which
>>>> is independent of the existence of a language PI (this way it becomes also 
>>>> possible to allow
>>>> compilation of external scripts denoted with the script-element, which do 
>>>> not need a page language
>>>> to be set as the file's extension allows the appropriate script engine to 
>>>> be loaded and used for
>>>> execution). A compile-PI would allow for turning compilation of scripts on 
>>>> by just adding the PI
>>>>  or   to FXML files (and  to 
>>>> turn off), which seems to
>>>> be simple and self-documentary. In general employing such compile PIs 
>>>> allows for setting compilation
>>>> of scripts on and off throughout an FXML file.)
>>>>
>>>> ---rony
>>>>
>>>>
>>>> On 04.04.2020 18:03, Rony G. Flatscher wrote:
>>>>
>>>>> Hi Kevin,
>>>>>
>>>>> On 03.04.2020 01:21, Kevin Rushforth wrote:
>>>>>> I see that you updated the PR and sent it for review.
>>>>>>
>>>>>> Before we formally review it in the PR, let's finish the discussion as 
>>>>>> to whether this is a useful
>>>>>> feature, and if so, what form this feature should take.
>>>>>>
>>>>>>  From my point of view, this does seem like a useful feature. Would 
>>>>>> other users of FXML benefit
>>>>>> from it?
>>>>> Script code should be executed faster after compilation, so any FXML page 
>>>>&g

A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabe

[Rony G. Flatscher]

There is a new WIP at <https://github.com/openjdk/jfx/pull/192>:

This WIP 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. It
extends PR 187 #187.

To further ease spotting scripts that cause a ScriptException a message in 
the form of
"filename: caused ScriptException" gets added to the exception handling in 
either of the three
locations: an error message, a stack trace or a wrap-up into a 
RuntimeException (having three
different kinds of reporting ScriptExceptions may be questioned, however 
none of these tear down
the FXML GUI).

This WIP comes with proper test units as well. As per Kevin's suggestion a 
warning gets logged
whenever a script cannot be compiled and the fallback gets used.

It is suggested to use this WIP as it includes the compilation by default with 
a safe fallback to
evaluate the uncompiled script, if compilation (unexpectedly) fails.

Again, any feedback, discussion welcome!

---rony

P.S.: In the log history there is a commit message "Make message more 
pregnant.", it should have
read "Make messages more terse." instead|.||
|


On 17.04.2020 19:37, Rony G. Flatscher wrote:
> There is a new WIP at <https://github.com/openjdk/jfx/pull/187> which adds a 
> compile PI (process
> instruction) for turning on and off script compilation if the script engine 
> implements the
> Compilable interface.
>
> By default compilation is off (no compilation), such that one needs to add a 
> compile PI
> ("") at the top to activate this feature. Supplying "true" 
> (default) or "false" as the PI
> data turns this feature on and off.
>
> The WIP comes with adapted test units that test "compile on" for an entire 
> fxml file, "compile off",
> alternating using "compile on and off", and alternating using "compile off 
> and on". This will test
> all variants of applying the compile PI for all categories of scripts.
>
> Any feedback appreciated!
>
> ---rony
>
> P.S.: FXML files that contain unknown PIs do not cause a runtime error by 
> FXMLLoader, they just get
> ignored. Therefore one could apply the compile PI to FXML files that are used 
> in older JavaFX runtimes.
>
> P.P.S.: In the next days I will also add Kevin's idea in a separate version 
> that will have a
> fallback solution in case a compilation is (unexpectedly) not successful, 
> reverting to
> (interpretative) evaluation/execution of the script. In that version it is 
> planned to have
> compilation on by default as in the case of a compilation failure there will 
> be a safe backup solution.
>
>
> On 14.04.2020 19:52, Kevin Rushforth wrote:
>> Yes, I agree that enough time has gone by. Go ahead with your proposal. I 
>> would wait a bit to
>> create the CSR until the review is far enough along to know which direction 
>> we intend to go.
>>
>> Unless there is a real concern about possible regressions if scripts are 
>> compiled by default, I
>> think "enabled by default" is the way to go. Your argument that such script 
>> engines are broken
>> seems reasonable, since this only applies to script engines that implement 
>> javax.script.Compilable
>> in the first place. We still might want to add way to turn compilation off 
>> for individual scripts.
>> One other thing to consider is that if compilation fails, it might make 
>> sense to log a warning and
>> fall back to the existing interpreted mode.
>>
>> Does anyone else have any concerns with this?
>>
>> -- Kevin
>>
>>
>> On 4/14/2020 9:48 AM, Rony G. Flatscher wrote:
>>> Hi there,
>>>
>>> as there was probably enough time that has passed by I would intend to 
>>> create a CSR in the next days
>>> with the PR as per Kevin's suggestion.
>>>
>>> (For the case that this feature should not be active by default, the CSR 
>>> will suggest to define a
>>> new "compile" PI in the form  (default, if no PI 
>>> data given: true), which
>>> is independent of the existence of a language PI (this way it becomes also 
>>> possible to allow
>>> compilation of external scripts denoted with the script-element, which do 
>>> not need a page language
>>> to be set as the file's extension allows the appropriate script engine to 
>>> be loaded and used for
>>> execution). A compile-PI would allow for turning compilation o

WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

There is a new WIP at <https://github.com/openjdk/jfx/pull/187> which adds a 
compile PI (process
instruction) for turning on and off script compilation if the script engine 
implements the
Compilable interface.

By default compilation is off (no compilation), such that one needs to add a 
compile PI
("") at the top to activate this feature. Supplying "true" (default) 
or "false" as the PI
data turns this feature on and off.

The WIP comes with adapted test units that test "compile on" for an entire fxml 
file, "compile off",
alternating using "compile on and off", and alternating using "compile off and 
on". This will test
all variants of applying the compile PI for all categories of scripts.

Any feedback appreciated!

---rony

P.S.: FXML files that contain unknown PIs do not cause a runtime error by 
FXMLLoader, they just get
ignored. Therefore one could apply the compile PI to FXML files that are used 
in older JavaFX runtimes.

P.P.S.: In the next days I will also add Kevin's idea in a separate version 
that will have a
fallback solution in case a compilation is (unexpectedly) not successful, 
reverting to
(interpretative) evaluation/execution of the script. In that version it is 
planned to have
compilation on by default as in the case of a compilation failure there will be 
a safe backup solution.


On 14.04.2020 19:52, Kevin Rushforth wrote:
> Yes, I agree that enough time has gone by. Go ahead with your proposal. I 
> would wait a bit to
> create the CSR until the review is far enough along to know which direction 
> we intend to go.
>
> Unless there is a real concern about possible regressions if scripts are 
> compiled by default, I
> think "enabled by default" is the way to go. Your argument that such script 
> engines are broken
> seems reasonable, since this only applies to script engines that implement 
> javax.script.Compilable
> in the first place. We still might want to add way to turn compilation off 
> for individual scripts.
> One other thing to consider is that if compilation fails, it might make sense 
> to log a warning and
> fall back to the existing interpreted mode.
>
> Does anyone else have any concerns with this?
>
> -- Kevin
>
>
> On 4/14/2020 9:48 AM, Rony G. Flatscher wrote:
>> Hi there,
>>
>> as there was probably enough time that has passed by I would intend to 
>> create a CSR in the next days
>> with the PR as per Kevin's suggestion.
>>
>> (For the case that this feature should not be active by default, the CSR 
>> will suggest to define a
>> new "compile" PI in the form  (default, if no PI 
>> data given: true), which
>> is independent of the existence of a language PI (this way it becomes also 
>> possible to allow
>> compilation of external scripts denoted with the script-element, which do 
>> not need a page language
>> to be set as the file's extension allows the appropriate script engine to be 
>> loaded and used for
>> execution). A compile-PI would allow for turning compilation of scripts on 
>> by just adding the PI
>>  or   to FXML files (and  to 
>> turn off), which seems to
>> be simple and self-documentary. In general employing such compile PIs allows 
>> for setting compilation
>> of scripts on and off throughout an FXML file.)
>>
>> ---rony
>>
>>
>> On 04.04.2020 18:03, Rony G. Flatscher wrote:
>>
>>> Hi Kevin,
>>>
>>> On 03.04.2020 01:21, Kevin Rushforth wrote:
>>>> I see that you updated the PR and sent it for review.
>>>>
>>>> Before we formally review it in the PR, let's finish the discussion as to 
>>>> whether this is a useful
>>>> feature, and if so, what form this feature should take.
>>>>
>>>>  From my point of view, this does seem like a useful feature. Would other 
>>>> users of FXML benefit
>>>> from it?
>>> Script code should be executed faster after compilation, so any FXML page 
>>> that hosts script code
>>> may
>>> benefit.
>>>
>>> The benefits depend on the type of script (and maybe its size and its 
>>> complexity) and also on the
>>> types of event handlers the scripts serve, e.g. move or drag event handlers 
>>> may benefit
>>> significantly. This is because repeated invocation of compiled script event 
>>> handlers do not cause
>>> the reparsing of that script's source and interpreting it on each 
>>> invocation, which may be
>>> expensive
>>> depending on the script engine, but rather allows the immediate 
>>> ev

Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Kevin Rushforth]

Yes, I agree that enough time has gone by. Go ahead with your proposal. 
I would wait a bit to create the CSR until the review is far enough 
along to know which direction we intend to go.


Unless there is a real concern about possible regressions if scripts are 
compiled by default, I think "enabled by default" is the way to go. Your 
argument that such script engines are broken seems reasonable, since 
this only applies to script engines that implement 
javax.script.Compilable in the first place. We still might want to add 
way to turn compilation off for individual scripts. One other thing to 
consider is that if compilation fails, it might make sense to log a 
warning and fall back to the existing interpreted mode.


Does anyone else have any concerns with this?

-- Kevin


On 4/14/2020 9:48 AM, Rony G. Flatscher wrote:

Hi there,

as there was probably enough time that has passed by I would intend to create a 
CSR in the next days
with the PR as per Kevin's suggestion.

(For the case that this feature should not be active by default, the CSR will 
suggest to define a
new "compile" PI in the form  (default, if no PI data 
given: true), which
is independent of the existence of a language PI (this way it becomes also 
possible to allow
compilation of external scripts denoted with the script-element, which do not 
need a page language
to be set as the file's extension allows the appropriate script engine to be 
loaded and used for
execution). A compile-PI would allow for turning compilation of scripts on by 
just adding the PI
 or   to FXML files (and  to turn 
off), which seems to
be simple and self-documentary. In general employing such compile PIs allows 
for setting compilation
of scripts on and off throughout an FXML file.)

---rony


On 04.04.2020 18:03, Rony G. Flatscher wrote:


Hi Kevin,

On 03.04.2020 01:21, Kevin Rushforth wrote:

I see that you updated the PR and sent it for review.

Before we formally review it in the PR, let's finish the discussion as to 
whether this is a useful
feature, and if so, what form this feature should take.

 From my point of view, this does seem like a useful feature. Would other users 
of FXML benefit
from it?

Script code should be executed faster after compilation, so any FXML page that 
hosts script code may
benefit.

The benefits depend on the type of script (and maybe its size and its 
complexity) and also on the
types of event handlers the scripts serve, e.g. move or drag event handlers may 
benefit
significantly. This is because repeated invocation of compiled script event 
handlers do not cause
the reparsing of that script's source and interpreting it on each invocation, 
which may be expensive
depending on the script engine, but rather allows the immediate 
evaluation/execution of the compiled
script by the script engine.


I'm not certain whether we want it to be implicit, compiling the script if the 
script engine in
question implements Compilable, or via a new keyword or tag. What are the pros 
/ cons?

In principle there are three possibilities:

 1) If a script engine implements javax.script.Compilable, compile the 
script and execute the
 compiled version. In the case of event handlers compile and buffer the 
compiled script and
 execute the compiled script each time its registered event fires.

   o Pro: immediately benefits all existing FXML pages that host scripts
   o Con: it is theoretically possible (albeit quite unlikely) that there 
are scripts that fail
 compiling but have been employed successfully in interpreted mode

 2) Introduce some form of an optional attribute (e.g. 
"compile={true|false}") to the FXML
 language PI that switches on compilation of scripts hosted in FXML 
definitions if the script
 engine implements the javax.script.Compilable interface. If missing it would default 
to "false".
 (Alternatively, add a "compile" PI, that if present causes the compilation 
of scripts, if the
 script engine supports it. It would be an error if the "compile" PI was 
present, but the
 "language" PI was not.)

   o Pro: compilation of FXML hosted scripts is done only, if the FXML 
definition of the language
 PI gets changed
   o Con: benefit not made available automatically to existing FXML pages 
that host scripts

 3) Another possibility would be to define a boolean attribute/property 
"compile" for script and
 node elements and only compile the scripts, if the property is set to true

   o Pro: compilation of FXML hosted scripts is done only, if the FXML 
definition gets changed
 accordingly
   o Con: potential benefit not made available automatically to existing 
FXML pages that host scripts

2 and 3 could be combined, where 2 would define the default compilation 
behavior that then could be
overruled individually by 3.

The question would be whether 2 

Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

Hi there,

as there was probably enough time that has passed by I would intend to create a 
CSR in the next days
with the PR as per Kevin's suggestion.

(For the case that this feature should not be active by default, the CSR will 
suggest to define a
new "compile" PI in the form  (default, if no PI data 
given: true), which
is independent of the existence of a language PI (this way it becomes also 
possible to allow
compilation of external scripts denoted with the script-element, which do not 
need a page language
to be set as the file's extension allows the appropriate script engine to be 
loaded and used for
execution). A compile-PI would allow for turning compilation of scripts on by 
just adding the PI
 or   to FXML files (and  to turn 
off), which seems to
be simple and self-documentary. In general employing such compile PIs allows 
for setting compilation
of scripts on and off throughout an FXML file.)

---rony


On 04.04.2020 18:03, Rony G. Flatscher wrote:

> Hi Kevin,
>
> On 03.04.2020 01:21, Kevin Rushforth wrote:
>> I see that you updated the PR and sent it for review.
>>
>> Before we formally review it in the PR, let's finish the discussion as to 
>> whether this is a useful
>> feature, and if so, what form this feature should take.
>>
>> From my point of view, this does seem like a useful feature. Would other 
>> users of FXML benefit
>> from it?
> Script code should be executed faster after compilation, so any FXML page 
> that hosts script code may
> benefit.
>
> The benefits depend on the type of script (and maybe its size and its 
> complexity) and also on the
> types of event handlers the scripts serve, e.g. move or drag event handlers 
> may benefit
> significantly. This is because repeated invocation of compiled script event 
> handlers do not cause
> the reparsing of that script's source and interpreting it on each invocation, 
> which may be expensive
> depending on the script engine, but rather allows the immediate 
> evaluation/execution of the compiled
> script by the script engine.
>
>> I'm not certain whether we want it to be implicit, compiling the script if 
>> the script engine in
>> question implements Compilable, or via a new keyword or tag. What are the 
>> pros / cons?
> In principle there are three possibilities:
>
> 1) If a script engine implements javax.script.Compilable, compile the 
> script and execute the
> compiled version. In the case of event handlers compile and buffer the 
> compiled script and
> execute the compiled script each time its registered event fires. 
>
>   o Pro: immediately benefits all existing FXML pages that host scripts
>   o Con: it is theoretically possible (albeit quite unlikely) that there 
> are scripts that fail
> compiling but have been employed successfully in interpreted mode
>
> 2) Introduce some form of an optional attribute (e.g. 
> "compile={true|false}") to the FXML
> language PI that switches on compilation of scripts hosted in FXML 
> definitions if the script
> engine implements the javax.script.Compilable interface. If missing it 
> would default to "false".
> (Alternatively, add a "compile" PI, that if present causes the 
> compilation of scripts, if the
> script engine supports it. It would be an error if the "compile" PI was 
> present, but the
> "language" PI was not.)
>
>   o Pro: compilation of FXML hosted scripts is done only, if the FXML 
> definition of the language
> PI gets changed
>   o Con: benefit not made available automatically to existing FXML pages 
> that host scripts
>
> 3) Another possibility would be to define a boolean attribute/property 
> "compile" for script and
> node elements and only compile the scripts, if the property is set to true
>
>   o Pro: compilation of FXML hosted scripts is done only, if the FXML 
> definition gets changed
> accordingly
>   o Con: potential benefit not made available automatically to existing 
> FXML pages that host scripts
>
> 2 and 3 could be combined, where 2 would define the default compilation 
> behavior that then could be
> overruled individually by 3.
>
> The question would be whether 2 or/and 3 are really necessary as it can be 
> expected that compilation
> of scripts by the script engines would find the same errors as while 
> interpreting the very same
> scripts (if not, the script engine is badly broken and it could be argued 
> that then the
> interpretation part of the script engine might be expected to be broken as 
> well which would be quite
> dangerous from an integrity POV; the same considera

Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

Hi Kevin,

On 03.04.2020 01:21, Kevin Rushforth wrote:
> I see that you updated the PR and sent it for review.
>
> Before we formally review it in the PR, let's finish the discussion as to 
> whether this is a useful
> feature, and if so, what form this feature should take.
>
> From my point of view, this does seem like a useful feature. Would other 
> users of FXML benefit
> from it?

Script code should be executed faster after compilation, so any FXML page that 
hosts script code may
benefit.

The benefits depend on the type of script (and maybe its size and its 
complexity) and also on the
types of event handlers the scripts serve, e.g. move or drag event handlers may 
benefit
significantly. This is because repeated invocation of compiled script event 
handlers do not cause
the reparsing of that script's source and interpreting it on each invocation, 
which may be expensive
depending on the script engine, but rather allows the immediate 
evaluation/execution of the compiled
script by the script engine.

> I'm not certain whether we want it to be implicit, compiling the script if 
> the script engine in
> question implements Compilable, or via a new keyword or tag. What are the 
> pros / cons?

In principle there are three possibilities:

1) If a script engine implements javax.script.Compilable, compile the 
script and execute the
compiled version. In the case of event handlers compile and buffer the 
compiled script and
execute the compiled script each time its registered event fires. 

  o Pro: immediately benefits all existing FXML pages that host scripts
  o Con: it is theoretically possible (albeit quite unlikely) that there 
are scripts that fail
compiling but have been employed successfully in interpreted mode

2) Introduce some form of an optional attribute (e.g. 
"compile={true|false}") to the FXML
language PI that switches on compilation of scripts hosted in FXML 
definitions if the script
engine implements the javax.script.Compilable interface. If missing it 
would default to "false".
(Alternatively, add a "compile" PI, that if present causes the compilation 
of scripts, if the
script engine supports it. It would be an error if the "compile" PI was 
present, but the
"language" PI was not.)

  o Pro: compilation of FXML hosted scripts is done only, if the FXML 
definition of the language
PI gets changed
  o Con: benefit not made available automatically to existing FXML pages 
that host scripts

3) Another possibility would be to define a boolean attribute/property 
"compile" for script and
node elements and only compile the scripts, if the property is set to true

  o Pro: compilation of FXML hosted scripts is done only, if the FXML 
definition gets changed
accordingly
  o Con: potential benefit not made available automatically to existing 
FXML pages that host scripts

2 and 3 could be combined, where 2 would define the default compilation 
behavior that then could be
overruled individually by 3.

The question would be whether 2 or/and 3 are really necessary as it can be 
expected that compilation
of scripts by the script engines would find the same errors as while 
interpreting the very same
scripts (if not, the script engine is badly broken and it could be argued that 
then the
interpretation part of the script engine might be expected to be broken as well 
which would be quite
dangerous from an integrity POV; the same consideration applies as well if the 
execution of the
compiled script would behave differently to interpreting the very same script 
by the same script
engine).

The current WIP implements 1 above and includes an appropriate test unit.

> What do others think?



> In either case, we would need to make sure that this doesn't present any 
> security concerns in the
> presence of a security manager. As long as a user-provided class is on the 
> stack, it will be fine,
> but we would need to ensure that.

The compilation of scripts needs to be done by the Java script engines 
(implementing both,
javax.script.Engine and javax.script.Compilable) as well as controlling the 
execution of compiled
scripts ([javax.script.CompiledScript]
(https://docs.oracle.com/en/java/javase/14/docs/api/java.scripting/javax/script/CompiledScript.html)).

---rony



> On 4/2/2020 10:41 AM, Rony G. Flatscher wrote:
>> After merging master, applying some fixes and changing the title to reflect 
>> the change from WIP to a
>> pull request I would kindly request a review of this pull request!
>>
>> Here the URL: <https://github.com/openjdk/jfx/pull/129>, title changed to: 
>> "8238080: FXMLLoader: if
>> script engines implement javax.script.Compilable compile scripts".
>>
>> ---rony
>>
>>
>> On 28.02.2020 19

Re: Question ad CSR (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Kevin Rushforth]

Regarding the CSR, that's usually done after the enhancement is agreed 
upon in principle, and the review is far enough along that you are ready 
to write up the spec changes, new API, and/or new interfaces.


Since you don't have direct JBS access you will need a sponsor to do 
that part for you, in which case, creating a comment in the PR with the 
contents of the CSR is the way to go (once we get to that point). Unless 
there ends up being new public API, this will be mainly the spec changes 
in FXMLLoader and the "Introduction to FXML" guide.


-- Kevin


On 4/3/2020 9:57 AM, Rony G. Flatscher wrote:

Hi Kevin,

thank you for your feedback and actions!

Ad "please create a CSR request and add link to it in JDK-8238080": 
preliminarily researching [CSR
FAQ] (https://wiki.openjdk.java.net/display/csr/CSR+FAQs) the question "Q: How do I 
create a CSR ?"
gets answered with:


"A: _Do not directly create a CSR from the Create Menu._ JIRA will let you do 
this right up until

_the moment you try to save it and find your typing was in vain._ Instead you 
should go to the
target bug, select "More", and from the drop down menu select "Create CSR". 
This is required to
properly associate the CSR with the main bug, just as is done for backports.".

Looking at [JDK-8238080] (https://bugs.openjdk.java.net/browse/JDK-8238080) 
there is no possibility
for me to follow the above advice as I have no "More" button/link to select! 
The reason probably
being that I cannot log on.

Once I have the CSR text formulated (may take a little while) what should I do 
with it?

---rony


On 03.04.2020 01:21, Kevin Rushforth wrote:

Hi Rony,

I see that you updated the PR and sent it for review.

Before we formally review it in the PR, let's finish the discussion as to 
whether this is a useful
feature, and if so, what form this feature should take.

 From my point of view, this does seem like a useful feature. Would other users 
of FXML benefit
from it?

I'm not certain whether we want it to be implicit, compiling the script if the 
script engine in
question implements Compilable, or via a new keyword or tag. What are the pros 
/ cons?

What do others think?

In either case, we would need to make sure that this doesn't present any 
security concerns in the
presence of a security manager. As long as a user-provided class is on the 
stack, it will be fine,
but we would need to ensure that.

-- Kevin


On 4/2/2020 10:41 AM, Rony G. Flatscher wrote:

After merging master, applying some fixes and changing the title to reflect the 
change from WIP to a
pull request I would kindly request a review of this pull request!

Here the URL: <https://github.com/openjdk/jfx/pull/129>, title changed to: 
"8238080: FXMLLoader: if
script engines implement javax.script.Compilable compile scripts".

---rony


On 28.02.2020 19:22, Rony G. Flatscher wrote:

Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is 
currently in review, and
has an appropriate test unit going with it as well, please review.

There should be no compatibility issue with this implementation.

Discussion: another solution could be to not compile by default. Rather 
compile, if some new
information is present with the FXML file which cannot possibly be present in 
existing FXML files.
In this scenario one possible and probably simple solution would be to only 
compile scripts if the
language process instruction (e.g. ) contains an appropriate 
attribute with a
value
indicating that compilation should be carried out (e.g.: compile="true"). This 
way only FXML with
PIs having this attribute set to true would be affected. If desired I could try 
to come up with a
respective solution as well.

---rony

[1] "Implementation and test unit": <https://github.com/openjdk/jfx/pull/129>

[2] "JDK-8238080 : FXMLLoader: if script engines implement 
javax.script.Compilable compile
scripts":
<https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>

[3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and 
ARGV":
<https://github.com/openjdk/jfx/pull/122/commits>


On 24.01.2020 16:26, Kevin Rushforth wrote:


Thank you for filing this enhancement request. As an enhancement it should be 
discussed on this
list before proceeding with a pull request (although a "WIP" or Draft PR can be 
used to illustrate
the concept).

For my part, this seems like a reasonable enhancement, as long as there are no 
compatibility
issues, but it would be good to hear from application developers who heavily 
use FXML.

-- Kevin


On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:

Just filed a RFE with the following information:

     * Component:
     o JavaFX

     * Subcomponent:
     o fxml: JavaFX FXML

     * Synopsis:
     o "FXMLLoader: if script engines implement javax.

Question ad CSR (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

Hi Kevin,

thank you for your feedback and actions!

Ad "please create a CSR request and add link to it in JDK-8238080": 
preliminarily researching [CSR
FAQ] (https://wiki.openjdk.java.net/display/csr/CSR+FAQs) the question "Q: How 
do I create a CSR ?"
gets answered with:

> "A: _Do not directly create a CSR from the Create Menu._ JIRA will let you do 
> this right up until
_the moment you try to save it and find your typing was in vain._ Instead you 
should go to the
target bug, select "More", and from the drop down menu select "Create CSR". 
This is required to
properly associate the CSR with the main bug, just as is done for backports.".

Looking at [JDK-8238080] (https://bugs.openjdk.java.net/browse/JDK-8238080) 
there is no possibility
for me to follow the above advice as I have no "More" button/link to select! 
The reason probably
being that I cannot log on.

Once I have the CSR text formulated (may take a little while) what should I do 
with it?

---rony


On 03.04.2020 01:21, Kevin Rushforth wrote:
> Hi Rony,
>
> I see that you updated the PR and sent it for review.
>
> Before we formally review it in the PR, let's finish the discussion as to 
> whether this is a useful
> feature, and if so, what form this feature should take.
>
> From my point of view, this does seem like a useful feature. Would other 
> users of FXML benefit
> from it?
>
> I'm not certain whether we want it to be implicit, compiling the script if 
> the script engine in
> question implements Compilable, or via a new keyword or tag. What are the 
> pros / cons?
>
> What do others think?
>
> In either case, we would need to make sure that this doesn't present any 
> security concerns in the
> presence of a security manager. As long as a user-provided class is on the 
> stack, it will be fine,
> but we would need to ensure that.
>
> -- Kevin
>
>
> On 4/2/2020 10:41 AM, Rony G. Flatscher wrote:
>> After merging master, applying some fixes and changing the title to reflect 
>> the change from WIP to a
>> pull request I would kindly request a review of this pull request!
>>
>> Here the URL: <https://github.com/openjdk/jfx/pull/129>, title changed to: 
>> "8238080: FXMLLoader: if
>> script engines implement javax.script.Compilable compile scripts".
>>
>> ---rony
>>
>>
>> On 28.02.2020 19:22, Rony G. Flatscher wrote:
>>> Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is 
>>> currently in review, and
>>> has an appropriate test unit going with it as well, please review.
>>>
>>> There should be no compatibility issue with this implementation.
>>>
>>> Discussion: another solution could be to not compile by default. Rather 
>>> compile, if some new
>>> information is present with the FXML file which cannot possibly be present 
>>> in existing FXML files.
>>> In this scenario one possible and probably simple solution would be to only 
>>> compile scripts if the
>>> language process instruction (e.g. ) contains an 
>>> appropriate attribute with a
>>> value
>>> indicating that compilation should be carried out (e.g.: compile="true"). 
>>> This way only FXML with
>>> PIs having this attribute set to true would be affected. If desired I could 
>>> try to come up with a
>>> respective solution as well.
>>>
>>> ---rony
>>>
>>> [1] "Implementation and test unit": 
>>> <https://github.com/openjdk/jfx/pull/129>
>>>
>>> [2] "JDK-8238080 : FXMLLoader: if script engines implement 
>>> javax.script.Compilable compile
>>> scripts":
>>> <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>
>>>
>>> [3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with 
>>> FILENAME and ARGV":
>>> <https://github.com/openjdk/jfx/pull/122/commits>
>>>
>>>
>>> On 24.01.2020 16:26, Kevin Rushforth wrote:
>>>
>>>> Thank you for filing this enhancement request. As an enhancement it should 
>>>> be discussed on this
>>>> list before proceeding with a pull request (although a "WIP" or Draft PR 
>>>> can be used to illustrate
>>>> the concept).
>>>>
>>>> For my part, this seems like a reasonable enhancement, as long as there 
>>>> are no compatibility
>>>> issues, but it would be good to hear from application developers who 
>>>> heavily use FXML.
>>

Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Kevin Rushforth]

Hi Rony,

I see that you updated the PR and sent it for review.

Before we formally review it in the PR, let's finish the discussion as 
to whether this is a useful feature, and if so, what form this feature 
should take.


From my point of view, this does seem like a useful feature. Would 
other users of FXML benefit from it?


I'm not certain whether we want it to be implicit, compiling the script 
if the script engine in question implements Compilable, or via a new 
keyword or tag. What are the pros / cons?


What do others think?

In either case, we would need to make sure that this doesn't present any 
security concerns in the presence of a security manager. As long as a 
user-provided class is on the stack, it will be fine, but we would need 
to ensure that.


-- Kevin


On 4/2/2020 10:41 AM, Rony G. Flatscher wrote:

After merging master, applying some fixes and changing the title to reflect the 
change from WIP to a
pull request I would kindly request a review of this pull request!

Here the URL: <https://github.com/openjdk/jfx/pull/129>, title changed to: 
"8238080: FXMLLoader: if
script engines implement javax.script.Compilable compile scripts".

---rony


On 28.02.2020 19:22, Rony G. Flatscher wrote:

Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is 
currently in review, and
has an appropriate test unit going with it as well, please review.

There should be no compatibility issue with this implementation.

Discussion: another solution could be to not compile by default. Rather 
compile, if some new
information is present with the FXML file which cannot possibly be present in 
existing FXML files.
In this scenario one possible and probably simple solution would be to only 
compile scripts if the
language process instruction (e.g. ) contains an appropriate 
attribute with a value
indicating that compilation should be carried out (e.g.: compile="true"). This 
way only FXML with
PIs having this attribute set to true would be affected. If desired I could try 
to come up with a
respective solution as well.

---rony

[1] "Implementation and test unit": <https://github.com/openjdk/jfx/pull/129>

[2] "JDK-8238080 : FXMLLoader: if script engines implement javax.script.Compilable 
compile scripts":
<https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>

[3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and 
ARGV":
<https://github.com/openjdk/jfx/pull/122/commits>


On 24.01.2020 16:26, Kevin Rushforth wrote:


Thank you for filing this enhancement request. As an enhancement it should be 
discussed on this
list before proceeding with a pull request (although a "WIP" or Draft PR can be 
used to illustrate
the concept).

For my part, this seems like a reasonable enhancement, as long as there are no 
compatibility
issues, but it would be good to hear from application developers who heavily 
use FXML.

-- Kevin


On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:

Just filed a RFE with the following information:

    * Component:
    o JavaFX

    * Subcomponent:
    o fxml: JavaFX FXML

    * Synopsis:
    o "FXMLLoader: if script engines implement javax.script.Compilabel compile 
scripts"

    * Descriptions:
    o "FXMLLoader is able to execute scripts in Java script languages 
(javax.script.ScriptEngine
  implementations) if such a Java script language gets defined as the 
controller language in
  the FXML file.

  If a script engine implements the javax.script.Compilable interface, 
then such scripts
could
  be compiled and the resulting javax.script.CompiledScript could be 
executed instead using
  its eval() methods.

  Evaluating the CompiledScript objects may help speed up the execution 
of script
invocations,
  especially for scripts defined for event attributes in FXML elements 
(e.g. like
onMouseMove)
  which may be repetitevly invoked and evaluated."

    * System /OS/Java Runtime Information:
    o "All systems."

Received the internal review ID: "9063426"

---rony

Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

After merging master, applying some fixes and changing the title to reflect the 
change from WIP to a
pull request I would kindly request a review of this pull request!

Here the URL: <https://github.com/openjdk/jfx/pull/129>, title changed to: 
"8238080: FXMLLoader: if
script engines implement javax.script.Compilable compile scripts".

---rony


On 28.02.2020 19:22, Rony G. Flatscher wrote:
> Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is 
> currently in review, and
> has an appropriate test unit going with it as well, please review.
>
> There should be no compatibility issue with this implementation.
>
> Discussion: another solution could be to not compile by default. Rather 
> compile, if some new
> information is present with the FXML file which cannot possibly be present in 
> existing FXML files.
> In this scenario one possible and probably simple solution would be to only 
> compile scripts if the
> language process instruction (e.g. ) contains an appropriate 
> attribute with a value
> indicating that compilation should be carried out (e.g.: compile="true"). 
> This way only FXML with
> PIs having this attribute set to true would be affected. If desired I could 
> try to come up with a
> respective solution as well.
>
> ---rony
>
> [1] "Implementation and test unit": <https://github.com/openjdk/jfx/pull/129>
>
> [2] "JDK-8238080 : FXMLLoader: if script engines implement 
> javax.script.Compilable compile scripts":
> <https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>
>
> [3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with 
> FILENAME and ARGV":
> <https://github.com/openjdk/jfx/pull/122/commits>
>
>
> On 24.01.2020 16:26, Kevin Rushforth wrote:
>
>> Thank you for filing this enhancement request. As an enhancement it should 
>> be discussed on this
>> list before proceeding with a pull request (although a "WIP" or Draft PR can 
>> be used to illustrate
>> the concept).
>>
>> For my part, this seems like a reasonable enhancement, as long as there are 
>> no compatibility
>> issues, but it would be good to hear from application developers who heavily 
>> use FXML.
>>
>> -- Kevin
>>
>>
>> On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:
>>> Just filed a RFE with the following information:
>>>
>>>    * Component:
>>>    o JavaFX
>>>
>>>    * Subcomponent:
>>>    o fxml: JavaFX FXML
>>>
>>>    * Synopsis:
>>>    o "FXMLLoader: if script engines implement javax.script.Compilabel 
>>> compile scripts"
>>>
>>>    * Descriptions:
>>>    o "FXMLLoader is able to execute scripts in Java script languages 
>>> (javax.script.ScriptEngine
>>>  implementations) if such a Java script language gets defined as 
>>> the controller language in
>>>  the FXML file.
>>>
>>>  If a script engine implements the javax.script.Compilable 
>>> interface, then such scripts
>>> could
>>>  be compiled and the resulting javax.script.CompiledScript could be 
>>> executed instead using
>>>  its eval() methods.
>>>
>>>  Evaluating the CompiledScript objects may help speed up the 
>>> execution of script
>>> invocations,
>>>  especially for scripts defined for event attributes in FXML 
>>> elements (e.g. like
>>> onMouseMove)
>>>  which may be repetitevly invoked and evaluated."
>>>
>>>    * System /OS/Java Runtime Information:
>>>    o "All systems."
>>>
>>> Received the internal review ID: "9063426"
>>>
>>> ---rony 

A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is 
currently in review, and
has an appropriate test unit going with it as well, please review.

There should be no compatibility issue with this implementation.

Discussion: another solution could be to not compile by default. Rather 
compile, if some new
information is present with the FXML file which cannot possibly be present in 
existing FXML files.
In this scenario one possible and probably simple solution would be to only 
compile scripts if the
language process instruction (e.g. ) contains an appropriate 
attribute with a value
indicating that compilation should be carried out (e.g.: compile="true"). This 
way only FXML with
PIs having this attribute set to true would be affected. If desired I could try 
to come up with a
respective solution as well.

---rony

[1] "Implementation and test unit": <https://github.com/openjdk/jfx/pull/129>

[2] "JDK-8238080 : FXMLLoader: if script engines implement 
javax.script.Compilable compile scripts":
<https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8238080>

[3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME 
and ARGV":
<https://github.com/openjdk/jfx/pull/122/commits>


On 24.01.2020 16:26, Kevin Rushforth wrote:

> Thank you for filing this enhancement request. As an enhancement it should be 
> discussed on this
> list before proceeding with a pull request (although a "WIP" or Draft PR can 
> be used to illustrate
> the concept).
>
> For my part, this seems like a reasonable enhancement, as long as there are 
> no compatibility
> issues, but it would be good to hear from application developers who heavily 
> use FXML.
>
> -- Kevin
>
>
> On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:
>> Just filed a RFE with the following information:
>>
>>    * Component:
>>    o JavaFX
>>
>>    * Subcomponent:
>>    o fxml: JavaFX FXML
>>
>>    * Synopsis:
>>    o "FXMLLoader: if script engines implement javax.script.Compilabel 
>> compile scripts"
>>
>>    * Descriptions:
>>    o "FXMLLoader is able to execute scripts in Java script languages 
>> (javax.script.ScriptEngine
>>  implementations) if such a Java script language gets defined as the 
>> controller language in
>>  the FXML file.
>>
>>  If a script engine implements the javax.script.Compilable 
>> interface, then such scripts
>> could
>>  be compiled and the resulting javax.script.CompiledScript could be 
>> executed instead using
>>  its eval() methods.
>>
>>  Evaluating the CompiledScript objects may help speed up the 
>> execution of script
>> invocations,
>>  especially for scripts defined for event attributes in FXML 
>> elements (e.g. like
>> onMouseMove)
>>  which may be repetitevly invoked and evaluated."
>>
>>    * System /OS/Java Runtime Information:
>>    o "All systems."
>>
>> Received the internal review ID: "9063426"
>>
>> ---rony 

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

Hi Kevin,

On 29.01.2020 13:24, Kevin Rushforth wrote:
> The RFE you filed is now available here:
>
> https://bugs.openjdk.java.net/browse/JDK-8238080

thank you very much!

Cheers

---rony

P.S.: Have not received any automatic notification e-mail so far.

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Kevin Rushforth]

Hi Rony,

The RFE you filed is now available here:

https://bugs.openjdk.java.net/browse/JDK-8238080

-- Kevin


On 1/25/2020 6:32 AM, Rony G. Flatscher wrote:

Hi Kevin,

On 24.01.2020 16:50, Kevin Rushforth wrote:

This bug was transferred to the JDK project on 28-Nov-2019. I don't know why 
you didn't get an
email at that time, but I will inquire of the team who processes incoming bugs.

Also, I'll keep an eye out for the RFE you filed today, and let you know when 
it is transferred in
case there is still a problem with the notification.

thank you very much!

---rony



On 1/22/2020 9:52 AM, Rony G. Flatscher wrote:

Hi Anthony,

On 22.01.2020 17:07, Anthony Vanelverdinghe wrote:

Your issue has been converted into a JDK issue, with your testcase attached [1].

Thank you *very* much for this information!


Normally you should’ve received an e-mail at the time of this conversion,

Just searched all my e-mail folders and could not find it (looking for 
"FXMLLoader" in the subject
of e-mails as the bug title contains that word) but could not find a matching 
e-mail for whatever
reasons.


but you can check this yourself by using the internal review ID as in [2]. If 
you’d like to
contribute a fix, see [3].

  
Kind regards, Anthony


  
[1] https://bugs.openjdk.java.net/browse/JDK-8234959

<https://bugs.openjdk.java.net/browse/JDK-8234959>

[2] https://bugs.openjdk.java.net/browse/JI-9062887
<https://bugs.openjdk.java.net/browse/JI-9062887>

[3] https://github.com/openjdk/jfx <https://github.com/openjdk/jfx>


Thank you also for these links (and I learned something new on how to check for 
it using the
internal review id with your [2], thanks a lot for this hint as well)!

Will go back and study all the necessary procedures (forgot a lot since reading 
them the last time)
and will try to contribute the fix in the proper way but it may take me a 
little while (currently
quite busy around here).

---

Maybe one more question: there would be an optimization possible by compiling 
scripts for script
engines that have the javax.script.Compilable interface implemented and use the 
compiled version to
execute/evaluate the scripts (may be helpful for event handler code e.g. for 
onMouseMove event
handlers). Can the fix include such an optimization or should there be a 
separate discussion/RFE for
it beforehand? (Adding this would be trivial in the context of the fix, however 
the bug description
would not hint at such an optimization.)

---rony

Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

On 24.01.2020 16:26, Kevin Rushforth wrote:
> Thank you for filing this enhancement request. As an enhancement it should be 
> discussed on this
> list before proceeding with a pull request (although a "WIP" or Draft PR can 
> be used to illustrate
> the concept).
Sure, will do after creating an appropriate PR for adding the ARGV and FILENAME 
entries to the
engine scope bindings [1] (may take a little while).
> For my part, this seems like a reasonable enhancement, as long as there are 
> no compatibility
> issues, but it would be good to hear from application developers who heavily 
> use FXML.

+1

---rony


[1] https://bugs.openjdk.java.net/browse/JDK-8234959

>
>
> On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:
>> Just filed a RFE with the following information:
>>
>>    * Component:
>>    o JavaFX
>>
>>    * Subcomponent:
>>    o fxml: JavaFX FXML
>>
>>    * Synopsis:
>>    o "FXMLLoader: if script engines implement javax.script.Compilabel 
>> compile scripts"
>>
>>    * Descriptions:
>>    o "FXMLLoader is able to execute scripts in Java script languages 
>> (javax.script.ScriptEngine
>>  implementations) if such a Java script language gets defined as the 
>> controller language in
>>  the FXML file.
>>
>>  If a script engine implements the javax.script.Compilable 
>> interface, then such scripts
>> could
>>  be compiled and the resulting javax.script.CompiledScript could be 
>> executed instead using
>>  its eval() methods.
>>
>>  Evaluating the CompiledScript objects may help speed up the 
>> execution of script
>> invocations,
>>  especially for scripts defined for event attributes in FXML 
>> elements (e.g. like
>> onMouseMove)
>>  which may be repetitevly invoked and evaluated."
>>
>>    * System /OS/Java Runtime Information:
>>    o "All systems."
>>
>> Received the internal review ID: "9063426"
>>
>> ---rony 

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

Hi Kevin,

On 24.01.2020 16:50, Kevin Rushforth wrote:
> This bug was transferred to the JDK project on 28-Nov-2019. I don't know why 
> you didn't get an
> email at that time, but I will inquire of the team who processes incoming 
> bugs.
>
> Also, I'll keep an eye out for the RFE you filed today, and let you know when 
> it is transferred in
> case there is still a problem with the notification.

thank you very much!

---rony


>
> On 1/22/2020 9:52 AM, Rony G. Flatscher wrote:
>> Hi Anthony,
>>
>> On 22.01.2020 17:07, Anthony Vanelverdinghe wrote:
>>> Your issue has been converted into a JDK issue, with your testcase attached 
>>> [1].
>> Thank you *very* much for this information!
>>
>>> Normally you should’ve received an e-mail at the time of this conversion,
>> Just searched all my e-mail folders and could not find it (looking for 
>> "FXMLLoader" in the subject
>> of e-mails as the bug title contains that word) but could not find a 
>> matching e-mail for whatever
>> reasons.
>>
>>> but you can check this yourself by using the internal review ID as in [2]. 
>>> If you’d like to
>>> contribute a fix, see [3].
>>>
>>>  
>>> Kind regards, Anthony
>>>
>>>  
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8234959
>>> <https://bugs.openjdk.java.net/browse/JDK-8234959>
>>>
>>> [2] https://bugs.openjdk.java.net/browse/JI-9062887
>>> <https://bugs.openjdk.java.net/browse/JI-9062887>
>>>
>>> [3] https://github.com/openjdk/jfx <https://github.com/openjdk/jfx>
>>>
>> Thank you also for these links (and I learned something new on how to check 
>> for it using the
>> internal review id with your [2], thanks a lot for this hint as well)!
>>
>> Will go back and study all the necessary procedures (forgot a lot since 
>> reading them the last time)
>> and will try to contribute the fix in the proper way but it may take me a 
>> little while (currently
>> quite busy around here).
>>
>> ---
>>
>> Maybe one more question: there would be an optimization possible by 
>> compiling scripts for script
>> engines that have the javax.script.Compilable interface implemented and use 
>> the compiled version to
>> execute/evaluate the scripts (may be helpful for event handler code e.g. for 
>> onMouseMove event
>> handlers). Can the fix include such an optimization or should there be a 
>> separate discussion/RFE for
>> it beforehand? (Adding this would be trivial in the context of the fix, 
>> however the bug description
>> would not hint at such an optimization.)
>>
>> ---rony

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Kevin Rushforth]

Hi Rony,

This bug was transferred to the JDK project on 28-Nov-2019. I don't know 
why you didn't get an email at that time, but I will inquire of the team 
who processes incoming bugs.


Also, I'll keep an eye out for the RFE you filed today, and let you know 
when it is transferred in case there is still a problem with the 
notification.


-- Kevin


On 1/22/2020 9:52 AM, Rony G. Flatscher wrote:

Hi Anthony,

On 22.01.2020 17:07, Anthony Vanelverdinghe wrote:

Your issue has been converted into a JDK issue, with your testcase attached [1].

Thank you *very* much for this information!


Normally you should’ve received an e-mail at the time of this conversion,

Just searched all my e-mail folders and could not find it (looking for 
"FXMLLoader" in the subject
of e-mails as the bug title contains that word) but could not find a matching 
e-mail for whatever
reasons.


but you can check this yourself by using the internal review ID as in [2]. If 
you’d like to
contribute a fix, see [3].

  


Kind regards, Anthony

  


[1] https://bugs.openjdk.java.net/browse/JDK-8234959
<https://bugs.openjdk.java.net/browse/JDK-8234959>

[2] https://bugs.openjdk.java.net/browse/JI-9062887 
<https://bugs.openjdk.java.net/browse/JI-9062887>

[3] https://github.com/openjdk/jfx <https://github.com/openjdk/jfx>


Thank you also for these links (and I learned something new on how to check for 
it using the
internal review id with your [2], thanks a lot for this hint as well)!

Will go back and study all the necessary procedures (forgot a lot since reading 
them the last time)
and will try to contribute the fix in the proper way but it may take me a 
little while (currently
quite busy around here).

---

Maybe one more question: there would be an optimization possible by compiling 
scripts for script
engines that have the javax.script.Compilable interface implemented and use the 
compiled version to
execute/evaluate the scripts (may be helpful for event handler code e.g. for 
onMouseMove event
handlers). Can the fix include such an optimization or should there be a 
separate discussion/RFE for
it beforehand? (Adding this would be trivial in the context of the fix, however 
the bug description
would not hint at such an optimization.)

---rony

Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Kevin Rushforth]

Thank you for filing this enhancement request. As an enhancement it 
should be discussed on this list before proceeding with a pull request 
(although a "WIP" or Draft PR can be used to illustrate the concept).


For my part, this seems like a reasonable enhancement, as long as there 
are no compatibility issues, but it would be good to hear from 
application developers who heavily use FXML.


-- Kevin


On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:

Just filed a RFE with the following information:

   * Component:
   o JavaFX

   * Subcomponent:
   o fxml: JavaFX FXML

   * Synopsis:
   o "FXMLLoader: if script engines implement javax.script.Compilabel compile 
scripts"

   * Descriptions:
   o "FXMLLoader is able to execute scripts in Java script languages 
(javax.script.ScriptEngine
 implementations) if such a Java script language gets defined as the 
controller language in
 the FXML file.

 If a script engine implements the javax.script.Compilable interface, 
then such scripts could
 be compiled and the resulting javax.script.CompiledScript could be 
executed instead using
 its eval() methods.

 Evaluating the CompiledScript objects may help speed up the execution 
of script invocations,
 especially for scripts defined for event attributes in FXML elements 
(e.g. like onMouseMove)
 which may be repetitevly invoked and evaluated."

   * System /OS/Java Runtime Information:
   o "All systems."

Received the internal review ID: "9063426"

---rony

"Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

Just filed a RFE with the following information:

  * Component:
  o JavaFX

  * Subcomponent:
  o fxml: JavaFX FXML

  * Synopsis:
  o "FXMLLoader: if script engines implement javax.script.Compilabel 
compile scripts"

  * Descriptions:
  o "FXMLLoader is able to execute scripts in Java script languages 
(javax.script.ScriptEngine
implementations) if such a Java script language gets defined as the 
controller language in
the FXML file.

If a script engine implements the javax.script.Compilable interface, 
then such scripts could
be compiled and the resulting javax.script.CompiledScript could be 
executed instead using
its eval() methods.

Evaluating the CompiledScript objects may help speed up the execution 
of script invocations,
especially for scripts defined for event attributes in FXML elements 
(e.g. like onMouseMove)
which may be repetitevly invoked and evaluated."

  * System /OS/Java Runtime Information:
  o "All systems."

Received the internal review ID: "9063426"

---rony

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

On 23.01.2020 18:09, Anthony Vanelverdinghe wrote:
> On 22/01/2020 18:52, Rony G. Flatscher wrote:
... cut ...
>> Maybe one more question: there would be an optimization possible by 
>> compiling scripts for script
>> engines that have the javax.script.Compilable interface implemented and use 
>> the compiled version
>> to execute/evaluate the scripts (may be helpful for event handler code e.g. 
>> for onMouseMove event
>> handlers). Can the fix include such an optimization or should there be a 
>> separate discussion/RFE
>> for it beforehand? (Adding this would be trivial in the context of the fix, 
>> however the bug
>> description would not hint at such an optimization.)
> In my opinion, this should be filed as a separate issue, since it's unrelated 
> to the current issue
> and is an enhancement, rather than a bug.

Thank you very much Anthony, will do.

---rony

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Anthony Vanelverdinghe]

On 22/01/2020 18:52, Rony G. Flatscher wrote:

Hi Anthony,

On 22.01.2020 17:07, Anthony Vanelverdinghe wrote:
Your issue has been converted into a JDK issue, with your testcase 
attached [1].


Thank you *very* much for this information!

Normally you should’ve received an e-mail at the time of this 
conversion,


Just searched all my e-mail folders and could not find it (looking for 
"FXMLLoader" in the subject of e-mails as the bug title contains that 
word) but could not find a matching e-mail for whatever reasons.


but you can check this yourself by using the internal review ID as in 
[2]. If you’d like to contribute a fix, see [3].


Kind regards, Anthony

[1] https://bugs.openjdk.java.net/browse/JDK-8234959 
<https://bugs.openjdk.java.net/browse/JDK-8234959>


[2] https://bugs.openjdk.java.net/browse/JI-9062887 
<https://bugs.openjdk.java.net/browse/JI-9062887>


[3] https://github.com/openjdk/jfx <https://github.com/openjdk/jfx>

Thank you also for these links (and I learned something new on how to 
check for it using the internal review id with your [2], thanks a lot 
for this hint as well)!


Will go back and study all the necessary procedures (forgot a lot 
since reading them the last time) and will try to contribute the fix 
in the proper way but it may take me a little while (currently quite 
busy around here).


---

Maybe one more question: there would be an optimization possible by 
compiling scripts for script engines that have the 
javax.script.Compilable interface implemented and use the compiled 
version to execute/evaluate the scripts (may be helpful for event 
handler code e.g. for onMouseMove event handlers). Can the fix include 
such an optimization or should there be a separate discussion/RFE for 
it beforehand? (Adding this would be trivial in the context of the 
fix, however the bug description would not hint at such an optimization.)


In my opinion, this should be filed as a separate issue, since it's 
unrelated to the current issue and is an enhancement, rather than a bug.


---rony



Kind regards, Anthony

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

Hi Anthony,

On 22.01.2020 17:07, Anthony Vanelverdinghe wrote:
> Your issue has been converted into a JDK issue, with your testcase attached 
> [1].

Thank you *very* much for this information!

> Normally you should’ve received an e-mail at the time of this conversion,

Just searched all my e-mail folders and could not find it (looking for 
"FXMLLoader" in the subject
of e-mails as the bug title contains that word) but could not find a matching 
e-mail for whatever
reasons.

> but you can check this yourself by using the internal review ID as in [2]. If 
> you’d like to
> contribute a fix, see [3].
>
>  
>
> Kind regards, Anthony
>
>  
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8234959
> <https://bugs.openjdk.java.net/browse/JDK-8234959>
>
> [2] https://bugs.openjdk.java.net/browse/JI-9062887 
> <https://bugs.openjdk.java.net/browse/JI-9062887>
>
> [3] https://github.com/openjdk/jfx <https://github.com/openjdk/jfx>
>
Thank you also for these links (and I learned something new on how to check for 
it using the
internal review id with your [2], thanks a lot for this hint as well)!

Will go back and study all the necessary procedures (forgot a lot since reading 
them the last time)
and will try to contribute the fix in the proper way but it may take me a 
little while (currently
quite busy around here).

---

Maybe one more question: there would be an optimization possible by compiling 
scripts for script
engines that have the javax.script.Compilable interface implemented and use the 
compiled version to
execute/evaluate the scripts (may be helpful for event handler code e.g. for 
onMouseMove event
handlers). Can the fix include such an optimization or should there be a 
separate discussion/RFE for
it beforehand? (Adding this would be trivial in the context of the fix, however 
the bug description
would not hint at such an optimization.)

---rony

RE: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Anthony Vanelverdinghe]

Hi Rony

Your issue has been converted into a JDK issue, with your testcase attached 
[1]. Normally you should’ve received an e-mail at the time of this conversion, 
but you can check this yourself by using the internal review ID as in [2]. If 
you’d like to contribute a fix, see [3].

Kind regards, Anthony

[1] https://bugs.openjdk.java.net/browse/JDK-8234959
[2] https://bugs.openjdk.java.net/browse/JI-9062887
[3] https://github.com/openjdk/jfx


From: Rony G. Flatscher<mailto:rony.flatsc...@wu.ac.at>
Sent: Wednesday, 22 January 2020 14:44
To: openjfx-dev@openjdk.java.net<mailto:openjfx-dev@openjdk.java.net>
Subject: Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying 
filename to script engine, not supplying event object as argument to script

Last November I submitted an appropriate bug report and mailed the testcase on 
November 27th per
Oracle's request without hearing anything to this date.

Therefore I was wondering how long such an assessment usually takes place and 
what to do? (Maybe it
"fell off the desk" due to the end-of-year stress and Christmas vacation?) Any 
advice?

---rony


On 21.11.2019 15:39, Rony G. Flatscher wrote:
> As the zip-archive attachment got stripped, for a brief time the zip-archive 
> can be fetched from
> <https://www.dropbox.com/s/l4uesrwm0iw5vb9/testcaseFXMLLoaderScriptEngines.zip?dl=0>.
>
> ---rony
>
> On 21.11.2019 15:29, Rony G. Flatscher wrote:
>> On 15.11.2019 16:08, Rony G. Flatscher wrote:
>>> On 14.11.2019 22:57, Kevin Rushforth wrote:
>>>> On 11/14/2019 10:12 AM, Rony G. Flatscher wrote:
>>>>> On 14.11.2019 16:34, Rony G. Flatscher wrote:
>>>>>> On 13.11.2019 19:50, Kevin Rushforth wrote:
>>>>>>> On 11/13/2019 9:42 AM, Rony G. Flatscher wrote:
>>>>> ... cut ...
>>>>>>>> To reproduce the testcase one would need ooRexx and the Java bridge 
>>>>>>>> BSF4ooRexx (all
>>>>>>>> opensource) for
>>>>>>>> which I could come up with a zip-archive (assuming binaries within 
>>>>>>>> should be 64-bit) and a
>>>>>>>> script to
>>>>>>>> set up the environment either for Windows, Linux or MacOS, whatever 
>>>>>>>> you advise. Would that be
>>>>>>>> o.k.?
>>>>>>> We prefer not to rely on third-party libraries for test cases. In any 
>>>>>>> case we would not be able to
>>>>>>> use that for a regression test / unit test.
>>>>> If test units really seem to be important in this particular case, one 
>>>>> possibility would be to
>>>>> create a minimalistic ScriptEngine implementation in pure Java just for 
>>>>> the sole purpose to allow
>>>>> the creation of a test unit that is able to assert that FXMLLoader puts 
>>>>> the ScriptEngine.ARGV and
>>>>> ScriptEngine.FILENAME entries into the ENGINE_SCOPE Bindings. E.g. having 
>>>>> the ScriptEngine's eval()
>>>>> methods return the ScriptContext at invocation time in order to allow 
>>>>> inspection of the Bindings.
>>>>> This way it would become also possible to write in addition test units 
>>>>> that also check whether all
>>>>> FXML elements that carry a fx:id are really placed into the GLOBAL_SCOPE 
>>>>> Bindings.
>>>> Something like that seems reasonable, and would avoid a dependence on 
>>>> Nashorn, which in addition
>>>> to having all the problems you mentioned, is deprecated for removal.
>>>>
>>>>> However,
>>>> Did you have something more to add?
>>> No, sorry for that. Rewrote my e-mail and had sent it too early by mistake 
>>> and without noticing.
>>>
>>> Will study all the procedures and create a testcase to be submitted at 
>>> <https://bugreport.java.com/>
>>> as per your advice (and will report back under this thread once submitted). 
>>> The testcase would use
>>> an artificial ScriptEngine implementation that could be used for testunit 
>>> testing as well. This
>>> might take a while due to other obligations that I will have to meet during 
>>> the next few days.
>>>
>>> ---rony
>> O.K., so came up with a test case that contains an artificial script engine 
>> implementation for
>> logging the eval() invocations together with the scripts to execute and the 
>> ScriptContext
>> ENGINE_SCOPE and GLOBAL_SCOPE Bindings at the time of th

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

Last November I submitted an appropriate bug report and mailed the testcase on 
November 27th per
Oracle's request without hearing anything to this date.

Therefore I was wondering how long such an assessment usually takes place and 
what to do? (Maybe it
"fell off the desk" due to the end-of-year stress and Christmas vacation?) Any 
advice?

---rony


On 21.11.2019 15:39, Rony G. Flatscher wrote:
> As the zip-archive attachment got stripped, for a brief time the zip-archive 
> can be fetched from
> .
>
> ---rony
>
> On 21.11.2019 15:29, Rony G. Flatscher wrote:
>> On 15.11.2019 16:08, Rony G. Flatscher wrote:
>>> On 14.11.2019 22:57, Kevin Rushforth wrote:
 On 11/14/2019 10:12 AM, Rony G. Flatscher wrote:
> On 14.11.2019 16:34, Rony G. Flatscher wrote:
>> On 13.11.2019 19:50, Kevin Rushforth wrote:
>>> On 11/13/2019 9:42 AM, Rony G. Flatscher wrote:
> ... cut ...
 To reproduce the testcase one would need ooRexx and the Java bridge 
 BSF4ooRexx (all
 opensource) for
 which I could come up with a zip-archive (assuming binaries within 
 should be 64-bit) and a
 script to
 set up the environment either for Windows, Linux or MacOS, whatever 
 you advise. Would that be
 o.k.?
>>> We prefer not to rely on third-party libraries for test cases. In any 
>>> case we would not be able to
>>> use that for a regression test / unit test.
> If test units really seem to be important in this particular case, one 
> possibility would be to
> create a minimalistic ScriptEngine implementation in pure Java just for 
> the sole purpose to allow
> the creation of a test unit that is able to assert that FXMLLoader puts 
> the ScriptEngine.ARGV and
> ScriptEngine.FILENAME entries into the ENGINE_SCOPE Bindings. E.g. having 
> the ScriptEngine's eval()
> methods return the ScriptContext at invocation time in order to allow 
> inspection of the Bindings.
> This way it would become also possible to write in addition test units 
> that also check whether all
> FXML elements that carry a fx:id are really placed into the GLOBAL_SCOPE 
> Bindings.
 Something like that seems reasonable, and would avoid a dependence on 
 Nashorn, which in addition
 to having all the problems you mentioned, is deprecated for removal.

> However,
 Did you have something more to add?
>>> No, sorry for that. Rewrote my e-mail and had sent it too early by mistake 
>>> and without noticing.
>>>
>>> Will study all the procedures and create a testcase to be submitted at 
>>> 
>>> as per your advice (and will report back under this thread once submitted). 
>>> The testcase would use
>>> an artificial ScriptEngine implementation that could be used for testunit 
>>> testing as well. This
>>> might take a while due to other obligations that I will have to meet during 
>>> the next few days.
>>>
>>> ---rony
>> O.K., so came up with a test case that contains an artificial script engine 
>> implementation for
>> logging the eval() invocations together with the scripts to execute and the 
>> ScriptContext
>> ENGINE_SCOPE and GLOBAL_SCOPE Bindings at the time of the invocation. (It is 
>> meant to be also usable
>> for creating script engine related test units for Java script hosts.)
>>
>> Packaged the source and binaries of that script engine as jar file that one 
>> merely needs to put on
>> the CLASSPATH or add as a module.
>>
>> An updated FXMLLoader patch suggesting a fix is included as well. This 
>> version appends the line
>> number to the file name if the script to be evaluated is embedded in the 
>> fxml-file, such that in
>> case of an error it becomes possible to quickly find it in larger fxml files.
>>
>> With the zip-archive done I went to the Oracle Java Bug Database and just 
>> entered a bug report at
>>  got the internal "ID 
>> : 9062887".
>>
>> As it was not possible to attach/upload the zip-archive at this point, I 
>> will attach the zip-archive
>> (contains all sources and binaries) to this e-mail. The contained 
>>  reads:
>>
>> Testcase that demonstrates that FXMLLoader does not set [1]
>> ScriptEngine.FILENAME and [2] ScriptEngine.ARGV entries in
>> ScriptContext.ENGINE_SCOPE Bindings.
>>
>> To run the test case:
>>
>> - unzip testcaseFXMLLoaderScriptEngines.zip,
>>
>> - change into "testcaseFXMLLoaderScriptEngines" subdirectory,
>>
>> - run the testcase by issuing the following command:
>>
>>   - Unix:
>>
>>     java -cp .:RgfPseudoScriptEngine.jar 
>> FXMLLoaderTestCase4ScriptEngineScope
>>
>>   - Windows:
>>
>>     java -cp .;RgfPseudoScriptEngine.jar 
>> FXMLLoaderTestCase4ScriptEngineScope
>>
>> FXMLLoaderTestCase4ScriptEngineScope loads 

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

As the zip-archive attachment got stripped, for a brief time the zip-archive 
can be fetched from
.

---rony

On 21.11.2019 15:29, Rony G. Flatscher wrote:
> On 15.11.2019 16:08, Rony G. Flatscher wrote:
>> On 14.11.2019 22:57, Kevin Rushforth wrote:
>>> On 11/14/2019 10:12 AM, Rony G. Flatscher wrote:
 On 14.11.2019 16:34, Rony G. Flatscher wrote:
> On 13.11.2019 19:50, Kevin Rushforth wrote:
>> On 11/13/2019 9:42 AM, Rony G. Flatscher wrote:
 ... cut ...
>>> To reproduce the testcase one would need ooRexx and the Java bridge 
>>> BSF4ooRexx (all
>>> opensource) for
>>> which I could come up with a zip-archive (assuming binaries within 
>>> should be 64-bit) and a
>>> script to
>>> set up the environment either for Windows, Linux or MacOS, whatever you 
>>> advise. Would that be
>>> o.k.?
>> We prefer not to rely on third-party libraries for test cases. In any 
>> case we would not be able to
>> use that for a regression test / unit test.
 If test units really seem to be important in this particular case, one 
 possibility would be to
 create a minimalistic ScriptEngine implementation in pure Java just for 
 the sole purpose to allow
 the creation of a test unit that is able to assert that FXMLLoader puts 
 the ScriptEngine.ARGV and
 ScriptEngine.FILENAME entries into the ENGINE_SCOPE Bindings. E.g. having 
 the ScriptEngine's eval()
 methods return the ScriptContext at invocation time in order to allow 
 inspection of the Bindings.
 This way it would become also possible to write in addition test units 
 that also check whether all
 FXML elements that carry a fx:id are really placed into the GLOBAL_SCOPE 
 Bindings.
>>> Something like that seems reasonable, and would avoid a dependence on 
>>> Nashorn, which in addition
>>> to having all the problems you mentioned, is deprecated for removal.
>>>
 However,
>>> Did you have something more to add?
>> No, sorry for that. Rewrote my e-mail and had sent it too early by mistake 
>> and without noticing.
>>
>> Will study all the procedures and create a testcase to be submitted at 
>> 
>> as per your advice (and will report back under this thread once submitted). 
>> The testcase would use
>> an artificial ScriptEngine implementation that could be used for testunit 
>> testing as well. This
>> might take a while due to other obligations that I will have to meet during 
>> the next few days.
>>
>> ---rony
> O.K., so came up with a test case that contains an artificial script engine 
> implementation for
> logging the eval() invocations together with the scripts to execute and the 
> ScriptContext
> ENGINE_SCOPE and GLOBAL_SCOPE Bindings at the time of the invocation. (It is 
> meant to be also usable
> for creating script engine related test units for Java script hosts.)
>
> Packaged the source and binaries of that script engine as jar file that one 
> merely needs to put on
> the CLASSPATH or add as a module.
>
> An updated FXMLLoader patch suggesting a fix is included as well. This 
> version appends the line
> number to the file name if the script to be evaluated is embedded in the 
> fxml-file, such that in
> case of an error it becomes possible to quickly find it in larger fxml files.
>
> With the zip-archive done I went to the Oracle Java Bug Database and just 
> entered a bug report at
>  got the internal "ID : 
> 9062887".
>
> As it was not possible to attach/upload the zip-archive at this point, I will 
> attach the zip-archive
> (contains all sources and binaries) to this e-mail. The contained 
>  reads:
>
> Testcase that demonstrates that FXMLLoader does not set [1]
> ScriptEngine.FILENAME and [2] ScriptEngine.ARGV entries in
> ScriptContext.ENGINE_SCOPE Bindings.
>
> To run the test case:
>
> - unzip testcaseFXMLLoaderScriptEngines.zip,
>
> - change into "testcaseFXMLLoaderScriptEngines" subdirectory,
>
> - run the testcase by issuing the following command:
>
>   - Unix:
>
>     java -cp .:RgfPseudoScriptEngine.jar 
> FXMLLoaderTestCase4ScriptEngineScope
>
>   - Windows:
>
>     java -cp .;RgfPseudoScriptEngine.jar 
> FXMLLoaderTestCase4ScriptEngineScope
>
> FXMLLoaderTestCase4ScriptEngineScope loads "demo_01.fxml" which is a 
> controller
> that uses the pseudo script language 
> rgf.scriptEngine.RgfPseudoScriptEngine,
> which logs the eval() invocations with the script code and the Bindings 
> of the
> ScriptContext.
>
> Comparing "demo_01.fxml" and the output of the above test case 
> demonstrates that
> FXMLLoader does not popuplate the [3] ENGINE_SCOPE Bindings with the 
> filename of
> the script that gets evaluated, nor does add the ARGV entry to the 
> ENGINE_SCOPE
> Bindings 

"Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

On 15.11.2019 16:08, Rony G. Flatscher wrote:
> On 14.11.2019 22:57, Kevin Rushforth wrote:
>> On 11/14/2019 10:12 AM, Rony G. Flatscher wrote:
>>> On 14.11.2019 16:34, Rony G. Flatscher wrote:
 On 13.11.2019 19:50, Kevin Rushforth wrote:
> On 11/13/2019 9:42 AM, Rony G. Flatscher wrote:
>>> ... cut ...
>> To reproduce the testcase one would need ooRexx and the Java bridge 
>> BSF4ooRexx (all
>> opensource) for
>> which I could come up with a zip-archive (assuming binaries within 
>> should be 64-bit) and a
>> script to
>> set up the environment either for Windows, Linux or MacOS, whatever you 
>> advise. Would that be
>> o.k.?
> We prefer not to rely on third-party libraries for test cases. In any 
> case we would not be able to
> use that for a regression test / unit test.
>>> If test units really seem to be important in this particular case, one 
>>> possibility would be to
>>> create a minimalistic ScriptEngine implementation in pure Java just for the 
>>> sole purpose to allow
>>> the creation of a test unit that is able to assert that FXMLLoader puts the 
>>> ScriptEngine.ARGV and
>>> ScriptEngine.FILENAME entries into the ENGINE_SCOPE Bindings. E.g. having 
>>> the ScriptEngine's eval()
>>> methods return the ScriptContext at invocation time in order to allow 
>>> inspection of the Bindings.
>>> This way it would become also possible to write in addition test units that 
>>> also check whether all
>>> FXML elements that carry a fx:id are really placed into the GLOBAL_SCOPE 
>>> Bindings.
>> Something like that seems reasonable, and would avoid a dependence on 
>> Nashorn, which in addition
>> to having all the problems you mentioned, is deprecated for removal.
>>
>>> However,
>> Did you have something more to add?
> No, sorry for that. Rewrote my e-mail and had sent it too early by mistake 
> and without noticing.
>
> Will study all the procedures and create a testcase to be submitted at 
> 
> as per your advice (and will report back under this thread once submitted). 
> The testcase would use
> an artificial ScriptEngine implementation that could be used for testunit 
> testing as well. This
> might take a while due to other obligations that I will have to meet during 
> the next few days.
>
> ---rony

O.K., so came up with a test case that contains an artificial script engine 
implementation for
logging the eval() invocations together with the scripts to execute and the 
ScriptContext
ENGINE_SCOPE and GLOBAL_SCOPE Bindings at the time of the invocation. (It is 
meant to be also usable
for creating script engine related test units for Java script hosts.)

Packaged the source and binaries of that script engine as jar file that one 
merely needs to put on
the CLASSPATH or add as a module.

An updated FXMLLoader patch suggesting a fix is included as well. This version 
appends the line
number to the file name if the script to be evaluated is embedded in the 
fxml-file, such that in
case of an error it becomes possible to quickly find it in larger fxml files.

With the zip-archive done I went to the Oracle Java Bug Database and just 
entered a bug report at
 got the internal "ID : 
9062887".

As it was not possible to attach/upload the zip-archive at this point, I will 
attach the zip-archive
(contains all sources and binaries) to this e-mail. The contained  
reads:

Testcase that demonstrates that FXMLLoader does not set [1]
ScriptEngine.FILENAME and [2] ScriptEngine.ARGV entries in
ScriptContext.ENGINE_SCOPE Bindings.

To run the test case:

- unzip testcaseFXMLLoaderScriptEngines.zip,

- change into "testcaseFXMLLoaderScriptEngines" subdirectory,

- run the testcase by issuing the following command:

  - Unix:

    java -cp .:RgfPseudoScriptEngine.jar 
FXMLLoaderTestCase4ScriptEngineScope

  - Windows:

    java -cp .;RgfPseudoScriptEngine.jar 
FXMLLoaderTestCase4ScriptEngineScope

FXMLLoaderTestCase4ScriptEngineScope loads "demo_01.fxml" which is a 
controller
that uses the pseudo script language rgf.scriptEngine.RgfPseudoScriptEngine,
which logs the eval() invocations with the script code and the Bindings of 
the
ScriptContext.

Comparing "demo_01.fxml" and the output of the above test case demonstrates 
that
FXMLLoader does not popuplate the [3] ENGINE_SCOPE Bindings with the 
filename of
the script that gets evaluated, nor does add the ARGV entry to the 
ENGINE_SCOPE
Bindings in the case of events (just an entry named "event").

In case of errors (during development or deployment) it is not possible
therefore to point to the file (external file or the fxml-file defining
explicitly script code) in which a runtime error has occurred.

In the case of an event callback, if ARGV was defined the script code could
directly fetch and interact with the supplied 

Request for review: JDK-8200224 - Multiple press event when JFXPanel gains focus.

[Florian Kirmaier]

Hello,

Please review this fix for "Multiple press event when JFXPanel gains focus.
Ticket: https://bugs.openjdk.java.net/browse/JDK-8087914
Pull Request: https://github.com/javafxports/openjdk-jfx/pull/591

Florian Kirmaier
from JPro <https://www.jpro.one>

Review Request: JDK-8221377: Fix mistakes in FX API docs

[Nir Lisker]

Hi Kevin,

Please review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8221377
http://cr.openjdk.java.net/~nlisker/8221377/webrev.00/

Thanks,
Nir

Review Request: JDK-8226850: Use an EnumSet for DirtyBits instead of an ordinal-based mask

[Nir Lisker]

Hi Kevin/Ambarish,

Please review the simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8226850
http://cr.openjdk.java.net/~nlisker/8226850/webrev.00/

Thanks,
Nir

Review-Request for JDK-8227367

[Michael Ennen]

 Hi,

I'd like to request a review for JDK-8227367 available as a PR on
Github:

https://github.com/javafxports/openjdk-jfx/pull/518
<https://github.com/javafxports/openjdk-jfx/pull/168>

https://bugs.openjdk.java.net/browse/JDK-8227367
<https://bugs.openjdk.java.net/browse/JDK-8209970>

Thanks.


-- 
Michael Ennen

Review Request: JDK-8226912: Color, Point2D and Point3D's fields should be made final

[Nir Lisker]

Hi Kevin,

Please review the simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8226912
http://cr.openjdk.java.net/~nlisker/8226912/webrev.00/

I planned this for 14, but it can go into 13 too.

- Nir

Review Request: JDK-8226454: Point2D and Point3D should implement Interpolatable

[Nir Lisker]

Hi,

Please review the fix for:

https://bugs.openjdk.java.net/browse/JDK-8226454
http://cr.openjdk.java.net/~nlisker/8226454/webrev.00/

Thanks,
Nir

Re: Review Request: JDK-8222258: Add exclusion scope for LightBase

[Kevin Rushforth]

I'll put it on my review queue.

This will need a CSR. The proposed docs from the patch can be used as a 
starting point; they may need some minor updates, but are close enough 
to get started.


-- Kevin


On 5/3/2019 9:00 PM, Nir Lisker wrote:

Hi Kevin / Ambarish,

Please review the fix for:

https://bugs.openjdk.java.net/browse/JDK-858
http://cr.openjdk.java.net/~nlisker/858/webrev.01/

Details are in the JBS issue.

Thanks,
Nir

Review Request: JDK-8222258: Add exclusion scope for LightBase

[Nir Lisker]

Hi Kevin / Ambarish,

Please review the fix for:

https://bugs.openjdk.java.net/browse/JDK-858
http://cr.openjdk.java.net/~nlisker/858/webrev.01/

Details are in the JBS issue.

Thanks,
Nir

Re: Review Request: JDK-8222066: Change JavaFX release version to 11.0.3 in 11-dev

[Kevin Rushforth]

+1

On 4/12/2019 1:22 AM, Johan Vos wrote:

Hi Kevin,

Please review 
http://cr.openjdk.java.net/~jvos/8222066/webrev.00/ which fixes 
https://bugs.openjdk.java.net/browse/JDK-8222066


- Johan

Review Request: JDK-8222066: Change JavaFX release version to 11.0.3 in 11-dev

[Johan Vos]

Hi Kevin,

Please review http://cr.openjdk.java.net/~jvos/8222066/webrev.00/ which
fixes https://bugs.openjdk.java.net/browse/JDK-8222066

- Johan

Re: Review Request: JDK-8221708: Update Eclipse project files for non-modular projects

[Nir Lisker]

The patch requires an Eclipse developer to test it.

Please kindly volunteer yourselves :)

On Sat, Mar 30, 2019 at 4:28 PM Nir Lisker  wrote:

> Hi,
>
> Please review the fix for:
>
> https://bugs.openjdk.java.net/browse/JDK-8221708
> http://cr.openjdk.java.net/~nlisker/8221708/webrev.00/
>
> Users of Eclipse are encourage to comment.
>
> Thanks,
> Nir
>

Review Request: JDK-8222073: Revert unintentional change to Dialog.java

[Nir Lisker]

Hi Kevin,

Please review the fix for:

https://bugs.openjdk.java.net/browse/JDK-8222073
http://cr.openjdk.java.net/~nlisker/8222073/webrev.00/

Nir

Review Request: JDK-8221708: Update Eclipse project files for non-modular projects

[Nir Lisker]

Hi,

Please review the fix for:

https://bugs.openjdk.java.net/browse/JDK-8221708
http://cr.openjdk.java.net/~nlisker/8221708/webrev.00/

Users of Eclipse are encourage to comment.

Thanks,
Nir

Review Request: JDK-8211014: Fix mistakes in FX API docs

[Nir Lisker]

Hi,

Please review the fix for:

https://bugs.openjdk.java.net/browse/JDK-8211014
http://cr.openjdk.java.net/~nlisker/8211014/webrev.00/

Thanks,
Nir

Review Request: JDK-8217470: Upgrade Direct3D9 shader model from 2.0 to 3.0

[Nir Lisker]

Hi,

Please review the fix for:

https://bugs.openjdk.java.net/browse/JDK-8217470
http://cr.openjdk.java.net/~nlisker/8217470/webrev.00/

Thanks,
Nir

Review-Request for JDK-8217333

[Michael Ennen]

 Hi,

I'd like to request a review for JDK-8217333 available as a PR on
Github:

https://github.com/javafxports/openjdk-jfx/pull/346
<https://github.com/javafxports/openjdk-jfx/pull/168>

https://bugs.openjdk.java.net/browse/JDK-8217333
<https://bugs.openjdk.java.net/browse/JDK-8209970>

Thanks.


-- 
Michael Ennen

Review Request: JDK-8217270: Broken link to cssref.html in javafx.controls package docs

[Nir Lisker]

Hi,

Please review the simple fix for:

https://bugs.openjdk.java.net/browse/JDK-8217270
http://cr.openjdk.java.net/~nlisker/8217270/webrev.00/

Thanks,
Nir

Re: Review request for [JDK-8217259] release notes 11.0.2

[Kevin Rushforth]

Approved.

-- Kevin


On 1/16/2019 7:06 AM, Johan Vos wrote:

Hi Kevin,

Please review http://cr.openjdk.java.net/~jvos/8217259/webrev.00/
which contains the release notes for 11.0.2

A layouted version of this is also available at 
https://github.com/johanvos/openjdk-jfx/blob/jfx-11/doc-files/release-notes-11.0.2.md


Thanks,

- Johan

Review request for [JDK-8217259] release notes 11.0.2

[Johan Vos]

Hi Kevin,

Please review http://cr.openjdk.java.net/~jvos/8217259/webrev.00/
which contains the release notes for 11.0.2

A layouted version of this is also available at
https://github.com/johanvos/openjdk-jfx/blob/jfx-11/doc-files/release-notes-11.0.2.md

Thanks,

- Johan

Review Request: JDK-8210361: Add images to docs for public API classes of controls

[Nir Lisker]

Hi,

Please review the current state of the patch for:

https://bugs.openjdk.java.net/browse/JDK-8210361
http://cr.openjdk.java.net/~nlisker/8210361/webrev.00/

Thanks,
Nir

Review request for JDK-8207957

[Sam']

Hi,

I've made a PR here https://github.com/javafxports/openjdk-jfx/pull/289
regarding https://bugs.openjdk.java.net/browse/JDK-8207957

Anyone interested in the TableView (TreeTableView) can  review and gives
its opinion regarding the subject.

Cheers,
Sam'

Review-Request for JDK-8215629

[Michael Ennen]

 Hi,

I'd like to request a review for JDK-8209970 available as a PR on
Github:

https://github.com/javafxports/openjdk-jfx/pull/328
<https://github.com/javafxports/openjdk-jfx/pull/168>

https://bugs.openjdk.java.net/browse/JDK-8215629
<https://bugs.openjdk.java.net/browse/JDK-8209970>

Thanks.

-- 
Michael Ennen

Review Request: make caching of native libs more robust

[Johan Vos]

An easy to reproduce issue is to change permissions on the cache dir used
by Javafx for storing native libs.
If JavaFX can't write anymore to that cache dir, applications will fail
since all pipelines use native code.

The fix at https://github.com/javafxports/openjdk-jfx/pull/300 makes the
caching mechanism more robust, with a fallback to the tmp dir, and with the
option to always override the location of the cache dir.

Note: In the end, I still hope this is something that can be provide by the
JDK itself (extracting native code from jars or jmods and storing it). For
now, we probably just want to make the system more robust.

- Johan

Re: Review request: Long scrolling on mac [JDK-8183399]

[Johan Vos]

webrev at http://cr.openjdk.java.net/~jvos/8183399/webrev.00/

On Mon, Nov 19, 2018 at 7:03 PM Johan Vos  wrote:

> Please review the fix for issue
> https://bugs.openjdk.java.net/browse/JDK-8183399 (
> https://github.com/javafxports/openjdk-jfx/issues/38) which should be
> fixed with PR #274
> (https://github.com/javafxports/openjdk-jfx/pull/274)
>
> It only affects mac builds.
>
> - Johan
>

Review request: Long scrolling on mac [JDK-8183399]

[Johan Vos]

Please review the fix for issue
https://bugs.openjdk.java.net/browse/JDK-8183399 (
https://github.com/javafxports/openjdk-jfx/issues/38) which should be fixed
with PR #274
(https://github.com/javafxports/openjdk-jfx/pull/274)

It only affects mac builds.

- Johan

Review request for JDK-8207839, JDK-8207932, JDK-8208076 (variation selector)

[Nakajima Akira]

Hi Kevin, Phil,

Please review the following Github PR.

JDK-8207839
https://github.com/javafxports/openjdk-jfx/pull/125
https://github.com/javafxports/openjdk-jfx/pull/125/files

JDK-8207932
https://github.com/javafxports/openjdk-jfx/pull/126
https://github.com/javafxports/openjdk-jfx/pull/126/commits/ea68aa71d94e370e739556df0afeea105b1d07cb

JDK-8208076
https://github.com/javafxports/openjdk-jfx/pull/137
https://github.com/javafxports/openjdk-jfx/pull/137/commits/4e882cc9810e63e5c2df89a935b5d7842c5df16f


Thanks,
Akira Nakajima

Review request for JDK-8213541

[Sam']

If anyone wants to review my PR for
https://bugs.openjdk.java.net/browse/JDK-8213541 , it would be valuable.

PR can be seen here : https://github.com/javafxports/openjdk-jfx/pull/281

Re: Code review of jpackager tool (JEP 343)

[Phil Race]

Anyone interested in jpackager should definitely subscribe to 
core-libs-...@openjdk.java.net
to comment on it and follow the review. Admittedly core-libs-dev is a 
very active list and
this is just one topic, but it is the place to make your views known on 
the tool, since
it is a "core" JDK feature, not an FX feature, even though it is 
inspired by javapackager.


-phil.

On 10/24/18 6:09 AM, Rachel Greenham wrote:
ah. as i only followed this list to follow progress on this, turns out 
I'm in the wrong place. :-)


--
Rachel

On 24/10/2018 13:10, Kevin Rushforth wrote:

The code review for the jpackager tool, JEP 343, is underway:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/056186.html 



The review is being done on the core-libs-...@openjdk.java.net 
mailing list, so please direct all feedback on the implementation of 
the JEP to that list.

Re: Review request backport 8210386

[Laurent Bourgès]

Thank you,

Laurent

Le lun. 29 oct. 2018 à 19:35, Kevin Rushforth 
a écrit :

> Looks good. Approved for pushing to 11-dev.
>
> -- Kevin
>
> On 10/29/2018 11:27 AM, Johan Vos wrote:
> > new webrev including test:
> > http://cr.openjdk.java.net/~jvos/8210386/webrev.01/
> > <http://cr.openjdk.java.net/%7Ejvos/8210386/webrev.00/>
> >
> > On Mon, Oct 29, 2018 at 5:55 PM Kevin Rushforth
> > mailto:kevin.rushfo...@oracle.com>> wrote:
> >
> > The newly added test, which was pushed to jfx-dev as part of the
> > fix for
> > 12, is missing from the webrev:
> >
> > tests/system/src/test/java/test/com/sun/marlin/ScaleClipTest.java
> >
> > The rest looks OK.
> >
> > -- Kevin
> >
> >
> > On 10/29/2018 9:39 AM, Johan Vos wrote:
> > > Hi Kevin,
> > >
> > > Please review the backport of 8210386 (Marlin fixes) into 11-dev
> > (target
> > > 11.0.1)
> > > http://cr.openjdk.java.net/~jvos/8210386/webrev.00/
> > <http://cr.openjdk.java.net/%7Ejvos/8210386/webrev.00/>
> > >
> > > - Johan
> >
>
>

Re: Review request backport 8210386

[Kevin Rushforth]

Looks good. Approved for pushing to 11-dev.

-- Kevin

On 10/29/2018 11:27 AM, Johan Vos wrote:

new webrev including test:
http://cr.openjdk.java.net/~jvos/8210386/webrev.01/ 
<http://cr.openjdk.java.net/%7Ejvos/8210386/webrev.00/>


On Mon, Oct 29, 2018 at 5:55 PM Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


The newly added test, which was pushed to jfx-dev as part of the
fix for
12, is missing from the webrev:

tests/system/src/test/java/test/com/sun/marlin/ScaleClipTest.java

The rest looks OK.

-- Kevin


On 10/29/2018 9:39 AM, Johan Vos wrote:
> Hi Kevin,
>
    > Please review the backport of 8210386 (Marlin fixes) into 11-dev
(target
> 11.0.1)
> http://cr.openjdk.java.net/~jvos/8210386/webrev.00/
<http://cr.openjdk.java.net/%7Ejvos/8210386/webrev.00/>
>
> - Johan

Re: Review request backport 8210386

[Johan Vos]

new webrev including test:
http://cr.openjdk.java.net/~jvos/8210386/webrev.01/
<http://cr.openjdk.java.net/~jvos/8210386/webrev.00/>

On Mon, Oct 29, 2018 at 5:55 PM Kevin Rushforth 
wrote:

> The newly added test, which was pushed to jfx-dev as part of the fix for
> 12, is missing from the webrev:
>
> tests/system/src/test/java/test/com/sun/marlin/ScaleClipTest.java
>
> The rest looks OK.
>
> -- Kevin
>
>
> On 10/29/2018 9:39 AM, Johan Vos wrote:
> > Hi Kevin,
> >
> > Please review the backport of 8210386 (Marlin fixes) into 11-dev (target
> > 11.0.1)
> > http://cr.openjdk.java.net/~jvos/8210386/webrev.00/
> >
> > - Johan
>
>

Re: Review request backport 8210386

[Kevin Rushforth]

The newly added test, which was pushed to jfx-dev as part of the fix for 
12, is missing from the webrev:


tests/system/src/test/java/test/com/sun/marlin/ScaleClipTest.java

The rest looks OK.

-- Kevin


On 10/29/2018 9:39 AM, Johan Vos wrote:

Hi Kevin,

Please review the backport of 8210386 (Marlin fixes) into 11-dev (target
11.0.1)
http://cr.openjdk.java.net/~jvos/8210386/webrev.00/

- Johan

Review request backport 8210386

[Johan Vos]

Hi Kevin,

Please review the backport of 8210386 (Marlin fixes) into 11-dev (target
11.0.1)
http://cr.openjdk.java.net/~jvos/8210386/webrev.00/

- Johan

Re: Code review of jpackager tool (JEP 343)

[Rachel Greenham]

ah. as i only followed this list to follow progress on this, turns out 
I'm in the wrong place. :-)


--
Rachel

On 24/10/2018 13:10, Kevin Rushforth wrote:

The code review for the jpackager tool, JEP 343, is underway:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/056186.html 



The review is being done on the core-libs-...@openjdk.java.net mailing 
list, so please direct all feedback on the implementation of the JEP 
to that list.


-- Kevin

Code review of jpackager tool (JEP 343)

[Kevin Rushforth]

The code review for the jpackager tool, JEP 343, is underway:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/056186.html

The review is being done on the core-libs-...@openjdk.java.net mailing 
list, so please direct all feedback on the implementation of the JEP to 
that list.


-- Kevin

Re: Updated code review policies posted on OpenJFX Project Wiki

[Kevin Rushforth]

A great idea that I had forgotten about. One of the Reviewers mentioned 
the same to me off list.


-- Kevin


On 10/21/2018 3:05 AM, Nir Lisker wrote:

Hi Kevin,

I suggest to add an "areas of expertise" (modules or JBS components) 
to the list of reviewers on the Code Reviews page, at least for active 
Reviewers, similarly to how it is done on the Code Ownership page.
It doesn't need to have a formal function (I brought up that idea 
during the spring discussion), but would help to know who to consult 
with when working on some part of OpenJFX.


- Nir

On Sat, Oct 20, 2018 at 12:18 AM Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


The updated code review policies that we discussed last spring are
now
posted on the OpenJFX Project Wiki [1]. For the most part, we have
effectively been following these for the past several months. This
will
formalize the policies in a way that will be easier to point new
contributors at. If any of you have any suggestions for making it
easier
to understand, please send them our way.

The executive summary is:

1. Recorded the fact that Johan and I are co-leads of the OpenJFX
Project

2. Formalized the concept of a Reviewer role and recorded the initial
list of Reviewers

3. Formalized the code review requirements:
* simple fixes: 1"R"eviewer
* higher-impact fixes: 2 reviewers (one of whom must be "R"eviewer)
* new features / API changes: discus proposed changes on this
list, CSR
review (including approval by a Lead) for API changes + 2
reviewers for
implementation

4. Formalized the streamlined GitHub code review (as long as you
complete all review steps, including RFR email to this list, prior
to PR
being merged, there is no need for a webrev or additional review
to push
it to HG jfx-dev repo)

I am working on an update to the GitHub CONTRIBUTING.md page [2] to
align with these newly-formalized policies, primarily to help
first time
contributors, but also as a reminder for all contributors. I'll
send a
PR when ready (tomorrow or Monday).

-- Kevin

[1] https://wiki.openjdk.java.net/display/OpenJFX/Code+Reviews
[2]

https://github.com/javafxports/openjdk-jfx/blob/develop/.github/CONTRIBUTING.md

Re: Review Request: JDK-8212728: Update the Eclipse classpath file for the Swing module

[Kevin Rushforth]

+1

My mistake for missing this. Sorry about that. I'll get it pushed for 
you shortly.


-- Kevin

On 10/21/2018 12:49 PM, Nir Lisker wrote:

Hi,

Please review the simple fix for:

https://bugs.openjdk.java.net/browse/JDK-8212728
http://cr.openjdk.java.net/~nlisker/8212728/webrev.00/

Thanks,
Nir

Review Request: JDK-8212728: Update the Eclipse classpath file for the Swing module

[Nir Lisker]

Hi,

Please review the simple fix for:

https://bugs.openjdk.java.net/browse/JDK-8212728
http://cr.openjdk.java.net/~nlisker/8212728/webrev.00/

Thanks,
Nir

Re: Updated code review policies posted on OpenJFX Project Wiki

[Nir Lisker]

Hi Kevin,

I suggest to add an "areas of expertise" (modules or JBS components) to the
list of reviewers on the Code Reviews page, at least for active Reviewers,
similarly to how it is done on the Code Ownership page.
It doesn't need to have a formal function (I brought up that idea during
the spring discussion), but would help to know who to consult with when
working on some part of OpenJFX.

- Nir

On Sat, Oct 20, 2018 at 12:18 AM Kevin Rushforth 
wrote:

> The updated code review policies that we discussed last spring are now
> posted on the OpenJFX Project Wiki [1]. For the most part, we have
> effectively been following these for the past several months. This will
> formalize the policies in a way that will be easier to point new
> contributors at. If any of you have any suggestions for making it easier
> to understand, please send them our way.
>
> The executive summary is:
>
> 1. Recorded the fact that Johan and I are co-leads of the OpenJFX Project
>
> 2. Formalized the concept of a Reviewer role and recorded the initial
> list of Reviewers
>
> 3. Formalized the code review requirements:
> * simple fixes: 1"R"eviewer
> * higher-impact fixes: 2 reviewers (one of whom must be "R"eviewer)
> * new features / API changes: discus proposed changes on this list, CSR
> review (including approval by a Lead) for API changes + 2 reviewers for
> implementation
>
> 4. Formalized the streamlined GitHub code review (as long as you
> complete all review steps, including RFR email to this list, prior to PR
> being merged, there is no need for a webrev or additional review to push
> it to HG jfx-dev repo)
>
> I am working on an update to the GitHub CONTRIBUTING.md page [2] to
> align with these newly-formalized policies, primarily to help first time
> contributors, but also as a reminder for all contributors. I'll send a
> PR when ready (tomorrow or Monday).
>
> -- Kevin
>
> [1] https://wiki.openjdk.java.net/display/OpenJFX/Code+Reviews
> [2]
>
> https://github.com/javafxports/openjdk-jfx/blob/develop/.github/CONTRIBUTING.md
>
>

Re: Review request for #151 Open VirtualFlow and other relating

[Sam']

I'm sorry if I'm created a new thread as I don't understand how mailing
lists work and I tend to avoid them as much as possible.

The JBS bug can be found here :
https://bugs.openjdk.java.net/browse/JDK-8207942

Sorry for the wrong one I initially posted.

1) I guess we are doing great work on the API thanks to "nlisker" awesome
involvement. We may need some help around some method naming in
TableViewSkinBase. The discussion can be found here :

https://github.com/javafxports/openjdk-jfx/pull/163#discussion_r225394151


2) I'm guessing the CSR stands for *Compatibility & Specification Review *
: https://openjdk.java.net/groups/csr/
I'm grateful to Eugene to fill it. If you need any help, don't hesitate to
ask.

I apologize for the trouble I make in Github or this mailing list as I have
never done such a thing before and I have not anticipated the work and the
process would be so big.

Thank you all

Re: Review request for #151 Open VirtualFlow and other relating classes API for sublclassing

[Kevin Rushforth]

Samir,

Also, can you provide JBS bug ID? The one you listed in your review 
request is the wrong Bug ID.


Thanks.

-- Kevin


On 10/15/2018 6:00 AM, Kevin Rushforth wrote:
Note that since this is request for a new feature there are a couple 
other steps needed:


1. The proposed feature, along with the new API should be discussed 
and general agreement reached. This is already in progress and is 
happening in the PR itself. Mainly what needs to be ironed out now is 
to make sure that each new API method is needed, and its purpose and 
use is well understood and documents. Some of you have already 
provided valuable feedback on this.


2. Once we have general agreement on the API additions, a CSR will be 
needed (Eugene has graciously agreed to file it with my help). The CSR 
will then need review / approval from a lead: Johan or myself.


The code review can proceed in parallel with #2 -- it will need (at 
least) two reviewers.


With Oracle Code One approaching, I expect there will be some delay in 
the first two items, but we still have plenty of time to get this in 
for openjfx12.


Thanks!

-- Kevin


On 10/15/2018 5:27 AM, Sam' wrote:

Please review the GitHub pull request:
https://github.com/javafxports/openjdk-jfx/pull/163
<https://github.com/javafxports/openjdk-jfx/pull/235>

which fixes:


Open VirtualFlow API for subclassing

https://github.com/javafxports/openjdk-jfx/issues/151
<https://bugs.openjdk.java.net/browse/JDK-8188810>

Thank you,
Samir

Re: Review request for #151 Open VirtualFlow and other relating classes API for sublclassing

[Kevin Rushforth]

Note that since this is request for a new feature there are a couple 
other steps needed:


1. The proposed feature, along with the new API should be discussed and 
general agreement reached. This is already in progress and is happening 
in the PR itself. Mainly what needs to be ironed out now is to make sure 
that each new API method is needed, and its purpose and use is well 
understood and documents. Some of you have already provided valuable 
feedback on this.


2. Once we have general agreement on the API additions, a CSR will be 
needed (Eugene has graciously agreed to file it with my help). The CSR 
will then need review / approval from a lead: Johan or myself.


The code review can proceed in parallel with #2 -- it will need (at 
least) two reviewers.


With Oracle Code One approaching, I expect there will be some delay in 
the first two items, but we still have plenty of time to get this in for 
openjfx12.


Thanks!

-- Kevin


On 10/15/2018 5:27 AM, Sam' wrote:

Please review the GitHub pull request:
https://github.com/javafxports/openjdk-jfx/pull/163
<https://github.com/javafxports/openjdk-jfx/pull/235>

which fixes:


Open VirtualFlow API for subclassing

https://github.com/javafxports/openjdk-jfx/issues/151
<https://bugs.openjdk.java.net/browse/JDK-8188810>

Thank you,
Samir

Review request for #151 Open VirtualFlow and other relating classes API for sublclassing

[Sam']

Please review the GitHub pull request:
https://github.com/javafxports/openjdk-jfx/pull/163
<https://github.com/javafxports/openjdk-jfx/pull/235>

which fixes:


Open VirtualFlow API for subclassing

https://github.com/javafxports/openjdk-jfx/issues/151
<https://bugs.openjdk.java.net/browse/JDK-8188810>

Thank you,
Samir

Review request for 8188810, "Fonts are blurry on Ubuntu 16.04 and Debian 9"

[John Neffenger]

Please review the GitHub pull request:

JDK-8188810: Fonts are blurry on Ubuntu 16.04 and Debian 9
https://github.com/javafxports/openjdk-jfx/pull/235

which fixes:

Fonts are blurry on Ubuntu 16.04 and Debian 9
https://bugs.openjdk.java.net/browse/JDK-8188810

Thank you,
John

Re: Review request for 8210359

[Kevin Rushforth]

I reviewed/approved on GitHub.

-- Kevin


On 9/4/2018 6:52 AM, Johan Vos wrote:

Please review webrev http://cr.openjdk.java.net/~jvos/8210359/webrev.00/
for https://bugs.openjdk.java.net/browse/JDK-8210359

(this allows to build OpenJFX SDK on platforms that don't include Swing,
e.g. armv6hf)

Review request for 8210359

[Johan Vos]

Please review webrev http://cr.openjdk.java.net/~jvos/8210359/webrev.00/
for https://bugs.openjdk.java.net/browse/JDK-8210359

(this allows to build OpenJFX SDK on platforms that don't include Swing,
e.g. armv6hf)

Re: Review request for 8209969

[Kevin Rushforth]

+1

On 9/3/2018 7:17 AM, Johan Vos wrote:

Please review the trivial fix for
https://bugs.openjdk.java.net/browse/JDK-8209969 at
http://cr.openjdk.java.net/~jvos/8209969/webrev.00/ (approved and merged in
github via
https://github.com/javafxports/openjdk-jfx/commit/3fbd1165a31cf07a4dcb6c854597adeccc4af7c7
  )

Thanks,

- Johan

Review request for 8209969

[Johan Vos]

Please review the trivial fix for
https://bugs.openjdk.java.net/browse/JDK-8209969 at
http://cr.openjdk.java.net/~jvos/8209969/webrev.00/ (approved and merged in
github via
https://github.com/javafxports/openjdk-jfx/commit/3fbd1165a31cf07a4dcb6c854597adeccc4af7c7
 )

Thanks,

- Johan

Review Request for JDK-8209968: Fix rounding error in image scale calculation

[John Hendrikx]

Hi,

This is a review request for:

https://bugs.openjdk.java.net/browse/JDK-8209968

The PR is on Github:

https://github.com/javafxports/openjdk-jfx/pull/170

--John

Review-Request for JDK-8209970

[Michael Ennen]

Hi,

I'd like to request a review for JDK-8209970 available as a PR on
Github:

https://github.com/javafxports/openjdk-jfx/pull/168

https://bugs.openjdk.java.net/browse/JDK-8209970

Thanks.

-- 
Michael Ennen

[12] Review request: 8209791 : OpenJFX build fails in PrismPrint.c due to missing JNICALL

[Priyanka Mangal]

Hi,

Please review this fix :
Webrev : http://cr.openjdk.java.net/~dkumar/primanga/8209791/webrev.00/
JBS : https://bugs.openjdk.java.net/browse/JDK-8209791

Thanks,
Priyanka

Review-Request for JDK-8209764

[Michael Ennen]

Hi,

I'd like to request a review for JDK-8209764 available as
a PR on Github:

https://github.com/javafxports/openjdk-jfx/pull/155

Thanks.

-- 
Michael Ennen

Review-Request for JDK-8209765

[Michael Ennen]

Hi,

I'd like to request a review for JDK-8209765 available as
a PR on Github:

https://github.com/javafxports/openjdk-jfx/pull/153

Thanks.

-- 
Michael Ennen

[12] Review request: 8203379: Remove javapackager sources from OpenJFX repo

[Kevin Rushforth]

Please review the following to remove the javapackager sources and build 
logic from the openjfx repo:


https://bugs.openjdk.java.net/browse/JDK-8203379
http://cr.openjdk.java.net/~kcr/8203379/webrev.00/

See JBS for more information.

-- Kevin

Review Request: JDK-8209015: Update Eclipse project files

[Nir Lisker]

Hi,

Please review the fix for:

https://bugs.openjdk.java.net/browse/JDK-8209015
http://cr.openjdk.java.net/~nlisker/8209015/webrev.00/

Thanks,
Nir