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

[Daniel Fuchs]

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