On Thu, 31 Oct 2024 22:51:24 GMT, Andy Goryachev <ango...@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
