Mostly minor, but a couple substantive comments.
I skimmed the tests this time, but didn't hit them as hard.
On 5/14/2018 1:20 PM, Valerie Peng wrote:
Hi Brad,
The latest webrev is at
http://cr.openjdk.java.net/~valeriep/8146293/webrev.04/
The only difference between webrev.03 and webrev.04 is the review
comments from the CSR.
SignedObject.java
-----------------
Now that SignedObject is no longer in the scope of the CSR, we talked
about updating the class javadoc with an example about setting the
parameters before passing the Signature object in. I didn't see that,
so did you want to at least give an example of it here? I don't expect
that would require a separate CSR.
Signature.java
--------------
Copyright year update.
PSSParameterSpec.java
---------------------
Nit: add vertical space between description and params. 98/99, 104/105.
125: Here you do need a period, since it's the end of a sentence and
you need to separate the two sentence. Periods after both sentences.
162/164: If you're going to fix some of the other spots in this file,
you might also add spaces here.
RSAPrivateCrtKeySpec.java
-------------------------
Out of curiosity, here and in a couple of other places, what prompted
you to completely reword these constructor sentences? It's always
bothered me, and I noticed it was done between webrev.01 and .02. It
was approved in the CSR, so yay!
RSAPublicKeySpec.java
RSAPrivateKeySpec.java
----------------------
95: Shouldn't you move the null statement to the @return line instead
of the method summary? e.g.
* Returns the parameters associated with this key.
*
* @return the parameters associated with this key, or null if not present?
javax/crypto/spec/package-info.java
-----------------------------------
Copyright year update.
SignerInfo.java
PKCS10.java
---------------
This is what we talked about earlier. In X509CRL.java,
X509CRLImpl.java, X509Certificate.java, X509CertImpl.java, you are
setting the parameters after the init calls, but here you are doing them
before. Probably should be consistent.
RSAUtil.java
------------
44: Extra ","
RSAPadding.java
---------------
106/109: Make final?
RSASignature.java
-----------------
281. Indent 4 more spaces.
MGF1.java
---------
31. If you wouldn't mind adding RFC here?
RSAPSSSignature.java
--------------------
191: Would you mind inserting a comment that you "skip the JCA overhead"?
264: -> PSSParameterSpec.TRAILER_FIELD_BC instead of hardcoding 1?
418: Thanks for adding all of the "stepX" comments. That really helps
readability! I'm assuming no material changes were made here? I looked
at the impl code heavily on the last review, but not as much this time.
TestOAEPWithParams.java
-----------------------
50: Should we also add SHA-384, SHA-512 here?
Offsets.java
------------
43: Should we also add all of the missing RSA variants as well?
SHA{1,224,256...}withRSA
Thanks,
Brad
