Change next_proto member type.

Change next_proto member type.

Change type of next_proto members of HttpNetworkTransaction and SSLConfig from
vector<string> to NextProtoVector.  This offers better type safety and clarity.
Also, when SSLClientConfig needs to remove HTTP/2 protocols due to inadequate
security, it would feel silly having to convert strings back to NextProto.  It
just makes sense to postpone generating strings until very close to the wire.

BUG=436835
Committed: https://crrev.com/0d23cf415d4d0f7538750db34aa20297ff5aa169
Cr-Commit-Position: refs/heads/master@{#307894}

[Bence, 2014-12-10 17:51:03]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2014-12-10 17:51:03]

Ryan: PTAL.  Thanks.

[Ryan Hamilton, 2014-12-10 20:09:00]

thanks for doing this. seems like a nice cleanup.  Just a few small comments.

https://codereview.chromium.org/794563003/diff/40001/net/base/net_log_util.cc
File net/base/net_log_util.cc (right):

https://codereview.chromium.org/794563003/diff/40001/net/base/net_log_util.cc...
net/base/net_log_util.cc:460: }
How about:

std::string next_protos_string;
for (const NextProto proto : next_protos) {
  if (!next_protos_string.empty()) 
    next_protos_string.append(",")'

  next_protos_string.append(SSLClientSocket::NextProtoToString(proto));
}

https://codereview.chromium.org/794563003/diff/40001/net/base/net_log_util.cc...
net/base/net_log_util.cc:461: status_dict->SetString("next_protos",
next_protos_string);
Alternatively, you could consider using a ListValue instead of a comma separated
string.

https://codereview.chromium.org/794563003/diff/40001/net/http/http_network_se...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/794563003/diff/40001/net/http/http_network_se...
net/http/http_network_session.h:208: void GetNextProtos(NextProtoVector*
next_protos) const;
nit: Can you add a comment which explains that this method actually populates
|next_protos|? (Since it's named Get, my brain thinks it's going to return
something, but of course it uses an out-param instead)

https://codereview.chromium.org/794563003/diff/40001/net/socket/next_proto.cc
File net/socket/next_proto.cc (right):

https://codereview.chromium.org/794563003/diff/40001/net/socket/next_proto.cc...
net/socket/next_proto.cc:5: #include "net/socket/next_proto.h"
nice catch

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:242: for (const auto& it : next_protos) {
Prefer using the actual type to auto if the type is short/simple.

for (const NextProto next_proto : next_protos) {
}


(It's not actually an iterator, right?)

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:260: for (const auto& it : next_proto_strings) {
ditto.

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:263: memcpy(&wire_protos[wire_protos.size() -
it.size()], it.data(), it.size());
This code (which you did not write) looks completely insane :>

Can you add a todo to possibly rewrite this? And in the meantime, can you add a
comment to explain what it's doing?

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.h:215: const NextProtoVector& next_protos);
Can this fit on the previos line? (probably not)

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1786: j !=
ssl_config_.next_protos.end(); ++j) {
for (NextProto next_proto : ssl_config_.next_protos) {

[Bence, 2014-12-10 22:01:24]

Ryan: PTAL.

https://codereview.chromium.org/794563003/diff/40001/net/base/net_log_util.cc
File net/base/net_log_util.cc (right):

https://codereview.chromium.org/794563003/diff/40001/net/base/net_log_util.cc...
net/base/net_log_util.cc:460: }
On 2014/12/10 20:08:59, Ryan Hamilton wrote:
> How about:
> 
> std::string next_protos_string;
> for (const NextProto proto : next_protos) {
>   if (!next_protos_string.empty()) 
>     next_protos_string.append(",")'
> 
>   next_protos_string.append(SSLClientSocket::NextProtoToString(proto));
> }

Done.

https://codereview.chromium.org/794563003/diff/40001/net/http/http_network_se...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/794563003/diff/40001/net/http/http_network_se...
net/http/http_network_session.h:208: void GetNextProtos(NextProtoVector*
next_protos) const;
On 2014/12/10 20:09:00, Ryan Hamilton wrote:
> nit: Can you add a comment which explains that this method actually populates
> |next_protos|? (Since it's named Get, my brain thinks it's going to return
> something, but of course it uses an out-param instead)

Done.

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:242: for (const auto& it : next_protos) {
On 2014/12/10 20:09:00, Ryan Hamilton wrote:
> Prefer using the actual type to auto if the type is short/simple.
> 
> for (const NextProto next_proto : next_protos) {
> }
> 
> 
> (It's not actually an iterator, right?)

Done.

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:260: for (const auto& it : next_proto_strings) {
On 2014/12/10 20:09:00, Ryan Hamilton wrote:
> ditto.

Done.

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:263: memcpy(&wire_protos[wire_protos.size() -
it.size()], it.data(), it.size());
On 2014/12/10 20:09:00, Ryan Hamilton wrote:
> This code (which you did not write) looks completely insane :>
> 
> Can you add a todo to possibly rewrite this? And in the meantime, can you add
a
> comment to explain what it's doing?

Done.

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.h:215: const NextProtoVector& next_protos);
On 2014/12/10 20:09:00, Ryan Hamilton wrote:
> Can this fit on the previos line? (probably not)

Unfortunately not.

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/794563003/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1786: j !=
ssl_config_.next_protos.end(); ++j) {
On 2014/12/10 20:09:00, Ryan Hamilton wrote:
> for (NextProto next_proto : ssl_config_.next_protos) {

Done.

[Ryan Hamilton, 2014-12-10 22:12:40]

lgtm

https://codereview.chromium.org/794563003/diff/100001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/794563003/diff/100001/net/http/http_network_s...
net/http/http_network_session.h:208: // Populates *next_protos with protocols.
nit: put ||s around the name of the parameter.

[Bence, 2014-12-11 12:24:31]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-12-11 12:24:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/794563003/120001

[Bence, 2014-12-11 12:25:09]

Thanks, Ryan.

https://codereview.chromium.org/794563003/diff/100001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/794563003/diff/100001/net/http/http_network_s...
net/http/http_network_session.h:208: // Populates *next_protos with protocols.
On 2014/12/10 22:12:40, Ryan Hamilton wrote:
> nit: put ||s around the name of the parameter.

Done.

[commit-bot: I haz the power, 2014-12-11 14:09:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-11 14:10:37]

Patchset 7 (id:??) landed as
https://crrev.com/0d23cf415d4d0f7538750db34aa20297ff5aa169
Cr-Commit-Position: refs/heads/master@{#307894}