<AWT Dev> RfR JDK-8055160

[Alan Bateman]

On 17/03/2015 01:14, Mandy Chung wrote:
>
> On 3/16/2015 1:52 PM, Pete Brunet wrote:
>> The following patch to accessibility related code is for the modularity
>> effort.
>>
>> http://cr.openjdk.java.net/~ptbrunet/JDK-8055160/webrev.00/
Mandy cc'ed me with your review comments I skimmed through the webrev 
and have a few other comments.

I agree that "AccessibilitySPI" looks a bit odd and would be more 
consistent as AccessibilityProvider. Putting it in the spi sub-package 
is something to consider too, esp. if it looks like there might be other 
service types coming.

Is there a reason why it is an abstract class rather than an interface? 
If it stays as an abstract class then it would be good to add a 
protected constructor with javadoc.

Are there are any security considerations here? Can I create my own 
AccessibilitySPI and put it on the class path when running with a 
security manager?

As it's a new type then you'll since @since 1.9.

Should the javadoc for getDefaultToolkit make it clear that service 
providers are located using the system class loader?

I read the comment in Toolkit.loadAssistiveTechnologies and it suggests 
to me that the entries in the properties files are class names and I 
wonder if this comment needs to updated as the implementation looks like 
the value is a list of provider names now.

-Alan.







>
> Toolkit:
>    line 799-804: this can simply be replaced with
>    Class<?> clazz = Class.forName(atName, false, cl);
>
>    line 886-892: can be shortened to just:
>    If the requested name matches the {@linkplain
>    javax.accessibility.AccessibilitySPI#name name of the service 
> provider},
>    the {@linkplain javax.accessibility.AccessibilitySPI#activate 
> activate}
>    method will be invoked to activate the matching service provider.
>
>    I think the statement "The service providers has two methods..." is
>    not needed.
>    line 894: @implSpec would apply [1] here as it's the implementation
>    specification. I think line 896 can say this implementation will
>    look at a properties file.... IMO, you can take out "fall back".
>
> javax.accessibility.AccessibilitySPI
>   - I sample several SPI names from javadoc and maybe 
> AccessibilityProvider?
>     Just to pass this to you if you consider it an option.
>   - I think you can use @apiNote instead of @implNote there (see [1])
>
> For the test, since you support multiple providers, perhaps good
> to add one more test case to activate two providers and load two
> providers but only one is activated.
>
> Thanks
> Mandy
> [1] http://openjdk.java.net/jeps/8068562
>