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
