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

[Xuelei Fan]

On 6/7/2018 6:29 PM, Weijun Wang wrote:
> Have you updated it yourself?
I did not.

> I have a proposal at
> 
>     http://cr.openjdk.java.net/~weijun/9999999/webrev.hello-cookie-manager/
> 
Looks fine to me.

Minor nit:  Would you mind limit the maximum line character to 80 
characters (line 250)?

Thanks,
Xuelei

> --Max
> 
>> On Jun 6, 2018, at 10:40 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>
>> On 6/5/2018 8:48 PM, Weijun Wang wrote:
>>>> On 6/5/2018 6:54 AM, Weijun Wang wrote:
>>>>> HelloCookieManager.java:
>>>>>    44     HelloCookieManager(SecureRandom secureRandom) {
>>>>>    45         this.secureRandom = secureRandom;
>>>>>    46     }
>>>>>    47
>>>>>    48     HelloCookieManager valueOf(ProtocolVersion protocolVersion) {
>>>>>    Why not just create a static method and make HelloCookieManager abstract?
>>>>>     static HelloCookieManager of(SecureRandom secureRandom, ProtocolVersion protocolVersion)
>>>>> I haven't seen "raw" HelloCookieManager used anywhere.
>>>> For every SSLContext, the SecureRandom may be different, so we need a different HelloCookieManager instance and cookie managers for each SSLContext instance:
>>>>
>>>> SSLContextImpl.java
>>>> -------------------
>>>>   253   helloCookieManager = new HelloCookieManager(secureRandom);
>>> So the main benefit of the current implementation is that once a HelloCookieManager is created you can reuse the 3 child HelloCookieManager objects inside it. Right?
>> Yes.
>>
>>> If so, I'm OK with the current structure.
>> I see you concerns.  The structure looks weird in some sense, although it is easier for a package private class.  Let me see if I can make an update in the next changeset.
>>
>> Thanks,
>> Xuelei
>>
>>> My major concern is, since the "base" HelloCookieManager is never useful we should take great care not to create an instance of it. How about this?
>>> 1. Make HelloCookieManager abstract.
>>> 2. Create an non-abstract inner class HelloCookieManager$Builder and move the 3 real instances there.
>>> 3. Inside SSLContextImpl, store a field of HelloCookieManager$Builder, and in getHelloCookieManager(), call "builder.get(protocolVersion)" to return one of the 3 real instances.
>>> If the reuse of the 3 real instances is not a big benefit, you can just make HelloCookieManager abstract and create a new child instance in every getHelloCookieManager() call.
>>> Anyway, I feel uncomfortable that the base HelloCookieManager class can be instantiated.
>>> Thanks
>>> Max
>>>>>> On Jun 5, 2018, at 12:12 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>>>>>
>>>>>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.01
>