Add functionality to pppapi TCPSocket to support secure sockets in flash

This adds the following to functions to the ppapi TCPSocket interface:
1) GetServer certificate, which returns the server X509Certificate if an SSL connection has been established.
2) AddChainBuilding certificate. This is currently unimplemented in Chrome but the interface and plumbing has been added so it can easily be hooked up. This should add a trusted/untrusted chain building certificate to be used by the client for a particular connection when performing the SSL handshake.

BUG=114626
TEST=out/Release/browser_tests --gtest_filter=*PPAPITest.*TCP*Trusted*


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

[raymes, 2012-04-04 22:58:45]

(Note this is the final CL in the series).

[Ryan Sleevi, 2012-04-06 00:37:49]

http://codereview.chromium.org/9699100/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_socket.cc (right):

http://codereview.chromium.org/9699100/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_socket.cc:291:
CHECK(ssl_info.cert.get());
Note that this can fail for a variety of reasons. The most common is that a
certificate has a validity date range that exceeds the systems' clock
representation - eg: 2038 for 32-bit time_t (OpenSSL) or earlier than 1601
(Windows).

I have a bug open to go back through and 'gracefully' handle these. So I'd
encourage you not to CHECK() here and think about how you might gracefully
handle this.

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.h (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.h:38: X509Certificate
GetServerCertificate();
bool GetServerCertificate(X509Certificate* cert);

avoid the extra copy and allow things to fail.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:14:
PPB_X509Certificate_Fields::PPB_X509Certificate_Fields() {
nit: Same line for empty ctors when they fit.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:145: return PP_FALSE;
Is this correct? I thought the caller supplied a byte array, rather than a
certificate handle. How are they expected to get such a resource handle? I'm
assuming |resource| should be an ArrayBufferVar?

Note, I would also suggest that you take the ArrayBufferVar and sanity check it
by going ArrayBufferVar to X509Certificate to ArrayBufferVar. You want to make
sure that |resource| is a valid, parsable certificate before returning PP_TRUE.
If you just stored the raw bytes as is, the caller wouldn't have any signals
whether or not |resource| was a valid cert.

Of course, since this is all not implemented, it's not something you can test
right now ;)

http://codereview.chromium.org/9699100/diff/15001/ppapi/thunk/ppb_tcp_socket_...
File ppapi/thunk/ppb_tcp_socket_private_api.h (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/thunk/ppb_tcp_socket_...
ppapi/thunk/ppb_tcp_socket_private_api.h:28: virtual PP_Resource
GetServerCertificate() = 0;
See previous comments

[yzshen1, 2012-04-06 19:24:33]

https://chromiumcodereview.appspot.com/9699100/diff/15001/content/browser/ren...
File content/browser/renderer_host/pepper_message_filter.h (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/content/browser/ren...
content/browser/renderer_host/pepper_message_filter.h:140: const
std::vector<std::vector<char> >& trusted_certs,
include vector, please.

https://chromiumcodereview.appspot.com/9699100/diff/15001/content/browser/ren...
File content/browser/renderer_host/pepper_tcp_socket.cc (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/content/browser/ren...
content/browser/renderer_host/pepper_tcp_socket.cc:285: CHECK(socket_.get());
You don't need this CHECK, it will die below if it is NULL, right? (And also
line 291).

https://chromiumcodereview.appspot.com/9699100/diff/15001/content/browser/ren...
content/browser/renderer_host/pepper_tcp_socket.cc:294: CHECK(success);
If it failed, do we have to crash the whole browser?

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/api/private/p...
File ppapi/api/private/ppb_tcp_socket_private.idl (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/api/private/p...
ppapi/api/private/ppb_tcp_socket_private.idl:12: M19 = 0.4
M20?

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/api/private/p...
ppapi/api/private/ppb_tcp_socket_private.idl:81: * Returns the server's
<code>PPB_Flash_X509Certificate</code> for a socket
Please update the name to PPB_X509Certificate_Private.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/api/private/p...
ppapi/api/private/ppb_tcp_socket_private.idl:83: * <code>SSL_Handshake</code>.
If no SSL connection has been established, a
No underscore in the middle.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/api/private/p...
ppapi/api/private/ppb_tcp_socket_private.idl:92: * Add a trusted/untrusted chain
building certificate to be used for this
Add -> Adds.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/api/private/p...
ppapi/api/private/ppb_tcp_socket_private.idl:93: * connection. The
<code>PP_Resource</code> must be a
PP_Resource -> certificate, please.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/api/private/p...
ppapi/api/private/ppb_tcp_socket_private.idl:94: *
<code>PPB_Flash_X509Certificate<code>. <code>PP_TRUE</code> is returned
Please update the name to PPB_X509Certificate_Private.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
File ppapi/cpp/private/tcp_socket_private.cc (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
ppapi/cpp/private/tcp_socket_private.cc:18: template <> const char*
interface_name<PPB_TCPSocket_Private>() {
Please explicitly add 0_4 for this one.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
ppapi/cpp/private/tcp_socket_private.cc:19: return
PPB_TCPSOCKET_PRIVATE_INTERFACE;
ditto.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
ppapi/cpp/private/tcp_socket_private.cc:30:
has_interface<PPB_TCPSocket_Private_0_3>()) && instance.pp_instance()) {
I used to be told to not validate instance here. Because the implementation will
do that. We would like to keep the cpp wrapper simple.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
ppapi/cpp/private/tcp_socket_private.cc:103: pp_resource(), server_name,
server_port,
wrong indent. (and also line 108)

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
ppapi/cpp/private/tcp_socket_private.cc:115: if
(has_interface<PPB_TCPSocket_Private>())
Use {} for more-than-one-line body, please. (And please fix all if blocks that
don't follow this rule in the file.)

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
ppapi/cpp/private/tcp_socket_private.cc:117:
get_interface<PPB_TCPSocket_Private>()->GetServerCertificate(
The handling of ref count here is probably incorrect. GetServerCertificate() is
supposed to add 1 ref for the caller, right?
Here you should use X509Certificate(PASS_REF, balh).

Please be careful about ref counting, it is hard to find out later.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
File ppapi/cpp/private/tcp_socket_private.h (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/cpp/private/t...
ppapi/cpp/private/tcp_socket_private.h:38: X509Certificate
GetServerCertificate();
Actually, you should have 'Private' in the name of X509Certificate cpp wrapper.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc:124: {
PPB_TCPSOCKET_PRIVATE_INTERFACE_0_4,
Wrong indent, and line 127.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/proxy/ppb_tcp...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/proxy/ppb_tcp...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:21: class
PPB_X509Certificate_Fields;
I think you should put this in the .h file and move the include here. :)

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/proxy/ppb_tcp...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:45: const std::string& server_name,
wrong indent.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/proxy/ppb_tcp...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:49: OVERRIDE;
put OVERRIDE on the same line as the last parameter.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/proxy/ppb_tcp...
File ppapi/proxy/ppb_tcp_socket_private_proxy.h (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/proxy/ppb_tcp...
ppapi/proxy/ppb_tcp_socket_private_proxy.h:49: const PPB_X509Certificate_Fields&
certificate_fields);
You could forward declare this class, right?

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/shared_impl/p...
File ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:19: for (size_t
i = 0; i < fields.values_.GetSize(); ++i) {
[nit, optional] Can you use
virtual ListValue* DeepCopy() const OVERRIDE;
and then swap with values_?

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:100: :
Resource(type, instance),
I think the convention is to *not* add 4 more spaces in this case.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/shared_impl/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:102: {
And move it to the end of the previous line.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/shared_impl/p...
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/shared_impl/p...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:139: return PP_FALSE;
Will you get warning on some compilers for this? You have code  that can never
be reached.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/shared_impl/p...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:141:
ppapi::thunk::PPB_X509Certificate_Private_API* resource =
You could use thunk::EnterResourceNoLock?

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/shared_impl/p...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:147: PP_Var der_var =
resource->GetField(PP_X509CERTIFICATE_PRIVATE_RAW);
You are leaking PP_Var reference, right? Please be careful about PP_Resource and
PP_Var references.

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/tests/test_tc...
File ppapi/tests/test_tcp_socket_private_trusted.cc (right):

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/tests/test_tc...
ppapi/tests/test_tcp_socket_private_trusted.cc:7: #include <stdlib.h>
Why you need this one?

https://chromiumcodereview.appspot.com/9699100/diff/15001/ppapi/tests/test_tc...
ppapi/tests/test_tcp_socket_private_trusted.cc:60: ASSERT_EQ(cert.GetField(
[nit, optional]
ASSERT_EQ(
    cert.GetField(...).AsString(),
    "Test CA");
will be more clear.

[raymes, 2012-04-06 23:22:47]

http://codereview.chromium.org/9699100/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9699100/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.h:140: const
std::vector<std::vector<char> >& trusted_certs,
On 2012/04/06 19:24:33, yzshen1 wrote:
> include vector, please.

Done.

http://codereview.chromium.org/9699100/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_tcp_socket.cc (right):

http://codereview.chromium.org/9699100/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_socket.cc:285: CHECK(socket_.get());
On 2012/04/06 19:24:33, yzshen1 wrote:
> You don't need this CHECK, it will die below if it is NULL, right? (And also
> line 291).

Done.

http://codereview.chromium.org/9699100/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_socket.cc:291:
CHECK(ssl_info.cert.get());
On 2012/04/06 00:37:49, Ryan Sleevi wrote:
> Note that this can fail for a variety of reasons. The most common is that a
> certificate has a validity date range that exceeds the systems' clock
> representation - eg: 2038 for 32-bit time_t (OpenSSL) or earlier than 1601
> (Windows).
> 
> I have a bug open to go back through and 'gracefully' handle these. So I'd
> encourage you not to CHECK() here and think about how you might gracefully
> handle this.

Done.

http://codereview.chromium.org/9699100/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/pepper_tcp_socket.cc:294: CHECK(success);
On 2012/04/06 19:24:33, yzshen1 wrote:
> If it failed, do we have to crash the whole browser?

Probably not. If we can't correctly parse the cert, it's probably best to just
return an empty cert for now. Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/api/private/ppb_tcp_s...
File ppapi/api/private/ppb_tcp_socket_private.idl (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_socket_private.idl:12: M19 = 0.4
On 2012/04/06 19:24:33, yzshen1 wrote:
> M20?

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_socket_private.idl:81: * Returns the server's
<code>PPB_Flash_X509Certificate</code> for a socket
On 2012/04/06 19:24:33, yzshen1 wrote:
> Please update the name to PPB_X509Certificate_Private.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_socket_private.idl:83: * <code>SSL_Handshake</code>.
If no SSL connection has been established, a
On 2012/04/06 19:24:33, yzshen1 wrote:
> No underscore in the middle.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_socket_private.idl:92: * Add a trusted/untrusted chain
building certificate to be used for this
On 2012/04/06 19:24:33, yzshen1 wrote:
> Add -> Adds.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_socket_private.idl:93: * connection. The
<code>PP_Resource</code> must be a
On 2012/04/06 19:24:33, yzshen1 wrote:
> PP_Resource -> certificate, please.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/api/private/ppb_tcp_s...
ppapi/api/private/ppb_tcp_socket_private.idl:94: *
<code>PPB_Flash_X509Certificate<code>. <code>PP_TRUE</code> is returned
On 2012/04/06 19:24:33, yzshen1 wrote:
> Please update the name to PPB_X509Certificate_Private.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:18: template <> const char*
interface_name<PPB_TCPSocket_Private>() {
On 2012/04/06 19:24:33, yzshen1 wrote:
> Please explicitly add 0_4 for this one.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:19: return
PPB_TCPSOCKET_PRIVATE_INTERFACE;
On 2012/04/06 19:24:33, yzshen1 wrote:
> ditto.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:30:
has_interface<PPB_TCPSocket_Private_0_3>()) && instance.pp_instance()) {
On 2012/04/06 19:24:33, yzshen1 wrote:
> I used to be told to not validate instance here. Because the implementation
will
> do that. We would like to keep the cpp wrapper simple.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:103: pp_resource(), server_name,
server_port,
On 2012/04/06 19:24:33, yzshen1 wrote:
> wrong indent. (and also line 108)

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:115: if
(has_interface<PPB_TCPSocket_Private>())
On 2012/04/06 19:24:33, yzshen1 wrote:
> Use {} for more-than-one-line body, please. (And please fix all if blocks that
> don't follow this rule in the file.)

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:117:
get_interface<PPB_TCPSocket_Private>()->GetServerCertificate(
On 2012/04/06 19:24:33, yzshen1 wrote:
> The handling of ref count here is probably incorrect. GetServerCertificate()
is
> supposed to add 1 ref for the caller, right?
> Here you should use X509Certificate(PASS_REF, balh).
> 
> Please be careful about ref counting, it is hard to find out later.

Thanks, I actually didn't know the regular constructor incremented.

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.h (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.h:38: X509Certificate
GetServerCertificate();
On 2012/04/06 00:37:49, Ryan Sleevi wrote:
> bool GetServerCertificate(X509Certificate* cert);
> 
> avoid the extra copy and allow things to fail.

A copy to the out param would still have to happen. A null resource is returned
to indicate failure.

http://codereview.chromium.org/9699100/diff/15001/ppapi/native_client/src/sha...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb.cc:124: {
PPB_TCPSOCKET_PRIVATE_INTERFACE_0_4,
On 2012/04/06 19:24:33, yzshen1 wrote:
> Wrong indent, and line 127.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.cc (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:21: class
PPB_X509Certificate_Fields;
On 2012/04/06 19:24:33, yzshen1 wrote:
> I think you should put this in the .h file and move the include here. :)

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:45: const std::string& server_name,
On 2012/04/06 19:24:33, yzshen1 wrote:
> wrong indent.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.cc:49: OVERRIDE;
On 2012/04/06 19:24:33, yzshen1 wrote:
> put OVERRIDE on the same line as the last parameter.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/proxy/ppb_tcp_socket_...
File ppapi/proxy/ppb_tcp_socket_private_proxy.h (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/proxy/ppb_tcp_socket_...
ppapi/proxy/ppb_tcp_socket_private_proxy.h:49: const PPB_X509Certificate_Fields&
certificate_fields);
On 2012/04/06 19:24:33, yzshen1 wrote:
> You could forward declare this class, right?

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/p...
File ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:14:
PPB_X509Certificate_Fields::PPB_X509Certificate_Fields() {
On 2012/04/06 00:37:49, Ryan Sleevi wrote:
> nit: Same line for empty ctors when they fit.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:19: for (size_t
i = 0; i < fields.values_.GetSize(); ++i) {
On 2012/04/06 19:24:33, yzshen1 wrote:
> [nit, optional] Can you use
> virtual ListValue* DeepCopy() const OVERRIDE;
> and then swap with values_?

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:100: :
Resource(type, instance),
On 2012/04/06 19:24:33, yzshen1 wrote:
> I think the convention is to *not* add 4 more spaces in this case.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/p...
ppapi/shared_impl/private/ppb_x509_certificate_private_shared.cc:102: {
On 2012/04/06 19:24:33, yzshen1 wrote:
> And move it to the end of the previous line.

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:139: return PP_FALSE;
On 2012/04/06 19:24:33, yzshen1 wrote:
> Will you get warning on some compilers for this? You have code  that can never
> be reached.
I don't see a warning about this.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:141:
ppapi::thunk::PPB_X509Certificate_Private_API* resource =
On 2012/04/06 19:24:33, yzshen1 wrote:
> You could use thunk::EnterResourceNoLock?

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:145: return PP_FALSE;
This is exactly what happens already. The caller first has to initialize a Cert
resource with Initialize() which verifies that it is valid. Then they pass the
resource handle of the cert to this function which keeps the bytes only.

On 2012/04/06 00:37:49, Ryan Sleevi wrote:
> Is this correct? I thought the caller supplied a byte array, rather than a
> certificate handle. How are they expected to get such a resource handle? I'm
> assuming |resource| should be an ArrayBufferVar?
> 
> Note, I would also suggest that you take the ArrayBufferVar and sanity check
it
> by going ArrayBufferVar to X509Certificate to ArrayBufferVar. You want to make
> sure that |resource| is a valid, parsable certificate before returning
PP_TRUE.
> If you just stored the raw bytes as is, the caller wouldn't have any signals
> whether or not |resource| was a valid cert.
> 
> Of course, since this is all not implemented, it's not something you can test
> right now ;)

http://codereview.chromium.org/9699100/diff/15001/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:147: PP_Var der_var =
resource->GetField(PP_X509CERTIFICATE_PRIVATE_RAW);
On 2012/04/06 19:24:33, yzshen1 wrote:
> You are leaking PP_Var reference, right? Please be careful about PP_Resource
and
> PP_Var references.

Sorry, this is bad. I think I've fixed it in the correct way. It would be nice
to have a wrapper that does this when it goes out of scope.

http://codereview.chromium.org/9699100/diff/15001/ppapi/tests/test_tcp_socket...
File ppapi/tests/test_tcp_socket_private_trusted.cc (right):

http://codereview.chromium.org/9699100/diff/15001/ppapi/tests/test_tcp_socket...
ppapi/tests/test_tcp_socket_private_trusted.cc:7: #include <stdlib.h>
On 2012/04/06 19:24:33, yzshen1 wrote:
> Why you need this one?

Done.

http://codereview.chromium.org/9699100/diff/15001/ppapi/tests/test_tcp_socket...
ppapi/tests/test_tcp_socket_private_trusted.cc:60: ASSERT_EQ(cert.GetField(
On 2012/04/06 19:24:33, yzshen1 wrote:
> [nit, optional]
> ASSERT_EQ(
>     cert.GetField(...).AsString(),
>     "Test CA");
> will be more clear.

Done.

[yzshen1, 2012-04-09 17:55:41]

Only a few nits left.
Thanks!

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (right):

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:29: if
((has_interface<PPB_TCPSocket_Private>() ||
Please change all PPB_TCPSocket_Private to PPB_TCPSocket_Private_0_4.

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:31:
PassRefFromConstructor(get_interface<PPB_TCPSocket_Private>()->Create(
And you need to handle 0.4 and 0.3 separately, just as you did for Connect(),
for example.

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/x509_cert...
File ppapi/cpp/private/x509_certificate_private.cc (right):

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/x509_cert...
ppapi/cpp/private/x509_certificate_private.cc:7: #include "ppapi/cpp/pass_ref.h"
Haha! Sort, please! :)

http://codereview.chromium.org/9699100/diff/23008/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

http://codereview.chromium.org/9699100/diff/23008/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:143:
enter_config(certificate, true);
This is not 'config'. Maybe you could call it enter or enter_cert?

[yzshen1, 2012-04-09 17:55:53]

Only a few nits left.
Thanks!

[raymes, 2012-04-09 19:35:50]

Thanks for the review!

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/tcp_socke...
File ppapi/cpp/private/tcp_socket_private.cc (right):

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:29: if
((has_interface<PPB_TCPSocket_Private>() ||
On 2012/04/09 17:55:42, yzshen1 wrote:
> Please change all PPB_TCPSocket_Private to PPB_TCPSocket_Private_0_4.

Done.

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/tcp_socke...
ppapi/cpp/private/tcp_socket_private.cc:31:
PassRefFromConstructor(get_interface<PPB_TCPSocket_Private>()->Create(
On 2012/04/09 17:55:42, yzshen1 wrote:
> And you need to handle 0.4 and 0.3 separately, just as you did for Connect(),
> for example.

Done.

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/x509_cert...
File ppapi/cpp/private/x509_certificate_private.cc (right):

http://codereview.chromium.org/9699100/diff/23008/ppapi/cpp/private/x509_cert...
ppapi/cpp/private/x509_certificate_private.cc:7: #include "ppapi/cpp/pass_ref.h"
On 2012/04/09 17:55:42, yzshen1 wrote:
> Haha! Sort, please! :)

Done.

http://codereview.chromium.org/9699100/diff/23008/ppapi/shared_impl/private/t...
File ppapi/shared_impl/private/tcp_socket_private_impl.cc (right):

http://codereview.chromium.org/9699100/diff/23008/ppapi/shared_impl/private/t...
ppapi/shared_impl/private/tcp_socket_private_impl.cc:143:
enter_config(certificate, true);
On 2012/04/09 17:55:42, yzshen1 wrote:
> This is not 'config'. Maybe you could call it enter or enter_cert?

Done.

[yzshen1, 2012-04-09 20:25:06]

lgtm

[raymes, 2012-04-09 21:25:13]

+OWNERS
brettw: 
content/*
chrome/test/ui/ppapi_uitest.cc
webkit/plugins/ppapi

[brettw, 2012-04-11 20:24:22]

lgtm

[commit-bot: I haz the power, 2012-04-12 00:05:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/9699100/37002

[commit-bot: I haz the power, 2012-04-12 03:38:24]

Change committed as 131918