On 3/17/15 3:51 PM, Pete Brunet wrote:
> 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.
Here is the new comment for loadAssistiveTechnologies for review:
/**
* Loads accessibility support using the property
assistive_technologies.
* The form is assistive_technologies= followed by a comma-separated
list of
* assistive technology providers to load. The order in which
providers are
* loaded is determined by the order in which the ServiceLoader
discovers
* implementations of the AccessibilityProvider interface, not by
the order
* of provider names in the property list. When a provider is found its
* accessibility implementation will be started by calling the
provider's
* activate method. All errors are handled via an AWTError exception.
*
* The assumption is made that assistive technology providers are
supplied
* as part of java.desktop module or specified on the classpath.
*/
>
> 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
>>>
>>
>
