Add SSL wrapper for linux and mac

Add SSL wrapper for linux and mac.  This allows notifier to use chrome's SSL layer instead of OpenSSL.

BUG=none
TEST=none


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

[ncarter (slow), 2009-10-16 04:09:33]

Pretty nifty.

Can you think of a way to unit test this?

After you address the issues here (which are mostly focused at improving the
readability of this code -- it took me a while to figure out what these
components were up to) please add a  Chrome network stack expert as a second
reviewer.  I mostly reviewed this from the perspective of libjingle's socket
adapter framework, which is what I'm more familiar with.  From that perspective,
it looks reasonable.

http://codereview.chromium.org/270074/diff/9003/8004
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/270074/diff/9003/8004#newcode14
Line 14: int MapPosixError(int err) {
Put this in an unnamed namespace.

http://codereview.chromium.org/270074/diff/9003/8005
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h (right):

http://codereview.chromium.org/270074/diff/9003/8005#newcode20
Line 20: class TransportSocket : public net::ClientSocket, public
sigslot::has_slots<> {
Please add class comments including a paragraph about what this object does. 
(What does it transport, to where?)

http://codereview.chromium.org/270074/diff/9003/8005#newcode29
Line 29: virtual int Connect(net::CompletionCallback* callback);
This block of 5 virtual methods should start with a comment:

// net::ClientSocket implementation

http://codereview.chromium.org/270074/diff/9003/8005#newcode38
Line 38: virtual int Read(net::IOBuffer* buf, int buf_len,
// net::Socket implementation

http://codereview.chromium.org/270074/diff/9003/8005#newcode46
Line 46: virtual void OnConnectEvent(talk_base::AsyncSocket * socket);
I don't think these four need to be virtual.  They're just callback handlers,
right?

http://codereview.chromium.org/270074/diff/9003/8005#newcode61
Line 61: };
DISALLOW_COPY...

http://codereview.chromium.org/270074/diff/9003/8005#newcode63
Line 63: class SSLSocketAdapter : public talk_base::AsyncSocketAdapter {
Add class comments.  I'd find it useful to say that SSLSocketAdapter provides an
SSL interface to libjingle that's backed by Chrome's SSL implementation.  Please
also state the relationship between this type and TransportSocket.

http://codereview.chromium.org/270074/diff/9003/8005#newcode101
Line 101: case STATE_WRITE_COMPLETE:
Add a default:

http://codereview.chromium.org/270074/diff/9003/8005#newcode115
Line 115: int data_transfered_;
"transferred" has a antepenultimate double-r.

http://codereview.chromium.org/270074/diff/9003/8005#newcode117
Line 117: enum {
Give this enum type a name ("State" seems to be conventional among the various
net::Sockets) and make state_ use the enum type.  Also, enums belong at the
beginning of the section.

http://codereview.chromium.org/270074/diff/9003/8005#newcode126
Line 126: };
DISALLOW_COPY...

http://codereview.chromium.org/270074/diff/9003/8006
File chrome/browser/sync/notifier/communicator/xmpp_socket_adapter.cc (right):

http://codereview.chromium.org/270074/diff/9003/8006#newcode12
Line 12: #if OS_LINUX || OS_MAC
Wrap these in defined()  (elsewhere in this file too)

http://codereview.chromium.org/270074/diff/9003/8006#newcode140
Line 140: #if OS_WIN
defined()

http://codereview.chromium.org/270074/diff/9003/8006#newcode332
Line 332: #if OS_WIN
defined(OS_WIN)

http://codereview.chromium.org/270074/diff/9003/8006#newcode334
Line 334: static_cast<talk_base::SSLAdapter*>(socket_);
I think these assignments should be indented 4 spaces.

[zork, 2009-10-16 22:02:04]

Fixed these style issues.  I'll look into replicating unittests used on the
interfaces there are made to emulate.
-Zach

On Thu, Oct 15, 2009 at 9:09 PM, <nick@chromium.org> wrote:

> Pretty nifty.
>
> Can you think of a way to unit test this?
>
> After you address the issues here (which are mostly focused at improving
> the
> readability of this code -- it took me a while to figure out what these
> components were up to) please add a  Chrome network stack expert as a
> second
> reviewer.  I mostly reviewed this from the perspective of libjingle's
> socket
> adapter framework, which is what I'm more familiar with.  From that
> perspective,
> it looks reasonable.
>
>
> http://codereview.chromium.org/270074/diff/9003/8004
> File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.cc
> (right):
>
> http://codereview.chromium.org/270074/diff/9003/8004#newcode14
> Line 14: int MapPosixError(int err) {
> Put this in an unnamed namespace.
>
> http://codereview.chromium.org/270074/diff/9003/8005
> File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h
> (right):
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode20
> Line 20: class TransportSocket : public net::ClientSocket, public
> sigslot::has_slots<> {
> Please add class comments including a paragraph about what this object
> does.  (What does it transport, to where?)
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode29
> Line 29: virtual int Connect(net::CompletionCallback* callback);
> This block of 5 virtual methods should start with a comment:
>
> // net::ClientSocket implementation
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode38
> Line 38: virtual int Read(net::IOBuffer* buf, int buf_len,
> // net::Socket implementation
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode46
> Line 46: virtual void OnConnectEvent(talk_base::AsyncSocket * socket);
> I don't think these four need to be virtual.  They're just callback
> handlers, right?
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode61
> Line 61: };
> DISALLOW_COPY...
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode63
> Line 63: class SSLSocketAdapter : public talk_base::AsyncSocketAdapter {
> Add class comments.  I'd find it useful to say that SSLSocketAdapter
> provides an SSL interface to libjingle that's backed by Chrome's SSL
> implementation.  Please also state the relationship between this type
> and TransportSocket.
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode101
> Line 101: case STATE_WRITE_COMPLETE:
> Add a default:
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode115
> Line 115: int data_transfered_;
> "transferred" has a antepenultimate double-r.
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode117
> Line 117: enum {
> Give this enum type a name ("State" seems to be conventional among the
> various net::Sockets) and make state_ use the enum type.  Also, enums
> belong at the beginning of the section.
>
> http://codereview.chromium.org/270074/diff/9003/8005#newcode126
> Line 126: };
> DISALLOW_COPY...
>
> http://codereview.chromium.org/270074/diff/9003/8006
> File chrome/browser/sync/notifier/communicator/xmpp_socket_adapter.cc
> (right):
>
> http://codereview.chromium.org/270074/diff/9003/8006#newcode12
> Line 12: #if OS_LINUX || OS_MAC
> Wrap these in defined()  (elsewhere in this file too)
>
> http://codereview.chromium.org/270074/diff/9003/8006#newcode140
> Line 140: #if OS_WIN
> defined()
>
> http://codereview.chromium.org/270074/diff/9003/8006#newcode332
> Line 332: #if OS_WIN
> defined(OS_WIN)
>
> http://codereview.chromium.org/270074/diff/9003/8006#newcode334
> Line 334: static_cast<talk_base::SSLAdapter*>(socket_);
> I think these assignments should be indented 4 spaces.
>
>
> http://codereview.chromium.org/270074
>

[wtc, 2009-10-22 20:28:12]

Zach, please see my review comments below.

http://codereview.chromium.org/270074/diff/15001/15003
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/270074/diff/15001/15003#newcode8
Line 8: #include "net/base/io_buffer.h"
Nit: io_buffer.h and net_errors.h are already included by
ssl_socket_adapter.h.

http://codereview.chromium.org/270074/diff/15001/15003#newcode15
Line 15: namespace {
Nit: add a blank line after this line.

http://codereview.chromium.org/270074/diff/15001/15003#newcode69
Line 69: net::SSLConfig ssl_config;
Rather than setting the members of ssl_config to hardcoded
values, you should use SSLConfigService to get the values
of these configuration settings.

http://codereview.chromium.org/270074/diff/15001/15003#newcode71
Line 71: ssl_config.ssl3_enabled = false;
If you need to use hardcoded values, please set
ssl3_enabled to true.

http://codereview.chromium.org/270074/diff/15001/15003#newcode75
Line 75: net::ClientSocketFactory::GetDefaultFactory()->CreateSSLClientSocket(
Nit: should this be indented by 4 instead of 2?

http://codereview.chromium.org/270074/diff/15001/15003#newcode80
Line 80: if (result == net::ERR_IO_PENDING || result == 0) {
Nit: compare 'result' with net::OK instead of 0.

Why do you return 0 on net::ERR_IO_PENDING?  Connect
may return net::ERR_IO_PENDING, and then fail asynchronously
later.

http://codereview.chromium.org/270074/diff/15001/15003#newcode90
Line 90: } else {
Nit: don't use 'else' after a return statement.  This is
less of an issue for the short if-else at lines 80-84, but
here, it'll save you the need the indent the long switch
statement.

Note: all of my comments about the Send method also apply to
the Recv method below.

http://codereview.chromium.org/270074/diff/15001/15003#newcode92
Line 92: case STATE_NONE:
Please follow the example in the style guide on how to
place the braces {} for a case in a switch statement.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/270074/diff/15001/15003#newcode99
Line 99: transport_buf_ = NULL;
You should also set transport_buf_ to NULL if 'result' is
a real error (result is < 0 and not equal to net::ERR_IO_PENDING).  You can do
this like this:

  if (result == net::ERR_IO_PENDING) {
    state_ = STATE_WRITE;
    SetError(EWOULDBLOCK);
  } else {
    transport_buf_ = NULL;
  }

http://codereview.chromium.org/270074/diff/15001/15003#newcode115
Line 115: return -1;
-1 is the value of net::ERR_IO_PENDING.  Will this cause
confusion?

http://codereview.chromium.org/270074/diff/15001/15003#newcode120
Line 120: return -1;
Same question here.

http://codereview.chromium.org/270074/diff/15001/15003#newcode189
Line 189: // TODO(zork): Is Idle?
IsIdle means there is no received but unread data on the
socket.

http://codereview.chromium.org/270074/diff/15001/15003#newcode209
Line 209: read_buffer_ = buf;
If you change read_buffer_ to a scoped_refptr as I suggested,
you need to set read_buffer_ to NULL when you no longer need
that reference to the net::IOBuffer.

http://codereview.chromium.org/270074/diff/15001/15003#newcode229
Line 229: // TODO(zork): Implement.
These two methods only affect performance, so it's okay to
not implement them.

http://codereview.chromium.org/270074/diff/15001/15003#newcode254
Line 254: int result = socket_->Recv(read_buffer_->data(), read_buffer_len_);
Why do we call socket_->Recv here, but not call
socket_->Send in OnWriteEvent below?

http://codereview.chromium.org/270074/diff/15001/15004
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h (right):

http://codereview.chromium.org/270074/diff/15001/15004#newcode24
Line 24: class TransportSocket : public net::ClientSocket, public
sigslot::has_slots<> {
Just curious: what's the sigslot::has_slots<> base class
used for?

http://codereview.chromium.org/270074/diff/15001/15004#newcode26
Line 26: TransportSocket(talk_base::AsyncSocket* socket,
Nit: use a consistent placement of * in your code.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Pointer_and_Re...

In Chromium, it's more common put the * next to the type,
but you may have a local convention in Sync.

http://codereview.chromium.org/270074/diff/15001/15004#newcode29
Line 29: void SetAddr(const talk_base::SocketAddress& addr) {
Nit: should we use a lowercase name set_addr for this getter?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

http://codereview.chromium.org/270074/diff/15001/15004#newcode63
Line 63: net::IOBuffer* read_buffer_;
Since net::IOBuffer is a reference-counted type, we should
always store a pointer to net::IOBuffer in a scoped_refptr.

http://codereview.chromium.org/270074/diff/15001/15004#newcode83
Line 83: // StartSSL returns 0 if successful.
Please document the following:

What does StartSSL return on failure?

What does the 'restartable' argument do?

http://codereview.chromium.org/270074/diff/15001/15004#newcode88
Line 88: // Create the default SSL adapter for this platform
Nit: missing period (.).

http://codereview.chromium.org/270074/diff/15001/15004#newcode91
Line 91: virtual int Send(const void *pv, size_t cb);
Nit: Our style guide discourages short parameter names like pv
and cb.  So I'd use longer names like buf and len instead.

http://codereview.chromium.org/270074/diff/15001/15004#newcode103
Line 103: void OnConnected(int result) {
Nit: It seems that OnConnected and OnIO should be defined
in the .cc file.

http://codereview.chromium.org/270074/diff/15001/15004#newcode139
Line 139: friend class TransportSocket;
Nit: move this friend class declaration to the beginning
part of the 'private' section.  Although friend declarations
are not addressed by the style guide, I treat them as
"Typedefs and Enums".

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/270074/diff/15001/15005
File chrome/browser/sync/notifier/communicator/xmpp_socket_adapter.cc (right):

http://codereview.chromium.org/270074/diff/15001/15005#newcode13
Line 13: #include
"chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h"
Why do we use ssl_socket_adapter on Linux and Mac but
ssladapter.h on Windows?

http://codereview.chromium.org/270074/diff/15001/15005#newcode20
Line 20: #if OS_WIN
Use #if defined(OS_WIN).

http://codereview.chromium.org/270074/diff/15001/15002
File chrome/chrome.gyp (right):

http://codereview.chromium.org/270074/diff/15001/15002#newcode2969
Line 2969: 'browser/gtk/sync_setup_wizard_gtk.cc',
Some of the changes to chrome.gyp, such as the sync setup
wizard, seem unrelated to the SSL socket adapter.  Are you
sure they belong in this CL?

[Zachary Kuznia, 2009-10-23 17:43:06]

http://codereview.chromium.org/270074/diff/15001/15003
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/270074/diff/15001/15003#newcode8
Line 8: #include "net/base/io_buffer.h"
On 2009/10/22 20:28:12, wtc wrote:
> Nit: io_buffer.h and net_errors.h are already included by
> ssl_socket_adapter.h.

Done.

http://codereview.chromium.org/270074/diff/15001/15003#newcode15
Line 15: namespace {
On 2009/10/22 20:28:12, wtc wrote:
> Nit: add a blank line after this line.

Done.

http://codereview.chromium.org/270074/diff/15001/15003#newcode69
Line 69: net::SSLConfig ssl_config;
On 2009/10/22 20:28:12, wtc wrote:
> Rather than setting the members of ssl_config to hardcoded
> values, you should use SSLConfigService to get the values
> of these configuration settings.

Done.

http://codereview.chromium.org/270074/diff/15001/15003#newcode75
Line 75: net::ClientSocketFactory::GetDefaultFactory()->CreateSSLClientSocket(
On 2009/10/22 20:28:12, wtc wrote:
> Nit: should this be indented by 4 instead of 2?

Done.

http://codereview.chromium.org/270074/diff/15001/15003#newcode80
Line 80: if (result == net::ERR_IO_PENDING || result == 0) {
On 2009/10/22 20:28:12, wtc wrote:
> Nit: compare 'result' with net::OK instead of 0.
> 
> Why do you return 0 on net::ERR_IO_PENDING?  Connect
> may return net::ERR_IO_PENDING, and then fail asynchronously
> later.

This interface is expected to return success when it gets EWOULDBLOCK, and then
the caller checks back later.

http://codereview.chromium.org/270074/diff/15001/15003#newcode90
Line 90: } else {
On 2009/10/22 20:28:12, wtc wrote:
> Nit: don't use 'else' after a return statement.  This is
> less of an issue for the short if-else at lines 80-84, but
> here, it'll save you the need the indent the long switch
> statement.
> 
> Note: all of my comments about the Send method also apply to
> the Recv method below.

Done.

http://codereview.chromium.org/270074/diff/15001/15003#newcode92
Line 92: case STATE_NONE:
On 2009/10/22 20:28:12, wtc wrote:
> Please follow the example in the style guide on how to
> place the braces {} for a case in a switch statement.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

Done.

http://codereview.chromium.org/270074/diff/15001/15003#newcode99
Line 99: transport_buf_ = NULL;
On 2009/10/22 20:28:12, wtc wrote:
> You should also set transport_buf_ to NULL if 'result' is
> a real error (result is < 0 and not equal to net::ERR_IO_PENDING).  You can do
> this like this:
> 
>   if (result == net::ERR_IO_PENDING) {
>     state_ = STATE_WRITE;
>     SetError(EWOULDBLOCK);
>   } else {
>     transport_buf_ = NULL;
>   }

Done.

http://codereview.chromium.org/270074/diff/15001/15003#newcode115
Line 115: return -1;
On 2009/10/22 20:28:12, wtc wrote:
> -1 is the value of net::ERR_IO_PENDING.  Will this cause
> confusion?

This interface is just expected to return a negative value on error.  To
determine the actually error, it calls GetError().

http://codereview.chromium.org/270074/diff/15001/15003#newcode189
Line 189: // TODO(zork): Is Idle?
On 2009/10/22 20:28:12, wtc wrote:
> IsIdle means there is no received but unread data on the
> socket.

I've never seen this reached, is this function required on this interface?

http://codereview.chromium.org/270074/diff/15001/15003#newcode209
Line 209: read_buffer_ = buf;
On 2009/10/22 20:28:12, wtc wrote:
> If you change read_buffer_ to a scoped_refptr as I suggested,
> you need to set read_buffer_ to NULL when you no longer need
> that reference to the net::IOBuffer.

Done.

http://codereview.chromium.org/270074/diff/15001/15003#newcode229
Line 229: // TODO(zork): Implement.
On 2009/10/22 20:28:12, wtc wrote:
> These two methods only affect performance, so it's okay to
> not implement them.

Ok, removed the NOTREACHED()

http://codereview.chromium.org/270074/diff/15001/15003#newcode254
Line 254: int result = socket_->Recv(read_buffer_->data(), read_buffer_len_);
On 2009/10/22 20:28:12, wtc wrote:
> Why do we call socket_->Recv here, but not call
> socket_->Send in OnWriteEvent below?

Added the call to Send() below.

http://codereview.chromium.org/270074/diff/15001/15004
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h (right):

http://codereview.chromium.org/270074/diff/15001/15004#newcode24
Line 24: class TransportSocket : public net::ClientSocket, public
sigslot::has_slots<> {
This is how libjingle hooks up callbacks.

On 2009/10/22 20:28:12, wtc wrote:
> Just curious: what's the sigslot::has_slots<> base class
> used for?

http://codereview.chromium.org/270074/diff/15001/15004#newcode26
Line 26: TransportSocket(talk_base::AsyncSocket* socket,
Fixed to proper Chromium style.

On 2009/10/22 20:28:12, wtc wrote:
> Nit: use a consistent placement of * in your code.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Pointer_and_Re...
> 
> In Chromium, it's more common put the * next to the type,
> but you may have a local convention in Sync.

http://codereview.chromium.org/270074/diff/15001/15004#newcode29
Line 29: void SetAddr(const talk_base::SocketAddress& addr) {
Formatted correctly.

On 2009/10/22 20:28:12, wtc wrote:
> Nit: should we use a lowercase name set_addr for this getter?
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

http://codereview.chromium.org/270074/diff/15001/15004#newcode63
Line 63: net::IOBuffer* read_buffer_;
Fixed the type on this.

On 2009/10/22 20:28:12, wtc wrote:
> Since net::IOBuffer is a reference-counted type, we should
> always store a pointer to net::IOBuffer in a scoped_refptr.

http://codereview.chromium.org/270074/diff/15001/15004#newcode83
Line 83: // StartSSL returns 0 if successful.
Done.

On 2009/10/22 20:28:12, wtc wrote:
> Please document the following:
> 
> What does StartSSL return on failure?
> 
> What does the 'restartable' argument do?

http://codereview.chromium.org/270074/diff/15001/15004#newcode91
Line 91: virtual int Send(const void *pv, size_t cb);
Renamed all these.

On 2009/10/22 20:28:12, wtc wrote:
> Nit: Our style guide discourages short parameter names like pv
> and cb.  So I'd use longer names like buf and len instead.

http://codereview.chromium.org/270074/diff/15001/15004#newcode103
Line 103: void OnConnected(int result) {
Moved these into the cc file.

On 2009/10/22 20:28:12, wtc wrote:
> Nit: It seems that OnConnected and OnIO should be defined
> in the .cc file.

http://codereview.chromium.org/270074/diff/15001/15004#newcode139
Line 139: friend class TransportSocket;
Done.

On 2009/10/22 20:28:12, wtc wrote:
> Nit: move this friend class declaration to the beginning
> part of the 'private' section.  Although friend declarations
> are not addressed by the style guide, I treat them as
> "Typedefs and Enums".
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/270074/diff/15001/15005
File chrome/browser/sync/notifier/communicator/xmpp_socket_adapter.cc (right):

http://codereview.chromium.org/270074/diff/15001/15005#newcode13
Line 13: #include
"chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h"
On 2009/10/22 20:28:12, wtc wrote:
> Why do we use ssl_socket_adapter on Linux and Mac but
> ssladapter.h on Windows?

Ideally, we could use this interface on all three platforms, but the current
implementation on Windows works perfectly fine, so I'm not going to switch it
over until this interface is complete and thoroughly tested.

http://codereview.chromium.org/270074/diff/15001/15005#newcode20
Line 20: #if OS_WIN
On 2009/10/22 20:28:12, wtc wrote:
> Use #if defined(OS_WIN).

Done.

http://codereview.chromium.org/270074/diff/15001/15002
File chrome/chrome.gyp (right):

http://codereview.chromium.org/270074/diff/15001/15002#newcode2969
Line 2969: 'browser/gtk/sync_setup_wizard_gtk.cc',
On 2009/10/22 20:28:12, wtc wrote:
> Some of the changes to chrome.gyp, such as the sync setup
> wizard, seem unrelated to the SSL socket adapter.  Are you
> sure they belong in this CL?

The only two lines in this file that belong in this CL are the ones that add in
ssl_socket_adapater.cc/h.  The rest gets picked up from patching in other CLs
that allow the Linux version of sync to build.

[wtc, 2009-10-23 19:42:47]

LGTM.  Please check in this CL after fixing the nits below.
I don't need to review it again.

http://codereview.chromium.org/270074/diff/15001/15003
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/270074/diff/15001/15003#newcode189
Line 189: // TODO(zork): Is Idle?
On 2009/10/23 17:43:06, zork wrote:
>
> I've never seen this reached, is this function required on this interface?

IsConnectedAndIdle is required if the client socket will
be used by HTTP.  (IsConnectedAndIdle is used by HTTP's
keep-alive connection code to test if a keep-alive
connection can still be reused.)

Since your client socket won't be used by HTTP, it should
be OK to not implement IsConnectedAndIdle.

http://codereview.chromium.org/270074/diff/22004/22006#newcode10
Line 10: #include "net/base/ssl_config_service.h"
Nit: list this after net/socket/client_socket_factory.h.

http://codereview.chromium.org/270074/diff/22004/22006#newcode108
Line 108: state_ = STATE_NONE;
Note: you don't need to use braces {} for cases that don't
declare local variables.  So I'd only use braces for the
STATE_NONE case here and in the Recv method below.

http://codereview.chromium.org/270074/diff/22004/22006#newcode194
Line 194: #if defined(OS_LINUX) || defined(OS_MACOSX)
Let's initialize read_buffer_len_ and write_buffer_len_ to 0
for safety.

http://codereview.chromium.org/270074/diff/22004/22006#newcode268
Line 268: void TransportSocket::OnWriteEvent(talk_base::AsyncSocket * socket) {
Add a "Not supported" or "Not implemented" comment to these
two methods to explain why they simply return false.

http://codereview.chromium.org/270074/diff/22004/22006#newcode294
Line 294: 
Let's also reset read_buffer_len_ to 0 for safety.

http://codereview.chromium.org/270074/diff/22004/22006#newcode320
Line 320: 
Let's also reset write_buffer_len_ to 0 for safety.

http://codereview.chromium.org/270074/diff/22004/22006#newcode322
Line 322: 
Change write_buffer_->data() to buffer->data() and
write_buffer_len_ to buffer_len because you just set
write_buffer_ to NULL on the previous line.

This error means this line probably isn't executed often
or at all in practice.

http://codereview.chromium.org/270074/diff/22004/22007
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h (right):

http://codereview.chromium.org/270074/diff/22004/22007#newcode95
Line 95: virtual int Send(const void* buf, size_t len);
If the original 'pv' and 'cb' parameter names come from the
talk_base::AsyncSocketAdapter class, then it's OK to use
them here.

[wtc, 2009-10-23 19:47:54]

http://codereview.chromium.org/270074/diff/22004/22007
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.h (right):

http://codereview.chromium.org/270074/diff/22004/22007#newcode89
Line 89: // restartable is not implemented.
Document that restartable must be false.