Implement PPB_Flash_TCPSocket.InitiateSSL.

Implement PPB_Flash_TCPSocket.InitiateSSL.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97005

[yzshen1, 2011-08-03 23:28:33]

Hi,

William: please review changes in client_socket_factory.h and the code in
pepper_message_filter.cc that creates SSL client sockets.

Brett: please everything else.

Thanks!

http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:603: const
net::SSLConfig&
Reviewers: is the indentation here correct?

[brettw, 2011-08-08 20:08:43]

The Pepper stuff LGTM, hopefully Will can take a quick glance at the SSL usage
to make sure we didn't mess anything up, since I'm pretty unfamiliar with proper
usage.

http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:234:
scoped_ptr<net::SSLConfig> ssl_config_;
Can you add a comment that these are lazily created and you should use
GetSSLConfig/ClientSocketContext to retrieve them?

http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:603: const
net::SSLConfig&
On 2011/08/03 23:28:34, yzshen1 wrote:
> Reviewers: is the indentation here correct?

I think so.

[yzshen1, 2011-08-08 21:51:40]

Thanks, Brett!

William: Would you please take a look? Thanks!

http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/pepper_message_filter.cc:234:
scoped_ptr<net::SSLConfig> ssl_config_;
On 2011/08/08 20:08:43, brettw wrote:
> Can you add a comment that these are lazily created and you should use
> GetSSLConfig/ClientSocketContext to retrieve them?

Done.

[willchan no longer on Chromium, 2011-08-08 22:00:28]

I just got off the plane a few hours back. I'll try to get to this
today. If not, I will get to it tomorrow.

On Mon, Aug 8, 2011 at 2:51 PM,  <yzshen@chromium.org> wrote:
> Thanks, Brett!
>
> William: Would you please take a look? Thanks!
>
>
>
http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
> File content/browser/renderer_host/pepper_message_filter.cc (right):
>
>
http://codereview.chromium.org/7535007/diff/1/content/browser/renderer_host/p...
> content/browser/renderer_host/pepper_message_filter.cc:234:
> scoped_ptr<net::SSLConfig> ssl_config_;
> On 2011/08/08 20:08:43, brettw wrote:
>>
>> Can you add a comment that these are lazily created and you should use
>> GetSSLConfig/ClientSocketContext to retrieve them?
>
> Done.
>
> http://codereview.chromium.org/7535007/
>

[willchan no longer on Chromium, 2011-08-09 17:05:26]

This has some stuff I'd like wtc to take a look at too. I'm going to chat with
him later today about the changes to ClientSocketFactory and the use of
SSLClientSocket.

http://codereview.chromium.org/7535007/diff/5001/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/5001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:332:
OnInitiateSSLCompleted(net::ERR_FAILED);
We're trying not to use ERR_FAILED anymore. This should probably be
ERR_UNEXPECTED.

[yzshen1, 2011-08-09 17:23:20]

Thanks, William.

http://codereview.chromium.org/7535007/diff/5001/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/5001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:332:
OnInitiateSSLCompleted(net::ERR_FAILED);
On 2011/08/09 17:05:26, willchan wrote:
> We're trying not to use ERR_FAILED anymore. This should probably be
> ERR_UNEXPECTED.

Done.

[wtc, 2011-08-09 21:21:29]

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:238:
scoped_ptr<net::CertVerifier> cert_verifier_;
The net::CertVerifier should be stored in net::SSLClientSocketContext.
You should not need a cert_verifier_ member.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:327:
host_port_pair.set_host(server_name);
This means you can only use the default HTTPS port 443.
Ideally you should pass not only the server_name but also
the port in the message.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:608:
ssl_config_.reset(new net::SSLConfig());
Ideally you should pass the SSLConfig in the message, rather
than creating a default SSLConfig object.

