Max,

Please find comments in line.

On 6/29/2016 8:18 PM, Wang Weijun wrote:
On Jun 30, 2016, at 9:39 AM, Valerie Peng <valerie.p...@oracle.com> wrote:

Hi Max,

Changes look fine. Just some comments/questions as below.

<src/java.base/share/classes/sun/security/tools/keytool/Resources.java>
- line 46, fix this {0} as well?
I don't see {0} in keytool/Resources.java.

There is one in jarsigner/Resources.java:

    46         {"signerClass.is.not.a.signing.mechanism", "{0} is not a signing 
mechanism"},

You mean it's useless now?
Right, it's under the jarsigner directory. My eyes were crossed. ;)
Is this used still? I didn't find reference to this one.
<test/sun/security/tools/keytool/i18n.html>
- line 53, better to use -providerClass?
Both should work.

-addprovider works because SUN is already a loaded provider.
-providerclass works because sun.security.provider.Sun is a public class in the 
same module.

I prefer -addprovider because -providerclass is only for legacy providers 
loaded with reflection.

In fact, I noticed that SUN is also not in ServiceLoader.load(Provider.class), 
which means it is not a service. Is that why you suggest the test load it with 
-providerclass?
I only saw -provider not -addprovider?
Correct, SUN provider is not exported as a provider for ServiceLoader. I don't have preference to use -providerClass over -addprovider. Whatever is the expected usage is fine with me.

Thanks,
Valerie
Thanks
Max

Thanks,
Valerie

On 6/28/2016 6:09 PM, Wang Weijun wrote:
Ping again to security-dev. Anyone can approve it?

The latest webrev is at

    http://cr.openjdk.java.net/~weijun/8130302/webrev.06

Change from webrev.05 [1] is tiny.

Thanks
Max

[1] http://cr.openjdk.java.net/~weijun/8130302/webrev.06/interdiff.patch.html

On Jun 16, 2016, at 9:33 AM, Wang Weijun <weijun.w...@oracle.com> wrote:


On Jun 16, 2016, at 7:50 AM, Valerie Peng <valerie.p...@oracle.com> wrote:

No big difference to me.
Good, I'll remove the cast.

@security-dev, can someone approve the whole webrev.05?

   http://cr.openjdk.java.net/~weijun/8130302/webrev.05

Thanks
Max

Valerie

On 6/15/2016 8:40 AM, Wang Weijun wrote:
On Jun 15, 2016, at 10:57 PM, Mandy Chung<mandy.ch...@oracle.com>  wrote:

241             throw (InvalidParameterException)

This cast should not be needed?

} catch (UcryptoException ue) {
  throw (InvalidParameterException)
      new InvalidParameterException("Error using " + configArg).
          initCause(ue.getCause());
}

initCause() returns Throwable but the method's signature throws 
InvalidParameterException.

Perhaps have a local variable for InvalidParameterException exception.
Valerie, are you OK with this?

--Max

Mandy

Reply via email to