Linux: add next-protocol-negotiation to libssl.

Linux: add next-protocol-negotiation to libssl.

This is an experimental, client only implementation of
next-protocol-negotiation:

http://www.imperialviolet.org/binary/draft-agl-tls-nextprotoneg-00.html

This only affects the internal copy of libssl and is only active when built
with use_system_ssl=0, which is not currently the default.

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

[wtc, 2009-11-20 02:29:08]

Cool!

I'm still reviewing the NSS changes.  Let me send you
my comments on the Chromium changes first.

http://codereview.chromium.org/415005/diff/2001/3001
File base/nss_init.cc (right):

http://codereview.chromium.org/415005/diff/2001/3001#newcode118
Line 118: #if defined(SYSTEM_SSL)
This change isn't really necessary.  It's an optimization
for the not-SYSTEM_SSL case, right?

http://codereview.chromium.org/415005/diff/2001/3002
File build/linux/system.gyp (right):

http://codereview.chromium.org/415005/diff/2001/3002#newcode93
Line 93: 'SYSTEM_SSL',
How about renaming this macro USE_SYSTEM_SSL?

http://codereview.chromium.org/415005/diff/2001/3003
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/415005/diff/2001/3003#newcode61
Line 61: // The list of application level protocols supported. If set, this will
Does the order of the protocols matter?

http://codereview.chromium.org/415005/diff/2001/3003#newcode64
Line 64: std::string next_protos;
Nit: don't abbreviate "protocols".

http://codereview.chromium.org/415005/diff/2001/3004
File net/base/ssl_info.h (right):

http://codereview.chromium.org/415005/diff/2001/3004#newcode17
Line 17: SSLInfo() : cert_status(0), security_bits(-1) { }
Initialize next_proto_status to kNextProtoUnsupported.

http://codereview.chromium.org/415005/diff/2001/3004#newcode47
Line 47: // Next Protocol Negotiation allows a TLS client and server to come to
an
Nit: add "(NPN)".

http://codereview.chromium.org/415005/diff/2001/3004#newcode51
Line 51: kNextProtoUnsupported = 0,  // The server doesn't support NPN.
Nit: don't abbreviate "protocol" in these enum constants and
the class members.

We should give this enum type a name and declare
next_proto_status to be of this enum type rather than int.

http://codereview.chromium.org/415005/diff/2001/3005
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/415005/diff/2001/3005#newcode54
Line 54: ssl_config_.next_protos = "\007http1.1\004spdy";
Should we list spdy first?

