On 5/7/2014 12:40 PM, Phil Race wrote:
On 5/7/14 12:26 PM, Mandy Chung wrote:
On 5/7/2014 11:26 AM, Phil Race wrote:
https://bugs.openjdk.java.net/browse/JDK-8038875
http://cr.openjdk.java.net/~prr/8038875/
Thanks for taking this on. I have some minor comments:
line 129: if sun.java2d.renderer is set, it finds a custom
RenderingEngine from the bootclasspath (as the current class loader
is the bootstrap class loader). On the other hand, in the original
implementation, the service loader loads from the extension class
loader. I'm not proposing to use a different class loader since this
is not a supported way to plugin a custom rendering engine. Just to
mention it to call out the difference.
Probably it should always have been the bootstrap class loader
since this is a test mechanism for replacing an internal body of code.
Okay good.
But permissions are the same in both cases, aren't they ?
yes. The only difference is that previously someone could put the
service provider implementation into the extension directory that will
be loaded. As this is a test mechanism, what you have is good.
You may consider to use the 3-arg version of Class.forName and not to
initialize the class. This is called within a doPrivileged block and
it's generally a good convention to invoke the class initialization
of the specified class (from the system property) outside the
doPrivileged block. Static analysis tool may consider catching the
1-arg version of Class.forName and flag it as a warning.
I suppose could do that but I'm not sure how much it matters besides
shutting up a tool
given that
a) its going to be on the bootclasspath
b) its got to be specified by a property that can only be set on the
command line
or privileged code
Agree this doesn't matter much. As I replied in another email, it may
be good to follow up if the doPrivileged block is needed or not after
this change.
line 134-140: PiscesRenderingEngine is always present, right? Would
it be better just to return "new PiscesRenderingEngine()"?
It looked as if it is always there, although I'm not sure if that was
intended or is just
the way the build happened to work. So I'd prefer to leave it as ..
That's fine and I understand. I generally recommend not using
reflection unless it's necessary :)
Mandy
