CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)

[Joe Wang]

Hi Daniel,

Looks good to me!

-Joe

On 12/5/2012 8:17 AM, Daniel Fuchs wrote:
> Hi,
>
> Please find below a revised version of the changes for
> the javax.xml.parsers package.
>
> It hopefully incorporates all comments I have received so far.
>
> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.01/> 
>
>
> * better wording/formating for Javadoc changes
> * using for( : ) syntax in findServiceProvider
> * improved // comments explaining why the additional layer of
>   RuntimeException is needed when wrapping ServiceConfigurationError.
>
> best regards,
>
> -- daniel
>
> On 12/4/12 2:54 PM, Alan Bateman wrote:
>> On 03/12/2012 19:04, Daniel Fuchs wrote:
>>> Hi,
>>>
>>> This is a first webrev in a series that addresses a change intended
>>> for JDK 8:
>>>
>>> 7169894: JAXP Plugability Layer: using service loader
>>>
>>> I've been building up on Joe's work and Paul & Alan comments
>>> from an earlier discussion thread [1]
>>>
>>> This here addresses changes in the javax.xml.parsers
>>> package for the SAXParserFactory and DocumentBuilderFactory - and
>>> more specifically their no-argument newInstance() method.
>>>
>>> This change replaces the custom code that was reading the
>>> META-INF/services/ resources by a simple call to ServiceLoader.
>>>
>>> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.00/> 
>>>
>>>
>> Thank you very much for taking this one on. I think your approach to
>> take javax.xml.parsers on its own is good as the previous review rounds
>> got really stuck in the mire due to the number of factories, code
>> duplication, and subtle specification and implementation differences.
>>
>> In the class descriptions it suggests that the default implementation
>> may be "installed as a module" but we aren't there yet so I'm not sure
>> about using the term "module". I think it should be okay to say
>> "installed as an extension" as ServiceLoader uses this term.
>>
>> The class description also suggests that a SCE will be wrapped as a
>> FactoryConfigurationException but in FactoryFinder I see that it actual
>> wraps a RuntimeException. I think you can just use the no-arg
>> constructor then then use initCause to set the cause to the SCE.
>>
>> Also the statement "If a misconfigured provider is encountered and SCE
>> is thrown" can probably be changed to "If SCE is thrown" as it can be
>> thrown for other reasons too.
>>
>> Minor nit is that the updates to DocumentBuilderFactory's class
>> description requires a really wide screen compared to the rest of the
>> javadoc.
>>
>> Minor implementation suggestion for findServiceProvider is that you can
>> use for-each to iterate through the providers.
>>
>> Otherwise I think you've captured all the points of feedback from the
>> original review.
>>
>> Finally one suggestion is to make keep notes on the "before & after"
>> behavior, this is something that will be important for release and
>> compatibility notes.
>>
>> -Alan.
>>
>>
>>
>>
>>
>