[OpenJDK 2D-Dev] Integrated: 8263486: Clean up MTLSurfaceDataBase.h
On Thu, 13 May 2021 11:43:17 GMT, Ajit Ghaisas wrote: > This PR addresses some cleanup activities : > - Cleaned up MTLSurfaceDataBase.h & MTLSurfaceData.m > - Removed OpenGL references from MTLPipelineStatesStorage.m & > MTLRenderQueue.m This pull request has now been integrated. Changeset: 72c9567b Author:Ajit Ghaisas URL: https://git.openjdk.java.net/jdk/commit/72c9567b4663fc816e4b85b46ea49b20ea78bd72 Stats: 79 lines in 6 files changed: 0 ins; 55 del; 24 mod 8263486: Clean up MTLSurfaceDataBase.h Reviewed-by: serb - PR: https://git.openjdk.java.net/jdk/pull/4010
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v11]
On Fri, 21 May 2021 21:24:10 GMT, Phil Race wrote: > if the aspect ratio is consistent as I expect is typical we should support > that Ok, i see your point. I can do that for sure, it is just a change of API for now, no implementation will be affected since Windows does not support non-square icons for files and passage about exact size can not be guaranteed will save us from "i requested 128x20 and you returned 128x128" complaints. One will get what platform is able to serve. However, i do not see why we should limit the maximum requested size. > I can't agree that IAE is wrong for a negative size. How about non-existing file or file? Also IAE? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v12]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Chenged implementation specification based on CSR review - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/548dcef1..56285783 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2875=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2875=10-11 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v11]
On Thu, 20 May 2021 22:25:05 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: > > Specification of getSystemIcon reworded to get rid of the non-public > class reference and to better describe some cornor cases I was reviewing the CSR so comments in the CSR are the obvious place for that. We can continue it here if you like 1) Will you be updating to explain how we map to (theoretical) non-square icons ? > As for the square icons - yes, we imply that the greater dimension is > specified so in case of 256x128 icon > the 256x256 icon will be returned with the 256x128 bitmap inside and the > 256x256 reported size. FWIW I think now I understand that you mean we will always return a square icon, that may be an issue. Imagine if the OS has icons that are 256x128 or 128x256 and we need to display a bunch of them side by side and tow by row you will find it impossible to layout well. Surely we should return the same if we can It is *theoretically* possible that the platform will do something bananas like return 128x256 then 256x256 then 384x256 alternative images in which case we'd need logic to handle that as sensibly as possible, acc. to the way MRI handles cuch cases, but if the aspect ratio is consistent as I expect is typical we should support that 2) I can't agree that IAE is wrong for a negative size. The argument that a developer may make a mistake is not sufficient. Virtually every Image related API we have throws an exception (usually IAE) if you pass <= 0 and folks get that right all the time. Allowing it here would be anomalous. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Fri, 21 May 2021 20:43:05 GMT, Phil Race wrote:
> I haven't seen any response to this comment I made a couple of days ago and I
> suspect it got missed
>
> > Weijun Wang has updated the pull request incrementally with one additional
> > commit since the last revision:
> > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>
> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
>
> > 57: ProcessCommunicator
> > 58: .executeChildProcess(Consumer.class, new
> > String[0]);
> > 59: if (!"Hello".equals(processResults.getStdOut())) {
>
> Who or what prompted this change ?
I replied right in the thread but unfortunately GitHub does not display it at
the end of page.
This is because the process is now launched with
`-Djava.security.manager=allow` (because of another change in
https://github.com/openjdk/jdk/pull/4071), and a new warning is displayed in
stderr. Therefore I switched to stdout.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: [OpenJDK 2D-Dev] RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 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:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
I haven't seen any response to this comment I made a couple of days ago and I
suspect it got missed
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {
Who or what prompted this change ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: [OpenJDK 2D-Dev] RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
> 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.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
update FtpClient.java
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4138/files
- new: https://git.openjdk.java.net/jdk/pull/4138/files/d460efb8..60d97a4c
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4138=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4138=01-02
Stats: 23 lines in 1 file changed: 0 ins; 12 del; 11 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: [OpenJDK 2D-Dev] RFR: 8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print" [v2]
On Fri, 21 May 2021 20:16:41 GMT, Anton Litvinov wrote: >> Hello, >> >> Could you please review the following fix for the bug specific to macOS. The >> bug consists in the fact that if the method >> "java.awt.print.Printable.print(Graphics, PageFormat, int)" throws >> "java.awt.print.PrinterException" or "java.lang.RuntimeException" during the >> call "java.awt.print.PrinterJob.print()", then the exception is caught and >> ignored by JDK and a user cannot learn that printing failed and what caused >> failure of printing, because "PrinterJob.print()" method does not throw >> "PrinterException" or the occurred exception is not reported by JDK through >> the error stream. >> >> ROOT CAUSE OF THE BUG: >> The root cause of the bug is the fact that in the method >> "sun.lwawt.macosx.CPrinterJob.printAndGetPageFormatArea(final Printable, >> final Graphics, final PageFormat, final int)" from the file >> "src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java" the >> exception thrown during execution of the expression >> >> "int pageResult = printable.print(graphics, pageFormat, pageIndex);" >> >> is caught but is not returned to a developer by any mean or is not printed >> out to the error stream. >> >> THE FIX: >> The fix implements propagation of the occurred and caught exception to the >> level of the user's code executing "PrinterJob.print()" method. Propagation >> of the exception by storing it in the instance variable of "CPrinterJob" >> object is implemented, because the engaged code always is executed: >> - on 2 threads (non-EDT thread, EDT thread) in case when >> "PrinterJob.print()" is called by the user on a non-EDT thread; >> - on 3 threads (2 EDT threads, a temporary thread started by JDK to execute >> "CPrinterJob._safePrintLoop(long, long );") when "PrinterJob.print()" is >> called on EDT thread. >> >> The regression test which is part of the fix was also successfully executed >> on MS Windows OS and Linux OS. >> >> Thank you, >> Anton > > Anton Litvinov has updated the pull request incrementally with one additional > commit since the last revision: > > Second version of the fix Hello Phil, I have uploaded the 2nd version of the fix, which addresses your remarks. Could you please review it, also I provided answers to all your comments. Changes in the 2nd version of the fix are following: 1. The created atomic reference object now stores "Throwable" instance instead of "Exception", the variable itself is renamed to "printErrorRef". 2. The method "setLastPrintEx" introduced by the 1st version of the fix is completely removed. 3. "catch" blocks in the method "CPrinterJob.printAndGetPageFormatArea" now catches "Throwable" instead of "Exception". 4. The regression test now tests also "RuntimeException" on EDT and on the main thread. 5. The regression test now does not show any JDK print dialog. Thank you, Anton - PR: https://git.openjdk.java.net/jdk/pull/4036
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v11]
On Fri, 21 May 2021 20:10:55 GMT, Phil Race wrote: > Please note that I added a number of comments in the CSR itself. > I had suggestions about what to do in many of the cases but there's a couple > of cases where > I could not either divine what was intended or know the best way to phrase it. I am responding to CSR comments right now but honestly i would prefer to do a review in one place - preferably here. For some reason i do not receive any notifications from JBS about new comments in CSR so i had to monitor it manually by refreshing the page. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 15:37:48 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
>
>> 548: * @throws IOException if the connection was unsuccessful.
>> 549: */
>> 550: @SuppressWarnings("removal")
>
> Could the scope of the annotation be further reduced by applying it to the
> two places where `doPrivileged` is called in this method?
I'll probably need to assign the doPriv result on L631 to a tmp variable and
then assign it to `s`. Are you OK with this ugliness? Update: Ah, I see you
already have similar suggestion in the next comment.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: [OpenJDK 2D-Dev] RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 15:35:05 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
>
>> 118: vals[1] =
>> Integer.getInteger("sun.net.client.defaultConnectTimeout",
>> 300_000).intValue();
>> 119: return System.getProperty("file.encoding",
>> "ISO8859_1");
>> 120: }
>
> It is a bit strange that "file.encoding" seem to get a special treatment -
> but I guess that's OK.
You might say we thus avoid wasting the return value, as much as possible.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: [OpenJDK 2D-Dev] RFR: 8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print" [v2]
> Hello, > > Could you please review the following fix for the bug specific to macOS. The > bug consists in the fact that if the method > "java.awt.print.Printable.print(Graphics, PageFormat, int)" throws > "java.awt.print.PrinterException" or "java.lang.RuntimeException" during the > call "java.awt.print.PrinterJob.print()", then the exception is caught and > ignored by JDK and a user cannot learn that printing failed and what caused > failure of printing, because "PrinterJob.print()" method does not throw > "PrinterException" or the occurred exception is not reported by JDK through > the error stream. > > ROOT CAUSE OF THE BUG: > The root cause of the bug is the fact that in the method > "sun.lwawt.macosx.CPrinterJob.printAndGetPageFormatArea(final Printable, > final Graphics, final PageFormat, final int)" from the file > "src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java" the > exception thrown during execution of the expression > > "int pageResult = printable.print(graphics, pageFormat, pageIndex);" > > is caught but is not returned to a developer by any mean or is not printed > out to the error stream. > > THE FIX: > The fix implements propagation of the occurred and caught exception to the > level of the user's code executing "PrinterJob.print()" method. Propagation > of the exception by storing it in the instance variable of "CPrinterJob" > object is implemented, because the engaged code always is executed: > - on 2 threads (non-EDT thread, EDT thread) in case when "PrinterJob.print()" > is called by the user on a non-EDT thread; > - on 3 threads (2 EDT threads, a temporary thread started by JDK to execute > "CPrinterJob._safePrintLoop(long, long );") when "PrinterJob.print()" is > called on EDT thread. > > The regression test which is part of the fix was also successfully executed > on MS Windows OS and Linux OS. > > Thank you, > Anton Anton Litvinov has updated the pull request incrementally with one additional commit since the last revision: Second version of the fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/4036/files - new: https://git.openjdk.java.net/jdk/pull/4036/files/db2ce06b..47051517 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4036=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4036=00-01 Stats: 86 lines in 2 files changed: 33 ins; 20 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/4036.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4036/head:pull/4036 PR: https://git.openjdk.java.net/jdk/pull/4036
Re: [OpenJDK 2D-Dev] RFR: 8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print" [v2]
On Wed, 19 May 2021 15:23:48 GMT, Phil Race wrote:
>> Anton Litvinov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Second version of the fix
>
> src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java line 400:
>
>> 398: throw (PrinterException) lastPrintEx;
>> 399: } else if (lastPrintEx instanceof RuntimeException) {
>> 400: throw (RuntimeException) lastPrintEx;
>
> I don't see you testing this case
> And do I understand correctly that this only throws the exception at the end
> of the printloop after trying all pages ?
In the 2nd version of the fix I added testing of the scenario with
"RuntimeException".
Yes, this code is executed only after the execution of the "printLoop" is fully
finished for each of all page ranges. If printing of one page fails within one
page range, then printing of other pages does not occur.
"CPrinterJob.print(PrintRequestAttributeSet)" is blocking until printing is
finished or fails, and my code is executed in the very end of the method.
-
PR: https://git.openjdk.java.net/jdk/pull/4036
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v11]
On Thu, 20 May 2021 22:25:05 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: > > Specification of getSystemIcon reworded to get rid of the non-public > class reference and to better describe some cornor cases Please note that I added a number of comments in the CSR itself. I had suggestions about what to do in many of the cases but there's a couple of cases where I could not either divine what was intended or know the best way to phrase it. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v3]
> 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:
feedback from Phil
reverted:
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/bfa955ad..9a3ec578
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4071=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4071=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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: [OpenJDK 2D-Dev] RFR: 8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print"
On Wed, 19 May 2021 21:40:49 GMT, Phil Race wrote:
>> Hello,
>>
>> Could you please review the following fix for the bug specific to macOS. The
>> bug consists in the fact that if the method
>> "java.awt.print.Printable.print(Graphics, PageFormat, int)" throws
>> "java.awt.print.PrinterException" or "java.lang.RuntimeException" during the
>> call "java.awt.print.PrinterJob.print()", then the exception is caught and
>> ignored by JDK and a user cannot learn that printing failed and what caused
>> failure of printing, because "PrinterJob.print()" method does not throw
>> "PrinterException" or the occurred exception is not reported by JDK through
>> the error stream.
>>
>> ROOT CAUSE OF THE BUG:
>> The root cause of the bug is the fact that in the method
>> "sun.lwawt.macosx.CPrinterJob.printAndGetPageFormatArea(final Printable,
>> final Graphics, final PageFormat, final int)" from the file
>> "src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java" the
>> exception thrown during execution of the expression
>>
>> "int pageResult = printable.print(graphics, pageFormat, pageIndex);"
>>
>> is caught but is not returned to a developer by any mean or is not printed
>> out to the error stream.
>>
>> THE FIX:
>> The fix implements propagation of the occurred and caught exception to the
>> level of the user's code executing "PrinterJob.print()" method. Propagation
>> of the exception by storing it in the instance variable of "CPrinterJob"
>> object is implemented, because the engaged code always is executed:
>> - on 2 threads (non-EDT thread, EDT thread) in case when
>> "PrinterJob.print()" is called by the user on a non-EDT thread;
>> - on 3 threads (2 EDT threads, a temporary thread started by JDK to execute
>> "CPrinterJob._safePrintLoop(long, long );") when "PrinterJob.print()" is
>> called on EDT thread.
>>
>> The regression test which is part of the fix was also successfully executed
>> on MS Windows OS and Linux OS.
>>
>> Thank you,
>> Anton
>
> src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java line 823:
>
>> 821: }
>> 822: } catch (Exception e) {
>> 823: // Original code bailed on any exception
>
> Throwable ?
> And what was wrong with bailing ? That comment has been there since the code
> was brought into OPenJDK so I can't tell what the motivation was.
> But I'm getting a bit lost what is going on here.
>
> The spec
> https://docs.oracle.com/en/java/javase/11/docs/api/java.desktop/java/awt/print/Printable.html#print(java.awt.Graphics,java.awt.print.PageFormat,int)
>
> says
> Throws:
> PrinterException - thrown when the print job is terminated.
>
> I take that to mean we should not carry on printing .. but it looks like we
> just note the exception and carry on.
> I'm not sure this code (pre-existing behaviour) is right.
>
> We do want to grab the exception so it doesn't mess up interverning code but
> don't we want this to reach the caller of PrinterJob.print - in the form of a
> PrinterExcepton ?
In the 2nd version of the fix I substituted "Exception" for "Throwable" in this
catch block and in second "catch" block several lines below. Also in the 2nd
fix version I removed this comment about bailing.
My opinion is the same, the code before the fix did not work according to the
specification and was ignoring any caught exceptions. Perhaps, the reason of
this is involvement of more than 1 thread in the code flow standing behind
"PrinterJob.print" method execution and execution of the JDK's code through JNI
in this code flow. Thus propagating the exception is impossible across threads
and it is not easy to stop printing done by macOS at the moment, when the
exception occurs in Java code in JDK. This pattern is used in the file
"CPrinterJob.java" in other places, even there is the same comment in the
method "CPrinterJob.getPageformatPrintablePeekgraphics". At the same time
propagating "RuntimeException" from "PrinterJob.print" on Windows, Linux and
now on macOS after this fix also is not according to the specification but
still consistency in JDK behavior on these OS versions should be good.
As I understand, the method "CPrinterJob.printAndGetPageFormatArea" returns
"null" as the "Rectangle2D", when the exception is caught, and this "null"
value should automatically lead to returning of "NSZeroRect" from the
Objective-C method "(NSRect)rectForPage:(NSInteger)pageNumber" in the file
"src/java.desktop/macosx/native/libawt_lwawt/awt/PrinterView.m", from which
"CPrinterJob.printAndGetPageFormatArea" Java method was invoked. And this
"NSZeroRect" return value is indication of the error for macOS, which further
leads to stopping of the printing process.
-
PR: https://git.openjdk.java.net/jdk/pull/4036
Re: [OpenJDK 2D-Dev] RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
On Fri, 21 May 2021 18:26:48 GMT, Phil Race wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> adjust order of VM options > > test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3: > >> 1: /* >> 2: * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > Probably the (c) update was meant to be on the .sh file that is actually > modified. Oops, I'll back it out. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4071
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 19:11:45 GMT, Alexey Ivanov wrote:
>> But how you got them via this method? I am not sure what parameters should
>> be passed to it.
>
> Didn't you answer your question already? If `FileSystemView.getRoots()`
> returns Desktop in Windows shell namespace. Otherwise, I don't know a way to
> get a reference to a *virtual* folder in Windows shell namespace which
> doesn't reference a file system object.
>
> Alex's example uses "3D Objects" folder which is one of the known folders and
> has its own icon instead of the standard folder icon.
>
> It's possible that I don't understand the question clearly.
>
> Alex's fix affects WindowsPlacesBar on the left of JFileChooser in Windows
> LaF, the icons there look crispier in High DPI environment.
> But how you got them via this method? I am not sure what parameters should be
> passed to it.
`
String userdir = System.getenv("userprofile");
Icon icon = FileSystemView.getFileSystemView().getSystemIcon(new File(userdir +
"\\3D Objects"), 64);
`
For some of the libraries getting file reference is quite easy, for some - not
so much. But still doable.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print"
On Wed, 19 May 2021 15:25:34 GMT, Phil Race wrote:
>> Hello,
>>
>> Could you please review the following fix for the bug specific to macOS. The
>> bug consists in the fact that if the method
>> "java.awt.print.Printable.print(Graphics, PageFormat, int)" throws
>> "java.awt.print.PrinterException" or "java.lang.RuntimeException" during the
>> call "java.awt.print.PrinterJob.print()", then the exception is caught and
>> ignored by JDK and a user cannot learn that printing failed and what caused
>> failure of printing, because "PrinterJob.print()" method does not throw
>> "PrinterException" or the occurred exception is not reported by JDK through
>> the error stream.
>>
>> ROOT CAUSE OF THE BUG:
>> The root cause of the bug is the fact that in the method
>> "sun.lwawt.macosx.CPrinterJob.printAndGetPageFormatArea(final Printable,
>> final Graphics, final PageFormat, final int)" from the file
>> "src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java" the
>> exception thrown during execution of the expression
>>
>> "int pageResult = printable.print(graphics, pageFormat, pageIndex);"
>>
>> is caught but is not returned to a developer by any mean or is not printed
>> out to the error stream.
>>
>> THE FIX:
>> The fix implements propagation of the occurred and caught exception to the
>> level of the user's code executing "PrinterJob.print()" method. Propagation
>> of the exception by storing it in the instance variable of "CPrinterJob"
>> object is implemented, because the engaged code always is executed:
>> - on 2 threads (non-EDT thread, EDT thread) in case when
>> "PrinterJob.print()" is called by the user on a non-EDT thread;
>> - on 3 threads (2 EDT threads, a temporary thread started by JDK to execute
>> "CPrinterJob._safePrintLoop(long, long );") when "PrinterJob.print()" is
>> called on EDT thread.
>>
>> The regression test which is part of the fix was also successfully executed
>> on MS Windows OS and Linux OS.
>>
>> Thank you,
>> Anton
>
> src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java line 264:
>
>> 262: Exception oldEx = lastPrintExRef.getAndSet(newEx);
>> 263: if (printOldEx && (oldEx != null)) {
>> 264: oldEx.printStackTrace();
>
> Why not Throwable ?
> I suggest to not do this. ie don't print and don't replace Instead swallow
> the new exception
> and let the original problem that started it get propagated. That seems more
> likely to be useful.
Thank you for this remark. I agree using "Throwable" is more solid approach. In
the 2nd version of the fix I use "Throwable" in this "AtomicReference" object
and I removed completely this method "setLastPrintEx".
-
PR: https://git.openjdk.java.net/jdk/pull/4036
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 18:16:36 GMT, Sergey Bylokhov wrote: >>> It is accessible from the "JFileChooser" drop-down menu. On my system, the >>> Libraries at that drop down menu contain "GIT", "Documents", "Music". >>> https://docs.microsoft.com/en-us/windows/client-management/windows-libraries >>> >>> There are also "known folders" such as "Network" or "My computer" which do >>> not have a real path but can be navigated in the JFileChooser and has an >>> icon. >> >> Ok, got it. Well, since they can be translated into the file system paths - >> even if these paths do not belong to physical filesystem - they are >> supported by the new API. Not for all of them i am able to receive the high >> resolution icons - like "Recent Items" for some reason only provides 32x32 >> and 16x16 no matter what size i am asking for. Others such as Documents, My >> Computer or 3D Objects do provide all resolutions available. For example >> here's the screenshot of all the available icons for 3D Objects >>  > > But how you got them via this method? I am not sure what parameters should be > passed to it. Didn't you answer your question already? If `FileSystemView.getRoots()` returns Desktop in Windows shell namespace. Otherwise, I don't know a way to get a reference to a *virtual* folder in Windows shell namespace which doesn't reference a file system object. Alex's example uses "3D Objects" folder which is one of the known folders and has its own icon instead of the standard folder icon. It's possible that I don't understand the question clearly. Alex's fix affects WindowsPlacesBar on the left of JFileChooser in Windows LaF, the icons there look crispier in High DPI environment. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print"
On Wed, 19 May 2021 15:22:27 GMT, Phil Race wrote:
>> Hello,
>>
>> Could you please review the following fix for the bug specific to macOS. The
>> bug consists in the fact that if the method
>> "java.awt.print.Printable.print(Graphics, PageFormat, int)" throws
>> "java.awt.print.PrinterException" or "java.lang.RuntimeException" during the
>> call "java.awt.print.PrinterJob.print()", then the exception is caught and
>> ignored by JDK and a user cannot learn that printing failed and what caused
>> failure of printing, because "PrinterJob.print()" method does not throw
>> "PrinterException" or the occurred exception is not reported by JDK through
>> the error stream.
>>
>> ROOT CAUSE OF THE BUG:
>> The root cause of the bug is the fact that in the method
>> "sun.lwawt.macosx.CPrinterJob.printAndGetPageFormatArea(final Printable,
>> final Graphics, final PageFormat, final int)" from the file
>> "src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java" the
>> exception thrown during execution of the expression
>>
>> "int pageResult = printable.print(graphics, pageFormat, pageIndex);"
>>
>> is caught but is not returned to a developer by any mean or is not printed
>> out to the error stream.
>>
>> THE FIX:
>> The fix implements propagation of the occurred and caught exception to the
>> level of the user's code executing "PrinterJob.print()" method. Propagation
>> of the exception by storing it in the instance variable of "CPrinterJob"
>> object is implemented, because the engaged code always is executed:
>> - on 2 threads (non-EDT thread, EDT thread) in case when
>> "PrinterJob.print()" is called by the user on a non-EDT thread;
>> - on 3 threads (2 EDT threads, a temporary thread started by JDK to execute
>> "CPrinterJob._safePrintLoop(long, long );") when "PrinterJob.print()" is
>> called on EDT thread.
>>
>> The regression test which is part of the fix was also successfully executed
>> on MS Windows OS and Linux OS.
>>
>> Thank you,
>> Anton
>
> test/jdk/java/awt/print/PrinterJob/ExceptionFromPrintableIsIgnoredTest.java
> line 83:
>
>> 81: return NO_SUCH_PAGE;
>> 82: }
>> 83: throw new PrinterException("Exception from
>> Printable.print");
>
> Don't you want to also test this with some kind of RuntimeException ?
In the 1st version of the fix I did not test the scenario with
"RuntimeException", because despite of the fact that I had known that JDK 17
will pass on Windows and Linux the scenario with "RuntimeException" I still was
not sure if the behavior of older JDK release families is the same with JDK 17
on Windows and Linux, and just did not want to run the risk of failure of this
test during potential porting of the fix to older JDK release families.
But yesterday I did testing with my standalone test case attached to the bug
record in JBS and now I can certainly say that JDK 8u291-b10, JDK JDK 8-b132 do
not fail in the test scenarios with "RuntimeException" on Windows and Linux.
Therefore it is safe to include test scenario with "RuntimeException" to the
regression test.
In the 2nd version of the fix now I am testing also "RuntimeException" both on
EDT thread and on main thread.
-
PR: https://git.openjdk.java.net/jdk/pull/4036
Re: [OpenJDK 2D-Dev] RFR: 8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print"
On Wed, 19 May 2021 15:19:23 GMT, Phil Race wrote:
>> Hello,
>>
>> Could you please review the following fix for the bug specific to macOS. The
>> bug consists in the fact that if the method
>> "java.awt.print.Printable.print(Graphics, PageFormat, int)" throws
>> "java.awt.print.PrinterException" or "java.lang.RuntimeException" during the
>> call "java.awt.print.PrinterJob.print()", then the exception is caught and
>> ignored by JDK and a user cannot learn that printing failed and what caused
>> failure of printing, because "PrinterJob.print()" method does not throw
>> "PrinterException" or the occurred exception is not reported by JDK through
>> the error stream.
>>
>> ROOT CAUSE OF THE BUG:
>> The root cause of the bug is the fact that in the method
>> "sun.lwawt.macosx.CPrinterJob.printAndGetPageFormatArea(final Printable,
>> final Graphics, final PageFormat, final int)" from the file
>> "src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java" the
>> exception thrown during execution of the expression
>>
>> "int pageResult = printable.print(graphics, pageFormat, pageIndex);"
>>
>> is caught but is not returned to a developer by any mean or is not printed
>> out to the error stream.
>>
>> THE FIX:
>> The fix implements propagation of the occurred and caught exception to the
>> level of the user's code executing "PrinterJob.print()" method. Propagation
>> of the exception by storing it in the instance variable of "CPrinterJob"
>> object is implemented, because the engaged code always is executed:
>> - on 2 threads (non-EDT thread, EDT thread) in case when
>> "PrinterJob.print()" is called by the user on a non-EDT thread;
>> - on 3 threads (2 EDT threads, a temporary thread started by JDK to execute
>> "CPrinterJob._safePrintLoop(long, long );") when "PrinterJob.print()" is
>> called on EDT thread.
>>
>> The regression test which is part of the fix was also successfully executed
>> on MS Windows OS and Linux OS.
>>
>> Thank you,
>> Anton
>
> test/jdk/java/awt/print/PrinterJob/ExceptionFromPrintableIsIgnoredTest.java
> line 86:
>
>> 84: }
>> 85: });
>> 86: if (job.printDialog()) {
>
> why do we need a dialog ? If you remove this it can be an automated test.
Hello Phil. Thank you very much for review of this fix. First I am answering
your question, which is about why the test passes on Windows and which is your
first and separate comment not bound to the source code. The test passes on
Windows, because in JDK 17 for both Windows and Linux JDK works similarly
stably and reliably, JDK 17 on Window and Linux successfully propagates
"PrinterException" and "RuntimeException", if it is thrown from
"Printable.print" method. I had verified this using my standalone manual test
case "ExceptionFromPrintableIsIgnored.java" attached to the bug record in JBS,
before sending the 1st version of the fix for review. I did not explore, why it
is so, because it works on Windows and Linux and this bug is related to macOS,
but I think that it works on Windows and Linux, because in JDK implementations
for Windows and Linux printing is consistently done by JDK on the same thread
on which "PrinterJob.print" was called and no portions of the involved code are
exe
cuted asynchronously on other thread, like EDT in case of macOS.
I agree there is no need in the dialog in the regression test. In the 1st
version of the fix I did it deliberately, to make it more manual, because the
test initiates printing and there is no guarantee what type of printer would be
set up on the test host, if some virtual printer is used on the host, that
printer can show its own native dialog asking to specify location of a file, in
which the printed document should be saved. That is why I think that this test
cannot be fully automatic. In the 2nd version of the fix which I am submitting
right now I removed this unnecessary code showing JDK print dialog.
-
PR: https://git.openjdk.java.net/jdk/pull/4036
Re: [OpenJDK 2D-Dev] RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
On Tue, 18 May 2021 21:44:43 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.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> adjust order of VM options
The client changes are fine except for the one misplaced (c)
test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3:
> 1: /*
> 2: * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights
> reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
Probably the (c) update was meant to be on the .sh file that is actually
modified.
-
Marked as reviewed by prr (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v10]
On Fri, 21 May 2021 01:11:38 GMT, Sergey Bylokhov wrote: >>> But this is shared code, which has a specification it should work >>> everywhere, so even if on Linux and macOS it is not implemented in the best >>> way it should return something. >> >> The stuff that is returned by the common code is already tested in UIManager >> tests. This issue is windows specific. This implementation at this moment of >> time is windows specific. Once we extend implementation - and i mean real >> implementation with support for multiple resolution icons - this test will >> be updated to reflect it. This is not a specification conformance test, it >> is implementation logic test. > > How it could be tested by the UIManager tests since this is a new method > added in this class? I guess it should work just out of the box, no? What is > the reason to not enable it, there are some issues? I suggest running it now and do not wait while the new tck tests will be created and run, so we will not get a tck-red right before release. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 06:32:02 GMT, Alexander Zuev wrote: >> It is accessible from the "JFileChooser" drop-down menu. On my system, the >> Libraries at that drop down menu contain "GIT", "Documents", "Music". >> https://docs.microsoft.com/en-us/windows/client-management/windows-libraries >> >> There are also "known folders" such as "Network" or "My computer" which do >> not have a real path but can be navigated in the JFileChooser and has an >> icon. > >> It is accessible from the "JFileChooser" drop-down menu. On my system, the >> Libraries at that drop down menu contain "GIT", "Documents", "Music". >> https://docs.microsoft.com/en-us/windows/client-management/windows-libraries >> >> There are also "known folders" such as "Network" or "My computer" which do >> not have a real path but can be navigated in the JFileChooser and has an >> icon. > > Ok, got it. Well, since they can be translated into the file system paths - > even if these paths do not belong to physical filesystem - they are supported > by the new API. Not for all of them i am able to receive the high resolution > icons - like "Recent Items" for some reason only provides 32x32 and 16x16 no > matter what size i am asking for. Others such as Documents, My Computer or 3D > Objects do provide all resolutions available. For example here's the > screenshot of all the available icons for 3D Objects >  But how you got them via this method? I am not sure what parameters should be passed to it. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8263486: Clean up MTLSurfaceDataBase.h [v2]
On Fri, 21 May 2021 15:14:23 GMT, Ajit Ghaisas wrote: >> This PR addresses some cleanup activities : >> - Cleaned up MTLSurfaceDataBase.h & MTLSurfaceData.m >> - Removed OpenGL references from MTLPipelineStatesStorage.m & >> MTLRenderQueue.m > > Ajit Ghaisas 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: > > - remove x/yOffset fields > - Merge branch 'master' into cleanup_MTLSurfaceDataBase > - cleanup unused code Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4010
Re: [OpenJDK 2D-Dev] RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 07:06:00 GMT, Alan Bateman wrote: >> The JEP isn't PTT for 17 so there's plenty of time isn't there ? > > There are 827 files in this patch. Phil is right that adding SW at the class > level is introducing technical debt but if addressing that requires > refactoring and re-testing of AWT or font code then it would be better to > have someone from that area working on. Is there any reason why this can't be > going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I > don't think we should put Max through the wringer here as there are too many > areas to cover. Are you suggesting that the patch doesn't need testing as it is ? It should be the same either way. It is very straight forward to run the automated tests across all platforms these days. I get the impression that no one is guaranteeing to do this straight after integration. It sounds like it is up for deferral if time runs out. The amount of follow-on work that I am hearing about here, and may be for tests, does not make it sound like this JEP is nearly as done as first presented. If there was some expectation that groups responsible for an area would get involved with this issue which I am assured was already known about, then why was it not raised before and made part of the plan ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: [OpenJDK 2D-Dev] RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 14:00:39 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.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> typo on windows
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
> 118: vals[1] =
> Integer.getInteger("sun.net.client.defaultConnectTimeout",
> 300_000).intValue();
> 119: return System.getProperty("file.encoding",
> "ISO8859_1");
> 120: }
It is a bit strange that "file.encoding" seem to get a special treatment - but
I guess that's OK.
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
> 548: * @throws IOException if the connection was unsuccessful.
> 549: */
> 550: @SuppressWarnings("removal")
Could the scope of the annotation be further reduced by applying it to the two
places where `doPrivileged` is called in this method?
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 921:
> 919: }
> 920:
> 921: @SuppressWarnings("removal")
Could the scope be further reduced by applying it at line 926 in this method -
at the cost of creating a temporary variable? The code could probably be
further simplified by using a lambda...
PrivilegedAction pa = () -> new Socket(proxy);
@SuppressWarnings("removal")
var ps = AccessController.doPrivileged(pa);
s = ps;
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: [OpenJDK 2D-Dev] RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 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:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
src/java.base/share/classes/java/lang/SecurityManager.java line 104:
> 102: * method will throw an {@code UnsupportedOperationException}). If the
> 103: * {@systemProperty java.security.manager} system property is set to the
> 104: * special token "{@code allow}", then a security manager will not be
> set at
Can/should the `{@systemProperty ...}` tag be used more than once for a given
system property? I thought it should be used only once, at the place where the
system property is defined. Maybe @jonathan-gibbons can offer some more
guidance on this.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: [OpenJDK 2D-Dev] RFR: 8263486: Clean up MTLSurfaceDataBase.h [v2]
> This PR addresses some cleanup activities : > - Cleaned up MTLSurfaceDataBase.h & MTLSurfaceData.m > - Removed OpenGL references from MTLPipelineStatesStorage.m & > MTLRenderQueue.m Ajit Ghaisas 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: - remove x/yOffset fields - Merge branch 'master' into cleanup_MTLSurfaceDataBase - cleanup unused code - Changes: - all: https://git.openjdk.java.net/jdk/pull/4010/files - new: https://git.openjdk.java.net/jdk/pull/4010/files/4c167898..5cb3dd45 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4010=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4010=00-01 Stats: 27529 lines in 857 files changed: 12750 ins; 11509 del; 3270 mod Patch: https://git.openjdk.java.net/jdk/pull/4010.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4010/head:pull/4010 PR: https://git.openjdk.java.net/jdk/pull/4010
Re: [OpenJDK 2D-Dev] RFR: 8263486: Clean up MTLSurfaceDataBase.h
On Thu, 20 May 2021 21:48:18 GMT, Sergey Bylokhov wrote: >> This PR addresses some cleanup activities : >> - Cleaned up MTLSurfaceDataBase.h & MTLSurfaceData.m >> - Removed OpenGL references from MTLPipelineStatesStorage.m & >> MTLRenderQueue.m > > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceData.m > line 35: > >> 33: #include "jlong.h" >> 34: >> 35: jboolean MTLSD_InitMTLWindow(JNIEnv *env, BMTLSDOps *bmtlsdo); > > How the MTLSD_WINDOW is used? Do we use it for the layer-based rendering? It is used in MTLContex.m. It just indicates a surface that we render to. We blit from this surface to the CAMetalLayer lateron. It looks like MTLSD_WINDOW can be replaced with simple MTLSD_TEXTURE. This investigation and cleanup has already been identified during lanai code review (Bug - JDK-8263463) > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceDataBase.h > line 56: > >> 54: * jint x/yOffset; >> 55: * The offset in pixels of the viewport origin from the lower-left >> 56: * corner of the heavyweight drawable. > > Do we use these fields or they are always zero? Thanks for your review. It turned out that - they are always zero. I will remove these fields. - PR: https://git.openjdk.java.net/jdk/pull/4010
Re: [OpenJDK 2D-Dev] RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
> 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.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
typo on windows
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4138/files
- new: https://git.openjdk.java.net/jdk/pull/4138/files/e3f0405a..d460efb8
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4138=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4138=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 05:42:46 GMT, Sergey Bylokhov wrote: > It is accessible from the "JFileChooser" drop-down menu. On my system, the > Libraries at that drop down menu contain "GIT", "Documents", "Music". > https://docs.microsoft.com/en-us/windows/client-management/windows-libraries > > There are also "known folders" such as "Network" or "My computer" which do > not have a real path but can be navigated in the JFileChooser and has an icon. Ok, got it. Well, since they can be translated into the file system paths - even if these paths do not belong to physical filesystem - they are supported by the new API. Not for all of them i am able to receive the high resolution icons - like "Recent Items" for some reason only provides 32x32 and 16x16 no matter what size i am asking for. Others such as Documents, My Computer or 3D Objects do provide all resolutions available. For example here's the screenshot of all the available icons for 3D Objects  - PR: https://git.openjdk.java.net/jdk/pull/2875
