<AWT Dev> RfR JDK-8055160

[Pete Brunet]

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
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20150317/c17bded2/attachment.html>