> On Mar 27, 2015, at 04:41, Sean Mullan <sean.mul...@oracle.com> wrote: > > On 03/24/2015 05:47 AM, Wang Weijun wrote: >> Hi All >> >> Please review the code change at >> >> http://cr.openjdk.java.net/~weijun/8056174/webrev.00/ >> >> It provides a new jdk.Exported API JarSigner to perform jar signing. The >> class contains popular functions of the jarsigner tool. The jarsigner tool >> is unchanged and currently independent of the new class. > > * AlgorithmId > > - checKeyAlgSigAlgkMatch > > Should this be named "checkKeyAndSigAlgMatch"?
OK. > > * Builder > > - why are methods public when class is package-private? I forgot to move the class into sun.security.tools.jarsigner and make it public. Then it's accessible from both JarSigner (the API) and sun.security.tools.jarsigner.Main (the tool). > > * JarSigner > > - For digestAlg and sigAlg you should add a pointer to the relevant section > of the Standard Algorithm Names guide for a list of standard algorithms that > can be specified. Yes. > > * JarSignerException > > - is there ever a reason to add a ctor that takes a String for the exception > message? I added your comments from the other mail below. > Passing comment when skimming on the webrev: > > JarSignerException.ErrorCode - do you expect the caller will want > to process error handling differently with different type of error? > > I wonder if the error message would be good enough wrapping the > Throwable cause. Yes, I agree a ctor with arguments (message, cause) is enough. The type of cause is actually equivalent to the ErrorCode. The new message argument should be localized. > Should JarSignerException be an unchecked exception? In traditional Java style, it should be checked. Normally unchecked exception should not be expected if running normally, but here we will see TSA connection error, algorithm name error, etc. In fact I originally suggested unchecked exception, but my main reason is that a checked permission is not friendly with lambda and a little old fashioned. > > * API > > - copyright should be 2015 > > * options.sh > > - how is this relevant if jarsigner has not been updated to use the new API > yet? Also, we should avoid adding more shell script tests. Not relevant, I'll remove it. Thanks Max > > --Sean > >
