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

[Alan Bateman]

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.