Hi Alan,
Thanks a lot for your comments - some clarifications inline.
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.
OK - will do.
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.
I have refrained to do that because I think there might be code that
brutally does things like (Exception) error.getCause();
(I've seen this kind of pattern in the JAXP code - although
I don't remember exactly where...)
The reason is that FactoryConfigurationError is for some reason
obviously supposed to wrap only exceptions - as its constructor will
take only exceptions - and not throwables, and then overrides
getCause() to return a locally cached variable of type Exception - see
<http://hg.openjdk.java.net/jdk8/tl/jaxp/raw-file/tip/src/javax/xml/parsers/FactoryConfigurationError.java>
Using initCause() would therefore have no effect, unless the
implementation of javax.xml.parsers.FactoryConfigurationError is
changed to accept Throwable as cause - which could - I think have
undesirable side effects on existing code.
My guess is that this is an oddity inherited from the time when
Cause was not defined at Throwable level - but managed locally
on specific exceptions classes instead.
I am fearing that changing the implementation of getCause()
to allow setting the SCE directly as the cause might break
compatibility, causing some ClassCastException to be thrown here
and there.
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.
Will do.
Minor nit is that the updates to DocumentBuilderFactory's class
description requires a really wide screen compared to the rest of the
javadoc.
Will fix.
Minor implementation suggestion for findServiceProvider is that you can
use for-each to iterate through the providers.
I don't mind. Do you think it's better? I'll make the changes and send
an updated webrev...
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.
OK - but in what form? In the webrev header? Or do you think I should
add // comments in the code to make explicit the behavioral changes?
Or do you mean I should collect & gather them in some separate text
doc that will help writing the RN?
-Alan.
