Add the spdy protocol to the information displayed about a spdy session in net-internals.

Add the spdy protocol to the information displayed about a spdy session in net-internals.

TEST=Connect to https://www.google.com/ and then pull up chrome://net-internals/#spdy.  Notice the new Protocol Negotiatied column.


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

[Ryan Hamilton, 2012-03-07 17:35:50]

eroman

[Ryan Hamilton, 2012-03-07 17:36:19]

eroman: PTAL.  This is a tiny cl.
rtenneti: fyi

[eroman, 2012-03-07 19:27:32]

https://chromiumcodereview.appspot.com/9625009/diff/1/chrome/browser/resource...
File chrome/browser/resources/net_internals/spdy_view.js (right):

https://chromiumcodereview.appspot.com/9625009/diff/1/chrome/browser/resource...
chrome/browser/resources/net_internals/spdy_view.js:166:
tablePrinter.addCell(session.protocol_negotiated);
note: since this is a new field, when loading older logs it will end up printing
"undefined". That is probably OK, but you might want to enclose this in a
condition and add a comment to remove it in the future (say +2 chrome major
versions).

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1116:
reinterpret_cast<SSLClientSocket*>(connection_->socket());
Are you absolutely sure about this? Reinterpret cast scares the hell out of me.

[mmenke, 2012-03-07 19:32:31]

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1116:
reinterpret_cast<SSLClientSocket*>(connection_->socket());
On 2012/03/07 19:27:32, eroman wrote:
> Are you absolutely sure about this? Reinterpret cast scares the hell out of
me.

Drive by comment:  Looks like all the other instances of
reinterpret_cast<SSLClientSocket*> in this file either have a
"DCHECK(is_secure_)" or an "if (is_secure_)".  I'd suggest the latter here,
seeing as if that assumption ever changes, testing of net-internals is pretty
sparse, so it could easily go unnoticed.

[Ryan Hamilton, 2012-03-07 19:46:22]

https://chromiumcodereview.appspot.com/9625009/diff/1/chrome/browser/resource...
File chrome/browser/resources/net_internals/spdy_view.js (right):

https://chromiumcodereview.appspot.com/9625009/diff/1/chrome/browser/resource...
chrome/browser/resources/net_internals/spdy_view.js:166:
tablePrinter.addCell(session.protocol_negotiated);
On 2012/03/07 19:27:32, eroman wrote:
> note: since this is a new field, when loading older logs it will end up
printing
> "undefined". That is probably OK, but you might want to enclose this in a
> condition and add a comment to remove it in the future (say +2 chrome major
> versions).

I don't mind undefined, but I'm happy to add a conditional.  Can you point me at
an example I could use?  I'm pretty new to this part of the code base.

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1116:
reinterpret_cast<SSLClientSocket*>(connection_->socket());
On 2012/03/07 19:32:32, Matt Menke wrote:
> On 2012/03/07 19:27:32, eroman wrote:
> > Are you absolutely sure about this? Reinterpret cast scares the hell out of
> me.
> 
> Drive by comment:  Looks like all the other instances of
> reinterpret_cast<SSLClientSocket*> in this file either have a
> "DCHECK(is_secure_)" or an "if (is_secure_)".  I'd suggest the latter here,
> seeing as if that assumption ever changes, testing of net-internals is pretty
> sparse, so it could easily go unnoticed.

Heh, yes I agree.  This is already wrapped in an if (is_secure) on line 1114,
right?

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1116:
reinterpret_cast<SSLClientSocket*>(connection_->socket());
On 2012/03/07 19:27:32, eroman wrote:
> Are you absolutely sure about this? Reinterpret cast scares the hell out of
me.

eroman: yes in the sense that we do this repeatedly in spdy_session.cc:

grep 'reinterpret_cast<SSLClientSocket' net/spdy/spdy_session.cc | wc -l
8

[mmenke, 2012-03-07 19:47:59]

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1116:
reinterpret_cast<SSLClientSocket*>(connection_->socket());
On 2012/03/07 19:46:22, Ryan Hamilton wrote:
> On 2012/03/07 19:32:32, Matt Menke wrote:
> > On 2012/03/07 19:27:32, eroman wrote:
> > > Are you absolutely sure about this? Reinterpret cast scares the hell out
of
> > me.
> > 
> > Drive by comment:  Looks like all the other instances of
> > reinterpret_cast<SSLClientSocket*> in this file either have a
> > "DCHECK(is_secure_)" or an "if (is_secure_)".  I'd suggest the latter here,
> > seeing as if that assumption ever changes, testing of net-internals is
pretty
> > sparse, so it could easily go unnoticed.
> 
> Heh, yes I agree.  This is already wrapped in an if (is_secure) on line 1114,
> right?

Oops...Indeed it is.  Amusingly, I just looked at other instances of
reinterpret_cast<SSLClientSocket*>, without double-checking that this one had
it.

[eroman, 2012-03-07 19:49:14]

lgtm

[Ryan Hamilton, 2012-03-07 19:52:05]

On 2012/03/07 19:47:59, Matt Menke wrote:
> https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.cc
> File net/spdy/spdy_session.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9625009/diff/1/net/spdy/spdy_session.c...
> net/spdy/spdy_session.cc:1116:
> reinterpret_cast<SSLClientSocket*>(connection_->socket());
> On 2012/03/07 19:46:22, Ryan Hamilton wrote:
> > On 2012/03/07 19:32:32, Matt Menke wrote:
> > > On 2012/03/07 19:27:32, eroman wrote:
> > > > Are you absolutely sure about this? Reinterpret cast scares the hell out
> of
> > > me.
> > > 
> > > Drive by comment:  Looks like all the other instances of
> > > reinterpret_cast<SSLClientSocket*> in this file either have a
> > > "DCHECK(is_secure_)" or an "if (is_secure_)".  I'd suggest the latter
here,
> > > seeing as if that assumption ever changes, testing of net-internals is
> pretty
> > > sparse, so it could easily go unnoticed.
> > 
> > Heh, yes I agree.  This is already wrapped in an if (is_secure) on line
1114,
> > right?
> 
> Oops...Indeed it is.  Amusingly, I just looked at other instances of
> reinterpret_cast<SSLClientSocket*>, without double-checking that this one had
> it.

Heh, been there done that.  Better safe than sorry.  Thanks!

[commit-bot: I haz the power, 2012-03-07 19:52:28]

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

[commit-bot: I haz the power, 2012-03-07 23:14:27]

Change committed as 125485