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

[Alexey Ivanov]

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.

*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.
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.

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
>>