CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)
mandy.chung at oracle.com
Wed Dec 5 21:55:40 UTC 2012
Looks good to me. One question - the last bullet in the search order
covers the default implementation case:
"Platform default <code>DocumentBuilderFactory</code> instance."
Would it make sense to merge the statement "Otherwise, the default
implementation, if present, is returned" with the above statement? e.g.
"Default implementation of <code>DocumentBuilderFactory</code> if
On 12/5/2012 8:17 AM, Daniel Fuchs wrote:
> 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.
> * 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:
>>> 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 
>>> 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.
>> 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
>> 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.
More information about the core-libs-dev