> On Jun 10, 2015, at 4:54 PM, Pete Brunet <peter.bru...@oracle.com> wrote: > > Hi Mandy, That's fixed in the JDK-8078335 patch I submitted earlier in > the day to build-dev as a RfR. I tested that on Win and Mac. > > http://cr.openjdk.java.net/~ptbrunet/JDK-8078335/webrev.00/
Looks good to me. Mandy > > Pete > > On 6/10/15 6:08 PM, Mandy Chung wrote: >> Just a quick check, jdk.accessibility is only linked in windows image at the >> moment. It is a bug. Are you going to fix that in this changeset? I think >> you have to verify this change in windows as well as other platforms. >> >> Mandy >> >> >>> On Jun 10, 2015, at 3:33 PM, Pete Brunet <peter.bru...@oracle.com> wrote: >>> >>> Due to some other priorities it's been over 2 months since the last webrev. >>> An update is here: >>> http://cr.openjdk.java.net/~ptbrunet/JDK-8055160/webrev.03 >>> >>> The changes from webrev.02 are: >>> >>> 1) The test was changed to not use the service provider to test the >>> activation of the service provider. Instead a file is created when >>> Toolkit.getDefaultToolkit activates providers and tested for existence when >>> the test runs. >>> >>> 2) The copyright header in the new jdk.accessibility files were fixed. >>> >>> Pete >>> >>> On 4/3/15 3:59 PM, Pete Brunet wrote: >>>> Due to the recent push of JDK-8076182 (Open source Java Access Bridge) >>>> which exposed some files that were in closed the webrev needs a full >>>> re-review. I've also made the changes requested by Mandy. >>>> >>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8055160/webrev.02/ >>>> >>>> Pete >>>> >>>> On 3/23/15 4:41 PM, Mandy Chung wrote: >>>>> >>>>> On 3/19/2015 6:03 PM, Pete Brunet wrote: >>>>>> A new webrev is available at >>>>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8055160/webrev.01/ >>>>>> >>>>> line 820-821: this comment is incorrect. >>>>> >>>>> line 831-838: what happens if ServiceConfigurationException thrown or any >>>>> exception is thrown by the activate method? This should wrap with >>>>> AWTError as I mentioned in my previous review comment. This was hidden >>>>> with the test (see below). >>>>> >>>>> line 891-901: this example may not be necessary as the service loader >>>>> documentation should cover it. >>>>> >>>>>> >>>>>> 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. >>>>> Almost. For Foo, Bar providers, their activate method throwing >>>>> RuntimeException actually stops loading the second provider. The >>>>> activate method could perhaps update some static field defined in the >>>>> Load class if it's called (perhaps adding its name) so that you can tell >>>>> whether the expected providers are activated. UnusedProvider throwing >>>>> RuntimeException is good since you don't expect it's activated. >>>>> >>>>> Otherwise, looks good. >>>>> >>>>> Mandy >
