Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 28 May 2021 03:12:35 GMT, Phil Race wrote: >> There *is* a tiny refactoring here: a new variable `s2` is introduced so the >> 2nd `doPrivileged` call on line 636 is now also in a declaration statement >> (for `s2`) and therefore annotatable. Without this I cannot add the >> annotation on line 635. > > Ok. But I will quote you > "This happens when a deprecated method is called inside a static block. The > annotation can only be added to a declaration and here it must be the whole > class" > > So the way you explained this before made it sound like any time there was > any SM API usage in a static block, the entire class needed to be annotated. > > Why has the explanation changed ? I should have been more precise. An annotation can only be added on a declaration, whether it's a variable, a method, or a class. Static block is not a declaration and the only one covers it is the class. But then if it's on a local variable declaration inside a static block, we certainly can annotate on that variable. - PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 28 May 2021 02:50:55 GMT, Weijun Wang wrote:
>> src/java.desktop/share/classes/java/awt/Component.java line 630:
>>
>>> 628: }
>>> 629:
>>> 630: @SuppressWarnings("removal")
>>
>> I'm confused. I thought the reason this wasn't done in the JEP
>> implementation PR is because of refactoring
>> that was needed because of the usage in this static block and you could not
>> apply the annotation here.
>> Yet it seems you are doing exactly what was supposed to be impossible with
>> no refactoring.
>> Can you explain ?
>
> There *is* a tiny refactoring here: a new variable `s2` is introduced so the
> 2nd `doPrivileged` call on line 636 is now also in a declaration statement
> (for `s2`) and therefore annotatable. Without this I cannot add the
> annotation on line 635.
Ok. But I will quote you
"This happens when a deprecated method is called inside a static block. The
annotation can only be added to a declaration and here it must be the whole
class"
So the way you explained this before made it sound like any time there was any
SM API usage in a static block, the entire class needed to be annotated.
Why has the explanation changed ?
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]
On Thu, 27 May 2021 20:16:25 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. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - move one annotation to new method > - merge from master, two files removed, one needs merge > - keep only one systemProperty tag > - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java > - feedback from Sean, Phil and Alan > - add supresswarnings annotations automatically > - manual change before automatic annotating > - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal Two files were removed by JEP 403 and JEP 407, respectively. One method in `XMLSchemaFactory.java` got [its own](https://github.com/openjdk/jdk/commit/8c4719a58834dddcea39d69b199abf1aabf780e2#diff-593a224979eaff03e2a3df1863fcaf865364a31a2212cc0d1fe67a8458057857R429) `@SuppressWarnings` and have to be merged with the one here. Another file `ResourceBundle.java` had a portion of a method extracted into a [new method](https://github.com/openjdk/jdk/commit/a4c46e1e4f4f2f05c8002b2af683a390fc46b424#diff-59caf1a68085064b4b3eb4f6e33e440bb85ea93719f34660970e2d4eaf8ce469R3175) and the annotation must be moved there. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8182043: Access to Windows Large Icons [v16]
On Fri, 7 May 2021 17:53:49 GMT, Alexander Zuev wrote: >> This comment is also about the case of not fetching icons of sizes smaller >> than requested size. > > Sorry, missed that in my latest fix. Indeed there is no legitimate ways to > set scaling factor to less than 100% on Windows so yes, omitting the icons > that are less than expected size. As for starting the count from the correct > index - to get the correct index we would have to traverse the array of > possible resolutions anyways so there is no performance gain. The MRI image takes care of graphics transformation which might be the same as a scale factor of the screen for the onscreen rendering, but in general, it might be set to any value by the application when rendered to the image. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Thu, 27 May 2021 16:38:02 GMT, Alexander Zuev wrote:
>> Is this requirement is so important? can we return an MRI(same as on
>> Windows) which will have just one resolution? Otherwise what the user should
>> do if the requested size was 32x32 but returned image will be 21x21? Paint
>> the small icon or rescale it by the application?
>
>> Is this requirement is so important? can we return an MRI(same as on
>> Windows) which will have just one resolution?
>
> We might - when the implementation will be done on other platforms. Probably
> it will be done by me, probably - by someone else. Right now we return
> whatever we have so on Linux it is UIManager default icons for file or a
> folder (which is exactly what any file manager on Linux shows for any file
> and this is exactly what we promised in the method description). In the
> future it can change but for now it is all we can guarantee.
This is my point I think we should update this implementation to always return
MRI of the requested size, otherwise, the code example of this will look like
this:
Icon icon = fsv.getSystemIcon(file, width, height);
if (icon.getIconWidth() != width && icon.getIconHeight() != height) {
return scaleTheIconInTheSameWayAsBeforeTheFix(icon, width, height);
} else if (icon instanceof ImageIcon) {
ImageIcon imageIcon = (ImageIcon) icon;
if (icon.getImage() instanceof MultiResolutionImage) {
MultiResolutionImage mri = (MultiResolutionImage) icon.getImage();
return mri.getResolutionVariant(width, height);
} else {
return imageIcon;
}
} else {
return icon;
}
I pretty sure we can do better than the code above.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8264125: Specification of Taskbar::getIconImage doesn't mention that the returned image might not be equal to the Taskbar::setIconImage one. (eg on Mac OS) [v2]
On Wed, 26 May 2021 17:51:55 GMT, Alexander Zvegintsev wrote: >> This fix is to explicitly specify that `Taskbar::getIconImage` may return an >> object different from passed to `Taskbar::setIconImage`. >> >> Actually it is always returns a different object on macOS(the only OS which >> supports this feature), but I want to save some options if we decide to >> rework this. > > Alexander Zvegintsev has updated the pull request with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - update > - Merge branch 'openjdk:master' into Taskbar_getIconImageSpec_8264125 > - initial @mrserb @dbessono please review - PR: https://git.openjdk.java.net/jdk/pull/3250
Integrated: 8182043: Access to Windows Large Icons
On Mon, 8 Mar 2021 13:22:07 GMT, Alexander Zuev wrote: > Fix updated after first round of review. This pull request has now been integrated. Changeset: 7f52c50b Author:Alexander Zuev URL: https://git.openjdk.java.net/jdk/commit/7f52c50ba32eecf5f379f8db30ac6a5cc50b3b66 Stats: 408 lines in 6 files changed: 328 ins; 39 del; 41 mod 8182043: Access to Windows Large Icons Reviewed-by: aivanov, azvegint, prr - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]
> 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. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - move one annotation to new method - merge from master, two files removed, one needs merge - keep only one systemProperty tag - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - feedback from Sean, Phil and Alan - add supresswarnings annotations automatically - manual change before automatic annotating - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal - Changes: https://git.openjdk.java.net/jdk/pull/4073/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4073=04 Stats: 2022 lines in 825 files changed: 1884 ins; 10 del; 128 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: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang wrote:
>> The code change refactors classes that have a `SuppressWarnings("removal")`
>> annotation that covers more than 50KB of code. The big annotation is often
>> quite faraway from the actual deprecated API usage it is suppressing, and
>> with the annotation covering too big a portion it's easy to call other
>> deprecated methods without being noticed.
>>
>> The code change shows some common solutions to avoid such too wide
>> annotations:
>>
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local
>> variable and add annotation on the declaration, and then assign that value
>> to the original l-value if not void. The local variable will be called `tmp`
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>>
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update FtpClient.java
src/java.desktop/share/classes/java/awt/Component.java line 630:
> 628: }
> 629:
> 630: @SuppressWarnings("removal")
I'm confused. I thought the reason this wasn't done in the JEP implementation
PR is because of refactoring
that was needed because of the usage in this static block and you could not
apply the annotation here.
Yet it seems you are doing exactly what was supposed to be impossible with no
refactoring.
Can you explain ?
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]
On Mon, 24 May 2021 13:53:34 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. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > keep only one systemProperty tag Marked as reviewed by prr (Reviewer). I'm OK with this now given that the refactoring is already underway at https://github.com/openjdk/jdk/pull/4138 - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Thu, 27 May 2021 16:16:19 GMT, Sergey Bylokhov wrote: > Is this requirement is so important? can we return an MRI(same as on Windows) > which will have just one resolution? We might - when the implementation will be done on other platforms. Probably it will be done by me, probably - by someone else. Right now we return whatever we have so on Linux it is UIManager default icons for file or a folder (which is exactly what any file manager on Linux shows for any file and this is exactly what we promised in the method description). In the future it can change but for now it is all we can guarantee. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 21:39:19 GMT, Alexander Zuev wrote:
>> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 87:
>>
>>> 85: }
>>> 86:
>>> 87: if (implComplete && icon.getIconWidth() != size) {
>>
>> Why the size of the returned images might be different? The spec state the
>> size parameter in the getSystemIcon is the size of the icon in the userspace.
>
> The spec says that we are taking into consideration requested sizes but
> depending on the platform implementation exact match can not be guaranteed.
> On Windows it can so i'm checking for that.
Is this requirement is so important? can we return an MRI(same as on Windows)
which will have just one resolution? Otherwise what the user should do if the
requested size was 32x32 but returned image will be 21x21? Paint the small icon
or rescale it by the application?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
On Wed, 26 May 2021 13:49:24 GMT, Alexey Ushakov wrote: > Set black color for underlying window background to perform correct blending > operations in metal with window surface destination Yes, looks like we have the flashing problem back. It seems more serious than this particular bug. We have two options here: 1) accept this behavior for metal (It would be not a problem if we implement JDK-8265445) 2) reimplement all the composing operations via shaders instead of using metal blending rules - PR: https://git.openjdk.java.net/jdk/pull/4206
