[10] Review request for 8181659: Create an alternative fix for JDK-8167102, whose fix was backed out

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[10] Review request for 8181659: Create an alternative fix for JDK-8167102, whose fix was backed out

Anton Litvinov
Hello Prasanta and Phil,

Could you please review the following fix for the bug consisting in
creation of the alternative version of the fix for
(https://bugs.openjdk.java.net/browse/JDK-8167102). I am writing to you
directly, because you reviewed my first fix for this bug, which then
caused the bug (https://bugs.openjdk.java.net/browse/JDK-8181192) and
was backed out by the fix for JDK-8181192.

Bug: https://bugs.openjdk.java.net/browse/JDK-8181659
Webrev: http://cr.openjdk.java.net/~alitvinov/8181659/jdk10/webrev.00

This fix is actually the first version of the fix for JDK-8167102 with
insignificant changes:
- edited comment text;
- 1 redundant check on equality to "null" is removed;
- exclusion of the regression test
"test/jdk/java/awt/print/PageFormat/WrongPaperPrintingTest.java"
developed for JDK-8167102 is cancelled by removal of "@ignore" tag from
the test.

ADVANTAGES OF THE FIX:

1)  The fix is completely OS X specific, because the edited method
"sun.print.RasterPrinterJob.getPageFormatFromAttributes()" is invoked
only from the OS X native code in file
"src/java.desktop/macosx/native/libawt_lwawt/awt/CPrinterJob.m".

2)  Corrects the behavior of
"RasterPrinterJob.getPageFormatFromAttributes" method to return a new
"PageFormat" based on corresponding values of the existing "PageFormat"
set by the user through
"java.awt.print.PrinterJob.setPrintable(Printable, PageFormat)", if the
user does not supply "OrientationRequested", "MediaSizeName",
"MediaPrintableArea" attributes during the call to
"PrinterJob.print(PrintRequestAttributeSet)".

3) Eliminates the possibility of being blocked during the call
"Pageable.getPageFormat(int)", like it was with the previous version of
the fix, because now the fix calls "getPageFormat(int)" only for
"Pageable" of type "sun.print.OpenBook", which is used by JDK
especially, when "PageFormat" is the same for all pages of the document,
when the user calls "PrinterJob.setPrintable(Printable, PageFormat)".

I verified that this fix resolves the original bug JDK-8167102 and does
not cause the regression JDK-8181192.

Thank you,
Anton
Reply | Threaded
Open this post in threaded view
|

Re: [10] Review request for 8181659: Create an alternative fix for JDK-8167102, whose fix was backed out

Phil Race
Hi,

getPageFormatFromAttributes(..) was added to fix some mac specific issues :

https://bugs.openjdk.java.net/browse/JDK-8025988
https://bugs.openjdk.java.net/browse/JDK-8025990

which is why it is used only on mac, so I think it is OK to assume
it can be modified to fix a further Mac bug.

So this seems OK.

-phil.


On 11/23/2017 08:54 AM, Anton Litvinov wrote:
Hello Prasanta and Phil,

Could you please review the following fix for the bug consisting in creation of the alternative version of the fix for (https://bugs.openjdk.java.net/browse/JDK-8167102). I am writing to you directly, because you reviewed my first fix for this bug, which then caused the bug (https://bugs.openjdk.java.net/browse/JDK-8181192) and was backed out by the fix for JDK-8181192.

Bug: https://bugs.openjdk.java.net/browse/JDK-8181659
Webrev: http://cr.openjdk.java.net/~alitvinov/8181659/jdk10/webrev.00

This fix is actually the first version of the fix for JDK-8167102 with insignificant changes:
- edited comment text;
- 1 redundant check on equality to "null" is removed;
- exclusion of the regression test "test/jdk/java/awt/print/PageFormat/WrongPaperPrintingTest.java" developed for JDK-8167102 is cancelled by removal of "@ignore" tag from the test.

ADVANTAGES OF THE FIX:

1)  The fix is completely OS X specific, because the edited method "sun.print.RasterPrinterJob.getPageFormatFromAttributes()" is invoked only from the OS X native code in file "src/java.desktop/macosx/native/libawt_lwawt/awt/CPrinterJob.m".

2)  Corrects the behavior of "RasterPrinterJob.getPageFormatFromAttributes" method to return a new "PageFormat" based on corresponding values of the existing "PageFormat" set by the user through "java.awt.print.PrinterJob.setPrintable(Printable, PageFormat)", if the user does not supply "OrientationRequested", "MediaSizeName", "MediaPrintableArea" attributes during the call to "PrinterJob.print(PrintRequestAttributeSet)".

3) Eliminates the possibility of being blocked during the call "Pageable.getPageFormat(int)", like it was with the previous version of the fix, because now the fix calls "getPageFormat(int)" only for "Pageable" of type "sun.print.OpenBook", which is used by JDK especially, when "PageFormat" is the same for all pages of the document, when the user calls "PrinterJob.setPrintable(Printable, PageFormat)".

I verified that this fix resolves the original bug JDK-8167102 and does not cause the regression JDK-8181192.

Thank you,
Anton

Reply | Threaded
Open this post in threaded view
|

Re: [10] Review request for 8181659: Create an alternative fix for JDK-8167102, whose fix was backed out

Anton Litvinov
Hello Phil,

Thank you very much for review and approval of this fix.

Thank you,
Anton

On 27/11/2017 23:13, Phil Race wrote:
Hi,

getPageFormatFromAttributes(..) was added to fix some mac specific issues :

https://bugs.openjdk.java.net/browse/JDK-8025988
https://bugs.openjdk.java.net/browse/JDK-8025990

which is why it is used only on mac, so I think it is OK to assume
it can be modified to fix a further Mac bug.

