Pass chrome version and channel in CHLO message to server.

Pass chrome version and channel in CHLO message to server.

R=wtc@chromium.org, rch@chromium.org, thestig@chromium.org

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

[ramant (doing other things), 2014-06-04 21:30:35]

https://codereview.chromium.org/312193005/diff/1/chrome/browser/net/http_serv...
File chrome/browser/net/http_server_properties_manager.cc (right):

https://codereview.chromium.org/312193005/diff/1/chrome/browser/net/http_serv...
chrome/browser/net/http_server_properties_manager.cc:98: client_version = "C ";
Should we define an enum vs the first letter of channel?

[ramant (doing other things), 2014-06-04 21:47:00]

Hi Wan-Teh,
  Would appreciate if you could take a quick look at this CL. 

  The following is how the data in the packet looks like.

2d2d 2d2d 2d2d 2d2d 2d2d 2d2d 2d2d 2d2d  ----------------
2d2d 6170 6973 2e67 6f6f 676c 652e 636f  --apis.google.co
6d51 3031 3839 9ff9 5340 f7fe c958 3530  mQ0189..S@...X50
3955 2033 372e 302e 3230 3239 2e30       9U.37.0.2029.0

thanks
raman

https://codereview.chromium.org/312193005/diff/1/net/http/http_server_propert...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/312193005/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:203: // Saves the client version. It is passed
in QUIC's CHLO message.
I am not very happy with the comment. Would appreciate a better a comment. Will
update this comment at other places also.

[Ryan Hamilton, 2014-06-04 22:31:15]

Thanks for doing this!

https://codereview.chromium.org/312193005/diff/1/chrome/browser/net/http_serv...
File chrome/browser/net/http_server_properties_manager.cc (right):

https://codereview.chromium.org/312193005/diff/1/chrome/browser/net/http_serv...
chrome/browser/net/http_server_properties_manager.cc:98: client_version = "C ";
On 2014/06/04 21:30:36, ramant wrote:
> Should we define an enum vs the first letter of channel?

We could. On the other hand, the CHLO packet is padded to 1200 bytes, and it's
usually quite empty, so it's not like we're short on space. I've be inclined to
simply use the full channel string. Perhaps via GetVersionStringModifier()?

https://codereview.chromium.org/312193005/diff/1/chrome/browser/net/http_serv...
chrome/browser/net/http_server_properties_manager.cc:113:
client_version.append(version_info.Version());
This doesn't include Chrome, right? It's just 37.0.13.4? I think we need to put
at least Chrome in here, or else we'll get collisions between different browsers
(when we have multiple browsers :>) Perhaps we should use
ProductNameAndVersionForUserAgent()?

https://codereview.chromium.org/312193005/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/312193005/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.h:190: std::string client_version_;
It doesn't seem to me that client_version_ is something that belongs in the
HttpServerProperties objects. Perhaps in the NetworkSession, or in the
QuicStreamFactory.

https://codereview.chromium.org/312193005/diff/1/net/quic/crypto/crypto_proto...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/312193005/diff/1/net/quic/crypto/crypto_proto...
net/quic/crypto/crypto_protocol.h:96: const QuicTag kCVER = TAG('C', 'V', 'E',
'R');  // Client Version.
Heh, we used to have a CVER tag in an earlier version of the version negotiation
spec :>

That being said, I'm not in love with "VER" as part of this tag name, but I'm
not really sure why. I wonder if we should have something like Client
Description (CDSC) because it's more of a description than a version, or it will
be once more than one browser supports QUIC. You could almost use User Agent
(UA) though that might suggest that the *full* UA string, which we don't want.

Anyway, minor point.

[jar (doing other things), 2014-06-05 02:16:23]

https://codereview.chromium.org/312193005/diff/1/net/quic/crypto/crypto_proto...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/312193005/diff/1/net/quic/crypto/crypto_proto...
net/quic/crypto/crypto_protocol.h:96: const QuicTag kCVER = TAG('C', 'V', 'E',
'R');  // Client Version.
On 2014/06/04 22:31:15, Ryan Hamilton wrote:
> Heh, we used to have a CVER tag in an earlier version of the version
negotiation
> spec :>
> 
> That being said, I'm not in love with "VER" as part of this tag name, but I'm
> not really sure why. I wonder if we should have something like Client
> Description (CDSC) because it's more of a description than a version, or it
will
> be once more than one browser supports QUIC. You could almost use User Agent
> (UA) though that might suggest that the *full* UA string, which we don't want.
> 
> Anyway, minor point. 

