I've updated the webrev, hopefully one last time with feedback from Joe Darcy. http://cr.openjdk.java.net/~ddehaven/8132743/webrev.2/
- Relocated package Javadoc to above the package declaration in package-info.java - Reworded the Javadoc on the default JSObject ctor A point was brought up that the default ctor could probably be package-private, but there's no time to investigate what the impact would be so I will file a follow-up issue to investigate doing that at a later date. -DrD- > On Mar 4, 2016, at 7:44 AM, Kevin Rushforth <kevin.rushfo...@oracle.com> > wrote: > > Looks good to me. > > -- Kevin > > > David DeHaven wrote: >> Updated webrev: >> >> http://cr.openjdk.java.net/~ddehaven/8132743/webrev.1/ >> >> >> Added jdk.jsobject to MAIN_MODULES >> Made suggested Javadoc changes >> Gave JSException a real serialVersionUID >> >> -DrD- >> >> >> >> >>> On Mar 3, 2016, at 5:06 PM, David DeHaven <david.deha...@oracle.com> >>> wrote: >>> >>> >>> Adding it to MAIN_MODULES, I now see it in bootmodules.jimage: >>> /jdk.jsobject/jdk/internal/netscape/javascript/spi/JSObjectProvider.class >>> /jdk.jsobject/netscape/javascript/JSException.class >>> /jdk.jsobject/netscape/javascript/JSObject$ProviderLoader$1.class >>> /jdk.jsobject/netscape/javascript/JSObject$ProviderLoader.class >>> /jdk.jsobject/netscape/javascript/JSObject.class >>> >>> -DrD- >>> >>> >>> >>>> jdk9-dev is not the right mailing list. I bcc’ed it and add jigsaw-dev >>>> instead. >>>> >>>> >>>> >>>> >>>>> On Mar 3, 2016, at 3:57 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> >>>>> wrote: >>>>> >>>>> Looks OK to me. I did a quick test build and I can see the new package in >>>>> the exploded JDK, but not in the images. Maybe I did something wrong? >>>>> >>>>> >>>>> >>>> Good catch. >>>> >>>> jdk.jsobject needs to be added in MAIN_MODULES list in make/Images.gmk >>>> >>>> Mandy >>>> >>>> >>>> >>>>> -- Kevin >>>>> >>>>> >>>>> David DeHaven wrote: >>>>> >>>>> >>>>>>>> JBS Issue: >>>>>>>> >>>>>>>> >>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8132743 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Code review: >>>>>>>> >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~ddehaven/8132743/webrev.0/ >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> Looks okay. There is no @since - I guess it’s okay because >>>>>>> netscape.javascript has been shipped with plugin for long time. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> I can't track down when it was first included. It pre-dates anything >>>>>> I've looked at so far. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> package-info.java >>>>>>> "when running in an {@link java.applet.Applet Applet}” - is this true >>>>>>> when running with JavaFX webkit? >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> Yes, I believe so, assuming you have a JSObject representing the root >>>>>> window object. Maybe that should be reworded, I think just remove the >>>>>> "when running in an Applet" part. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> JSObject.java >>>>>>> @throws JSException is missing in the methods >>>>>>> >>>>>>> Does it throw NPE if the parameter is null? Or JSException - that >>>>>>> needs to be specified. >>>>>>> >>>>>>> Nit: it’d be good to wrap null with {@code null} in the javadoc. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> Ok. I can fix that. >>>>>> >>>>>> -DrD- >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >> >> >>