http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
net/socket/client_socket_factory.h:52: // set_socket() on it to set a valid
StreamSocket instance.
It is fine by me to support creating such a ClientSocketHandle.
We should add a unit test to verify this kind of ClientSocketHandle
can be used by SSLClientSocket successfully.

Right now SSLClientSocketNSS calls only the following
two methods on ClientSocketHandle:

ClientSocketHandle::socket()
ClientSocketHandle destructor

So a simple unit test can be used to test this.

http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:612: std::string /* server_name */)
SSLClientSocket also needs the port number.  Can you use
the HostPortPair type instead of std::string here?

http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppb_flash_tcp_so...
File ppapi/proxy/ppb_flash_tcp_socket_proxy.cc (right):

http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppb_flash_tcp_so...
ppapi/proxy/ppb_flash_tcp_socket_proxy.cc:92: void OnInitiateSSLCompleted(bool
succeeded);
I suggest that you change "InitiateSSL" to "SSLConnect"
in this class and in the message names.

[yzshen1, 2011-08-09 23:45:28]

Thanks, Wan-Teh!

I have replied to all your comments and I will upload a new patch once I make
sure that I understand some of your points correctly.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:238:
scoped_ptr<net::CertVerifier> cert_verifier_;
SSLClientSocketContext doesn't take ownership of its member |cert_verifier|. I
think this way may be more clear about who owns the CertVerifier instance.
Otherwise, I need to manually delete it when FlashTCPSocketManager is
destructed. 

On 2011/08/09 21:21:29, wtc wrote:
> The net::CertVerifier should be stored in net::SSLClientSocketContext.
> You should not need a cert_verifier_ member.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:327:
host_port_pair.set_host(server_name);
On 2011/08/09 21:21:29, wtc wrote:
> This means you can only use the default HTTPS port 443.
> Ideally you should pass not only the server_name but also
> the port in the message.

The constructors of SSLClientSocket* have the following comments:
// The hostname specified in |host_and_port| will be compared with the name(s)
// in the server's certificate during the SSL handshake.

According to the comments, the port number is not used when comparing with
server certificates. But I do see that it is used by
InitializeSSLPeerName/GetSSLCertRequestInfo/etc.

I am not familiar enough with this part of the codebase (or SSL in general). Is
it correct that I pass in the same port number as the underlying transport
socket? In what cases that this port number can be different from the one of the
underlying transport socket?

Thanks!

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:608:
ssl_config_.reset(new net::SSLConfig());
On 2011/08/09 21:21:29, wtc wrote:
> Ideally you should pass the SSLConfig in the message, rather
> than creating a default SSLConfig object.

Currently we only support a minimal set of operations for Flash, and it doesn't
need configurable settings. If I pass config in the message, it will always be
the same value because Flash will not try to set it.

However, I do agree that if we would like to make this a public interface for
all plugins, we probably need to add such function to the interface.

Please let me know whether it is okay to leave it this way.

http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
net/socket/client_socket_factory.h:52: // set_socket() on it to set a valid
StreamSocket instance.
Do you mean I need to add one in this CL?

On 2011/08/09 21:21:29, wtc wrote:
> It is fine by me to support creating such a ClientSocketHandle.
> We should add a unit test to verify this kind of ClientSocketHandle
> can be used by SSLClientSocket successfully.
> 
> Right now SSLClientSocketNSS calls only the following
> two methods on ClientSocketHandle:
> 
> ClientSocketHandle::socket()
> ClientSocketHandle destructor
> 
> So a simple unit test can be used to test this.

http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:612: std::string /* server_name */)
Please see reply to your comment in pepper_message_filter.cc.

On 2011/08/09 21:21:29, wtc wrote:
> SSLClientSocket also needs the port number.  Can you use
> the HostPortPair type instead of std::string here?

http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppb_flash_tcp_so...
File ppapi/proxy/ppb_flash_tcp_socket_proxy.cc (right):

