Last Comment Bug 888583 - Expose http/1 version numbers in about:networking
: Expose http/1 version numbers in about:networking
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: 21 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla25
Assigned To: Robert Bindar
:
Mentors:
: 888267 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-29 01:58 PDT by Robert Bindar
Modified: 2013-07-09 10:30 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (8.31 KB, patch)
2013-06-29 02:00 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
patch v2 (9.51 KB, patch)
2013-06-29 11:17 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
patch v3 (11.08 KB, patch)
2013-07-03 10:31 PDT, Robert Bindar
mcmanus: review-
Details | Diff | Splinter Review
Moving Spdy versions enum to nsHtpp.h (3.55 KB, patch)
2013-07-04 01:49 PDT, Robert Bindar
mcmanus: review+
Details | Diff | Splinter Review
Expose protocol version (10.74 KB, patch)
2013-07-04 01:50 PDT, Robert Bindar
valentin.gosu: review+
Details | Diff | Splinter Review
Move Spdy versions enum to nsHtpp.h (3.16 KB, patch)
2013-07-08 03:32 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
expose protocol version (10.73 KB, patch)
2013-07-08 06:51 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
expose protocol version (10.79 KB, patch)
2013-07-08 08:44 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
Expose protocol version (10.83 KB, patch)
2013-07-08 12:37 PDT, Robert Bindar
no flags Details | Diff | Splinter Review

Description Robert Bindar 2013-06-29 01:58:50 PDT
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130620122336
Comment 1 Robert Bindar 2013-06-29 02:00:15 PDT
Created attachment 769345 [details] [diff] [review]
patch v1
Comment 2 Robert Bindar 2013-06-29 02:01:57 PDT
added some spaces for keeping the integrity of the module.
Comment 3 Patrick McManus [:mcmanus] 2013-06-29 05:56:40 PDT
Comment on attachment 769345 [details] [diff] [review]
patch v1

Review of attachment 769345 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/NetDashboard.webidl
@@ +17,5 @@
>  dictionary HttpConnInfoDict {
>  	sequence<unsigned long> rtt;
>  	sequence<unsigned long> ttl;
>    sequence<octet> spdyVersion;
> +  sequence<octet> httpVersion;

indentation

::: netwerk/base/src/DashboardTypes.h
@@ +39,1 @@
>  };

it isn't possible to simultaneously have an http/1 version and a spdy version (spdy is really the prototype of http/2).. to make this future proof I'd like to see it collapsed to just one field that supports all 5 protocols (0.9, 1.0, 1.1, spdy/2, spdy/3) as well as future ones

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +68,5 @@
>      , mUsingSpdyVersion(0)
>      , mPriority(nsISupportsPriority::PRIORITY_NORMAL)
>      , mReportedSpdy(false)
>      , mEverUsedSpdy(false)
> +    , mHttpVersion(NS_HTTP_VERSION_1_1)

please name this mLastHttpResponseVersion

::: netwerk/protocol/http/nsHttpConnection.h
@@ +118,5 @@
>      nsresult PushBack(const char *data, uint32_t length);
>      nsresult ResumeSend();
>      nsresult ResumeRecv();
> +    int64_t  MaxBytesRead() { return mMaxBytesRead; }
> +    uint8_t GetHttpVersion() { return mHttpVersion; }

naming again..
Comment 4 Robert Bindar 2013-06-29 10:19:28 PDT
*** Bug 888267 has been marked as a duplicate of this bug. ***
Comment 5 Robert Bindar 2013-06-29 11:17:00 PDT
Created attachment 769405 [details] [diff] [review]
patch v2
Comment 6 Patrick McManus [:mcmanus] 2013-07-01 13:10:11 PDT
Comment on attachment 769405 [details] [diff] [review]
patch v2

Review of attachment 769405 [details] [diff] [review]:
-----------------------------------------------------------------

you've added a requirement that the enums of SPDY_VERSION and NS_HTTP_VERSION not overlap.. that's not really workable because

