On 12/18/12 8:00 PM, Joe Wang wrote:
On 12/18/2012 10:00 AM, Daniel Fuchs wrote:
On 12/18/12 6:29 PM, Joe Wang wrote:
Hi Daniel,
The call: T provider = findServiceProvider(type) in static <T> T
find(Class<T> type, String factoryId, ClassLoader cl, String
fallbackClassName) ignored factoryId, and assumed it's the same as
type.getName(). I looked back I had the same bug in my original patch.
I don't think that's a bug in the new code - but rather possibly a
bug in the old code ;-).
The old code did not have the problem since it's reading
"META-INF/services/" + factoryId directly.
There's no way you can pass a property name to the ServiceLoader.
True. That's why it looked a bit awkward since 'factoryId' is not
passed in as did before, as in findJarServiceProvider(factoryId).
The JAR Specification says:
"A service provider identifies itself by placing a
provider-configuration file in the resource directory
META-INF/services. The file's name should consist of the
fully-qualified name of the abstract service class."
So I think the current code is correct in ignoring factoryId - because
according to the spec the file name should be the same as the
abstract class name.
So the StAX API implied that factoryId could be anything, but that
would violate the JAR specification. In other words, it would only
work as intended if the factoryId is specified as a System Property,
or in stax.properties or jaxp.properties.
I would think we should return an error if factoryId != type.getName()
before the call "findServiceProvider(type)".
In fact I toyed with the idea of skipping the call to
findServiceProvider(type) when factoryId != type.getName().
I am not sure whether we should return an error or the skip directly to
returning the default implementation.
I mean - if a user tried to load the factory specified by foo.bar, and
foo.bar is not defined, shouldn't we return
the default implementation? I think that's what was happening before...
But if we return the default implementation - shouldn't any Service
Provider loadable through ServiceLoader take
precedence?
When the parameter is a classname - then the meaning is clear. Either
you can load the class or you can't.
When it's a factoryId - and the doc says "it is the same as a System
property" - then shouldn't the code
behave as if it were a System property?
-- daniel
-Joe
-- daniel
-Joe
On 12/18/2012 8:39 AM, Daniel Fuchs wrote:
Hi,
Thanks for the review.
I updated the webrev to keep track of your suggested change.
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/>
-- daniel
On 12/18/12 4:00 PM, Alan Bateman wrote:
I looked through this installment and aside from an aside from an
alignment issue at lines 101-102 in XMLEventFactory.java then it
looks
good to me.
Also thank you again for being so careful as you work through each of
these areas.
-Alan
