Hi, Mandy Thanks lot for your suggestions, update webrev as below and comment inline, thanks
http://cr.openjdk.java.net/~xyin/8201528/webrev.02/ <http://cr.openjdk.java.net/~xyin/8201528/webrev.02/> > On 9 Jun 2018, at 4:22 AM, mandy chung <[email protected]> wrote: > > > > On 6/8/18 12:07 AM, Chris Yin wrote: >> Hi, Mandy >> Many thanks for your detailed review and comments, updates new webrev >> as below, and comment inline, thanks >> webrev: http://cr.openjdk.java.net/~xyin/8201528/webrev.01/ > > > Thanks for adding the test description, that is very helpful. > This is looking pretty good. I have some suggestion on the test > arguments that one can translate to the command-line easily. > > 69 * PackageFromManifest runJar single > 73 * PackageFromManifest runUrlLoader single > > single means running with "testpack1.jar". What about putting > the JAR file name as the option which makes it very clear what > is being tested. Nit: "testpack1" could be confused with pack200 > and maybe simply test1.jar. How about Fixed, thanks > > @run main PackageFromManifest runJar test1.jar > > 77 * PackageFromManifest runJar 1 > 81 * PackageFromManifest runJar 2 > > These tests load two JAR files and load foo.Foo<n>. What do you > think to express: > > PackageFromManifest runJar test1.jar test2.jar foo.Foo1 > PackageFromManifest runJar test1.jar test2.jar foo.Foo2 > > 85 * PackageFromManifest runUrlLoader 1 > 89 * PackageFromManifest runUrlLoader 2 > > PackageFromManifest runUrlLoader test1.jar test2.jar foo.Foo1 > PackageFromManifest runUrlLoader test1.jar test2.jar foo.Foo2 Used new style as you suggested, that looks exactly match with the description :) > > Nit: I suggest to group the runType together. i.e. move line 34 > to above line 37. "<p>" not strictly needed as we won't generate > the javadoc. You can consider leaving it a blank line. Sure, grouped runType and removed <p> (oh, it’s added by IDE during code reformat) Thanks & Regards, Chris > > Mandy >
