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
