Hi Joe, This look good but the implementation might be overly complex, which makes it difficult to read.
First: 141 ClassLoader cl = ss.getContextClassLoader(); is misnamed, because as far as I can see this method returns the context class loader if not null, otherwise the system class loader. So cl is either the context class loader or the system class loader and is guaranteed to be non null. I would suggest adding a comment to make it clear. This means in turn that the new jarLookup(ClassLoader) method can be greatly simplified - you can do: final ClassLoader cl = Objects.requiresNonNull(loader); and then simplify the if (cl != null) else ... logic, because you now know that cl cannot be null. Best regards, -- daniel On 02/05/16 20:30, huizhe wang wrote:
Hi, Please review a change that adds a step using the ServiceLoader to XMLReaderFactory's provider-lookup process. Meanwhile, XMLReaderFactory is deprecated in favor of javax.xml.parsers.SAXParserFactory. JBS: https://bugs.openjdk.java.net/browse/JDK-8152912 Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8152912/webrev/ The change has been verified by SQE test that Frank will submit soon for review (that is, when he starts to work at his local time). Thanks, Joe
