Hi Alan, Thanks for the review.
On 3/17/15 3:42 AM, Alan Bateman wrote:
> 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.
l changed the name. I don't foresee any new providers so for the next
webrev I didn't create the subpackage, though I can do that if you'd prefer.
>
> 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.
I started with an interface and someone along the way suggested I use a
class. I changed it back to an interface.
>
> 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?
Mandy, Do you have insight on this?
>
> As it's a new type then you'll since @since 1.9.
I added that.
>
> Should the javadoc for getDefaultToolkit make it clear that service
> providers are located using the system class loader?
Is this better?
* If this Toolkit is not a headless implementation and if they
exist, service
* providers of {@link javax.accessibility.AccessibilityProvider}
will be loaded
* by the system class loader <--- new text
* if specified by the system property
* {@code javax.accessibility.assistive_technologies}.
>
> 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.
Thanks for catching that. I'll rework the comment.
Once I hear back from Mandy on the security manager topic I'll post a
new webrev.
Pete
>
> -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
>>
>
