Add a new GetNegotiatedProtocol method to StreamSocket and implement in all subclasses.

Add a new GetNegotiatedProtocol method to StreamSocket and implement in all subclasses.

BUG=120890
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130388

[Ryan Hamilton, 2012-03-30 19:29:53]

wtc: as we discussed, here is the second CL which adds a new
GetNegotiatedProtocol method to Stream socket.
rsleevi: FYI

[wtc, 2012-03-30 21:19:14]

Patch set 2 LGTM.

Note the bug in ssl_server_socket_nss.*.

We should remove the redundant SSLClientSocket::protocol_negotiated()
method.

http://codereview.chromium.org/9958028/diff/23/net/socket/socks5_client_socke...
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/socks5_client_socke...
net/socket/socks5_client_socket.cc:164: 

Nit: remove one blank line.

http://codereview.chromium.org/9958028/diff/23/net/socket/socks_client_socket.cc
File net/socket/socks_client_socket.cc (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/socks_client_socket...
net/socket/socks_client_socket.cc:184: return kProtoUnknown;

I wonder if we should call transport_->socket()->GetNegotiatedProtocol()
here.

I guess we can wait until it is known to be necessary.

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_client_socket.cc
File net/socket/ssl_client_socket.cc (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_client_socket.c...
net/socket/ssl_client_socket.cc:139: return
static_cast<NextProto>(protocol_negotiated());
Remove static_cast because protocol_negotiated() returns
NextProto.

We should remove the protocol_negotiated() method.

Nit: this method should be defined right after GetSSLCertRequestInfo
to match the declaration order in the .h file.

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_client_socket.h...
net/socket/ssl_client_socket.h:91: virtual NextProto GetNegotiatedProtocol()
const OVERRIDE;

Add a
  // StreamSocket:
comment here.

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_server_socket_n...
File net/socket/ssl_server_socket_nss.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_server_socket_n...
net/socket/ssl_server_socket_nss.h:64: virtual NextProto GetNegotiatedProtocol()
const OVERRIDE;

BUG: I believe SSLServerSocketNSS should simply use the
definition of GetNegotiatedProtocol() in SSLClientSocket.

http://codereview.chromium.org/9958028/diff/23/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/stream_socket.h#new...
net/socket/stream_socket.h:90: // Returns the protocol negotiated for this
socket.

Please mention the use of NPN in this comment.

Please document what the return value is if NPN is not
applicable.

http://codereview.chromium.org/9958028/diff/23/net/socket/tcp_client_socket_l...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/tcp_client_socket_l...
net/socket/tcp_client_socket_libevent.cc:555: NextProto
TCPClientSocketLibevent::GetNegotiatedProtocol() const {

Nit: define this after the GetConnectTimeMicros() method.

http://codereview.chromium.org/9958028/diff/23/net/socket/tcp_client_socket_l...
File net/socket/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/tcp_client_socket_l...
net/socket/tcp_client_socket_libevent.h:71: virtual NextProto
GetNegotiatedProtocol() const OVERRIDE;

Move this up to the StreamSocket implementation section.

[wtc, 2012-03-30 21:50:08]

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_server_socket_n...
File net/socket/ssl_server_socket_nss.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_server_socket_n...
net/socket/ssl_server_socket_nss.h:64: virtual NextProto GetNegotiatedProtocol()
const OVERRIDE;

On 2012/03/30 21:19:14, wtc wrote:
> 
> BUG: I believe SSLServerSocketNSS should simply use the
> definition of GetNegotiatedProtocol() in SSLClientSocket.

I was confused.  I thought this was the SSLClientSocketNSS
class.

SSLServerSocketNSS doesn't support NPN yet, so its
GetNegotiatedProtocol() method should simply return
kProtoUnknown.

[Ryan Hamilton, 2012-03-30 21:53:16]

Thanks for the super quick review!

http://codereview.chromium.org/9958028/diff/23/net/socket/socks5_client_socke...
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/socks5_client_socke...
net/socket/socks5_client_socket.cc:164: 
On 2012/03/30 21:19:14, wtc wrote:
> 
> Nit: remove one blank line.

Done.

http://codereview.chromium.org/9958028/diff/23/net/socket/socks_client_socket.cc
File net/socket/socks_client_socket.cc (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/socks_client_socket...
net/socket/socks_client_socket.cc:184: return kProtoUnknown;
On 2012/03/30 21:19:14, wtc wrote:
> 
> I wonder if we should call transport_->socket()->GetNegotiatedProtocol()
> here.

Done.

> I guess we can wait until it is known to be necessary.

Nah, I think we might as well do it now.  Delegating to the transport socket is
correct.  (It's always on top of a TCP socket which returns kProtoUnknown)

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_client_socket.cc
File net/socket/ssl_client_socket.cc (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_client_socket.c...
net/socket/ssl_client_socket.cc:139: return
static_cast<NextProto>(protocol_negotiated());
On 2012/03/30 21:19:14, wtc wrote:
> Remove static_cast because protocol_negotiated() returns
> NextProto.

Done.

> We should remove the protocol_negotiated() method.

Done.
 
> Nit: this method should be defined right after GetSSLCertRequestInfo
> to match the declaration order in the .h file.

GetSSLCertRequetInfo is pure virtual but I moved this new method to the
appropriate place in the file.

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_client_socket.h...
net/socket/ssl_client_socket.h:91: virtual NextProto GetNegotiatedProtocol()
const OVERRIDE;
On 2012/03/30 21:19:14, wtc wrote:
> 
> Add a
>   // StreamSocket:
> comment here.

Done.

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_server_socket_n...
File net/socket/ssl_server_socket_nss.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/ssl_server_socket_n...
net/socket/ssl_server_socket_nss.h:64: virtual NextProto GetNegotiatedProtocol()
const OVERRIDE;
On 2012/03/30 21:19:14, wtc wrote:
> 
> BUG: I believe SSLServerSocketNSS should simply use the
> definition of GetNegotiatedProtocol() in SSLClientSocket.

But this is a server socket which does not inherit from SSLClientSocket, so it
is not able to use that definition.  It looks like NPN is not supported by this
class, so I changed the definition to return kProtoUnknown and added a comment.

http://codereview.chromium.org/9958028/diff/23/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/stream_socket.h#new...
net/socket/stream_socket.h:90: // Returns the protocol negotiated for this
socket.
On 2012/03/30 21:19:14, wtc wrote:
> 
> Please mention the use of NPN in this comment.
> 
> Please document what the return value is if NPN is not
> applicable.

Done.

http://codereview.chromium.org/9958028/diff/23/net/socket/tcp_client_socket_l...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/tcp_client_socket_l...
net/socket/tcp_client_socket_libevent.cc:555: NextProto
TCPClientSocketLibevent::GetNegotiatedProtocol() const {
On 2012/03/30 21:19:14, wtc wrote:
> 
> Nit: define this after the GetConnectTimeMicros() method.

Done.

http://codereview.chromium.org/9958028/diff/23/net/socket/tcp_client_socket_l...
File net/socket/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/9958028/diff/23/net/socket/tcp_client_socket_l...
net/socket/tcp_client_socket_libevent.h:71: virtual NextProto
GetNegotiatedProtocol() const OVERRIDE;
On 2012/03/30 21:19:14, wtc wrote:
> 
> Move this up to the StreamSocket implementation section.

Whoops, done.

[commit-bot: I haz the power, 2012-03-30 21:56:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958028/7003

[commit-bot: I haz the power, 2012-03-30 21:56:55]

Can't apply patch for file net/socket/socket_test_util.cc.
While running patch -p1 --forward --force;
patching file net/socket/socket_test_util.cc
Hunk #1 FAILED at 1166.
1 out of 1 hunk FAILED -- saving rejects to file
net/socket/socket_test_util.cc.rej

[commit-bot: I haz the power, 2012-03-31 04:11:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958028/7003

[commit-bot: I haz the power, 2012-03-31 04:16:52]

Can't apply patch for file net/socket/socket_test_util.cc.
While running patch -p1 --forward --force;
patching file net/socket/socket_test_util.cc
Hunk #1 FAILED at 1166.
1 out of 1 hunk FAILED -- saving rejects to file
net/socket/socket_test_util.cc.rej

[commit-bot: I haz the power, 2012-03-31 15:34:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958028/7003

[commit-bot: I haz the power, 2012-03-31 15:43:49]

Try job failure for 9958028-7003 (retry) on android for steps "Compile, build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-03-31 16:28:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958028/1137

[commit-bot: I haz the power, 2012-03-31 16:47:24]

Try job failure for 9958028-1137 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Ryan Hamilton, 2012-04-01 04:31:14]

hclam: this CL ended up touch a few files outside of net/  Can you take a look? 
It's a simple CL that just adds a new mostly trivial method to a number of
StreamSocket subclasses.

[Sergey Ulanov, 2012-04-02 18:20:22]

LGTM, but see my comments. I think it's better to avoid adding more stuff of
StreamSocket interface when it is possible.

http://codereview.chromium.org/9958028/diff/7099/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

http://codereview.chromium.org/9958028/diff/7099/net/socket/stream_socket.h#n...
net/socket/stream_socket.h:90: // Returns the protocol negotiated via for this
socket, or
via NPN?

http://codereview.chromium.org/9958028/diff/7099/net/socket/stream_socket.h#n...
net/socket/stream_socket.h:91: // kProtoUnknown will be returned if NPN is not
applicable.
I think ideally this shouldn't be part of StreamSocket interface. Can we make a
separate interface called NegotiatingStreamSocket  or NpnStreamSocket that would
inherit from StreamSocket and add this method? That way you wouldn't have to fix
every single implementation of StreamSocket interface.

[Ryan Hamilton, 2012-04-02 20:57:32]

Thanks for the quick review.

http://codereview.chromium.org/9958028/diff/7099/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

http://codereview.chromium.org/9958028/diff/7099/net/socket/stream_socket.h#n...
net/socket/stream_socket.h:90: // Returns the protocol negotiated via for this
socket, or
On 2012/04/02 18:20:22, sergeyu wrote:
> via NPN?

Done.

http://codereview.chromium.org/9958028/diff/7099/net/socket/stream_socket.h#n...
net/socket/stream_socket.h:91: // kProtoUnknown will be returned if NPN is not
applicable.
On 2012/04/02 18:20:22, sergeyu wrote:
> I think ideally this shouldn't be part of StreamSocket interface. Can we make
a
> separate interface called NegotiatingStreamSocket  or NpnStreamSocket that
would
> inherit from StreamSocket and add this method? That way you wouldn't have to
fix
> every single implementation of StreamSocket interface. 

Yeah, I totally hear you.  The problem comes up because SPDY sessions can be
layered on top of pretty much any kind of socket.  SSL sockets and HTTP proxy
sockets both have the ability to negotiate the SPDY protocol.  However, SPDY
(via command line flags) can also live on top of, for example, a TCP socket.

This seems to imply that pretty much any StreamSocket subclass that SPDY session
can be layered on would need to extend this new class.  But that would be
roughly all of them :| 

It's definitely a thorny issue and we talked about it quite a bit internally.  I
also just chatted with wtc again after reading your comment.  We wish there were
a cleaner solution, but feel that this CL is about as good as we're going to
get.

[commit-bot: I haz the power, 2012-04-02 21:00:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958028/6063

[commit-bot: I haz the power, 2012-04-02 23:47:15]

Try job failure for 9958028-6063 (retry) on win_rel for step "browser_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-03 01:35:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958028/6063

[commit-bot: I haz the power, 2012-04-03 03:15:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958028/6063

[commit-bot: I haz the power, 2012-04-03 06:06:58]

Try job failure for 9958028-6063 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-03 06:34:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958028/6063

[commit-bot: I haz the power, 2012-04-03 09:34:03]

Try job failure for 9958028-6063 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "ui_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...