Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
On Tue, 19 Nov 2024 19:20:48 GMT, Phil Race wrote: >> This enablePrivileges param is interesting and obsolete. >> Either here or in an immediate follow-on we should get rid of it. >> I'm inclined to do it in a small follow-on, not here. >> javax.swing.LookAndFeel.makeIcon(.) is the only place that calls >> SwingUtilities2.makeIcon_Unprivileged so it won't be hard. > > I submitted https://bugs.openjdk.org/browse/JDK-8344569 ok..you can assign to me if you want.. - PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1849468982
Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
On Fri, 15 Nov 2024 20:17:57 GMT, Phil Race wrote:
>> src/java.desktop/share/classes/sun/swing/SwingUtilities2.java line 1708:
>>
>>> 1706: return (UIDefaults.LazyValue) (table) -> {
>>> 1707: byte[] buffer = enablePrivileges ?
>>> 1708: getIconBytes(baseClass, rootClass, imageFile)
>>
>> Suggestion:
>>
>> byte[] buffer = getIconBytes(baseClass, rootClass, imageFile);
>>
>>
>> Maybe you could move the method body to `makeIcon(Class, Class,
>> String)`
>
> This enablePrivileges param is interesting and obsolete.
> Either here or in an immediate follow-on we should get rid of it.
> I'm inclined to do it in a small follow-on, not here.
> javax.swing.LookAndFeel.makeIcon(.) is the only place that calls
> SwingUtilities2.makeIcon_Unprivileged so it won't be hard.
I submitted https://bugs.openjdk.org/browse/JDK-8344569
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1848940296
Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
On Fri, 15 Nov 2024 19:50:43 GMT, Phil Race wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove Suppresswarning
>
> src/java.desktop/share/classes/sun/awt/image/ImagingLib.java line 91:
>
>> 89: static {
>> 90:
>> 91: System.loadLibrary("mlib_image");
>
> why did you remove the try/catch ? Seems like a semantic change that should
> not be made.
>
> Also this whole class has
> @SuppressWarnings({"removal", "restricted"})
>
> I'm not sure I see anything else that is deprecated for removal, so doesn't
> it need to be updated ?
Other loadLibrary calls in other files in this PR were without try-catch so I
removed it as I thought it redundant..
ANyway, I will revert it back..
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1845311420
Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
On Fri, 15 Nov 2024 19:49:02 GMT, Phil Race wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove Suppresswarning
>
> src/java.desktop/share/classes/sun/awt/image/ImageWatched.java line 138:
>
>> 136: // My referent is null so we must prune in a second
>> pass.
>> 137: ret = true;
>> 138: } else if (update(myiw, img, info, x, y, w, h) == false) {
>
> In other cases, I might not say this, but here enough has changed anyway that
> I will ..
> update() is now a one-line method, called from just this one place.
> I think you could delete it and directly call myiw.imageUpdate(img, info, x,
> y, w, h);
I agree, in-lining it as myiw.imageUpdate(img, info, x, y, w, h); is simpler.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844627779
Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
On Fri, 15 Nov 2024 10:26:05 GMT, Prasanta Sadhukhan wrote: >> Since JEP 486 : Permanently Disable the Security Manager >> [https://bugs.openjdk.org/browse/JDK-8338625] is now integrated, calls to >> java.security.AccessController.doPrivileged are obsolete and can be removed. >> >> This PR takes care of some of the shared-platform files in the java.desktop >> module to have them removed. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Remove Suppresswarning Went through the changes. Looks like few SM imports and SuppressWarnings removal were missed which @prrace has already mentioned. - PR Review: https://git.openjdk.org/jdk/pull/22133#pullrequestreview-2439878392
Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
On Fri, 15 Nov 2024 10:26:05 GMT, Prasanta Sadhukhan
wrote:
>> Since JEP 486 : Permanently Disable the Security Manager
>> [https://bugs.openjdk.org/browse/JDK-8338625] is now integrated, calls to
>> java.security.AccessController.doPrivileged are obsolete and can be removed.
>>
>> This PR takes care of some of the shared-platform files in the java.desktop
>> module to have them removed.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove Suppresswarning
src/java.desktop/share/classes/sun/awt/image/ImageWatched.java line 138:
> 136: // My referent is null so we must prune in a second pass.
> 137: ret = true;
> 138: } else if (update(myiw, img, info, x, y, w, h) == false) {
In other cases, I might not say this, but here enough has changed anyway that I
will ..
update() is now a one-line method, called from just this one place.
I think you could delete it and directly call myiw.imageUpdate(img, info, x, y,
w, h);
src/java.desktop/share/classes/sun/awt/image/ImagingLib.java line 91:
> 89: static {
> 90:
> 91: System.loadLibrary("mlib_image");
why did you remove the try/catch ? Seems like a semantic change that should not
be made.
Also this whole class has
@SuppressWarnings({"removal", "restricted"})
I'm not sure I see anything else that is deprecated for removal, so doesn't it
need to be updated ?
src/java.desktop/share/classes/sun/awt/util/PerformanceLogger.java line 90:
> 88:
> 89: static {
> 90: String perfLoggingProp = System.getProperty("sun.perflog");
where's the removal of SuppressWarnings ?
src/java.desktop/share/classes/sun/java2d/opengl/OGLRenderQueue.java line 51:
> 49: * which will not get GCed before VM exit.
> 50: */
> 51: flusher = new QueueFlusher();
looks like you forgot to remove SuppressWarnings
src/java.desktop/share/classes/sun/print/PSPrinterJob.java line 342:
> 340:
> 341: private static void initStatic() {
> 342: //enable privileges so initProps can access system properties,
This whole comment is obsolete.
src/java.desktop/share/classes/sun/print/RasterPrinterJob.java line 985:
> 983: final GraphicsConfiguration gc = grCfg;
> 984:
> 985: PrintService service = getPrintService();
I'm fairly sure you missed removing a SuppressWarnings for this method
src/java.desktop/share/classes/sun/swing/JLightweightFrame.java line 108:
> 106: */
> 107: private static boolean copyBufferEnabled = "true".equals(
> 108: System.getProperty("swing.jlf.copyBufferEnabled", "true"));
looks like you forgot to remove
import sun.security.action.GetPropertyAction;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844381493
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844382940
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844387309
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844392243
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844400524
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844406654
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844408813
Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
On Fri, 15 Nov 2024 10:26:28 GMT, Glavo wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove Suppresswarning
>
> src/java.desktop/share/classes/sun/swing/SwingUtilities2.java line 1708:
>
>> 1706: return (UIDefaults.LazyValue) (table) -> {
>> 1707: byte[] buffer = enablePrivileges ?
>> 1708: getIconBytes(baseClass, rootClass, imageFile)
>
> Suggestion:
>
> byte[] buffer = getIconBytes(baseClass, rootClass, imageFile);
>
>
> Maybe you could move the method body to `makeIcon(Class, Class, String)`
This enablePrivileges param is interesting and obsolete.
Either here or in an immediate follow-on we should get rid of it.
I'm inclined to do it in a small follow-on, not here.
javax.swing.LookAndFeel.makeIcon(.) is the only place that calls
SwingUtilities2.makeIcon_Unprivileged so it won't be hard.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1844416602
Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
On Fri, 15 Nov 2024 10:26:05 GMT, Prasanta Sadhukhan
wrote:
>> Since JEP 486 : Permanently Disable the Security Manager
>> [https://bugs.openjdk.org/browse/JDK-8338625] is now integrated, calls to
>> java.security.AccessController.doPrivileged are obsolete and can be removed.
>>
>> This PR takes care of some of the shared-platform files in the java.desktop
>> module to have them removed.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove Suppresswarning
src/java.desktop/share/classes/sun/swing/SwingUtilities2.java line 1708:
> 1706: return (UIDefaults.LazyValue) (table) -> {
> 1707: byte[] buffer = enablePrivileges ?
> 1708: getIconBytes(baseClass, rootClass, imageFile)
Suggestion:
byte[] buffer = getIconBytes(baseClass, rootClass, imageFile);
Maybe you could move the method body to `makeIcon(Class, Class, String)`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22133#discussion_r1843554527
Re: RFR: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1 [v2]
> Since JEP 486 : Permanently Disable the Security Manager > [https://bugs.openjdk.org/browse/JDK-8338625] is now integrated, calls to > java.security.AccessController.doPrivileged are obsolete and can be removed. > > This PR takes care of some of the shared-platform files in the java.desktop > module to have them removed. Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision: Remove Suppresswarning - Changes: - all: https://git.openjdk.org/jdk/pull/22133/files - new: https://git.openjdk.org/jdk/pull/22133/files/3176a799..be586922 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22133&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22133&range=00-01 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/22133.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22133/head:pull/22133 PR: https://git.openjdk.org/jdk/pull/22133