As a matter of course... for these "less important" items, and most critically,
for items that don't vary from CHLO to CHLO, it is nice to have alphabetically
large initial letters, to best ensure the tags will be late in the tag listing.

Perhaps a nice unification (avoding a previously used tag??) would be:

User Agent Chrome Version
UACV

Other suggestions?  
YMMV (which is not a suggestion ;-) )

[ramant (doing other things), 2014-06-05 02:51:55]

PTAL.

Added Lei Zhang for chrome/browser changes.

thanks
raman

https://codereview.chromium.org/312193005/diff/1/chrome/browser/net/http_serv...
File chrome/browser/net/http_server_properties_manager.cc (right):

https://codereview.chromium.org/312193005/diff/1/chrome/browser/net/http_serv...
chrome/browser/net/http_server_properties_manager.cc:98: client_version = "C ";
On 2014/06/04 22:31:15, Ryan Hamilton wrote:
> On 2014/06/04 21:30:36, ramant wrote:
> > Should we define an enum vs the first letter of channel?
> 
> We could. On the other hand, the CHLO packet is padded to 1200 bytes, and it's
> usually quite empty, so it's not like we're short on space. I've be inclined
to
> simply use the full channel string. Perhaps via GetVersionStringModifier()?

Done.

https://codereview.chromium.org/312193005/diff/1/chrome/browser/net/http_serv...
chrome/browser/net/http_server_properties_manager.cc:113:
client_version.append(version_info.Version());
On 2014/06/04 22:31:15, Ryan Hamilton wrote:
> This doesn't include Chrome, right? It's just 37.0.13.4? I think we need to
put
> at least Chrome in here, or else we'll get collisions between different
browsers
> (when we have multiple browsers :>) Perhaps we should use
> ProductNameAndVersionForUserAgent()?

Done.

https://codereview.chromium.org/312193005/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/312193005/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.h:190: std::string client_version_;
On 2014/06/04 22:31:15, Ryan Hamilton wrote:
> It doesn't seem to me that client_version_ is something that belongs in the
> HttpServerProperties objects. Perhaps in the NetworkSession, or in the
> QuicStreamFactory.

Done.

https://codereview.chromium.org/312193005/diff/1/net/quic/crypto/crypto_proto...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/312193005/diff/1/net/quic/crypto/crypto_proto...
net/quic/crypto/crypto_protocol.h:96: const QuicTag kCVER = TAG('C', 'V', 'E',
'R');  // Client Version.
On 2014/06/04 22:31:15, Ryan Hamilton wrote:
> Heh, we used to have a CVER tag in an earlier version of the version
negotiation
> spec :>
> 
> That being said, I'm not in love with "VER" as part of this tag name, but I'm
> not really sure why. I wonder if we should have something like Client
> Description (CDSC) because it's more of a description than a version, or it
will
> be once more than one browser supports QUIC. You could almost use User Agent
> (UA) though that might suggest that the *full* UA string, which we don't want.
> 
> Anyway, minor point. 

Used kCDSC.
Done.

https://codereview.chromium.org/312193005/diff/1/net/quic/crypto/crypto_proto...
net/quic/crypto/crypto_protocol.h:96: const QuicTag kCVER = TAG('C', 'V', 'E',
'R');  // Client Version.
On 2014/06/05 02:16:23, jar wrote:
> On 2014/06/04 22:31:15, Ryan Hamilton wrote:
> > Heh, we used to have a CVER tag in an earlier version of the version
> negotiation
> > spec :>
> > 
> > That being said, I'm not in love with "VER" as part of this tag name, but
I'm
> > not really sure why. I wonder if we should have something like Client
> > Description (CDSC) because it's more of a description than a version, or it
> will
> > be once more than one browser supports QUIC. You could almost use User Agent
> > (UA) though that might suggest that the *full* UA string, which we don't
want.
> > 
> > Anyway, minor point. 
> 
> As a matter of course... for these "less important" items, and most
critically,
> for items that don't vary from CHLO to CHLO, it is nice to have alphabetically
> large initial letters, to best ensure the tags will be late in the tag
listing.
> 
> Perhaps a nice unification (avoding a previously used tag??) would be:
> 
> User Agent Chrome Version
> UACV
> 
> Other suggestions?  
> YMMV (which is not a suggestion ;-) )

