On Mon, 4 Dec 2023 23:31:47 GMT, Phil Race <p...@openjdk.org> wrote:
>> For as long as we've had the macOS port, it seems that queries on the
>> GraphicsConfiguration associated with
>> a printer would give you null for the defaultTransform.
>> Clearly this API isn't a popular one to call in such a context else we'd
>> have had lots of reports.
>> We have a test that would have caught it except that until recently the
>> macOS printing implementation
>> was swallowing exceptions it should not.
>
> Phil Race has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8320443
Could you also update the copyright year in `CPrinterJob.java` and
`PrinterDevice.java`?
src/java.desktop/share/classes/sun/print/RasterPrinterJob.java line 2096:
> 2094:
> 2095: protected synchronized void setGraphicsConfigInfo(AffineTransform
> at,
> 2096: double pw, double ph) {
Suggestion:
protected synchronized void setGraphicsConfigInfo(AffineTransform at,
double pw, double ph) {
To preserve alignment.
test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 30:
> 28: * @summary Checks that the PrinterGraphics is for a Printer
> GraphicsDevice.
> 29: * Test doesn't run unless there's a printer on the system.
> 30: * @key printer
Minor: we agreed to put `@key` after `@bug`.
test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 52:
> 50: public class PrinterDevice implements Printable {
> 51:
> 52: static volatile boolean failed = false;
Is it really needed? In all the cases where you assign `true` to the `failed`
field, you also explicitly throw an exception, which is enough to fail the test.
test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 71:
> 69: if (failed) {
> 70: throw new RuntimeException("Test failed but no exception
> propagated.");
> 71: }
A comment that `pj.print` should not throw exception would suffice, even though
it's implied by jtreg any way.
This statement is essentially unreachable if `failed` is set to `true`.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16773#pullrequestreview-1765958594
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416186891
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416198629
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416195194
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416196288