http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppb_flash_tcp_so...
ppapi/proxy/ppb_flash_tcp_socket_proxy.cc:92: void OnInitiateSSLCompleted(bool
succeeded);
On 2011/08/09 21:21:29, wtc wrote:
> I suggest that you change "InitiateSSL" to "SSLConnect"
> in this class and in the message names.

[I am open to making the change, just to make sure we have the same
understanding.]
The usage of PPB_Flash_TCPSocket is that prior to calling InitiateSSL, Connect
must be called. So the sequence is:
Connect -> InitiateSSL -> Read/Write...
This method is not intended to be an alternative to Connect.

Does it conform to your idea to have such a calling sequence:
Connect -> SSLConnect -> Read/Write... ?

[yzshen1, 2011-08-12 16:44:49]

Wan-Teh: Ping. Would you please take a look? Thanks!

On 2011/08/09 23:45:28, yzshen1 wrote:
> Thanks, Wan-Teh!
> 
> I have replied to all your comments and I will upload a new patch once I make
> sure that I understand some of your points correctly.
> 
>
http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
> File content/browser/renderer_host/pepper_message_filter.cc (right):
> 
>
http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
> content/browser/renderer_host/pepper_message_filter.cc:238:
> scoped_ptr<net::CertVerifier> cert_verifier_;
> SSLClientSocketContext doesn't take ownership of its member |cert_verifier|. I
> think this way may be more clear about who owns the CertVerifier instance.
> Otherwise, I need to manually delete it when FlashTCPSocketManager is
> destructed. 
> 
> On 2011/08/09 21:21:29, wtc wrote:
> > The net::CertVerifier should be stored in net::SSLClientSocketContext.
> > You should not need a cert_verifier_ member.
> 
>
http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
> content/browser/renderer_host/pepper_message_filter.cc:327:
> host_port_pair.set_host(server_name);
> On 2011/08/09 21:21:29, wtc wrote:
> > This means you can only use the default HTTPS port 443.
> > Ideally you should pass not only the server_name but also
> > the port in the message.
> 
> The constructors of SSLClientSocket* have the following comments:
> // The hostname specified in |host_and_port| will be compared with the name(s)
> // in the server's certificate during the SSL handshake.
> 
> According to the comments, the port number is not used when comparing with
> server certificates. But I do see that it is used by
> InitializeSSLPeerName/GetSSLCertRequestInfo/etc.
> 
> I am not familiar enough with this part of the codebase (or SSL in general).
Is
> it correct that I pass in the same port number as the underlying transport
> socket? In what cases that this port number can be different from the one of
the
> underlying transport socket?
> 
> Thanks!
> 
>
http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
> content/browser/renderer_host/pepper_message_filter.cc:608:
> ssl_config_.reset(new net::SSLConfig());
> On 2011/08/09 21:21:29, wtc wrote:
> > Ideally you should pass the SSLConfig in the message, rather
> > than creating a default SSLConfig object.
> 
> Currently we only support a minimal set of operations for Flash, and it
doesn't
> need configurable settings. If I pass config in the message, it will always be
> the same value because Flash will not try to set it.
> 
> However, I do agree that if we would like to make this a public interface for
> all plugins, we probably need to add such function to the interface.
> 
> Please let me know whether it is okay to leave it this way.
> 
>
http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
> File net/socket/client_socket_factory.h (right):
> 
>
http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
> net/socket/client_socket_factory.h:52: // set_socket() on it to set a valid
> StreamSocket instance.
> Do you mean I need to add one in this CL?
> 
> On 2011/08/09 21:21:29, wtc wrote:
> > It is fine by me to support creating such a ClientSocketHandle.
> > We should add a unit test to verify this kind of ClientSocketHandle
> > can be used by SSLClientSocket successfully.
> > 
> > Right now SSLClientSocketNSS calls only the following
> > two methods on ClientSocketHandle:
> > 
> > ClientSocketHandle::socket()
> > ClientSocketHandle destructor
> > 
> > So a simple unit test can be used to test this.
> 
> http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppapi_messages.h
> File ppapi/proxy/ppapi_messages.h (right):
> 
>
http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppapi_messages.h...
> ppapi/proxy/ppapi_messages.h:612: std::string /* server_name */)
> Please see reply to your comment in pepper_message_filter.cc.
> 
> On 2011/08/09 21:21:29, wtc wrote:
> > SSLClientSocket also needs the port number.  Can you use
> > the HostPortPair type instead of std::string here?
> 
>
http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppb_flash_tcp_so...
> File ppapi/proxy/ppb_flash_tcp_socket_proxy.cc (right):
> 
>
http://codereview.chromium.org/7535007/diff/7001/ppapi/proxy/ppb_flash_tcp_so...
> ppapi/proxy/ppb_flash_tcp_socket_proxy.cc:92: void OnInitiateSSLCompleted(bool
> succeeded);
> On 2011/08/09 21:21:29, wtc wrote:
> > I suggest that you change "InitiateSSL" to "SSLConnect"
> > in this class and in the message names.
> 
> [I am open to making the change, just to make sure we have the same
> understanding.]
> The usage of PPB_Flash_TCPSocket is that prior to calling InitiateSSL, Connect
> must be called. So the sequence is:
> Connect -> InitiateSSL -> Read/Write...
> This method is not intended to be an alternative to Connect.
> 
> Does it conform to your idea to have such a calling sequence:
> Connect -> SSLConnect -> Read/Write... ?

