RFR (JAXP) 8152912: SAX XMLReaderFactory needs to be ServiceLoader compliant

[huizhe wang]

On 5/3/2016 3:36 AM, Daniel Fuchs wrote:
> Hi Joe,
>
> This look good but the implementation might be overly complex,
> which makes it difficult to read.

It was basically the existing code with some cleanup. What's in 
jarLookup was a copy of the original code. As you can see I was eager to 
add ServiceLoader support and forget about it (deprecated) :-)
>
> 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.

Make sense. That method was changed. I now renamed it to getClassLoader 
with javadoc.
>
> 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.

The cl-null checks in both jarLookup and findServiceProvider are removed.

The new webrev also includes some cleanup of warnings, deprecation with 
javadocs (since Java SE 5) but without the annotation. I limited the 
patch to the sax package only.

New webrev:
http://cr.openjdk.java.net/~joehw/jdk9/8152912/webrev01/

Best,
Joe

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