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

[Daniel Fuchs]

Hi Joe,

This looks much better to me.
I just wonder about the @Deprecated("5") vs @Deprecated("1.5")
We started dropping the 1. for Java 5 - so I guess
@Deprecated("5") is OK. Hopefully Joe Darcy will be able
to confirm :-)

best regards,

-- daniel

On 04/05/16 02:22, huizhe wang wrote:
>
> 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
>>
>>
>