RFR: 8342453: Remove calls to doPrivileged in javafx.graphics/com.sun.javafx.tk

[Kevin Rushforth]

On Tue, 29 Oct 2024 00:56:27 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> This PR removes all doPrivileged calls from `com.sun.javafx.tk**` in the `javafx.graphics` module. 
>> 
>> Here is a quick overview of what I did for this fix:
>> 
>> 1. Changed all simple cases of `doPrivileged((PrivilegedAction<T>) () -> LAMBDA)` to `LAMBDA`, removing the `@SuppressWarnings("removal")` if possible. In case of an unused or unneeded local variable, I removed the local variable.
>> 2. Remove unused `AccessControlContext` variables, meaning those whose only use was to be passed into a doPrivileged call that is now gone.
>> 3. Removed all `@SuppressWarnings("removal")` annotations that were unneeded after the removed doPrivileged calls. In some cases there are annotations on a method or class. Those can only be removed if the doPrivileged calls were the only use of deprecated SM API in that method or class.
>> 4. Remove unused imports.
>> 
>> Finally, here are a few "best practices" that I tried to follow, and would ask others to follow when doing their piece of this. The idea is to reduce the cognitive load on the reviewers. It might take you a couple extra minutes, but will save time during the review:
>> 
>> * When removing the doPrivileged calls, please do the minimum amount of reformatting necessary to properly indent the body of the doPrivileged after it is removed. For example, don't wholesale reformat a method (or worse, an entire class) just because a couple doPrivileged calls are removed.
>> * Please try to not reorder the import statements when removing unused imports.
>> 
>> #### Notes to reviewers
>> 
>> As a helpful hint for reviewers, I recommend reviewing this using the "Hide whitespace" option.
>> 
>> An initial, limited review of this was done in my personal fork at kevinrushforth/jfx#4 if other reviwers are interested.
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java line 142:
> 
>> 140: 
>> 141:     private static final boolean multithreaded = initMultithreaded();
>> 142:     private static boolean initMultithreaded() {
> 
> Have you considered a pattern such as this?
> 
> 
>     private static final boolean multithreaded = ((Supplier<Boolean>)() -> {
>         // If it is not specified, or it is true, then it should
>         // be true. Otherwise it should be false.
>         String value = System.getProperty("quantum.multithreaded");
>         if (value == null) return true;
>         final boolean result = Boolean.parseBoolean(value);
>         if (verbose) {
>             System.out.println(result ? "Multi-Threading Enabled" : "Multi-Threading Disabled");
>         }
>         return result;
>     }).get();
> 
> 
> This would prevent a proliferation of many new init methods.

That's an interesting suggestion: trading a new method for a lambda (the prior code used a lambda to pass to the doPrivileged, so it's also less of a delta). I'll take a look at it and see if it looks better to me.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1608#discussion_r1821617743