RFR: 8342460: Remove calls to doPrivileged in javafx.web
Kevin Rushforth
kcr at openjdk.org
Fri Nov 1 13:43:36 UTC 2024
On Thu, 31 Oct 2024 22:51:24 GMT, Andy Goryachev <angorya at openjdk.org> wrote:
> Removes `doPrivileged` calls in the javafx.web module, excluding the code in `{android,ios}`.
Looks good except for one problem that needs to be fixed. I also left a question inline about removed comment lines.
@arapte Can you be the second reviewer?
modules/javafx.web/src/main/java/com/sun/webkit/Utilities.java line 119:
> 117: }
> 118:
> 119: return MethodHelper.invoke(method, instance, args);
The removal of the try/catch is causing a test failure in `JavaScriptBridgeTest` because `InvocationTargetException` is now thrown rather than `InvocationTargetException::getCause`.
Suggestion:
try {
return MethodHelper.invoke(method, instance, args);
} catch (InvocationTargetException ex) {
Throwable cause = ex.getCause();
throw cause != null ? cause : ex;
}
modules/javafx.web/src/main/java/com/sun/webkit/network/HTTP2Loader.java line 580:
> 578: errorCode = LoadListenerClient.MALFORMED_URL;
> 579: } catch (@SuppressWarnings("removal") AccessControlException ex) {
> 580: errorCode = LoadListenerClient.PERMISSION_DENIED;
Excellent. This was on my list for [JDK-8342998](https://bugs.openjdk.org/browse/JDK-8342998), so one less thing for me to do for that (actually, two less things, since you also took care of the one in `URLLoader.java`).
modules/javafx.web/src/main/java/javafx/scene/web/HTMLEditorSkin.java line 835:
> 833: @SuppressWarnings("removal")
> 834: Image icon = AccessController.doPrivileged((PrivilegedAction<Image>) () -> new Image(HTMLEditorSkin.class.getResource(iconName).toString()));
> 835: // button.setGraphic(new ImageView(icon));
I see you removed this comment. Do you think there is no value in leaving it?
modules/javafx.web/src/main/java/javafx/scene/web/HTMLEditorSkin.java line 861:
> 859: Image icon = AccessController.doPrivileged((PrivilegedAction<Image>) () -> new Image(HTMLEditorSkin.class.getResource(iconName).toString()));
> 860: ((StyleableProperty)toggleButton.graphicProperty()).applyStyle(null, new ImageView(icon));
> 861: // toggleButton.setGraphic(new ImageView(icon));
Same question as above.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1620#pullrequestreview-2410029043
PR Comment: https://git.openjdk.org/jfx/pull/1620#issuecomment-2451891315
PR Review Comment: https://git.openjdk.org/jfx/pull/1620#discussion_r1825816669
PR Review Comment: https://git.openjdk.org/jfx/pull/1620#discussion_r1825794129
PR Review Comment: https://git.openjdk.org/jfx/pull/1620#discussion_r1825827044
PR Review Comment: https://git.openjdk.org/jfx/pull/1620#discussion_r1825827478
More information about the openjfx-dev
mailing list