Display more information at chrome://net-internals/#spdy...

Display more information at chrome://net-internals/#spdy
In particular, list the mapping from host:port to port:alternate-protocol 
and the global spdy configuration state.

BUG=none
TEST=chrome://net-internals/#spdy


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

[Ryan Hamilton, 2011-02-08 23:12:11]

Hi Eric,

Can you take a look at this change?  I've attempted to add some additional SPDY
information to net-internals.  It works for me :>  That being said, I have no
idea how to unit test this (if that's even something we do).  Any pointers?

Thanks,

Ryan

[eroman, 2011-02-09 04:51:13]

LGTM after addressing my comments.

> I have no idea how to unit test this (if that's even
> something we do).  Any pointers?

Unfortunately we don't have any infrastructure in place for testing these
changes. The best practice right now is to just test manually and hope noone
messes it up in the future :)

Also, could you also send me a screenshot showing what this looks like?

My other main comment is regarding the text view -- we actually maintain two
separate drawing paths in the net internals code. One that is pretty for HTML
(i.e. what you just added), and also a text version. The text version is what we
use to generate the "net log dumps" for attaching to bug reports. Could you
mirror this information in the text dump as well? See dataview.js, specifically
onUpdateAllCompleted().

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
chrome/browser/resources/net_internals/index.html:153: <li>Use Atlernate
Protocol: <span id=spdyUseAlternateProtocolSpan>????</span></li>
typo: alternate

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
chrome/browser/resources/net_internals/index.html:171: <p>
nit: can you close this?

as in, <p></p>

or <p/>

or <br/> if you just want spacing.

Not a big deal, just one of my style pet peeves.

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
File chrome/browser/resources/net_internals/spdyview.js (right):

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
chrome/browser/resources/net_internals/spdyview.js:62:
this.spdyEnabledSpan_.innerHTML = spdyStatus.spdy_enabled;
please don't use innerHTML for setting text which originated from a variable (it
can be potentially dangerous, since could expand into other stuff).

Instead, please use:

node.innerText = XXX;

or:

addText(node, XXXX);

(Comment applies below too).

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
chrome/browser/resources/net_internals/spdyview.js:80: if
(spdyAlternateProtocolMappings != null &&
nit: would it be sufficient to do

if (spdyAlternateProtocolMappings) ?

[Ryan Hamilton, 2011-02-09 17:42:31]

I've addressed your comments, and have added the new bits to dataview.js (which
was surprisingly easy!)  Here's sample output:

----------------------------------------------
 SPDY Status
----------------------------------------------

SPDY Enabled: true
Use Alternate Protocol: true
Force SPDY Always: false
Force SPDY Over SSL: true
Next Protocols: http/1.1spdy/2

----------------------------------------------
 SPDY Sessions
----------------------------------------------

Host                Proxy      ID  Active streams  Unclaimed pushed  Max 
Initiated  Pushed  Pushed and claimed  Abandoned  Received frames  Secure  Sent
settings  Received settings  Error  
----                -----      --  --------------  ----------------  --- 
---------  ------  ------------------  ---------  ---------------  ------ 
-------------  -----------------  -----  
www.google.com:443  direct://  58  0               0                 100  3     
    0       0                   0          4                true    false       
  true               0      


----------------------------------------------
 Alternate Protocol Mappings
----------------------------------------------

Host               Alternate Protocol  
----               ------------------  
www.belshe.com:80  443:npn-spdy/2      

I'll reply separately with a screen shot.

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
chrome/browser/resources/net_internals/index.html:153: <li>Use Atlernate
Protocol: <span id=spdyUseAlternateProtocolSpan>????</span></li>
On 2011/02/09 04:51:13, eroman wrote:
> typo: alternate

Done.

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
chrome/browser/resources/net_internals/index.html:171: <p>
On 2011/02/09 04:51:13, eroman wrote:
> nit: can you close this?
> 
> as in, <p></p>
> 
> or <p/>
> 
> or <br/> if you just want spacing.
> 
> Not a big deal, just one of my style pet peeves.

Done.

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
File chrome/browser/resources/net_internals/spdyview.js (right):

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
chrome/browser/resources/net_internals/spdyview.js:62:
this.spdyEnabledSpan_.innerHTML = spdyStatus.spdy_enabled;
On 2011/02/09 04:51:13, eroman wrote:
> please don't use innerHTML for setting text which originated from a variable
(it
> can be potentially dangerous, since could expand into other stuff).
> 
> Instead, please use:
> 
> node.innerText = XXX;
> 
> or:
> 
> addText(node, XXXX);
> 
> (Comment applies below too).

Oh cute!  I didn't think about that.  Gotta love web security :> Done!

http://codereview.chromium.org/6465001/diff/10/chrome/browser/resources/net_i...
chrome/browser/resources/net_internals/spdyview.js:80: if
(spdyAlternateProtocolMappings != null &&
On 2011/02/09 04:51:13, eroman wrote:
> nit: would it be sufficient to do
> 
> if (spdyAlternateProtocolMappings) ?

That would be great, but I tried it and it appears not to work.   FWIW, the
"!=NULL && length > 0" idiom is used in several places in net_internals .js
files.

[eroman, 2011-02-09 19:46:50]

LGTM

http://codereview.chromium.org/6465001/diff/2011/chrome/browser/dom_ui/net_in...
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/6465001/diff/2011/chrome/browser/dom_ui/net_in...
chrome/browser/dom_ui/net_internals_ui.cc:1023:
*net::HttpStreamFactory::next_protos()));
nit: indent by 4.