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

[Zhengyu Gu]

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.


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

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