So this seems OK.

-phil.


On 11/23/2017 08:54 AM, Anton Litvinov wrote:
Hello Prasanta and Phil,

Could you please review the following fix for the bug consisting in creation of the alternative version of the fix for (https://bugs.openjdk.java.net/browse/JDK-8167102). I am writing to you directly, because you reviewed my first fix for this bug, which then caused the bug (https://bugs.openjdk.java.net/browse/JDK-8181192) and was backed out by the fix for JDK-8181192.

Bug: https://bugs.openjdk.java.net/browse/JDK-8181659
Webrev: http://cr.openjdk.java.net/~alitvinov/8181659/jdk10/webrev.00

This fix is actually the first version of the fix for JDK-8167102 with insignificant changes:
- edited comment text;
- 1 redundant check on equality to "null" is removed;
- exclusion of the regression test "test/jdk/java/awt/print/PageFormat/WrongPaperPrintingTest.java" developed for JDK-8167102 is cancelled by removal of "@ignore" tag from the test.

ADVANTAGES OF THE FIX:

1)  The fix is completely OS X specific, because the edited method "sun.print.RasterPrinterJob.getPageFormatFromAttributes()" is invoked only from the OS X native code in file "src/java.desktop/macosx/native/libawt_lwawt/awt/CPrinterJob.m".

2)  Corrects the behavior of "RasterPrinterJob.getPageFormatFromAttributes" method to return a new "PageFormat" based on corresponding values of the existing "PageFormat" set by the user through "java.awt.print.PrinterJob.setPrintable(Printable, PageFormat)", if the user does not supply "OrientationRequested", "MediaSizeName", "MediaPrintableArea" attributes during the call to "PrinterJob.print(PrintRequestAttributeSet)".

3) Eliminates the possibility of being blocked during the call "Pageable.getPageFormat(int)", like it was with the previous version of the fix, because now the fix calls "getPageFormat(int)" only for "Pageable" of type "sun.print.OpenBook", which is used by JDK especially, when "PageFormat" is the same for all pages of the document, when the user calls "PrinterJob.setPrintable(Printable, PageFormat)".

I verified that this fix resolves the original bug JDK-8167102 and does not cause the regression JDK-8181192.

Thank you,
Anton


Reply | Threaded
Open this post in threaded view
|

Re: [10] Review request for 8181659: Create an alternative fix for JDK-8167102, whose fix was backed out

Sergey Bylokhov
+1

On 28/11/2017 07:39, Anton Litvinov wrote:

> Hello Phil,
>
> Thank you very much for review and approval of this fix.
>
> Thank you,
> Anton
>
> On 27/11/2017 23:13, Phil Race wrote:
>> Hi,
>>
>> getPageFormatFromAttributes(..) was added to fix some mac specific
>> issues :
>> https://bugs.openjdk.java.net/browse/JDK-8025988 
>> https://bugs.openjdk.java.net/browse/JDK-8025990 which is why it is
>> used only on mac, so I think it is OK to assume it can be modified to
>> fix a further Mac bug. So this seems OK. -phil.
>>
>>
>> On 11/23/2017 08:54 AM, Anton Litvinov wrote:
>>> Hello Prasanta and Phil,
>>>
>>> Could you please review the following fix for the bug consisting in
>>> creation of the alternative version of the fix for
>>> (https://bugs.openjdk.java.net/browse/JDK-8167102). I am writing to
>>> you directly, because you reviewed my first fix for this bug, which
>>> then caused the bug
>>> (https://bugs.openjdk.java.net/browse/JDK-8181192) and was backed out
>>> by the fix for JDK-8181192.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8181659
>>> Webrev: http://cr.openjdk.java.net/~alitvinov/8181659/jdk10/webrev.00
>>>
>>> This fix is actually the first version of the fix for JDK-8167102
>>> with insignificant changes:
>>> - edited comment text;
>>> - 1 redundant check on equality to "null" is removed;
>>> - exclusion of the regression test
>>> "test/jdk/java/awt/print/PageFormat/WrongPaperPrintingTest.java"
>>> developed for JDK-8167102 is cancelled by removal of "@ignore" tag
>>> from the test.
>>>
>>> ADVANTAGES OF THE FIX:
>>>
>>> 1)  The fix is completely OS X specific, because the edited method
>>> "sun.print.RasterPrinterJob.getPageFormatFromAttributes()" is invoked
>>> only from the OS X native code in file
>>> "src/java.desktop/macosx/native/libawt_lwawt/awt/CPrinterJob.m".
>>>
>>> 2)  Corrects the behavior of
>>> "RasterPrinterJob.getPageFormatFromAttributes" method to return a new
>>> "PageFormat" based on corresponding values of the existing
>>> "PageFormat" set by the user through
>>> "java.awt.print.PrinterJob.setPrintable(Printable, PageFormat)", if
>>> the user does not supply "OrientationRequested", "MediaSizeName",
>>> "MediaPrintableArea" attributes during the call to
>>> "PrinterJob.print(PrintRequestAttributeSet)".
>>>
>>> 3) Eliminates the possibility of being blocked during the call
>>> "Pageable.getPageFormat(int)", like it was with the previous version
>>> of the fix, because now the fix calls "getPageFormat(int)" only for
>>> "Pageable" of type "sun.print.OpenBook", which is used by JDK
>>> especially, when "PageFormat" is the same for all pages of the
>>> document, when the user calls "PrinterJob.setPrintable(Printable,
>>> PageFormat)".
>>>
>>> I verified that this fix resolves the original bug JDK-8167102 and
>>> does not cause the regression JDK-8181192.
>>>
>>> Thank you,
>>> Anton
>>
>


--
Best regards, Sergey.