8337280: Include jdk.jsobject module with JavaFX #1529

kevinrushforth · 2024-08-02T21:29:15Z

Add the jdk.jsobject module to JavaFX. This module is currently delivered by the JDK, but will be terminally-deprecated in JDK 24 by JDK-8311530, which is out for review at openjdk/jdk#20555. We therefore plan to start delivering it with JavaFX in JavaFX 24.

Testing scenarios

Both JDK and JavaFX for some period of time, so we need to test various scenarios. I built and tested this PR branch with each of the following JDKs:

JDK 23 or earlier (I used the default boot JDK, which is currently JDK 22.0.2); this JDK includes jdk.jsobject as a normal, non-upgradable module
A locally-built JDK 24 from my 8311530-depr-jsobject branch, which has the fix for JDK-8311530; this JDK includes jdk.jsobject as a terminally-deprecated, upgradable module
A locally-built JDK 24 from my WIP-rm-jsobject branch, which removes the jdk.jsobject module, and is therefore a prototype of what will happen in JDK 26 (or later) when it is removed from the JDK

I ran a build and test of JavaFX from this PR branch using each of the above JDKs. All tests pass.

I then took the JavaFX artifacts built by JDK 1 (matching how we will deliver this in JavaFX 24), and ran the following additional tests, all of which match my expectations:

java --module-path=.../jfx/build/sdk/lib without any additional flags (required for JDK 1; recommended for JDK 3)
RESULT: JDKs 1 and 2 use the jdk.jsobject module from the JDK; JDK 3 uses the jdk.jsobject module from JavaFX
java --module-path=.../jfx/build/sdk/lib --upgrade-module-path=.../jfx/build/sdk/lib (recommended for JDK 2, OK for 3)
RESULT: JDK 1 will fail to launch; JDKs 2 and 3 use the jdk.jsobject module from JavaFX
jlink with .../jfx/build/jmods ahead of $JAVA_HOME/jmods (recommended for JDKs 2 and 3)
RESULT: JDK 1 will fail to link; JDKs 2 and 3 use the jdk.jsobject module from JavaFX
jlink with $JAVA_HOME/jmods ahead of .../jfx/build/jmods (required for JDK 1)
RESULT: JDKs 1 and 2 will use the jdk.jsobject module from the JDK; JDK 3 uses the jdk.jsobject module from JavaFX

I have not verified the maven artifacts, but they are being generated correctly.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8338249 to be approved
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8337280: Include jdk.jsobject module with JavaFX (Enhancement - P4)
JDK-8338249: Include jdk.jsobject module with JavaFX (CSR)

Reviewers