Changed CDSC to UACV
Done.

[Lei Zhang, 2014-06-05 07:30:46]

chrome/browser lgtm

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1089: chrome::VersionInfo version_info;
nit: swap with the next line to put |version_info| closer to where it's used.

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1090: client_version.append(" ");
nit: append(" ") -> push_back(' ')

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.h:179: Optional<std::string> client_version;
should this be named |quic_client_version| ?

[Ryan Hamilton, 2014-06-05 14:12:15]

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.h:179: Optional<std::string> client_version;
nit: quic_client_version; (or quic_client_description)

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

https://codereview.chromium.org/312193005/diff/40001/net/http/http_network_se...
net/http/http_network_session.h:119: std::string client_version;
nit: quic_, please.

https://codereview.chromium.org/312193005/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/312193005/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:249: void set_client_version(const
std::string& client_version) {
I'm still not enamored of "version". It makes me think QUIC version (the version
of the protocol). Here are some suggestions:

UAFP: User Agent Fingerprint (which is what this has been referred to internal)
set_user_agent_fingerprint()
UAID: User Agent ID set_user_agent_id()
UADS: User Agent Description set_user_agent_description()

(And of course use a quic_ prefix in the non-quic locations)

[ramant (doing other things), 2014-06-05 17:07:49]

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1089: chrome::VersionInfo version_info;
On 2014/06/05 07:30:47, Lei Zhang wrote:
> nit: swap with the next line to put |version_info| closer to where it's used.

Done.

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1090: client_version.append(" ");
On 2014/06/05 07:30:47, Lei Zhang wrote:
> nit: append(" ") -> push_back(' ')

Done.

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.h:179: Optional<std::string> client_version;
On 2014/06/05 07:30:47, Lei Zhang wrote:
> should this be named |quic_client_version| ?

Per jar/rch, changed |client_version| to |quic_user_agent_id|

https://codereview.chromium.org/312193005/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.h:179: Optional<std::string> client_version;
On 2014/06/05 14:12:15, Ryan Hamilton wrote:
> nit: quic_client_version; (or quic_client_description)

per Per jar/rch, changed |client_version| to |quic_user_agent_id|

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

https://codereview.chromium.org/312193005/diff/40001/net/http/http_network_se...
net/http/http_network_session.h:119: std::string client_version;
On 2014/06/05 14:12:15, Ryan Hamilton wrote:
> nit: quic_, please.

Done.

https://codereview.chromium.org/312193005/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/312193005/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.h:249: void set_client_version(const
std::string& client_version) {
On 2014/06/05 14:12:15, Ryan Hamilton wrote:
> I'm still not enamored of "version". It makes me think QUIC version (the
version
> of the protocol). Here are some suggestions:
> 
> UAFP: User Agent Fingerprint (which is what this has been referred to
internal)
> set_user_agent_fingerprint()
> UAID: User Agent ID set_user_agent_id()
> UADS: User Agent Description set_user_agent_description()
> 
> (And of course use a quic_ prefix in the non-quic locations)

jar suggested UAID. Changed all the code to use user_agent_id.

Thanks much jar and rch.

[Ryan Hamilton, 2014-06-05 17:53:55]

LGTM. I *really* like how this reads now. Thanks for bearing with my OCD naming
issues :>

[ramant (doing other things), 2014-06-05 19:07:36]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-06-05 19:11:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/312193005/70012

[ramant (doing other things), 2014-06-05 21:56:24]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-06-05 21:57:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/312193005/90001

[commit-bot: I haz the power, 2014-06-06 03:30:51]

Change committed as 275319