[yzshen1, 2011-08-12 21:31:39]

The following are comments from wtc:

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:224: const
net::SSLClientSocketContext& GetSSLClientSocketContext();
wtc: change this method to GetCertVerifier() to create
cert_verifier_ lazily.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:236:
scoped_ptr<net::SSLConfig> ssl_config_;
wtc: consider making this a regular member:
  net::SSLConfig ssl_config_;
because this is a small struct.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:237:
scoped_ptr<net::SSLClientSocketContext> ssl_client_socket_context_;
wtc: Delete this member.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:330:
manager_->GetSSLClientSocketContext()));
wtc:
  SSLClientSocketContext ssl_context;
  ssl_context.cert_verifier = cert_verifier_;
  socket_.reset(factory->CreateSSLClientSocket(
      handle, host_port_pair, manager_->GetSSLConfig(), NULL,
      ssl_context));

http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
net/socket/client_socket_factory.h:52: // set_socket() on it to set a valid
StreamSocket instance.
wtc: the unit test is:

  scoped_ptr<ClientSocketHandle> socket(new ClientSocketHandle);
  socket.set_socket(...);

  scoped_ptr<ClientSocketHandle> transport(socket.release());

  transport->socket()->Read();  // No need to test these

  // Just verify that such a simple ClientSocketHandle,
  // not associated with any client socket pool, can be]
  // destroyed safely.

[yzshen1, 2011-08-13 00:40:34]

Hi, Wan-Teh.

Thanks for your review! Please take another look.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:224: const
net::SSLClientSocketContext& GetSSLClientSocketContext();
On 2011/08/12 21:31:39, yzshen1 wrote:
> wtc: change this method to GetCertVerifier() to create
> cert_verifier_ lazily.

Done.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:236:
scoped_ptr<net::SSLConfig> ssl_config_;
On 2011/08/12 21:31:39, yzshen1 wrote:
> wtc: consider making this a regular member:
>   net::SSLConfig ssl_config_;
> because this is a small struct.

Done.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:237:
scoped_ptr<net::SSLClientSocketContext> ssl_client_socket_context_;
On 2011/08/12 21:31:39, yzshen1 wrote:
> wtc: Delete this member.

Done.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:327:
host_port_pair.set_host(server_name);
On 2011/08/09 21:21:29, wtc wrote:
> This means you can only use the default HTTPS port 443.
> Ideally you should pass not only the server_name but also
> the port in the message.

Done. Thanks for all the explanation.

