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

[huizhe wang]

Thanks Daniel!

On 5/9/2016 4:05 AM, Daniel Fuchs wrote:
> Hi Joe,
>
> This looks much better to me.
> I just wonder about the @Deprecated("5") vs @Deprecated("1.5")

Yes, the compiler accepted it just fine and produced javadocs similar to 
that of XMLReaderFactory where since="9".

Best,
Joe

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