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. <?language rexx?>) 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

Reply via email to