http://codereview.chromium.org/7535007/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:330:
manager_->GetSSLClientSocketContext()));
On 2011/08/12 21:31:39, yzshen1 wrote:
> wtc:
>   SSLClientSocketContext ssl_context;
>   ssl_context.cert_verifier = cert_verifier_;
>   socket_.reset(factory->CreateSSLClientSocket(
>       handle, host_port_pair, manager_->GetSSLConfig(), NULL,
>       ssl_context));  

Done.

http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/7535007/diff/7001/net/socket/client_socket_fac...
net/socket/client_socket_factory.h:52: // set_socket() on it to set a valid
StreamSocket instance.
On 2011/08/12 21:31:39, yzshen1 wrote:
> wtc: the unit test is:
> 
>   scoped_ptr<ClientSocketHandle> socket(new ClientSocketHandle);
>   socket.set_socket(...);
> 
>   scoped_ptr<ClientSocketHandle> transport(socket.release());
> 
>   transport->socket()->Read();  // No need to test these
> 
>   // Just verify that such a simple ClientSocketHandle,
>   // not associated with any client socket pool, can be]
>   // destroyed safely.

Done.

[wtc, 2011-08-15 19:35:12]

LGTM.  I reviewed Patch Set 5.  I suggest some changes below.
The most important ones are:
- Clarify the host/port parameters for Connect and SSLHandshake.
- The new unit test should either use a mock TCPClientSocket
  (be as simple as possible), or create an SSLClientSocket
  (go all the way).

Please ask me in person if you have questions about my
suggested changes.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:136: void
SSLHandshake(const std::string& host, uint16_t port);
When a proxy server is used, you should clarify whether
host/port refers to the proxy server or the destination
server.  This applies to both the Connect and SSLHandshake
methods.

It may be better to rename the arguments
for SSLHandshake to server_name/server_port, if the
host/port arguments for Connect may refer to the proxy
server.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:149: // There is a
request of initiating SSL that is pending.
This comment should say:
  // There is an SSL handshake request that is pending.
or
  // An SSL handshake is in progress.

And the comment on line 153 should say:
  // An SSL handshake has failed.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:237: net::SSLConfig
ssl_config_;
Please add a comment to note that you are using the default
SSL configuration settings, as opposed to Chrome's SSL settings.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:333:
OnSSLHandshakeCompleted(net::ERR_UNEXPECTED);
Please log an error message because net::ERR_UNEXPECTED
doesn't give us useful information.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:459: connection_state_ =
succeeded ? SSL_CONNECTED : INITIATE_SSL_FAILED;
Should INITIATE_SSL_IN_PROGRESS and INITIATE_SSL_FAILED be
renamed SSL_HANDSHAKE_IN_PROGRESS and SSL_HANDSHAKE_FAILED?

http://codereview.chromium.org/7535007/diff/20003/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/7535007/diff/20003/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:92: // any client socket pool, can be
destroyed safely.
Please move this comment before this unit test, at line 69.

You can also create an SSLClientSocket and call its Connect
method, as is done on lines 111-127 below (you can omit the
CapturingNetLog code).

Alternatively, you can use a mock TCPClientSocket as opposed
to a real TCPClientSocket.  Then you don't need to start up
the TestServer.

http://codereview.chromium.org/7535007/diff/20003/ppapi/c/private/ppb_flash_t...
File ppapi/c/private/ppb_flash_tcp_socket.h (right):

http://codereview.chromium.org/7535007/diff/20003/ppapi/c/private/ppb_flash_t...
ppapi/c/private/ppb_flash_tcp_socket.h:52: // handshake.
Please document |port|.  You can say it is only used to
identify an SSL server in the SSL session cache.

[yzshen1, 2011-08-15 23:09:32]

Thanks, Wan-Teh!

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:136: void
SSLHandshake(const std::string& host, uint16_t port);
Done. Thanks!

