Re: RFR: 8264139: Suppress removal warnings for Security Manager methods [v2]
On Fri, 4 Jun 2021 16:39:26 GMT, Kevin Rushforth wrote:
>> This PR adds the necessary `@SuppressWarnings("removal")` annotations for
>> the recently-integrated security manager deprecation, [JEP
>> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
>>
>> There are four commits:
>>
>> 1. 678b026 : A patch generated by @wangweij to add annotations to the
>> runtime (`modules/*/src/main/java`) using the same automated tooling that he
>> used as part of the implementation of JEP 411.
>> 2. 9698e87 : Same as above for the tests.
>> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a
>> static import) was causing a removal warning.
>> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added
>> to an entire class, or to a large method where only a small part of the
>> method uses a deprecated method. This was done using similar techniques to
>> the following fixes that Weijun did in openjdk/jdk#4172.
>>
>> The first two commits represent the bulk of the changes. Other than scanning
>> to ensure that there are no obvious errors, and testing, they probably don't
>> need the same level of scrutiny as the manual changes do.
>>
>> I tested this on all three platforms by doing a build / test with `JDK_HOME`
>> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the
>> build with `gradle -PLINT=removal` and verified that there were removal
>> warnings for the security manager APIs without this fix and none with this
>> fix.
>>
>> NOTE: The following files under `modules/javafx.web/src/android` and
>> `modules/javafx.web/src/ios` were not processed by the automated tool. As I
>> have no way to compile them, I chose not to manually fix them either, but
>> doing so would be trivial as a follow-up fix if desired.
>>
>>
>> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
>> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
>> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
>
> Kevin Rushforth has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review feedback
Looks good to me.
-
Marked as reviewed by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/528
Re: RFR: 8264139: Suppress removal warnings for Security Manager methods [v2]
On Fri, 4 Jun 2021 16:39:26 GMT, Kevin Rushforth wrote:
>> This PR adds the necessary `@SuppressWarnings("removal")` annotations for
>> the recently-integrated security manager deprecation, [JEP
>> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
>>
>> There are four commits:
>>
>> 1. 678b026 : A patch generated by @wangweij to add annotations to the
>> runtime (`modules/*/src/main/java`) using the same automated tooling that he
>> used as part of the implementation of JEP 411.
>> 2. 9698e87 : Same as above for the tests.
>> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a
>> static import) was causing a removal warning.
>> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added
>> to an entire class, or to a large method where only a small part of the
>> method uses a deprecated method. This was done using similar techniques to
>> the following fixes that Weijun did in openjdk/jdk#4172.
>>
>> The first two commits represent the bulk of the changes. Other than scanning
>> to ensure that there are no obvious errors, and testing, they probably don't
>> need the same level of scrutiny as the manual changes do.
>>
>> I tested this on all three platforms by doing a build / test with `JDK_HOME`
>> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the
>> build with `gradle -PLINT=removal` and verified that there were removal
>> warnings for the security manager APIs without this fix and none with this
>> fix.
>>
>> NOTE: The following files under `modules/javafx.web/src/android` and
>> `modules/javafx.web/src/ios` were not processed by the automated tool. As I
>> have no way to compile them, I chose not to manually fix them either, but
>> doing so would be trivial as a follow-up fix if desired.
>>
>>
>> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
>> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
>> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
>
> Kevin Rushforth has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review feedback
Marked as reviewed by aghaisas (Reviewer).
When built with JDK 17ea26 + `gradle -PLINT=removal` - I cross verified on my
macBook (macOS 10.15) that -
- without this fix - the warnings are generated
- with this fix - the warnings are not generated
-
PR: https://git.openjdk.java.net/jfx/pull/528
Re: RFR: 8264139: Suppress removal warnings for Security Manager methods [v2]
> This PR adds the necessary `@SuppressWarnings("removal")` annotations for the
> recently-integrated security manager deprecation, [JEP
> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
>
> There are four commits:
>
> 1. 678b026 : A patch generated by @wangweij to add annotations to the runtime
> (`modules/*/src/main/java`) using the same automated tooling that he used as
> part of the implementation of JEP 411.
> 2. 9698e87 : Same as above for the tests.
> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a
> static import) was causing a removal warning.
> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added
> to an entire class, or to a large method where only a small part of the
> method uses a deprecated method. This was done using similar techniques to
> the following fixes that Weijun did in openjdk/jdk#4172.
>
> The first two commits represent the bulk of the changes. Other than scanning
> to ensure that there are no obvious errors, and testing, they probably don't
> need the same level of scrutiny as the manual changes do.
>
> I tested this on all three platforms by doing a build / test with `JDK_HOME`
> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the
> build with `gradle -PLINT=removal` and verified that there were removal
> warnings for the security manager APIs without this fix and none with this
> fix.
>
> NOTE: The following files under `modules/javafx.web/src/android` and
> `modules/javafx.web/src/ios` were not processed by the automated tool. As I
> have no way to compile them, I chose not to manually fix them either, but
> doing so would be trivial as a follow-up fix if desired.
>
>
> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
Kevin Rushforth has updated the pull request incrementally with one additional
commit since the last revision:
Address review feedback
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/528/files
- new: https://git.openjdk.java.net/jfx/pull/528/files/4f87d187..84775ff5
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=528=01
- incr: https://webrevs.openjdk.java.net/?repo=jfx=528=00-01
Stats: 42 lines in 6 files changed: 6 ins; 25 del; 11 mod
Patch: https://git.openjdk.java.net/jfx/pull/528.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/528/head:pull/528
PR: https://git.openjdk.java.net/jfx/pull/528
Re: RFR: 8264139: Suppress removal warnings for Security Manager methods
On Fri, 4 Jun 2021 14:37:05 GMT, Weijun Wang wrote:
>> This PR adds the necessary `@SuppressWarnings("removal")` annotations for
>> the recently-integrated security manager deprecation, [JEP
>> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
>>
>> There are four commits:
>>
>> 1. 678b026 : A patch generated by @wangweij to add annotations to the
>> runtime (`modules/*/src/main/java`) using the same automated tooling that he
>> used as part of the implementation of JEP 411.
>> 2. 9698e87 : Same as above for the tests.
>> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a
>> static import) was causing a removal warning.
>> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added
>> to an entire class, or to a large method where only a small part of the
>> method uses a deprecated method. This was done using similar techniques to
>> the following fixes that Weijun did in openjdk/jdk#4172.
>>
>> The first two commits represent the bulk of the changes. Other than scanning
>> to ensure that there are no obvious errors, and testing, they probably don't
>> need the same level of scrutiny as the manual changes do.
>>
>> I tested this on all three platforms by doing a build / test with `JDK_HOME`
>> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the
>> build with `gradle -PLINT=removal` and verified that there were removal
>> warnings for the security manager APIs without this fix and none with this
>> fix.
>>
>> NOTE: The following files under `modules/javafx.web/src/android` and
>> `modules/javafx.web/src/ios` were not processed by the automated tool. As I
>> have no way to compile them, I chose not to manually fix them either, but
>> doing so would be trivial as a follow-up fix if desired.
>>
>>
>> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
>> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
>> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
>
> modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/ModuleHelper.java line
> 41:
>
>> 39:
>> 40: static {
>> 41: verbose =
>> AccessController.doPrivileged((PrivilegedAction) () ->
>
> You can merge this assignment with the declaration on line 38. Or you can
> keep this so the check of verbose is in the same block with its assignment.
I think I'll keep this one as is to minimize changes.
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java line 43:
>
>> 41:
>> 42: static {
>> 43: @SuppressWarnings("removal")
>
> Combine assignment and declaration?
Good idea. This allows the static block to be removed.
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/LinuxArch.java
> line 36:
>
>> 34:
>> 35: static {
>> 36: bits = AccessController.doPrivileged((PrivilegedAction)
>> () -> {
>
> Combine?
Good idea. This allows the static block to be removed.
> modules/javafx.web/src/main/java/com/sun/webkit/network/HTTP2Loader.java line
> 415:
>
>> 413: // Run the HttpClient in the page's access control context
>> 414: @SuppressWarnings("removal")
>> 415: CompletableFuture tmpResponse =
>> AccessController.doPrivileged((PrivilegedAction>) ()
>> -> {
>
> Is "var" enough?
Yes, I'll change it.
-
PR: https://git.openjdk.java.net/jfx/pull/528
Re: RFR: 8264139: Suppress removal warnings for Security Manager methods
On Fri, 4 Jun 2021 14:31:49 GMT, Weijun Wang wrote:
>> This PR adds the necessary `@SuppressWarnings("removal")` annotations for
>> the recently-integrated security manager deprecation, [JEP
>> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
>>
>> There are four commits:
>>
>> 1. 678b026 : A patch generated by @wangweij to add annotations to the
>> runtime (`modules/*/src/main/java`) using the same automated tooling that he
>> used as part of the implementation of JEP 411.
>> 2. 9698e87 : Same as above for the tests.
>> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a
>> static import) was causing a removal warning.
>> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added
>> to an entire class, or to a large method where only a small part of the
>> method uses a deprecated method. This was done using similar techniques to
>> the following fixes that Weijun did in openjdk/jdk#4172.
>>
>> The first two commits represent the bulk of the changes. Other than scanning
>> to ensure that there are no obvious errors, and testing, they probably don't
>> need the same level of scrutiny as the manual changes do.
>>
>> I tested this on all three platforms by doing a build / test with `JDK_HOME`
>> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the
>> build with `gradle -PLINT=removal` and verified that there were removal
>> warnings for the security manager APIs without this fix and none with this
>> fix.
>>
>> NOTE: The following files under `modules/javafx.web/src/android` and
>> `modules/javafx.web/src/ios` were not processed by the automated tool. As I
>> have no way to compile them, I chose not to manually fix them either, but
>> doing so would be trivial as a follow-up fix if desired.
>>
>>
>> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
>> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
>> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9969:
>
>> 9967: accessible =
>> Application.GetApplication().createAccessible();
>> 9968: accessible.setEventHandler(new Accessible.EventHandler() {
>> 9969: @SuppressWarnings("removal")
>
> Was this "deprecation" already useless before this change?
Yes. The code to which it applied was removed back in JDK 9, but we forgot to
remove the annotation.
-
PR: https://git.openjdk.java.net/jfx/pull/528
Re: RFR: 8264139: Suppress removal warnings for Security Manager methods
On Thu, 3 Jun 2021 23:34:38 GMT, Kevin Rushforth wrote:
> This PR adds the necessary `@SuppressWarnings("removal")` annotations for the
> recently-integrated security manager deprecation, [JEP
> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
>
> There are four commits:
>
> 1. 678b026 : A patch generated by @wangweij to add annotations to the runtime
> (`modules/*/src/main/java`) using the same automated tooling that he used as
> part of the implementation of JEP 411.
> 2. 9698e87 : Same as above for the tests.
> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a
> static import) was causing a removal warning.
> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added
> to an entire class, or to a large method where only a small part of the
> method uses a deprecated method. This was done using similar techniques to
> the following fixes that Weijun did in openjdk/jdk#4172.
>
> The first two commits represent the bulk of the changes. Other than scanning
> to ensure that there are no obvious errors, and testing, they probably don't
> need the same level of scrutiny as the manual changes do.
>
> I tested this on all three platforms by doing a build / test with `JDK_HOME`
> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the
> build with `gradle -PLINT=removal` and verified that there were removal
> warnings for the security manager APIs without this fix and none with this
> fix.
>
> NOTE: The following files under `modules/javafx.web/src/android` and
> `modules/javafx.web/src/ios` were not processed by the automated tool. As I
> have no way to compile them, I chose not to manually fix them either, but
> doing so would be trivial as a follow-up fix if desired.
>
>
> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
All change looks good. Some personal style preferences comments.
modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/ModuleHelper.java line 41:
> 39:
> 40: static {
> 41: verbose =
> AccessController.doPrivileged((PrivilegedAction) () ->
You can merge this assignment with the declaration on line 38. Or you can keep
this so the check of verbose is in the same block with its assignment.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java line 43:
> 41:
> 42: static {
> 43: @SuppressWarnings("removal")
Combine assignment and declaration?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/LinuxArch.java
line 36:
> 34:
> 35: static {
> 36: bits = AccessController.doPrivileged((PrivilegedAction)
> () -> {
Combine?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9969:
> 9967: accessible =
> Application.GetApplication().createAccessible();
> 9968: accessible.setEventHandler(new Accessible.EventHandler() {
> 9969: @SuppressWarnings("removal")
Was this "deprecation" already useless before this change?
modules/javafx.web/src/main/java/com/sun/webkit/network/HTTP2Loader.java line
415:
> 413: // Run the HttpClient in the page's access control context
> 414: @SuppressWarnings("removal")
> 415: CompletableFuture tmpResponse =
> AccessController.doPrivileged((PrivilegedAction>) ()
> -> {
Is "var" enough?
modules/javafx.web/src/main/java/com/sun/webkit/network/NetworkContext.java
line 245:
> 243: private static final Permission modifyThreadPerm = new
> RuntimePermission("modifyThread");
> 244:
> 245: @SuppressWarnings("removal")
Maybe move this onto the 1st line inside the method?
-
Marked as reviewed by weijun (no project role).
PR: https://git.openjdk.java.net/jfx/pull/528
RFR: 8264139: Suppress removal warnings for Security Manager methods
This PR adds the necessary `@SuppressWarnings("removal")` annotations for the
recently-integrated security manager deprecation, [JEP
411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
There are four commits:
1. 678b026 : A patch generated by @wangweij to add annotations to the runtime
(`modules/*/src/main/java`) using the same automated tooling that he used as
part of the implementation of JEP 411.
2. 9698e87 : Same as above for the tests.
3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a static
import) was causing a removal warning.
4. 4f87d18 : Manually reduced the scope of the annotation where it was added to
an entire class, or to a large method where only a small part of the method
uses a deprecated method. This was done using similar techniques to the
following fixes that Weijun did in openjdk/jdk#4172.
The first two commits represent the bulk of the changes. Other than scanning to
ensure that there are no obvious errors, and testing, they probably don't need
the same level of scrutiny as the manual changes do.
I tested this on all three platforms by doing a build / test with `JDK_HOME`
set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the
build with `gradle -PLINT=removal` and verified that there were removal
warnings for the security manager APIs without this fix and none with this fix.
NOTE: The following files under `modules/javafx.web/src/android` and
`modules/javafx.web/src/ios` were not processed by the automated tool. As I
have no way to compile them, I chose not to manually fix them either, but doing
so would be trivial as a follow-up fix if desired.
modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
-
Commit messages:
- Narrow the scope of @SuppressWarnings("removal") annotation
- Remove unused security manager imports
- Suppress removal warnings for Security Manager methods in tests
- 8264139: Suppress removal warnings for Security Manager methods
Changes: https://git.openjdk.java.net/jfx/pull/528/files
Webrev: https://webrevs.openjdk.java.net/?repo=jfx=528=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8264139
Stats: 586 lines in 194 files changed: 464 ins; 3 del; 119 mod
Patch: https://git.openjdk.java.net/jfx/pull/528.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/528/head:pull/528
PR: https://git.openjdk.java.net/jfx/pull/528