http://codereview.chromium.org/415005/diff/2001/3006
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/415005/diff/2001/3006#newcode366
Line 366: if (ssl_config_.next_protos.empty() == false) {
Use !ssl_config_.next_protos.empty().

http://codereview.chromium.org/415005/diff/2001/3006#newcode372
Line 372: LOG(INFO) << "SSL_SetNextProtoNeg failed.";
Should we also say "Old system nss?" here?

http://codereview.chromium.org/415005/diff/2001/3006#newcode586
Line 586: case SSL_NEXT_PROTO_NEGOTIATED:
The cases should be indented.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/415005/diff/2001/3006#newcode594
Line 594: ssl_info->next_proto_status = SSLInfo::kNextProtoNoOverlap;
Nit: add a break statement.

http://codereview.chromium.org/415005/diff/2001/3009
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/415005/diff/2001/3009#newcode143
Line 143: unsigned char buf[255],
Use
  unsigned char *buf,
You can add a length argument:
  unsigned int max_buf_len,
as the last parameter.  Many NSS functions have that kind
of prototype.  See
http://mxr.mozilla.org/security/source/security/nss/lib/freebl/blapi.h#789

[agl, 2009-11-20 22:23:43]

http://codereview.chromium.org/415005/diff/2001/3001
File base/nss_init.cc (right):

http://codereview.chromium.org/415005/diff/2001/3001#newcode118
Line 118: #if defined(SYSTEM_SSL)
On 2009/11/20 02:29:08, wtc wrote:
> This change isn't really necessary.  It's an optimization
> for the not-SYSTEM_SSL case, right?

No, it's required. I'm not sure why I didn't pick up on it before, but with a
statically linked libssl, dlsym will not find any of the symbols.

http://codereview.chromium.org/415005/diff/2001/3002
File build/linux/system.gyp (right):

http://codereview.chromium.org/415005/diff/2001/3002#newcode93
Line 93: 'SYSTEM_SSL',
On 2009/11/20 02:29:08, wtc wrote:
> How about renaming this macro USE_SYSTEM_SSL?

Done.

http://codereview.chromium.org/415005/diff/2001/3003
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/415005/diff/2001/3003#newcode61
Line 61: // The list of application level protocols supported. If set, this will
On 2009/11/20 02:29:08, wtc wrote:
> Does the order of the protocols matter?

Only the first one. Comment updated.

http://codereview.chromium.org/415005/diff/2001/3003#newcode64
Line 64: std::string next_protos;
On 2009/11/20 02:29:08, wtc wrote:
> Nit: don't abbreviate "protocols".

Done.

http://codereview.chromium.org/415005/diff/2001/3004
File net/base/ssl_info.h (right):

http://codereview.chromium.org/415005/diff/2001/3004#newcode17
Line 17: SSLInfo() : cert_status(0), security_bits(-1) { }
On 2009/11/20 02:29:08, wtc wrote:
> Initialize next_proto_status to kNextProtoUnsupported.

Done.

http://codereview.chromium.org/415005/diff/2001/3004#newcode47
Line 47: // Next Protocol Negotiation allows a TLS client and server to come to
an
On 2009/11/20 02:29:08, wtc wrote:
> Nit: add "(NPN)".

Done.

http://codereview.chromium.org/415005/diff/2001/3004#newcode51
Line 51: kNextProtoUnsupported = 0,  // The server doesn't support NPN.
On 2009/11/20 02:29:08, wtc wrote:
> Nit: don't abbreviate "protocol" in these enum constants and
> the class members.
> 
> We should give this enum type a name and declare
> next_proto_status to be of this enum type rather than int.

Done.

http://codereview.chromium.org/415005/diff/2001/3005
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/415005/diff/2001/3005#newcode54
Line 54: ssl_config_.next_protos = "\007http1.1\004spdy";
On 2009/11/20 02:29:08, wtc wrote:
> Should we list spdy first?

No, the reasoning is subtle so I've added a comment.

http://codereview.chromium.org/415005/diff/2001/3006
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/415005/diff/2001/3006#newcode366
Line 366: if (ssl_config_.next_protos.empty() == false) {
On 2009/11/20 02:29:08, wtc wrote:
> Use !ssl_config_.next_protos.empty().

Done.

http://codereview.chromium.org/415005/diff/2001/3006#newcode372
Line 372: LOG(INFO) << "SSL_SetNextProtoNeg failed.";
On 2009/11/20 02:29:08, wtc wrote:
> Should we also say "Old system nss?" here?

No. It makes sense for the calls above because an old NSS might not support some
of the option constants. However, an NSS that doesn't support NPN will cause a
link failure (dynamic or static) since we're using a new symbol.

http://codereview.chromium.org/415005/diff/2001/3006#newcode586
Line 586: case SSL_NEXT_PROTO_NEGOTIATED:
On 2009/11/20 02:29:08, wtc wrote:
> The cases should be indented.  See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

Right sorry. I've a hard time keeping six different styles straight :)

http://codereview.chromium.org/415005/diff/2001/3006#newcode594
Line 594: ssl_info->next_proto_status = SSLInfo::kNextProtoNoOverlap;
On 2009/11/20 02:29:08, wtc wrote:
> Nit: add a break statement.

Done.

http://codereview.chromium.org/415005/diff/2001/3009
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/415005/diff/2001/3009#newcode143
Line 143: unsigned char buf[255],
On 2009/11/20 02:29:08, wtc wrote:
> Use
>   unsigned char *buf,
> You can add a length argument:
>   unsigned int max_buf_len,
> as the last parameter.  Many NSS functions have that kind
> of prototype.  See
> http://mxr.mozilla.org/security/source/security/nss/lib/freebl/blapi.h#789

Done.

[wtc, 2009-11-21 00:57:06]

LGTM.  I found some bugs and have some suggested changes
below.

Re: proto: Please change "protocol" back to "proto" in
the Chromium part of the CL.  Sorry to jerk you around.
I learned that "proto" is a common abbreviation of
"protocol" in networking code.

http://codereview.chromium.org/415005/diff/6001/7005
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/415005/diff/6001/7005#newcode60
Line 60: ssl_config_.next_protocols = "\007http1.1\004spdy";
How about
  HTTP/1.1
(note the '/') and
  SPDY
?

http://codereview.chromium.org/415005/diff/6001/7008
File net/third_party/nss/ssl/ssl.def (right):

http://codereview.chromium.org/415005/diff/6001/7008#newcode144
Line 144: SSL_SetNextProtoNeg;
Nit: list the functions in alphabetical order.

http://codereview.chromium.org/415005/diff/6001/7009
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/415005/diff/6001/7009#newcode138
Line 138: SSL_IMPORT SECStatus SSL_SetNextProtoNeg(PRFileDesc *fd,
Let's use "Nego" or "Negotiation" in the function name.
"Nego" seems to be a common abbreviation of "negotiation",
as in SPNEGO.

http://codereview.chromium.org/415005/diff/6001/7009#newcode144
Line 144: unsigned *length,
Nit: rename this parameter buf_len.

http://codereview.chromium.org/415005/diff/6001/7010
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/415005/diff/6001/7010#newcode7826
Line 7826: return rv;	/* error code set by AppendHandshake */
Should this be ssl3_AppendHandshakeHeader?

http://codereview.chromium.org/415005/diff/6001/7010#newcode9118
Line 9118: PORT_Free(ss->ssl3.nextProto.data);
Let's also set ss->ssl3.nextProto.data to NULL after
freeing it.

http://codereview.chromium.org/415005/diff/6001/7011
File net/third_party/nss/ssl/ssl3ext.c (right):

http://codereview.chromium.org/415005/diff/6001/7011#newcode437
Line 437: size_t offset = 0;
Let's declare 'offset' and the 'i', 'j' in the next function
as unsigned int.  size_t is not commonly used in NSS.

http://codereview.chromium.org/415005/diff/6001/7011#newcode474
Line 474: if (data->data[i] == ss->opt.nextProtoNeg.data[j]) {
Nit: use && instead of a nested if.

http://codereview.chromium.org/415005/diff/6001/7011#newcode491
Line 491: ss->ssl3.nextProtoState = SSL_NEXT_PROTO_NO_OVERLAP;
If we get here because of the "goto pick_first" above,
ss->ssl3.nextProtoState should be set to SSL_NEXT_PROTO_NEGOTIATED.

ss->ssl3.nextProtoState should be set to
SSL_NEXT_PROTO_NO_OVERLAP only if we get here because
the nested for loop above finishes without finding a
match.

http://codereview.chromium.org/415005/diff/6001/7011#newcode512
Line 512: return 0;
This line is indented too much.

http://codereview.chromium.org/415005/diff/6001/7013
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/415005/diff/6001/7013#newcode343
Line 343: SECItem      nextProtoNeg;
Nit: Neg => Negotiation

Let's add this SECItem to the beginning of the sslOptions
structure, so we can blindly add new bit fields at the end of
the struct.

But perhaps it's better to add this SECItem to the
sslSocket structure, as a peer of its
    sslOptions       opt;
member, so that sslOptions contains only bit fields.

http://codereview.chromium.org/415005/diff/6001/7013#newcode828
Line 828: char		nextProtoState;	/* See NEXT_PROTO_* defines */
NEXT_PROTO_* => SSL_NEXT_PROTO_*

We may want to declare nextProtoState as an 'int' to
match the "int *state" parameter of SSL_GetNextProto.

http://codereview.chromium.org/415005/diff/6001/7014
File net/third_party/nss/ssl/sslsock.c (right):

http://codereview.chromium.org/415005/diff/6001/7014#newcode1257
Line 1257: SECStatus  rv = SECSuccess;
Delete 'rv', not used.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1269
Line 1269: ss->opt.nextProtoNeg.data = (unsigned char *) data;
Let's also set
  ss->opt.nextProtoNeg.type = siBuffer;

Who's going to free ss->opt.nextProtoNeg.data?

http://codereview.chromium.org/415005/diff/6001/7014#newcode1287
Line 1287: unsigned *length, unsigned max_buf_len)
Nit: unsigned => unsigned int

In NSS we usually don't omit 'int'.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1291
Line 1291: unsigned bytesToCopy;
Delete 'rv' and 'bytesToCopy' because they're not used.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1301
Line 1301: if (ss->ssl3.nextProtoState && ss->ssl3.nextProtoState) {
Nit: change the first ss->ssl3.nextProtoState to
    ss->ssl3.nextProtoState != SSL_NEXT_PROTO_NO_SUPPORT

Bug: I believe the second ss->ssl3.nextProtoState should be
changed to
    ss->ssl3.nextProto.data
or removed.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1302
Line 1302: *length = ss->ssl3.nextProtoState;
Bug: this should be
    ss->ssl3.nextProto.len;
                       ^^^

http://codereview.chromium.org/415005/diff/6001/7014#newcode1307
Line 1307: *length = 0;
If we get here because ss->ssl3.nextProto.data is NULL
(assuming the second ss->ssl3.nextProtoState above is
changed to ss->ssl3.nextProto.data), should we set an
error code (PORT_SetError(SEC_ERROR_INVALID_ARGS))
and return SECFailure?

[agl, 2009-11-23 21:50:06]

http://codereview.chromium.org/415005/diff/6001/7005
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/415005/diff/6001/7005#newcode60
net/flip/flip_session.cc:60: ssl_config_.next_protocols = "\007http1.1\004spdy";
On 2009/11/21 00:57:06, wtc wrote:
> How about
>   HTTP/1.1
> (note the '/') and
>   SPDY
> ?

These strings are inherently arbitrary and it's more work than I care to do to
change them now.

http://codereview.chromium.org/415005/diff/6001/7008
File net/third_party/nss/ssl/ssl.def (right):

http://codereview.chromium.org/415005/diff/6001/7008#newcode144
net/third_party/nss/ssl/ssl.def:144: SSL_SetNextProtoNeg;
On 2009/11/21 00:57:06, wtc wrote:
> Nit: list the functions in alphabetical order.

Done.

http://codereview.chromium.org/415005/diff/6001/7009
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/415005/diff/6001/7009#newcode138
net/third_party/nss/ssl/ssl.h:138: SSL_IMPORT SECStatus
SSL_SetNextProtoNeg(PRFileDesc *fd,
On 2009/11/21 00:57:06, wtc wrote:
> Let's use "Nego" or "Negotiation" in the function name.
> "Nego" seems to be a common abbreviation of "negotiation",
> as in SPNEGO.

Switching to Nego throughout.

http://codereview.chromium.org/415005/diff/6001/7009#newcode144
net/third_party/nss/ssl/ssl.h:144: unsigned *length,
On 2009/11/21 00:57:06, wtc wrote:
> Nit: rename this parameter buf_len.

Done.

http://codereview.chromium.org/415005/diff/6001/7010
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/415005/diff/6001/7010#newcode7826
net/third_party/nss/ssl/ssl3con.c:7826: return rv;	/* error code set by
AppendHandshake */
On 2009/11/21 00:57:06, wtc wrote:
> Should this be ssl3_AppendHandshakeHeader?

Done.

http://codereview.chromium.org/415005/diff/6001/7010#newcode9118
net/third_party/nss/ssl/ssl3con.c:9118: PORT_Free(ss->ssl3.nextProto.data);
On 2009/11/21 00:57:06, wtc wrote:
> Let's also set ss->ssl3.nextProto.data to NULL after
> freeing it.

Done.

http://codereview.chromium.org/415005/diff/6001/7011
File net/third_party/nss/ssl/ssl3ext.c (right):

http://codereview.chromium.org/415005/diff/6001/7011#newcode437
net/third_party/nss/ssl/ssl3ext.c:437: size_t offset = 0;
On 2009/11/21 00:57:06, wtc wrote:
> Let's declare 'offset' and the 'i', 'j' in the next function
> as unsigned int.  size_t is not commonly used in NSS.

Done.

http://codereview.chromium.org/415005/diff/6001/7011#newcode474
net/third_party/nss/ssl/ssl3ext.c:474: if (data->data[i] ==
ss->opt.nextProtoNeg.data[j]) {
On 2009/11/21 00:57:06, wtc wrote:
> Nit: use && instead of a nested if.

Done.

http://codereview.chromium.org/415005/diff/6001/7011#newcode491
net/third_party/nss/ssl/ssl3ext.c:491: ss->ssl3.nextProtoState =
SSL_NEXT_PROTO_NO_OVERLAP;
On 2009/11/21 00:57:06, wtc wrote:
> If we get here because of the "goto pick_first" above,
> ss->ssl3.nextProtoState should be set to SSL_NEXT_PROTO_NEGOTIATED.

NO_OVERLAP means that we're requesting a protocol that the server didn't
explicitly support. So this is correct.

http://codereview.chromium.org/415005/diff/6001/7011#newcode512
net/third_party/nss/ssl/ssl3ext.c:512: return 0;
On 2009/11/21 00:57:06, wtc wrote:
> This line is indented too much.

Done.

http://codereview.chromium.org/415005/diff/6001/7013
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/415005/diff/6001/7013#newcode343
net/third_party/nss/ssl/sslimpl.h:343: SECItem      nextProtoNeg;
On 2009/11/21 00:57:06, wtc wrote:
> Nit: Neg => Negotiation

Changed to Nego

> Let's add this SECItem to the beginning of the sslOptions
> structure, so we can blindly add new bit fields at the end of
> the struct.

Done

> But perhaps it's better to add this SECItem to the
> sslSocket structure, as a peer of its
>     sslOptions       opt;
> member, so that sslOptions contains only bit fields.

It seems that sslOptions should contain the options, irrespective of their
representation. If there was an array of sslOptions etc, then the packing would
be important, but since it's only including in a single place it makes no
difference to the generated code.

http://codereview.chromium.org/415005/diff/6001/7013#newcode828
net/third_party/nss/ssl/sslimpl.h:828: char		nextProtoState;	/* See NEXT_PROTO_*
defines */
On 2009/11/21 00:57:06, wtc wrote:
> NEXT_PROTO_* => SSL_NEXT_PROTO_*
> 
> We may want to declare nextProtoState as an 'int' to
> match the "int *state" parameter of SSL_GetNextProto.

Done.

http://codereview.chromium.org/415005/diff/6001/7014
File net/third_party/nss/ssl/sslsock.c (right):

http://codereview.chromium.org/415005/diff/6001/7014#newcode1257
net/third_party/nss/ssl/sslsock.c:1257: SECStatus  rv = SECSuccess;
On 2009/11/21 00:57:06, wtc wrote:
> Delete 'rv', not used.

Done.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1269
net/third_party/nss/ssl/sslsock.c:1269: ss->opt.nextProtoNeg.data = (unsigned
char *) data;
On 2009/11/21 00:57:06, wtc wrote:
> Let's also set
>   ss->opt.nextProtoNeg.type = siBuffer;
> 
> Who's going to free ss->opt.nextProtoNeg.data?

Currently we're taking a reference to the provided string, thus we don't free
it.

This works for now but the principle of least surprise suggests that we should
make a copy. Have changed the code to do that.

(Now freed in ssl_DestroySocketContents)

http://codereview.chromium.org/415005/diff/6001/7014#newcode1287
net/third_party/nss/ssl/sslsock.c:1287: unsigned *length, unsigned max_buf_len)
On 2009/11/21 00:57:06, wtc wrote:
> Nit: unsigned => unsigned int
> 
> In NSS we usually don't omit 'int'.

Done.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1291
net/third_party/nss/ssl/sslsock.c:1291: unsigned bytesToCopy;
On 2009/11/21 00:57:06, wtc wrote:
> Delete 'rv' and 'bytesToCopy' because they're not used.

Done.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1301
net/third_party/nss/ssl/sslsock.c:1301: if (ss->ssl3.nextProtoState &&
ss->ssl3.nextProtoState) {
On 2009/11/21 00:57:06, wtc wrote:
> Nit: change the first ss->ssl3.nextProtoState to
>     ss->ssl3.nextProtoState != SSL_NEXT_PROTO_NO_SUPPORT
> 
> Bug: I believe the second ss->ssl3.nextProtoState should be
> changed to
>     ss->ssl3.nextProto.data
> or removed.

Thanks. I cleared totally screwed this function up after redoing from the
previous review. Sorry about that.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1302
net/third_party/nss/ssl/sslsock.c:1302: *length = ss->ssl3.nextProtoState;
On 2009/11/21 00:57:06, wtc wrote:
> Bug: this should be
>     ss->ssl3.nextProto.len;
>                        ^^^

Done.

http://codereview.chromium.org/415005/diff/6001/7014#newcode1307
net/third_party/nss/ssl/sslsock.c:1307: *length = 0;
On 2009/11/21 00:57:06, wtc wrote:
> If we get here because ss->ssl3.nextProto.data is NULL
> (assuming the second ss->ssl3.nextProtoState above is
> changed to ss->ssl3.nextProto.data), should we set an
> error code (PORT_SetError(SEC_ERROR_INVALID_ARGS))
> and return SECFailure?

It should be perfectly valid to call this function when NPN didn't run. Or are
you suggesting testing the case that the state suggests that something should be
here, but we find a NULL pointer? (In the later case, I think it should be an
assert.)

[willchan no longer on Chromium, 2009-11-25 19:48:13]

Drive by review of the chromium code in the changelist.

http://codereview.chromium.org/415005/diff/9001/10003
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/415005/diff/9001/10003#newcode19
net/base/ssl_config_service.h:19: SSLConfig()
I think might be getting to the point where making a .cc file and putting the
constructor and destructor definition there make sense, since the inline
constructor and destructor is starting to get large with the inline constructors
and destructors for the std::vector<> and std::string members.  Perhaps this is
premature optimization.  I leave the choice up to you.

http://codereview.chromium.org/415005/diff/9001/10004
File net/base/ssl_info.h (right):

http://codereview.chromium.org/415005/diff/9001/10004#newcode51
net/base/ssl_info.h:51: enum NextProtoStatus {
enums should go first in the class definition:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/415005/diff/9001/10004#newcode59
net/base/ssl_info.h:59: std::string next_proto; // The negotiated protocol, if
any.
You should #include <string> for this.

http://codereview.chromium.org/415005/diff/9001/10005
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/415005/diff/9001/10005#newcode167
net/flip/flip_session.cc:167: ssl_config_.next_protos = "\007http1.1\004spdy";
I don't think this code makes sense here.  We will only be creating a
FlipSession if (1) we are hardcoded to assume the --testing-fixed-server is a
flip capable server (2) we've already done NPN and have selected Flip.

[agl, 2009-11-30 20:24:53]

http://codereview.chromium.org/415005/diff/9001/10004
File net/base/ssl_info.h (right):

http://codereview.chromium.org/415005/diff/9001/10004#newcode51
net/base/ssl_info.h:51: enum NextProtoStatus {
On 2009/11/25 19:48:13, willchan wrote:
> enums should go first in the class definition:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/415005/diff/9001/10004#newcode59
net/base/ssl_info.h:59: std::string next_proto; // The negotiated protocol, if
any.
On 2009/11/25 19:48:13, willchan wrote:
> You should #include <string> for this.

Done.

http://codereview.chromium.org/415005/diff/9001/10005
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/415005/diff/9001/10005#newcode167
net/flip/flip_session.cc:167: ssl_config_.next_protos = "\007http1.1\004spdy";
On 2009/11/25 19:48:13, willchan wrote:
> I don't think this code makes sense here.

It doesn't make sense here, it's only a placeholder until the actual protocol
negotiation is working. However, it makes the GFE folk's life easier for the
time being.

I've updated the comment to make this clear.

[wtc, 2009-12-01 23:49:23]

Adam,

Do you know why the SVN commit message for this CL doesn't
have a "Review URL" link?

(I've manually closed this code review with a "Committed"
link.)

[agl, 2009-12-01 23:51:41]

On Tue, Dec 1, 2009 at 3:49 PM,  <wtc@chromium.org> wrote:
> Do you know why the SVN commit message for this CL doesn't
> have a "Review URL" link?

Ah nuts. Because I screwed up and didn't put one in I'm afraid.


AGL

[wtc, 2009-12-02 00:01:14]

On 2009/12/01 23:51:41, agl wrote:
>
> Ah nuts. Because I screwed up and didn't put one in I'm afraid.

The "Review URL" link should be inserted by "gcl commit" automatically.
But "gcl commit" should also have closed the code review for you,
which didn't happen.  So I was just wondering if "gcl commit" had
some errors.

[agl, 2009-12-02 00:03:29]

On Tue, Dec 1, 2009 at 4:01 PM,  <wtc@chromium.org> wrote:
> The "Review URL" link should be inserted by "gcl commit" automatically.
> But "gcl commit" should also have closed the code review for you,
> which didn't happen.  So I was just wondering if "gcl commit" had
> some errors.

I don't use gcl (or git-cl).


AGL