Re: RFR: 8263202: Update Hebrew/Indonesian/Yiddish ISO 639 language codes to current [v2]
On Tue, 18 May 2021 23:35:12 GMT, Naoto Sato wrote:
>> test/jdk/java/util/Locale/LocaleTest.java line 683:
>>
>>> 681: * @bug 4052404 4778440 8263202
>>> 682: */
>>> 683: public void TestChangedISO639Codes() {
>>
>> Could probably be simplified with a DataProvider.
>
> That would be nice, but the test is not testng based, and it would be an
> entire test rewrite which I would not do it at this time.
I see. Good old test is still good.
-
PR: https://git.openjdk.java.net/jdk/pull/4069
Re: RFR: 8263202: Update Hebrew/Indonesian/Yiddish ISO 639 language codes to current [v2]
On Tue, 18 May 2021 23:39:37 GMT, Naoto Sato wrote: >> Please review the changes to the subject issue. java.util.Locale class has a >> long-standing issue for those obsolete ISO 639 languages where its >> normalization ends up in the obsolete codes. This change intends to flip the >> normalization towards the current codes, providing a system property for >> compatibility behavior. ResourceBundle class is also modified to load either >> obsolete/current bundles. For more detail, take a look at the CSR. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Locale's class description modification Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4069
Re: RFR: 8263202: Update Hebrew/Indonesian/Yiddish ISO 639 language codes to current [v2]
On Tue, 18 May 2021 22:22:06 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Locale's class description modification
>
> src/java.base/share/classes/java/util/Locale.java line 462:
>
>> 460: * backward compatible forms.
>> 461: *
>> 462: * The APIs added in 1.7 map between the old and new language codes,
>
> This paragraph needs a rewrite as well it seems, esp. the part that states
> "getLanguage and toString reflect the old code" is no longer true.
Good catch! In fact, I had modified this paragraph in my preliminary fix, but
it slipped away somehow along the fix. Corrected the PR and CSR as well.
> test/jdk/java/util/Locale/LocaleTest.java line 683:
>
>> 681: * @bug 4052404 4778440 8263202
>> 682: */
>> 683: public void TestChangedISO639Codes() {
>
> Could probably be simplified with a DataProvider.
That would be nice, but the test is not testng based, and it would be an entire
test rewrite which I would not do it at this time.
-
PR: https://git.openjdk.java.net/jdk/pull/4069
Re: RFR: 8263202: Update Hebrew/Indonesian/Yiddish ISO 639 language codes to current [v2]
> Please review the changes to the subject issue. java.util.Locale class has a > long-standing issue for those obsolete ISO 639 languages where its > normalization ends up in the obsolete codes. This change intends to flip the > normalization towards the current codes, providing a system property for > compatibility behavior. ResourceBundle class is also modified to load either > obsolete/current bundles. For more detail, take a look at the CSR. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Locale's class description modification - Changes: - all: https://git.openjdk.java.net/jdk/pull/4069/files - new: https://git.openjdk.java.net/jdk/pull/4069/files/dee95bf1..0f76ac45 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4069&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4069&range=00-01 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4069.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4069/head:pull/4069 PR: https://git.openjdk.java.net/jdk/pull/4069
Re: RFR: 8263202: Update Hebrew/Indonesian/Yiddish ISO 639 language codes to current
On Mon, 17 May 2021 16:55:35 GMT, Naoto Sato wrote:
> Please review the changes to the subject issue. java.util.Locale class has a
> long-standing issue for those obsolete ISO 639 languages where its
> normalization ends up in the obsolete codes. This change intends to flip the
> normalization towards the current codes, providing a system property for
> compatibility behavior. ResourceBundle class is also modified to load either
> obsolete/current bundles. For more detail, take a look at the CSR.
src/java.base/share/classes/java/util/Locale.java line 462:
> 460: * backward compatible forms.
> 461: *
> 462: * The APIs added in 1.7 map between the old and new language codes,
This paragraph needs a rewrite as well it seems, esp. the part that states
"getLanguage and toString reflect the old code" is no longer true.
test/jdk/java/util/Locale/LocaleTest.java line 683:
> 681: * @bug 4052404 4778440 8263202
> 682: */
> 683: public void TestChangedISO639Codes() {
Could probably be simplified with a DataProvider.
-
PR: https://git.openjdk.java.net/jdk/pull/4069
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
adjust order of VM options
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/900c28c0..bfa955ad
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=00-01
Stats: 59 lines in 18 files changed: 8 ins; 6 del; 45 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v2]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: feedback from Sean, Phil and Alan - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/eb6c566f..bb73093a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=00-01 Stats: 9 lines in 3 files changed: 4 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 18:38:52 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/java/security/AccessController.java line 877:
>>
>>> 875: @CallerSensitive
>>> 876: public static T doPrivileged(PrivilegedExceptionAction
>>> action,
>>> 877: @SuppressWarnings("removal")
>>> AccessControlContext context, Permission... perms)
>>
>> you might find it easier if you put the Permissions parameter on a new line.
>> There are several places in AccessController where the same thing happens.
>
> I'll try to update my auto-annotating program.
Turns out this only happens in this class:
$ rg '^\s*@SuppressWarnings("removal").*?,.'
src/java.base/share/classes/java/security/AccessController.java:449:
@SuppressWarnings("removal") AccessControlContext context, Permission... perms)
{
src/java.base/share/classes/java/security/AccessController.java:514:
@SuppressWarnings("removal") AccessControlContext context, Permission... perms)
{
src/java.base/share/classes/java/security/AccessController.java:877:
@SuppressWarnings("removal") AccessControlContext
context, Permission... perms)
I'll fix them manually.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Integrated: 8266949: Check possibility to disable OperationTimedOut on Unix
On Fri, 14 May 2021 21:26:33 GMT, Sergey Bylokhov wrote: > Our implementation of realSync on Unix is not straightforward, we try to move > the awt root window and catch the ConfigureNotify. The problem is that the > request to move the awt root window can be ignored, and it is possible that > the notification will not come. In this case, we throw non-specified > OperationTimedOut and it crashes the random tests here and there. > > I have applied the same logic as on Windows and Linux we will wait till > timeout(10 seconds) and then exit from the realsync. This pull request has now been integrated. Changeset: e6705c0e Author:Sergey Bylokhov URL: https://git.openjdk.java.net/jdk/commit/e6705c0e4b548a83197c3ea70bdef25ec65d4c00 Stats: 37 lines in 2 files changed: 14 ins; 17 del; 6 mod 8266949: Check possibility to disable OperationTimedOut on Unix Reviewed-by: azvegint, kizune - PR: https://git.openjdk.java.net/jdk/pull/4038
Re: RFR: 8266949: Check possibility to disable OperationTimedOut on Unix [v2]
On Sun, 16 May 2021 01:29:59 GMT, Sergey Bylokhov wrote: >> Our implementation of realSync on Unix is not straightforward, we try to >> move the awt root window and catch the ConfigureNotify. The problem is that >> the request to move the awt root window can be ignored, and it is possible >> that the notification will not come. In this case, we throw non-specified >> OperationTimedOut and it crashes the random tests here and there. >> >> I have applied the same logic as on Windows and Linux we will wait till >> timeout(10 seconds) and then exit from the realsync. > > Sergey Bylokhov has updated the pull request incrementally with one > additional commit since the last revision: > > Update SunToolkit.java Marked as reviewed by kizune (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4038
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 17:36:55 GMT, Alan Bateman wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>
> src/java.base/share/classes/java/security/AccessController.java line 877:
>
>> 875: @CallerSensitive
>> 876: public static T doPrivileged(PrivilegedExceptionAction
>> action,
>> 877: @SuppressWarnings("removal")
>> AccessControlContext context, Permission... perms)
>
> you might find it easier if you put the Permissions parameter on a new line.
> There are several places in AccessController where the same thing happens.
My rule is that a new line is added if there's only whitespaces before the
insert position and the previous line does not end with a comma. In this case,
unless I move `Permission... perms` onto a new line that an annotation on a new
line looks like covering both `context` and `perms`.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang wrote:
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, i18n, rmi, javadoc, swing, 2d, security,
> hotspot-runtime, nio, xml, beans, ~~core-libs~~, ~~net~~, compiler,
> ~~hotspot-gc~~). Mostly the rule is the same as how Skara adds labels, but
> there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
I reviewed non-client areas. Looks okay.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote:
> Please review this implementation of [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> The code change is divided into 3 commits. Please review them one by one.
>
> 1.
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
> The essential change for this JEP, including the `@Deprecate` annotations
> and spec change. It also update the default value of the
> `java.security.manager` system property to "disallow", and necessary test
> change following this update.
> 2.
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
> Manual changes to several files so that the next commit can be generated
> programatically.
> 3.
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
> Automatic changes to other source files to avoid javac warnings on
> deprecation for removal
>
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
> generated programmatically, see the comment below for more details. If you
> are only interested in a portion of the 3rd commit and would like to review
> it as a separate file, please comment here and I'll generate an individual
> webrev.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> any file to minimize unnecessary merge conflict.
>
> Furthermore, since the default value of `java.security.manager` system
> property is now "disallow", most of the tests calling
> `System.setSecurityManager()` need to launched with
> `-Djava.security.manager=allow`. This is covered in a different PR at
> https://github.com/openjdk/jdk/pull/4071.
Marked as reviewed by alanb (Reviewer).
src/java.base/share/classes/java/security/AccessController.java line 877:
> 875: @CallerSensitive
> 876: public static T doPrivileged(PrivilegedExceptionAction action,
> 877: @SuppressWarnings("removal")
> AccessControlContext context, Permission... perms)
you might find it easier if you put the Permissions parameter on a new line.
There are several places in AccessController where the same thing happens.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266949: Check possibility to disable OperationTimedOut on Unix [v2]
On Sun, 16 May 2021 01:29:59 GMT, Sergey Bylokhov wrote: >> Our implementation of realSync on Unix is not straightforward, we try to >> move the awt root window and catch the ConfigureNotify. The problem is that >> the request to move the awt root window can be ignored, and it is possible >> that the notification will not come. In this case, we throw non-specified >> OperationTimedOut and it crashes the random tests here and there. >> >> I have applied the same logic as on Windows and Linux we will wait till >> timeout(10 seconds) and then exit from the realsync. > > Sergey Bylokhov has updated the pull request incrementally with one > additional commit since the last revision: > > Update SunToolkit.java Marked as reviewed by azvegint (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4038
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. java.xml changes look good. Thanks. - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Reviewed i18n/java.time/charset. They all look good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. The changes in the net area look fine. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 15:19:21 GMT, Alan Bateman wrote: >> It includes both: >>  > > Thanks for checking, I assumed that was the case so wondering if it should be > dropped from the deprecation text to avoid the repeated sentence. My feeling is that it is better to be more specific than the generic removal text as this is the most significant class that is being deprecated for removal. Also, this says "the Security Manager" (note the space between) which really encompasses more than the `java.lang.SecurityManager` API and which is explained more completely in the JEP. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 12:42:08 GMT, Sean Mullan wrote: >> src/java.base/share/classes/java/lang/SecurityManager.java line 315: >> >>> 313: * >>> 314: * @since 1.0 >>> 315: * @deprecated The Security Manager is deprecated and subject to >>> removal in a >> >> Javadoc will prefix, in bold, "Deprecated, for removal: This API element is >> subject to removal in a future version". I'm just wondering if the sentence >> will be repeated here, can you check? > > It includes both: >  Thanks for checking, I assumed that was the case so wondering if it should be dropped from the deprecation text to avoid the repeated sentence. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8208747: [a11y] [macos] In Optionpane Demo, inside ComponentDialog Example, unable to navigate to all items, with VO on
On Tue, 18 May 2021 14:50:23 GMT, Alexander Zuev wrote: > > The bug is a confidential issue. Either the bug should not be confidential > > or this review should not be done here. > > I do not see any potential reason why this issue might be confidential. It is > not security related, can be reproduced using the readily available demo and > affects a large part of functionality. Removing the confidential label from > the issue. That is what I thought. Anyway, I have done some basic testing and the fix looks fine. It does solve the issue. I will do some more testing and will approve accordingly. - PR: https://git.openjdk.java.net/jdk/pull/4084
Re: RFR: 8264290: Create implementation for NSAccessibilityComponentGroup protocol peer
On Tue, 18 May 2021 13:25:04 GMT, Pankaj Bansal wrote: > Could you please give some details about how you have tested this? I used the SwingSet2 first demo - internal frame - and made sure that components that were previously reported as groups - internal frame, button group and such - are correctly reported as groups and there are no regressions with navigating their children using the accessibility shortcut keys. Also with debug output i made sure that new code is used to gather list of the children inside the group components. Additionally i tested it with few existing AWT tests and made sure that there are no regression there too. - PR: https://git.openjdk.java.net/jdk/pull/4046
Re: RFR: 8208747: [a11y] [macos] In Optionpane Demo, inside ComponentDialog Example, unable to navigate to all items, with VO on
On Tue, 18 May 2021 09:30:28 GMT, Pankaj Bansal wrote: > The bug is a confidential issue. Either the bug should not be confidential or > this review should not be done here. I do not see any potential reason why this issue might be confidential. It is not security related, can be reproduced using the readily available demo and affects a large part of functionality. Removing the confidential label from the issue. - PR: https://git.openjdk.java.net/jdk/pull/4084
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Tue, 18 May 2021 05:48:56 GMT, Alan Bateman wrote: > The changes look okay but a bit inconsistent on where -Djava...=allow is > inserted for tests that already set other system properties or other > parameters. Not a correctness issue, just looks odd in several places, e.g. > > test/jdk/java/lang/System/LoggerFinder/BaseLoggerFinderTest/BaseLoggerFinderTest.java > - the tests sets the system properties after -Xbootclasspath/a: but the > change means it sets properties before and after. > > test/jdk/java/lang/Class/getResource/ResourcesTest.java - you've added it in > the middle of the module and class path parameters. > > For uses using ProcessTools then it seems to be very random. I've updated the 3 cases you mentioned and will go through more. Yes it looks good to group system property settings together. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Tue, 18 May 2021 11:12:00 GMT, Daniel Fuchs wrote:
>> Please review the test changes for [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> With JEP 411 and the default value of `-Djava.security.manager` becoming
>> `disallow`, tests calling `System.setSecurityManager()` need
>> `-Djava.security.manager=allow` when launched. This PR covers such changes
>> for tier1 to tier3 (except for the JCK tests).
>>
>> To make it easier to focus your review on the tests in your area, this PR is
>> divided into multiple commits for different areas (~~serviceability~~,
>> ~~hotspot-compiler~~, i18n, rmi, javadoc, swing, 2d, security,
>> hotspot-runtime, nio, xml, beans, ~~core-libs~~, ~~net~~, compiler,
>> ~~hotspot-gc~~). Mostly the rule is the same as how Skara adds labels, but
>> there are several small tweaks:
>>
>> 1. When a file is covered by multiple labels, only one is chosen. I make my
>> best to avoid putting too many tests into `core-libs`. If a file is covered
>> by `core-libs` and another label, I categorized it into the other label.
>> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
>> `xml` commit.
>> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
>> `rmi` commit.
>> 4. One file not covered by any label --
>> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
>> the `swing` commit.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> all files to minimize unnecessary merge conflict.
>>
>> Please note that this PR can be integrated before the source changes for JEP
>> 411, as the possible values of this system property was already defined long
>> time ago in JDK 9.
>>
>> Most of the change in this PR is a simple adding of
>> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes
>> it was not `othervm` and we add one. Sometimes there's no `@run` at all and
>> we add the line.
>>
>> There are several tests that launch another Java process that needs to call
>> the `System.setSecurityManager()` method, and the system property is added
>> to `ProcessBuilder`, `ProcessTools`, or the java command line (if the test
>> is a shell test).
>>
>> 3 langtools tests are added into problem list due to
>> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>>
>> 2 SQL tests are moved because they need different options on the `@run` line
>> but they are inside a directory that has a `TEST.properties`:
>>
>> rename test/jdk/java/sql/{testng/test/sql/othervm =>
>> permissionTests}/DriverManagerPermissionsTests.java (93%)
>> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
>> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>> ```
>>
>> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
>
> test/jdk/javax/management/remote/mandatory/notif/NotificationEmissionTest.java
> line 34:
>
>> 32: * @run clean NotificationEmissionTest
>> 33: * @run build NotificationEmissionTest
>> 34: * @run main NotificationEmissionTest 1
>
> This test case (NotificationEmissionTest 1) calls
> `System.setProperty("java.security.policy", policyFile);` - even though it
> doesn't call System.setSecurityManager(); Should the @run command line for
> test case 1 be modified as well?
Or maybe the policy setting line should only be called when a security manager
is set? IIRC jtreg is able to restore system properties even in agentvm mode.
So maybe this really doesn't matter. We just want to modify as little as
possible in this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8264290: Create implementation for NSAccessibilityComponentGroup protocol peer
On Mon, 17 May 2021 08:22:59 GMT, Alexander Zuev wrote: > Initial implementation. Could you please give some details about how you have tested this? - PR: https://git.openjdk.java.net/jdk/pull/4046
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 06:31:06 GMT, Alan Bateman wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. > > src/java.base/share/classes/java/lang/SecurityManager.java line 315: > >> 313: * >> 314: * @since 1.0 >> 315: * @deprecated The Security Manager is deprecated and subject to >> removal in a > > Javadoc will prefix, in bold, "Deprecated, for removal: This API element is > subject to removal in a future version". I'm just wondering if the sentence > will be repeated here, can you check? It includes both:  - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang wrote:
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas. Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
I looked at serviceability, net, and corelibs. The changes look good to me -
though I have one question about the NotificationEmissionTest. I believe that
regardless of whether the new property is needed, test case 1 should be run in
/othervm too.
test/jdk/javax/management/remote/mandatory/notif/NotificationEmissionTest.java
line 34:
> 32: * @run clean NotificationEmissionTest
> 33: * @run build NotificationEmissionTest
> 34: * @run main NotificationEmissionTest 1
This test case (NotificationEmissionTest 1) calls
`System.setProperty("java.security.policy", policyFile);` - even though it
doesn't call System.setSecurityManager(); Should the @run command line for test
case 1 be modified as well?
-
Marked as reviewed by dfuchs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8182043: Access to Windows Large Icons [v9]
On Tue, 18 May 2021 00:29:21 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed documentation based on CSR review feedback Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8208747: [a11y] [macos] In Optionpane Demo, inside ComponentDialog Example, unable to navigate to all items, with VO on
On Tue, 18 May 2021 09:05:10 GMT, Alexander Zuev wrote: > Added accessibilityIndex function that correctly returns the index of a > child in parent container The bug is a confidential issue. Either the bug should not be confidential or this review should not be done here. - PR: https://git.openjdk.java.net/jdk/pull/4084
RFR: 8208747: [a11y] [macos] In Optionpane Demo, inside ComponentDialog Example, unable to navigate to all items, with VO on
Added accessibilityIndex function that correctly returns the index of a child in parent container - Commit messages: - 8208747: [a11y] [macos] In Optionpane Demo, inside ComponentDialog Example, unable to navigate to all items, with VO on Changes: https://git.openjdk.java.net/jdk/pull/4084/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4084&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8208747 Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4084/head:pull/4084 PR: https://git.openjdk.java.net/jdk/pull/4084
