[OpenJDK 2D-Dev] RFR: 8241829 Cleanup the code for PrinterJob on windows

[Alexey Ivanov]

On 01/04/2020 09:17, Prasanta Sadhukhan wrote:
>
> On 01-Apr-20 3:51 AM, Sergey Bylokhov wrote:
>> Hi, Prasanta.
>>
>> On 3/30/20 7:46 am, Prasanta Sadhukhan wrote:
>>> I guess it is better if you directly pass NULL to OpenPrinter, 
>>> rather than create an unnecessary variable.
>>
>> That was done only for documentation purposes.
>>
>>> Also, I think it will be good if we do a ClosePrinter() if 
>>> FindFirstPrinterChangeNotification() fails.
>>
>> Nice catch! webrev is updated:
>> http://cr.openjdk.java.net/~serb/8241829/webrev.01
>
> Looks good now. But since this is a cleanup exercise, I would have 
> liked to remove the unnecessary variable and still keep the 
> documentation, can't we (like "NULL in 1st argument indicates using 
> local printer server")? but anyway, not a dealbreaker, upto you.

I agree, I'd remove printerName local variable. You can modify the 
original comment:

      // If pPrinterName (the first parameter) is NULL, "it indicates 
the local printer server" - MSDN.

I'd also align jobject parameter to JNIEnv as done in other functions. 
However, it's nitpicking. :)


Otherwise, looks good to me.

Regards,
Alexey

>
>
> Regards
> Prasanta
>>
>>>
>>> Regards
>>> Prasanta
>>> On 30-Mar-20 7:13 PM, Sergey Bylokhov wrote:
>>>> Hello.
>>>> Please review the fix for jdk/client.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8241829
>>>> Fix: http://cr.openjdk.java.net/~serb/8241829/webrev.00
>>>>
>>>> In PrintServiceLookupProvider.java we always pass NULL to
>>>> the notifyFirstPrinterChange as a name of the printer, so
>>>> we can remove the code which expects a non-null value.