[8u] RFR 8153732: Windows remote printer changes do not reflect in lookupPrintServices()

[Alexey Ivanov]

Hi Zhengyu,

Looks good to me.

On 05/06/2019 16:07, Zhengyu Gu wrote:
> Hi Alexey,
>
> Thanks for reviewing.
>
> On 6/5/19 10:08 AM, Alexey Ivanov wrote:
>> Hi Zhengyu,
>>
>> In my opinion, backporting JDK-8055705 [1] which renames platform 
>> specific classes to a common name is unnecessary. The purpose of 
>> renaming was to simplify module definition which is irrelevant for 8.
>>
> We are trying to bring the code base close to later JDK, for the 
> benefits of future backports.

Got it!

>> *PrintServiceLookupProvider.java*
>> 133             // start the local printer listener thread
>> 134             Thread thr = new Thread(null, new 
>> PrinterChangeListener(),
>> 135                     "PrinterListener", 0);
>>
>> You create a new thread instance with PrinterChangeListener as the 
>> Runnable implementation. Yet PrinterChangeListener is still a 
>> subclass of Thread which seems redundant. You can make 
>> PrinterChangeListener implement Runnable without extending Thread.
>>
>> Otherwise, looks good to me.
>
> Thanks.
> Updated webrev: http://cr.openjdk.java.net/~zgu/JDK-8153732_8u/webrev.02/
>
>> However, I'd recommend backporting the follow-up fixes in one go, see 
>> my comment [3] to JDK-8153732 [2].
>>
>> First of all, JDK-8153732 caused a regression JDK-8212202 [4]: NPE is 
>> thrown if no printers are installed on the system.
>> JDK-8221412 [5] fixes another situation where the list of printers is 
>> not refreshed. In addition, it simplifies the native code.
>> And JDK-8221263 [6] updates the test making it more user-friendly.
>
> Yes, they are all on my backport list. Once this one is ready, all 
> others are ready to go (no conflicts or minor conflicts)

I see. The follow-up CRs are direct backports and don't require 
additional review. Sounds good.

-- 
Regards,
Alexey

> Thanks,
>
> -Zhengyu
>
>>
>> Regards,
>> Alexey
>>
>> P.S. I'm not a reviewer.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8055705
>> [2] https://bugs.openjdk.java.net/browse/JDK-8153732
>> [3] 
>> https://bugs.openjdk.java.net/browse/JDK-8153732?focusedCommentId=14256474#comment-14256474 
>>
>> [4] https://bugs.openjdk.java.net/browse/JDK-8212202
>> [5] https://bugs.openjdk.java.net/browse/JDK-8221412
>> [6] https://bugs.openjdk.java.net/browse/JDK-8221263
>>
>> On 04/06/2019 16:08, Zhengyu Gu wrote:
>>> Ping!
>>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>> On 5/28/19 9:03 AM, Zhengyu Gu wrote:
>>>> Hi,
>>>>
>>>> Could you please review this 8u backport? After JDK-8055705 
>>>> backport, the original patch still does not apply cleanly, due to 
>>>> missing following Thread API in JDK8u.
>>>>
>>>> public Thread​(ThreadGroup group,
>>>>                Runnable target,
>>>>                String name,
>>>>                long stackSize,
>>>>                boolean inheritThreadLocals)
>>>>
>>>> The original patch has inheritThreadLocals = false, but replacement 
>>>> API
>>>>
>>>> public Thread​(ThreadGroup group,
>>>>                Runnable target,
>>>>                String name,
>>>>                long stackSize)
>>>> has it default to true.
>>>>
>>>> Could AWT folks take a look?
>>>>
>>>>
>>>> JDK9 bug: https://bugs.openjdk.java.net/browse/JDK-8153732
>>>> Original patch: http://hg.openjdk.java.net/jdk/client/rev/732a3b600098
>>>>
>>>>
>>>> 8u Backport: http://cr.openjdk.java.net/~zgu/JDK-8153732_8u/webrev.01/
>>>>
>>>> Test:
>>>>    manual test: RemotePrinterStatusRefresh
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>