<AWT Dev> RfR JDK-8055160

[Pete Brunet]

A new webrev is available at
http://cr.openjdk.java.net/~ptbrunet/JDK-8055160/webrev.01/

The changes to the code are:
- renamed provider from AccessibilitySPI to AccessibilityProvider
- added a security check to the AccessibilityProvider constructor
- tweaked some comments/javadoc

The changes to the tests are:
- added an unused provider
- added a test activating two providers

Mandy, Regarding the last bullet I'm not sure I resolved your comment,
"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."  If not, please let me know.

Pete

On 3/17/15 4:24 PM, Pete Brunet wrote:
>
>
> 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
>>>>
>>>
>>
>

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