1] NS_HTTP_VERSIONS are sometimes compared, so later versions need higher numbers.. and VERSION_1_0 is defined as 10 but SPDY_VERSION_2 is set to 2
2] SPDY_VERSION_2 can't be set to something new (like 20) because it is used as an index in telemetry
Comment 7 Valentin Gosu [:valentin] 2013-07-01 13:58:24 PDT
As I see it, we have a few options of handling this:
1. Only hold protocol version as a string. This way we bypass the issue with comparing protocol versions.
2. Create a new class for the protocol version and overload the > operator. This seems like a really unsafe thing to do (I don't usually expect that 2 > 10 == true )
3. Make the protocolVersion in DashboardTypes.h private to the Dashboard so that people don't accidentally use this value.
Comment 8 Patrick McManus [:mcmanus] 2013-07-03 06:23:59 PDT
(In reply to Valentin Gosu from comment #7)
> As I see it, we have a few options of handling this:
> 1. Only hold protocol version as a string. This way we bypass the issue with
> comparing protocol versions.
> 2. Create a new class for the protocol version and overload the > operator.
> This seems like a really unsafe thing to do (I don't usually expect that 2 >
> 10 == true )
> 3. Make the protocolVersion in DashboardTypes.h private to the Dashboard so
> that people don't accidentally use this value.


Current comparisons are ok because they are restricted in scope. (http/1 minors vs each other, spdy revs vs each other).. I'm just worried about creep.

Can the dashboard just keep it as a string in its class? It would clearly make better output :)
Comment 9 Robert Bindar 2013-07-03 10:31:37 PDT
Created attachment 770891 [details] [diff] [review]
patch v3

I've added the protocol version as a string in dashboard.
Comment 10 Patrick McManus [:mcmanus] 2013-07-03 12:07:46 PDT
Comment on attachment 770891 [details] [diff] [review]
patch v3

Review of attachment 770891 [details] [diff] [review]:
-----------------------------------------------------------------

getting closer!

::: netwerk/base/src/Dashboard.cpp
@@ +12,5 @@
> +#define NS_HTTP_VERSION_0_9 9
> +#define NS_HTTP_VERSION_1_0 10
> +#define NS_HTTP_VERSION_1_1 11
> +#define SPDY_VERSION_2 2
> +#define SPDY_VERSION_3 3

if you end up needing them after revising this, dont redefine them.. use them via include

@@ +455,5 @@
> +HttpConnInfo::SetProtocolVersion(uint8_t pv)
> +{
> +    switch (pv) {
> +    case SPDY_VERSION_2:
> +        protocolVersion = "spdy/2";

protocolVersion.Assign(NS_LITERAL_CSTRING("spdy/2"));

::: netwerk/base/src/DashboardTypes.h
@@ +33,5 @@
>  struct HttpConnInfo
>  {
>      uint32_t ttl;
>      uint32_t rtt;
> +    nsCString protocolVersion;

This is really for JS, right? So It should be a nsString so you don't need the UTF16 copy conversion later.. might as well just do the assignment in utf16.

@@ +35,5 @@
>      uint32_t ttl;
>      uint32_t rtt;
> +    nsCString protocolVersion;
> +
> +    void SetProtocolVersion(uint8_t pv);

I would get rid of setProtocolVersion and create two functions - SetHTTP1ProtocolVersion and setHTTP2ProtocolVersion (spdy can use this one) and then the connection manager can use the right one given the context it already knows.
Comment 11 Valentin Gosu [:valentin] 2013-07-03 12:27:54 PDT
Comment on attachment 770891 [details] [diff] [review]
patch v3

Review of attachment 770891 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/Dashboard.cpp
@@ +12,5 @@
> +#define NS_HTTP_VERSION_0_9 9
> +#define NS_HTTP_VERSION_1_0 10
> +#define NS_HTTP_VERSION_1_1 11
> +#define SPDY_VERSION_2 2
> +#define SPDY_VERSION_3 3

NS_HTTP_VERSION_* you can get by importing nsHttp.h
However, ASpdySession.h isn't exported by moz.build
You can either export it, redefine the enum, or move the enum to a common header (maybe even nsHttp.h)
Comment 12 Robert Bindar 2013-07-04 01:49:54 PDT
Created attachment 771235 [details] [diff] [review]
Moving Spdy versions enum to nsHtpp.h
Comment 13 Robert Bindar 2013-07-04 01:50:48 PDT
Created attachment 771236 [details] [diff] [review]
Expose protocol version
Comment 14 Patrick McManus [:mcmanus] 2013-07-07 12:22:00 PDT
Comment on attachment 771235 [details] [diff] [review]
Moving Spdy versions enum to nsHtpp.h

Review of attachment 771235 [details] [diff] [review]:
-----------------------------------------------------------------

r=mcmanus with the changes I note and a successful try run on linux first please

::: netwerk/protocol/http/nsHttp.h
@@ +22,5 @@
>  #define NS_HTTP_VERSION_1_1     11
>  
> +namespace mozilla {
> +namespace net {
> +namespace Spdy {

just mozilla::net please

@@ +27,5 @@
> +    enum {
> +        SPDY_VERSION_2 = 2,
> +        SPDY_VERSION_3 = 3
> +    };
> +} } } // namespace mozilla::net::Spdy

each brace on its own line
Comment 15 Patrick McManus [:mcmanus] 2013-07-07 12:27:14 PDT
Comment on attachment 771236 [details] [diff] [review]
Expose protocol version

Review of attachment 771236 [details] [diff] [review]:
-----------------------------------------------------------------

I'm good with most of this.. valentin can be the final arbiter
Comment 16 Valentin Gosu [:valentin] 2013-07-07 22:26:51 PDT
Comment on attachment 771236 [details] [diff] [review]
Expose protocol version

Review of attachment 771236 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me
Comment 17 Robert Bindar 2013-07-08 03:32:15 PDT
Created attachment 771997 [details] [diff] [review]
Move Spdy versions enum to nsHtpp.h

r=mcmanus
Comment 18 Robert Bindar 2013-07-08 03:34:08 PDT
Please check in patch 771997 and patch 771236.
https://tbpl.mozilla.org/?tree=Try&rev=4ea4b43a67ef
Comment 20 Patrick McManus [:mcmanus] 2013-07-08 06:37:57 PDT
robert, also please actually run the tests on your next try run :) (you just built it)
Comment 21 Valentin Gosu [:valentin] 2013-07-08 06:43:06 PDT
Comment on attachment 771236 [details] [diff] [review]
Expose protocol version

Review of attachment 771236 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/Dashboard.cpp
@@ +464,5 @@
> +
> +void
> +HttpConnInfo::SetHTTP2ProtocolVersion(uint8_t pv)
> +{
> +    if (pv == Spdy::SPDY_VERSION_2)

This is no longer in the Spdy namespace in the latest patch. Please fix.
Comment 22 Robert Bindar 2013-07-08 06:47:45 PDT
sorry, my bad, I've forgotten to update the second patch too. I will do it now.
Comment 23 Robert Bindar 2013-07-08 06:51:58 PDT
Created attachment 772065 [details] [diff] [review]
expose protocol version

r=valentin.gosu
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-07-08 07:58:07 PDT
Patch 2 needs rebasing against m-c/inbound. Also, judging by comment 20, a Try run with tests run would also be appreciated :)
Comment 25 Robert Bindar 2013-07-08 08:44:59 PDT
Created attachment 772108 [details] [diff] [review]
expose protocol version

r=valentin.gosu
Comment 26 Robert Bindar 2013-07-08 08:45:43 PDT
I will come soon with a try tun with tests.
Comment 27 Robert Bindar 2013-07-08 12:37:54 PDT
Created attachment 772251 [details] [diff] [review]
Expose protocol version

r=valentin.gosu

Note You need to log in before you can comment on or make changes to this bug.