Hi Mandy,
On 12/5/12 10:55 PM, Mandy Chung wrote:
Hi Daniel,
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
present".
Good point - the description matches the internal implementation, but
maybe too literally:
ServiceLoader.load() may return the default implementation - or
may return null - hence the text 'the default implementation, if
present, is returned.'
If ServiceLoader.load() returns null, then the algorithm will again
try to instantiate the default implementation - usually using
Class.forName with the TCCL - hence the last bullet:
'Platform default <code>DocumentBuilderFactory</code> instance.'
This last step is necessary because the default implementation
may be present without being accessible through a service
provider (that's the default configuration: in JDK 8 - with no
user configuration, ServiceLoader.load() will never instantiate
the default implementation)
However - from a user point of view - I don't think it makes sense
to differentiate these two cases: As a user of the JAXP parser factories
I won't really care how the default implementation is instantiated,
will I? So indeed - I think we should merge the two!
Thanks,
-- daniel
Mandy
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.
