[OpenJDK 2D-Dev] RFR: 8038875: Remove use of ServiceLoader in finding class implementing sun.java2d.pipe. RenderingEngine

[Mandy Chung]

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