On 2011/08/15 19:35:12, wtc wrote:
> When a proxy server is used, you should clarify whether
> host/port refers to the proxy server or the destination
> server.  This applies to both the Connect and SSLHandshake
> methods.
> 
> It may be better to rename the arguments
> for SSLHandshake to server_name/server_port, if the
> host/port arguments for Connect may refer to the proxy
> server.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:149: // There is a
request of initiating SSL that is pending.
Ah, thanks!

On 2011/08/15 19:35:12, wtc wrote:
> This comment should say:
>   // There is an SSL handshake request that is pending.
> or
>   // An SSL handshake is in progress.
> 
> And the comment on line 153 should say:
>   // An SSL handshake has failed.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:237: net::SSLConfig
ssl_config_;
On 2011/08/15 19:35:12, wtc wrote:
> Please add a comment to note that you are using the default
> SSL configuration settings, as opposed to Chrome's SSL settings.

Done.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:333:
OnSSLHandshakeCompleted(net::ERR_UNEXPECTED);
On 2011/08/15 19:35:12, wtc wrote:
> Please log an error message because net::ERR_UNEXPECTED
> doesn't give us useful information.

Done.

http://codereview.chromium.org/7535007/diff/20003/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:459: connection_state_ =
succeeded ? SSL_CONNECTED : INITIATE_SSL_FAILED;
Yes! Sorry I missed one pattern when searching and replacing.
On 2011/08/15 19:35:12, wtc wrote:
> Should INITIATE_SSL_IN_PROGRESS and INITIATE_SSL_FAILED be
> renamed SSL_HANDSHAKE_IN_PROGRESS and SSL_HANDSHAKE_FAILED?

http://codereview.chromium.org/7535007/diff/20003/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/7535007/diff/20003/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:92: // any client socket pool, can be
destroyed safely.
On 2011/08/15 19:35:12, wtc wrote:
> Please move this comment before this unit test, at line 69.
> 
> You can also create an SSLClientSocket and call its Connect
> method, as is done on lines 111-127 below (you can omit the
> CapturingNetLog code).
> 
> Alternatively, you can use a mock TCPClientSocket as opposed
> to a real TCPClientSocket.  Then you don't need to start up
> the TestServer.

Done. Thanks!

http://codereview.chromium.org/7535007/diff/20003/ppapi/c/private/ppb_flash_t...
File ppapi/c/private/ppb_flash_tcp_socket.h (right):

http://codereview.chromium.org/7535007/diff/20003/ppapi/c/private/ppb_flash_t...
ppapi/c/private/ppb_flash_tcp_socket.h:52: // handshake.
On 2011/08/15 19:35:12, wtc wrote:
> Please document |port|.  You can say it is only used to
> identify an SSL server in the SSL session cache.

Done.

[wtc, 2011-08-15 23:17:23]

http://codereview.chromium.org/7535007/diff/16010/ppapi/cpp/private/flash_tcp...
File ppapi/cpp/private/flash_tcp_socket.h (right):

http://codereview.chromium.org/7535007/diff/16010/ppapi/cpp/private/flash_tcp...
ppapi/cpp/private/flash_tcp_socket.h:35: uint16_t server_port,
In our face-to-face discussion, you told me that proxy servers
are not used.

Given that, I suggested that both Connect and SSLHandshake
should use the same argument names, because they refer to
the same thing.

So I think the arguments should be named 'host' and 'port'
for both Connect and SSLHandshake, to imply that they are
the same.

[wtc, 2011-08-15 23:18:03]

You should also update the Description of the CL to reflect
the new function name.  Thanks.

[wtc, 2011-08-15 23:58:11]

LGTM!  (I reviewed Patch Set 6.)

http://codereview.chromium.org/7535007/diff/16010/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/7535007/diff/16010/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:743: }
Add
  EXPECT_EQ(net::OK, rv);

[commit-bot: I haz the power, 2011-08-16 19:32:32]

Change committed as 97005