On Thu, 31 Oct 2024 14:09:52 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
>> This PR removes uses of `AccessControler.doPrivileged()` from >> `javafx.graphics` package. All uses of `doPrivileged()` were removed >> _excluding_ `com.sun.javafx.tk` and `com.sun.ui.glass` subpackages - those >> are handled by [JDK-8342453](https://bugs.openjdk.org/browse/JDK-8342453) >> (already completed) and >> [JDK-8342454](https://bugs.openjdk.org/browse/JDK-8342454) respectively. >> >> Most of these changes are fairly straightforward and follow the standard >> replacement from `AccessController.doPrivileged(LAMBDA)` to just simply >> `LAMBDA` with a couple exceptions: >> >> - `StyleManager.java` where `loadStylesheet()` followed two steps - >> attempting straightforward stylesheet load via >> `loadStylesheetUnPrivileged()` and in case of `AccessControlException` >> retrying the load via `AccessController.doPrivileged` with read-only access >> to stylesheet JAR. Now that `doPrivileged` is deprecated and not used >> anymore, `loadStylesheetUnPrivileged()` was always successful and the >> fallback became essentially dead code. Fallback was removed and >> `loadStylesheetUnPrivileged()` was renamed to `loadStylesheet()`. >> - `Scene.java` integrates with `com.sun.javafx.tk` so only straightforward >> `doPrivileged()` calls were removed - `AccessControlContext` remained, as it >> needs to be removed from both tk and Scene via a follow-up >> ([JDK-8342993](https://bugs.openjdk.org/browse/JDK-8342993)) >> >> Building graphics module now produces less warnings than before this change >> with no new warning messages (all warnings refer to the use of `Unsafe` >> which is out of scope of this change). > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Scene: Remove missed doPrivileged use Overall this looks good. Here is my first batch of comments (there might be more, since I haven't looked at all files yet). modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 684: > 682: public static boolean hasFullScreenStage(final Screen screen) { > 683: @SuppressWarnings("removal") > 684: final List<Window> allWindows = Window.getWindows(); After this change there is an unused import of `ACCESS_WINDOW_LIST_PERMISSION` and the `SuppressWarnings` annotation is no longer needed. modules/javafx.graphics/src/main/java/com/sun/marlin/RendererStats.java line 362: > 360: private RendererStatsHolder() { > 361: AccessController.doPrivileged( > 362: (PrivilegedAction<Void>) () -> { There are unused imports after this change. modules/javafx.graphics/src/main/java/javafx/scene/PropertyHelper.java line 38: > 36: @SuppressWarnings("removal") > 37: boolean answer = > 38: > AccessController.doPrivileged((java.security.PrivilegedAction<Boolean>) () -> > { Following the removal of the doPrivileged call, the second sentence in the comment block of this method -- starting with "Note that ..." -- is wrong and can be removed. modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1448: > 1446: final AccessControlContext acc = AccessController.getContext(); > 1447: snapshotRunnableList.add(() -> { > 1448: AccessController.doPrivileged((PrivilegedAction<Void>) () > -> { I think that this method has a now-unneeded `SuppressWarnings` annotation. ------------- PR Review: https://git.openjdk.org/jfx/pull/1619#pullrequestreview-2408466054 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824820526 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824775661 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824792621 PR Review Comment: https://git.openjdk.org/jfx/pull/1619#discussion_r1824782309
