On 05/10/2012 19:22, Mandy Chung wrote:
Properties.java L1157 - since loadProviderFromProperty method is
called within a doPrivileged block, it doesn't seem to be necessary to
catch SecurityException.
Thanks for the quick review. You're right on catching the
SecurityException, this isn't necessary.
It uses the system class loader to load the service provider. It's
fine as it currently only supports the one provided by the platform
(JAXP) as the SPI is internal; otherwise, it may be appropriate to use
ServiceLoader.loadInstall. What you have is fine.
For the intended usage then loadInstalled would be fine but for testing
it may be useful to try out providers on the class path too.
test/java/util/Properties/LoadAndStoreXML.java
It's good to test the cases with and without security manager.
Could the test run both cases in the same VM (or even in the same
test) if you explicitly save the SecurityManager before the test runs
and restores it after the test finishes? It's minor and I just
thought that we might want to avoid othervm tests if possible.
There aren't tests for these methods so I added this basic sanity test
so that there is at least something in the jdk repository. You are right
that it's more efficient if we can avoid needing to run in othervm mode.
I've removed the @run tags and the same now exercises the methods both
with and without a security manager.
The updated webrev is here:
http://cr.openjdk.java.net/~alanb/8000354/webrev.02/
-Alan.
