On 12/19/2012 6:34 AM, Daniel Fuchs wrote:
Hi,
Please find below an updated webrev for the javax.xml.transform
package, taking into account Mandy's & Joe's comments - namely:
1. Fixed call to creationMethod.invoke.
2. Got rid of the 4 args FactoryFinder.newInstance method.
3. Got rid of the useBSClsLoader which was always passed as 'false'.
(thanks Mandy!)
I agree with the change. Just to clarify, the only case where
useBSClsLoader could be 'true' was in the old findJarServiceProvider
method.
There might be subtle things the ServiceLoader does that could be
different from the findJarServiceProvider process. In both processes,
ContextClassLoader is used first, then the old process would try the
classloader of the finder class which is the bootclassloader.
The ServiceLoader spec says: The ServiceLoader.load(service) is
equivalent to ServiceLoader.load(service,
Thread.currentThread().getContextClassLoader()) where "loader - The
class loader to be used to load provider-configuration files and
provider classes, or null if the system class loader (or, failing that,
the bootstrap class loader) is to be used "
So we need to make sure it works fine when ContextClassLoader is null,
or ContextClassLoader's parent is not the app class loader. I think we
have regression tests for the scenarios. Since you've run them, I don't
see there's any problem.
-Joe
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.transform/webrev.01/>
-- daniel
On 12/18/12 11:02 PM, Daniel Fuchs wrote:
On 12/18/12 9:06 PM, Mandy Chung wrote:
In FactoryFinder.newInstanceNoServiceLoader method, L223, 226 -
NoSuchMethodException will be thrown if such method doesn't exist.
creationMethod will not be null.
Thanks - yes - you're right of course - no need to check for null...
L236 - this change is not needed, right? The method is a static
no-arg method. You passed an additional argument creationMethod as the
first parameter although it's harmless as it's ignored.
Oops - my bad. That's a mistake - I did too many successive changes -
should be creationMethod.invoke(null) of course.
And my tests didn't even catch it!
A minor comment:
151 static<T> T newInstance(Class<T> type, String className,
ClassLoader cl, boolean doFallback)
152 throws TransformerFactoryConfigurationError
153 {
154 return newInstance(type, className, cl, doFallback,
false, false);
155 }
The FactoryFinder.newInstance method 4-argument version is only
called by
TransformerFactory.newInstance(String factoryClassName, ClassLoader
classLoader).
Perhaps you can clean this up TransformerFactory to call the
Factory.newInstance
method 6-argument version.
3 successive boolean parameters... I hate that ;-) Yes I think I can do
this cleanup...
Thanks Mandy,
-- daniel
Thanks
Mandy
