On 6/21/2012 5:44 AM, Alan Bateman wrote:
On 21/06/2012 08:55, Joe Wang wrote:
:
webrev:
http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/
It's great to get this work started.
The javadoc changes look okay to me and fine for this change. However
there are a few areas that could be made clearer, like specifying the
behavior for the case that is error encountered locating the
providers. Also it does not specific which class loader is used.
Also, javax.xml.transform.TransformerFactory begins with the following:
<quote>The system property that determines which Factory implementation
to create is named |"javax.xml.transform.TransformerFactory"|. This
property names a concrete subclass of the |TransformerFactory| abstract
class. If the property is not defined, a platform default is be
used.</quote>
That's misleading as well. I'll update the javadoc for the next review.
For the class loader, as discussed with Paul, since the ServiceLoader
does so much, we may be able to skip the 'using different classloader'
part, that is, simply calling load without classloader. The javadoc
states that it's equivalent to load with context class loader. But from
what I can, it found all that's placed in the endorsed dir or
bootclasspath. I need to do a few more tests on this.
On the implementation changes then I agree with Paul's comment that
there is a lot of duplication. I realize you have inherited some
technical debt but now seems a good opportunity to clean this up
instead of changing essentially the same code in lots of places.
As discussed, it's security required.
I don't understand the need for the Class.forName in
findServiceProvider as I thought this method should just use
ServiceLoader.
For XPath, Transformer, Datatype and Validation, it's possible get rid
of that since the factory finder is dedicated to a single factory. For
the parsers and stream, it's shared by multiple factories, Class.forName
is used to return Class by name, or factory id. Once we're clear on
what the other three steps that also use the factory id would become, we
can possibly further improve.
I see Paul also suggested that the default/fallback implementation not
be registered but an important need here is that the JAXP module
shouldn't need to include all of Xerces and Xalan. I think it would be
cleaner to use services mechanism rather than using an optional
dependency.
Maybe I'm a little confused. But the fallback to default implementation
is in the spec. It happens the jaxp ri is installed as an endorsed
technology here and loaded by the ServiceLoader.
-Joe
-Alan.
