RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

[Joe Wang]

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