<AWT Dev> RfR JDK-8055160

Pete Brunet peter.brunet at oracle.com
Tue Mar 17 21:24:40 UTC 2015

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
     * 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
     * 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
     * activate method.  All errors are handled via an AWTError exception.
     * The assumption is made that assistive technology providers are
     * 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/20150317/3a2e2d58/attachment.html>

More information about the awt-dev mailing list