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 @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 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. Mandy
