SSLContextImpl.java (was Re: Code Review Request: TLS 1.3 Implementation)

[Xuelei Fan]

Update: http://hg.openjdk.java.net/jdk/sandbox/rev/12e20a7d6e26

On 6/10/2018 10:37 PM, Weijun Wang wrote:
> The protocols (for example, SSLParameters::getProtocols) are now from new to old, which is opposite from the previous order. Why make this change?
> 
>    41  * Instances of this class are immutable after the context is initialized.
> 
> You mean instances of child of this class? It looks like this class itself can be init() multiple times.
> 
Good catch!  It it used to remind the implementation of this class. 
Reword to:
Implementation note: Instances of this class and the child classes are 
immutable, except that the context initialization (SSLContext.init()) 
may reset the key, trust managers and source of secure random.

>   414         return Arrays.asList(suites.toArray(new CipherSuite[0]));
> 
> Is this better than new ArrayList(suites)?
> 
I don't think so.  Updated to use ArrayList.

>   940                 candidates = new ProtocolVersion[refactored.size()];
>   941                 candidates = refactored.toArray(candidates);
> 
> Why not candidates = refactored.toArray(new ProtocolVersion[refactored.size()]);
> 
Hm, it's better.

>   943             System.out.println(refactored.toString());
> 
> Please remove.
> 
Good catch!

> Another thing in CipherSuite.java:
> 
>    803     static CipherSuite nameOf(String ciperSuiteName) {
> 
> Why isn't this method named valueOf()?
> 
There is a default builtin valueOf(String) method for enum type.

Thanks,
Xuelei

> No other comment.
> 
> Thanks
> Max
>