Ambarish Rapte (@arapte - Reviewer)
Jay Bhaskar (@jaybhaskar - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1529/head:pull/1529
$ git checkout pull/1529

Update a local copy of the PR:
$ git checkout pull/1529
$ git pull https://git.openjdk.org/jfx.git pull/1529/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1529

View PR using the GUI difftool:
$ git pr show -t 1529

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1529.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-02T21:29:33Z

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-08-02T21:30:29Z

@kevinrushforth This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8337280: Include jdk.jsobject module with JavaFX

Reviewed-by: arapte, jbhaskar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

kevinrushforth · 2024-08-02T21:31:14Z

modules/jdk.jsobject/src/main/java/netscape/javascript/JSObject.java

+    static {
+        System.err.println("KCR: JavaFX version of jdk.jsobject loaded");
+    }


This debug print statement is there to make it easy to see if the JavaFX version of this class is being loaded. I'll leave it during testing and review, but I'll remove it before the final review.

kevinrushforth · 2024-09-05T21:12:55Z

/reviewers 2

openjdk · 2024-09-05T21:13:22Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2024-09-05T21:18:02Z

Webrevs

AnirvanSarkar · 2024-09-06T01:31:13Z

settings.gradle

The update to copyright year has been reverted by commit 33355b4.

I can't remember why I did that, but I'll redo the change to 2024. Thanks.

AnirvanSarkar · 2024-09-06T05:04:11Z

Old version of classes netscape.javascript.JSObject and netscape.javascript.JSException seems to be delivered with JavaFX for Android [1].

Should these classes be removed?

[1] : https://github.com/openjdk/jfx/tree/jfx23/modules/javafx.web/src/android/java/netscape/javascript

mlbridge · 2024-09-06T13:32:49Z

Mailing list message from Johan Vos on openjfx-dev:

Hi Kevin,

Thanks for the extensive checks on these scenario's. I believe the scenario
where jlink is used with an "old" JDK (e.g. 21) in combination with the
most recent JavaFX release is very common. In that case, it seems to be
required to prepend the module path with $JAVA_HOME/jmods, correct?
If that is the case, we need to announce and explain it very well, as
developers will run into it and get confused.

- Johan

Op do 5 sep 2024 om 23:18 schreef Kevin Rushforth <kcr at openjdk.org>:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240906/56eed1be/attachment-0001.htm>

kevinrushforth · 2024-09-06T22:43:11Z

Old version of classes netscape.javascript.JSObject and netscape.javascript.JSException seems to be delivered with JavaFX for Android [1].

Should these classes be removed?

[1] : https://github.com/openjdk/jfx/tree/jfx23/modules/javafx.web/src/android/java/netscape/javascript

I'll let @johanvos speak to that. If this is desired, then it's probably best done as a follow-up, since I can't test the Android (or iOS) implementation.

kevinrushforth · 2024-09-06T22:52:55Z

I believe the scenario where jlink is used with an "old" JDK (e.g. 21) in combination with the most recent JavaFX release is very common. In that case, it seems to be required to prepend the module path with $JAVA_HOME/jmods, correct?

Yes.

If that is the case, we need to announce and explain it very well, as developers will run into it and get confused.

Agreed. I've marked this RFE as needing a release note, and will be sure to clarify this.

johanvos · 2024-09-08T08:38:07Z

Old version of classes netscape.javascript.JSObject and netscape.javascript.JSException seems to be delivered with JavaFX for Android [1].
Should these classes be removed?
[1] : https://github.com/openjdk/jfx/tree/jfx23/modules/javafx.web/src/android/java/netscape/javascript

I'll let @johanvos speak to that. If this is desired, then it's probably best done as a follow-up, since I can't test the Android (or iOS) implementation.

Yes, those can be removed. There is more (ios/android) code that can be removed, or that even should be removed. I'll file a follow-up for this.

johanvos · 2024-09-08T08:41:35Z

Old version of classes netscape.javascript.JSObject and netscape.javascript.JSException seems to be delivered with JavaFX for Android [1].
Should these classes be removed?
[1] : https://github.com/openjdk/jfx/tree/jfx23/modules/javafx.web/src/android/java/netscape/javascript

I'll let @johanvos speak to that. If this is desired, then it's probably best done as a follow-up, since I can't test the Android (or iOS) implementation.

Yes, those can be removed. There is more (ios/android) code that can be removed, or that even should be removed. I'll file a follow-up for this.

Created https://bugs.openjdk.org/browse/JDK-8339705 for this

…r later

kevinrushforth · 2024-09-25T18:36:36Z

After merging in the latest master yesterday, I discovered that the recently-enabled javac lint warnings and -Werror when compiling the test classes will fail the build with "removal" warnings for those tests that use JSObject, when I run using my JDK 24 build with the patch from openjdk/jdk#20555. This is a good thing, since it means that those tests were not using the version of jdk.jsojbect from JavaFX. This highlights the needs to use --upgradle-module-path in our unit tests when running with JDK 24, so I pushed a commit that does that.

I'm done with my testing, and will switch to creating the CSRs.

Reviewers: @arapte @jaybhaskar

kevinrushforth · 2024-10-04T21:41:32Z

I've created the CSRs for both the this PR and openjdk/jdk#20555.

jaybhaskar

+looks good to me, The existing web test has setMember like tests.

AnirvanSarkar · 2024-10-11T18:02:59Z

I noticed that other JavaFX modules have make/build.properties file.
It seems this file is used when co-bundling JavaFX with the JDK, when JDK is configured to be built with --with-import-modules option.
Should we also add this file for jdk.jsobject module in JavaFX ?

kevinrushforth · 2024-10-11T18:11:18Z

Yes, I missed that. Thanks for catching it. I'll add a make/build.properties file, although the last time I checked, there were other problems in building a JavaFX bundle that could be used with --with-import-modules, so it may not be sufficient.

jaybhaskar

Looks good to me.

kevinrushforth · 2024-10-18T16:37:38Z

/integrate

openjdk · 2024-10-18T16:37:58Z

Going to push as commit f5b18ad.
Since your change was applied there has been 1 commit pushed to the master branch:

f71c390: 8340003: Bump minimum JDK version for JavaFX to JDK 22

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-18T16:38:06Z

@kevinrushforth Pushed as commit f5b18ad.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8337280: Include jdk.jsobject module with JavaFX

1abb538

kevinrushforth commented Aug 2, 2024

View reviewed changes

kevinrushforth added 3 commits August 2, 2024 14:46

Update to pick up latest version from jdk master

83ce053

Merge branch 'master' into 8337280-jsobject

1891887

Update copyright years

33355b4

kevinrushforth mentioned this pull request Aug 12, 2024

8311530: Deprecate jdk.jsobject module for removal openjdk/jdk#20555

Closed

4 tasks

openjdk bot added the csr Need approved CSR to integrate pull request label Aug 12, 2024

kevinrushforth added 3 commits August 16, 2024 14:15

Merge branch 'master' into 8337280-jsobject

d37e3af

Merge branch 'master' into 8337280-jsobject

d5c5bd5

Merge branch 'master' into 8337280-jsobject

182fb1f

kevinrushforth marked this pull request as ready for review September 5, 2024 21:13

openjdk bot added the rfr Ready for review label Sep 5, 2024

AnirvanSarkar reviewed Sep 6, 2024

View reviewed changes

Update copyright year in settings.gradle

bb9a3b6

kevinrushforth added 2 commits September 24, 2024 10:32

Merge branch 'master' into 8337280-jsobject

6cec839

Use --upgrade-module-path when compiling or running tests on JDK 24 o…

2c00deb

…r later

Merge branch 'master' into 8337280-jsobject

6bfc99a

arapte approved these changes Oct 8, 2024

View reviewed changes

Revert debug print statement

69513ac

kevinrushforth requested a review from arapte October 10, 2024 12:28

arapte approved these changes Oct 10, 2024

View reviewed changes

jaybhaskar approved these changes Oct 10, 2024

View reviewed changes

Add modules/jdk.jsobject/make/build.properties to match other modules

4640d7f

kevinrushforth requested review from jaybhaskar and arapte October 11, 2024 18:49

arapte approved these changes Oct 15, 2024

View reviewed changes

jaybhaskar approved these changes Oct 16, 2024

View reviewed changes

openjdk bot added ready Ready to be integrated and removed csr Need approved CSR to integrate pull request labels Oct 17, 2024

Merge branch 'master' into 8337280-jsobject

ce91c15

openjdk bot added the integrated Pull request has been integrated label Oct 18, 2024

openjdk bot closed this Oct 18, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Oct 18, 2024

kevinrushforth deleted the 8337280-jsobject branch October 18, 2024 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8337280: Include jdk.jsobject module with JavaFX #1529

8337280: Include jdk.jsobject module with JavaFX #1529

kevinrushforth commented Aug 2, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 2, 2024

openjdk bot commented Aug 2, 2024 •

edited

Loading

kevinrushforth Aug 2, 2024

kevinrushforth commented Sep 5, 2024

openjdk bot commented Sep 5, 2024

mlbridge bot commented Sep 5, 2024 •

edited

Loading

AnirvanSarkar Sep 6, 2024

kevinrushforth Sep 6, 2024

AnirvanSarkar commented Sep 6, 2024

mlbridge bot commented Sep 6, 2024

kevinrushforth commented Sep 6, 2024

kevinrushforth commented Sep 6, 2024

johanvos commented Sep 8, 2024

johanvos commented Sep 8, 2024

kevinrushforth commented Sep 25, 2024

kevinrushforth commented Oct 4, 2024

jaybhaskar left a comment

AnirvanSarkar commented Oct 11, 2024

kevinrushforth commented Oct 11, 2024

jaybhaskar left a comment

kevinrushforth commented Oct 18, 2024

openjdk bot commented Oct 18, 2024

openjdk bot commented Oct 18, 2024

8337280: Include jdk.jsobject module with JavaFX #1529

8337280: Include jdk.jsobject module with JavaFX #1529

Conversation

kevinrushforth commented Aug 2, 2024 • edited by openjdk bot Loading

Testing scenarios

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 2, 2024

openjdk bot commented Aug 2, 2024 • edited Loading

kevinrushforth Aug 2, 2024

Choose a reason for hiding this comment

kevinrushforth commented Sep 5, 2024

openjdk bot commented Sep 5, 2024

mlbridge bot commented Sep 5, 2024 • edited Loading

Webrevs

AnirvanSarkar Sep 6, 2024

Choose a reason for hiding this comment

kevinrushforth Sep 6, 2024

Choose a reason for hiding this comment

AnirvanSarkar commented Sep 6, 2024

mlbridge bot commented Sep 6, 2024

kevinrushforth commented Sep 6, 2024

kevinrushforth commented Sep 6, 2024

johanvos commented Sep 8, 2024

johanvos commented Sep 8, 2024

kevinrushforth commented Sep 25, 2024

kevinrushforth commented Oct 4, 2024

jaybhaskar left a comment

Choose a reason for hiding this comment

AnirvanSarkar commented Oct 11, 2024

kevinrushforth commented Oct 11, 2024

jaybhaskar left a comment

Choose a reason for hiding this comment

kevinrushforth commented Oct 18, 2024

openjdk bot commented Oct 18, 2024

openjdk bot commented Oct 18, 2024

kevinrushforth commented Aug 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 2, 2024 •

edited

Loading

mlbridge bot commented Sep 5, 2024 •

edited

Loading