[OpenJDK 2D-Dev] [12] Review request for 8201818: [macosx] Printing attributes break page size set via "java.awt.print.Book" object

[Philip Race]

Approved.

-phil.

On 8/23/18, 8:33 AM, Anton Litvinov wrote:
> Hello Phil,
>
> Do you approve this version of the fix?
>
> Thank you,
> Anton
>
> On 22/08/2018 19:59, Anton Litvinov wrote:
>> Hello Phil,
>>
>> Thank you for review of this fix. It is correct, this code area was 
>> already touched by 3 fixes which you specified, but they are just 2 
>> attempts to fix the same issue and 1 rollback of the 1st fix, which 
>> was dictated by the fact that at that time JDK 9 passed RDP 1. 
>> Answers to your questions are following:
>>
>> 1.  No, absolutely, this bug is not a regression from the fix for 
>> 8181659. This bug exists in JDK for a long time, this is proved by 
>> the fact that I was able to reproduce it with JDK 8u112 b16, while 
>> the fix for 8167102 was never released in GA JDK 8 update, except for 
>> 1 released build JDK 8u131 b32. I consider this bug as an independent 
>> issue from 8167102, because of a difference in a test scenario (in 
>> 8167102 "java.awt.print.Printable" interface is involved, in this bug 
>> "java.awt.print.Pageable" interface is involved), but at the same 
>> time they both have the same root cause. The end user, which reported 
>> 8167102 changed conditions by moving its application from setting 
>> "Printable" object to a printer job to setting "Pageable" object 
>> ("java.awt.print.Book") and encountered this issue.
>>
>> 2.  Yes, sure, I verified that the bug 8167102 is still not 
>> reproducible with this fix by using the existing "jtreg" regression 
>> test "test/jdk/java/awt/print/PageFormat/WrongPaperPrintingTest.java".
>>
>> 3.  Today I have run all available "jtreg" manual and automatic 
>> regression tests in the directories specified below and in our closed 
>> directories (total 189 tests) using both JDK 12 compiled without and 
>> with the fix, and verified that no new test failed on JDK 12 with the 
>> fix.
>>
>> Directories with the executed regression tests:
>> - "test/jdk/java/awt/print"
>> - "test/jdk/javax/print"
>>
>> 4. Yes, I verified that the fix works as expected for specified by 
>> you 3 cases:
>> a)  no print dialog - attached to the bug record test case 
>> "PrintingAttributeBreaksBookPageSize.java"
>> b)  native print dialog - the created regression test 
>> "test/jdk/java/awt/print/PageFormat/WrongPaperForBookPrintingTest.java"
>> c)  swing print dialog - the regression test in point #b with 
>> modification in 1 line.
>>
>> Thank you,
>> Anton
>>
>> On 21/08/2018 00:37, Philip Race wrote:
>>> Hi,
>>>
>>> The fix looks reasonable.
>>> But we've touched the code being touched here quite recently and the 
>>> history
>>> there shows that this can be tricky to get right :
>>> 8167102: [macosx] PrintRequestAttributeSet breaks page size set 
>>> using PageFormat
>>> caused
>>> 8181192: [macos] javafx.print.PrinterJob.showPrintDialog() hangs on 
>>> macOS
>>> so then we had
>>> 8181659: Create an alternative fix for JDK-8167102, whose fix was 
>>> backed out
>>>
>>> Is this new bug a regression caused by the previous fix ?
>>> Have you verified the previous fix is still functional ?
>>> What tests have been run ?
>>> Can you make sure this works in the cases of
>>> a) no dialog
>>> b) swing dialog
>>> c) native dialog.
>>>
>>> -phil.
>>>
>>> On 8/17/18, 12:42 PM, Anton Litvinov wrote:
>>>> Hello,
>>>>
>>>> Could you please review the following fix for the bug.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8201818
>>>> Webrev: http://cr.openjdk.java.net/~alitvinov/8201818/jdk12/webrev.00
>>>>
>>>> The bug consists in the fact that, if any one printing attribute is 
>>>> contained in "PrintRequestAttributeSet" during the call to 
>>>> "java.awt.print.PrinterJob.print(PrintRequestAttributeSet)" method, 
>>>> a page size from "java.awt.print.PageFormat" object provided by 
>>>> "java.awt.print.Book" set to a printer job through 
>>>> "PrinterJob.setPageable(Pageable)" method is not respected and the 
>>>> printer job prints the document on a page of a size equal to a 
>>>> default page size of a selected printer.
>>>>
>>>> CAUSE OF THE BUG:
>>>> The root cause of the bug is 
>>>> "sun.print.RasterPrinterJob.getPageFormatFromAttributes()" method 
>>>> which returns "PageFormat" object constructed from default settings 
>>>> of the selected printer, if the attribute set in the printer job is 
>>>> not empty. For the case in this bug, when the user explicitly sets 
>>>> "Book" object for the printer job, this method should not return 
>>>> anything except for "null", because:
>>>>
>>>> 1. According to the documentation for the method 
>>>> "PrinterJob.print(PrintRequestAttributeSet)" from Java Platform SE 
>>>> 8 API Specification, if a user sets "java.awt.print.Pageable" 
>>>> object for "PrinterJob", then printing attributes related to media 
>>>> size, imageable area, page orientation will not influence anyhow 
>>>> the "PageFormat" object used for printing, and only "PageFormat" 
>>>> provided by the "Pageable" object will be used.
>>>> Statement: "For clients of the Pageable interface, the PageFormat 
>>>> will always be as supplied by that interface, on a per page basis."
>>>> Specification URL #1: 
>>>> https://docs.oracle.com/javase/8/docs/api/java/awt/print/PrinterJob.html#print-javax.print.attribute.PrintRequestAttributeSet-
>>>>
>>>> 2. Documentation on the method 
>>>> "PrinterJob.printDialog(PrintRequestAttributeSet)" acknowledges the 
>>>> point #1 by the statement "If using the Pageable interface, clients 
>>>> which intend to use media selected by the user must create a 
>>>> PageFormat derived from the user's selections.".
>>>> Specification URL #2: 
>>>> https://docs.oracle.com/javase/8/docs/api/java/awt/print/PrinterJob.html#printDialog-javax.print.attribute.PrintRequestAttributeSet-
>>>>
>>>> THE SOLUTION:
>>>> The solution consists in changing the method 
>>>> "RasterPrinterJob.getPageFormatFromAttributes()" to return "null" 
>>>> for cases, when the "Pageable" object set in the printer job is not 
>>>> an instance of "sun.print.OpenBook" class, which is exclusively 
>>>> used by the printer job to wrap "Printable" object set by the user 
>>>> through "PrinterJob.setPrintable(Printable)".
>>>>
>>>> The fix affects only macOS platform, because 
>>>> "RasterPrinterJob.getPageFormatFromAttributes()" method is called 
>>>> only from the macOS specific native code in the file 
>>>> "src/java.desktop/macosx/native/libawt_lwawt/awt/CPrinterJob.m".
>>>>
>>>> Thank you,
>>>> Anton
>>
>