Bug 19945 – List HTTP status code with response headers in resources tab of Web Inspector

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[sebwin]

Right now the only way to get at least a peek at the HTTP status code for a returned resource is in the Activity window, which lists failure codes.
I'd like to propose to have the status code always reported following (or preceding) the list of response headers for each resource in Web Inspector.

[Anthony Ricaud]

*** Bug 23965 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

I'd like this as well.

[Mark Rowe (bdash)]

*** Bug 26384 has been marked as a duplicate of this bug. ***

[Patrick Mueller]

I'm adding the HTTP method to the URL as part of Bug 22920, so now that top line looks very close to the actual HTTP request line that's sent.  It doesn't display the HTTP version that was sent with the request.  Not sure that's important.

So, I'm thinking that perhaps what we should do is show a new line underneath what now contains the URL, which is the status line returned from the server.  I'm not sure the HTTP version from the status line is actually easily available at the moment, but I believe the status code / text is.

This would make the text above the headers then look something like this:

   GET http://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html
   200 OK

Followed by the headers as they exist today.

What do you think?

[Timothy Hatcher]

That should be OK. But I like the URL line being one line and not adding another line.

[James Wheare]

As mentioned in Bug 26384:

"Version is useful info because it effects resource download pipelining behaviour."

Caching behaviour also varies between HTTP 1.0 and 1.1, and only 1.1 allows persistent connections. This can be crucial information to have when debugging HTTP requests.

See:
http://www.research.att.com/~bala/papers/h0vh1.html
http://stackoverflow.com/questions/246859/http-1-0-vs-1-1

[Timothy Hatcher]

Patrick, what if we added another collapsable section like "HTTP Information" or somthing better that contains the request method, status code and HTTP version. That way they are all grouped, collapsed by default and not taking up precious space. Then they can be labeled too.

[James Wheare]

How about this.

GET https://bugs.webkit.org/
HTTP/1.1 200 OK

Status line is just one line, and the method is combined onto the URL line. This is closer to how familiar command line tools such as CURL display the info.

The info is then easily available and not hidden away in collapsable areas that require too much interaction to reveal.

[Timothy Hatcher]

I'm just not convinced you need to see all of that, all the time. Thats why I proposed the collapsable area. Not everyone knows the HTTP spec, so mimicing the output is futile to many.

The original intent of the URL being up there was to be easy to read and copy.

I think we can consider an icon prefix before the URL (like the iChat status bubbles red, yellow, green) for errors (400s-500s) and 200s are green. Then a hover tooltip on the icon can give you the specific status code + status text.

I still like the collapasable area for HTTP info.

[Timothy Hatcher]

We can also remember the expanded state. So if you expand it once for one resource and leave it expanded, it will be expanded for every other resource. We should do that for headers, etc too.

[Timothy Hatcher]

We should alos badge error resources in the sidebar, so you don't have to click on them to see there was an error. The hover status could be on the sidebar badge too.

[James Wheare]

I don't follow your argument for saving space by adding an extra line for an expandable group label. What's the difference in space savings between one line that has all the info a developer would need versus one line that you need to click on to expand? (Not to mention that it's not enough to just click the label, you need to click that tiny little usability nightmare arrow)

Who is the target audience for this aspect of the Web Inspector? Developers who are looking for information about HTTP requests I would have though. I think it's safe to optimise for people who understand the HTTP spec in this case. Why make life harder for those who know what they're looking for?

This is a power tool after all. A lot of little details like this, where friendliness and neatness are optimised to the detriment of detail, power and usefulness make the difference between this and Firebug in my opinion. It's by no means an either/or situation, but more like an alignment of priorities.

[Patrick Mueller]

My gut tells me two static lines won't be an issue.  If we did an expandable section, I think we'd forgo the usual "title", and use the badge/URL as the title, and then expanded you'd see the other stuff.  That way you can still narrow it down to a single line.  We can try it both ways.  

The issue with clicking the "usability nightmare arrow" :-) can also be fixed.  Other expandable things in Inspector expand/collapse based on a single mouse click anywhere on the line, not just on the arrow.

I think the badge is a good idea, esp with a hover to get the exact status code.  I can't quite see where to put it, if you're thinking that it's an icon.  What about a colored bar between the resource icon and name?  Another thought is to use a tooltip for the entire icon/name column entries.  The tooltip could be the simple request line / response status, or we could try putting all the headers in as well.

Maybe a category for "error" status codes?  And/or a sort by status code.

Also having the expanded state of the header/etc sections be sticky across resources is a good idea. 

We also need to define what an "error" means - 4xx, 5xx?  Or how many categories of status codes do we have?  HTTP defines pretty clear classifications for 1xx, 2xx, 3xx, 4xx and 5xx, so it'd be easy to have 5, but that's probably overkill.

[Brian Weinstein]

Created attachment 39902 [details]
HTTP Status Code Screen Shot

Here is a screenshot with another top level list element for HTTP Information, that shows the request type and the status code. A patch is upcoming in a few minutes, just need to write a ChangeLog.

[Brian Weinstein]

Created attachment 39903 [details]
Fix

[Timothy Hatcher]

Comment on attachment 39903 [details]
Fix

> +    get httpInformation()
> +    {
> +        if (this._httpInformation === undefined)
> +            this._httpInformation = {};
> +        return this._httpInformation;
> +    },

Better written as:

return this._httpInformation || {};


> +    get sortedHttpInformation()

This should be sortedHTTPInformation. Accronyms should be all caps unless they are at the beginning. Ditto for _sortedHttpInformation.


> +            this._sortedHttpInformation.push({header: key, value: this.httpInformation[key]});
> +        this._sortedHttpInformation.sort(function(a,b) { return a.header.localeCompare(b.header) });

We should refactor this to not be about headers. Add a FIXME about this and/or file a bug.


> +    this.headersTreeOutline.appendChild(this.httpInformationTreeElement);

I think the HTTP Info should be first, since it will always be present. That will put it near the URL too.


> +    resource.addEventListener("httpInformation changed", this._refreshHttpInformationHeaders, this);

Should be _refreshHTTPInformationHeaders.


> +            if (this.resource.statusCode < 300) {
> +                statusImageSource = "Images/successGreenDot.png";
> +            } else if (this.resource.statusCode < 400) {
> +                statusImageSource = "Images/warningOrangeDot.png";
> +            } else {
> +                statusImageSource = "Images/errorRedDot.png"; 
> +            }

No need for the braces.


> +        this.httpInformationTreeElement.hidden = (typeof this.resource.statusCode === "undefined" || this.resource.statusCode == 0);

Maybe this can be hidden if this.httpInformationTreeElement has no children? Or does it always have a child? Maybe don't append the status code in the first place if it is "0".

Better written as:

this.httpInformationTreeElement.hidden = !this.resource.statusCode;


> +        resource.httpInformation["Request Method"] = payload.requestMethod;

A better place to update this is in Resource.js's "set requestMethod(x)".


> +        resource.httpInformation["Status Code"] = payload.statusCode + " " + WebInspector.Resource.StatusText[payload.statusCode];

A better place to update this is in Resource.js's "set statusCode(x)".

This code also wont fire the "httpInformation changed" event. I don't think you need the "httpInformation changed" event anyway, since these are not changed from C++ like the other members. Just remove that event and related code.

[Patrick Mueller]

I think visually it might work better if the status dot / URL was the title of a tree element which had a single child, the status code  / reason.  At the top as Tim suggests.  Also put the request method on the status dot / URL line.  Combines 4 lines into 2, removing the "HTTP Information" header completely.  

I'd say you might want to set the initial expanded state of the status code / reason based on whether it's 200 or not - if 200, collapsed, otherwise expanded.  That way it'll be collapsed for the usual, expected, non-interesting case, expanded for the others.

Wonder what it would look like if we combined the dot with the request method, like the times in the resource panel graphs - the request method in a colored bubble.  Probably garish.

For the Request Method / Status Code "Headers", I don't think there's any need to use the "headers" tree framework for these - the children are fixed, there are only two (or one with my suggestion), you can create the elements statically, add references to them for when you need to update the values.

[Brian Weinstein]

Created attachment 39929 [details]
Fix Take 2

[Brian Weinstein]

(In reply to comment #16)

> Better written as:
> 
> return this._httpInformation || {};

I tried this, but it didn't seem to be saving any values in the array, so I stuck with the orignal code.

> 
> > +    get sortedHttpInformation()
> 
> This should be sortedHTTPInformation. Accronyms should be all caps unless they
> are at the beginning. Ditto for _sortedHttpInformation.
> 

Done.

> 
> > +            this._sortedHttpInformation.push({header: key, value: this.httpInformation[key]});
> > +        this._sortedHttpInformation.sort(function(a,b) { return a.header.localeCompare(b.header) });
> 
> We should refactor this to not be about headers. Add a FIXME about this and/or
> file a bug.
> 

Added a FIXME.

> 
> > +    this.headersTreeOutline.appendChild(this.httpInformationTreeElement);
> 
> I think the HTTP Info should be first, since it will always be present. That
> will put it near the URL too.
> 

Done, moved under the URL.

> > +    resource.addEventListener("httpInformation changed", this._refreshHttpInformationHeaders, this);
> 
> Should be _refreshHTTPInformationHeaders.
> 

Done, removed the event but kept this function call.

> 
> > +            if (this.resource.statusCode < 300) {
> > +                statusImageSource = "Images/successGreenDot.png";
> > +            } else if (this.resource.statusCode < 400) {
> > +                statusImageSource = "Images/warningOrangeDot.png";
> > +            } else {
> > +                statusImageSource = "Images/errorRedDot.png"; 
> > +            }
> 
> No need for the braces.
> 

Removed the braces.

> 
> > +        this.httpInformationTreeElement.hidden = (typeof this.resource.statusCode === "undefined" || this.resource.statusCode == 0);
> 
> Maybe this can be hidden if this.httpInformationTreeElement has no children? Or
> does it always have a child? Maybe don't append the status code in the first
> place if it is "0".
> 
> Better written as:
> 
> this.httpInformationTreeElement.hidden = !this.resource.statusCode;
> 

Done.

> 
> > +        resource.httpInformation["Request Method"] = payload.requestMethod;
> 
> A better place to update this is in Resource.js's "set requestMethod(x)".
> 
> 
> > +        resource.httpInformation["Status Code"] = payload.statusCode + " " + WebInspector.Resource.StatusText[payload.statusCode];
> 
> A better place to update this is in Resource.js's "set statusCode(x)".
> 

Done.

> This code also wont fire the "httpInformation changed" event. I don't think you
> need the "httpInformation changed" event anyway, since these are not changed
> from C++ like the other members. Just remove that event and related code.

Removed the event.

[Timothy Hatcher]

Comment on attachment 39929 [details]
Fix Take 2

> +    this.statusCode = 0;

No need to set this to zero, it will be undefined by default and your code handles both.

> +        this.httpInformation["Request Method"] = this._requestMethod;

> +        this.httpInformation["Status Code"] = this._statusCode + " " + WebInspector.Resource.StatusText[this._statusCode];

> +    get httpInformation()
> +    {
> +        if (this._httpInformation === undefined)
> +            this._httpInformation = {};
> +
> +        return this._httpInformation;
> +    },
> +    
> +    set httpInformation(x)
> +    {
> +        if (this._httpInformation === x)
> +            return;
> +        
> +        this._httpInformation = x;
> +        delete this._sortedHTTPInformation;
> +    },
> +    
> +    get sortedHTTPInformation()
> +    {
> +        if (this._sortedHTTPInformation !== undefined)
> +            return this._sortedHTTPInformation;
> +        
> +        this._sortedHTTPInformation = [];
> +        for (var key in this.httpInformation)
> +            this._sortedHTTPInformation.push({header: key, value: this.httpInformation[key]});
> +        
> +        // FIXME: Refactor this to not be about headers.
> +        this._sortedHTTPInformation.sort(function(a,b) { return a.header.localeCompare(b.header) });
> +        
> +        return this._sortedHTTPInformation;
> +    },

Actually now that I look at this in the new patch I see how this can be improved more. And I agree with Patrick, this should not follow the headers model and not need for the sorting code, etc.

For one "Status Code" and "Request Method" should be localized, and they are not in this patch. Second the code above can be removed.

ResourceView.js should just create TreeElements for the 2 items (with localized titles) and append them to the "HTTP Information" item as children. Resource.js should have a setter for statusCode so it can fire an event when it changes. Then you can make ResourceView update the TreeElement from that event, etc.

Also you will need to add the strings you use in WebInspector.UIString to localizedStrings.js.

[Timothy Hatcher]

(In reply to comment #17)
> I think visually it might work better if the status dot / URL was the title of
> a tree element which had a single child, the status code  / reason.  At the top
> as Tim suggests.  Also put the request method on the status dot / URL line. 
> Combines 4 lines into 2, removing the "HTTP Information" header completely.  

Neat idea. I still think I prefer a a named section over making the url be the section.

> I'd say you might want to set the initial expanded state of the status code /
> reason based on whether it's 200 or not - if 200, collapsed, otherwise
> expanded.  That way it'll be collapsed for the usual, expected, non-interesting
> case, expanded for the others.

Also a good idea.

> Wonder what it would look like if we combined the dot with the request method,
> like the times in the resource panel graphs - the request method in a colored
> bubble.  Probably garish.

I see what you are saying... hmm it might be neat looking.

> For the Request Method / Status Code "Headers", I don't think there's any need
> to use the "headers" tree framework for these - the children are fixed, there
> are only two (or one with my suggestion), you can create the elements
> statically, add references to them for when you need to update the values.

I agree.

[Brian Weinstein]

Created attachment 39939 [details]
Refactored Fix + Better Handling of 0/Undefined

[Brian Weinstein]

Landed in http://trac.webkit.org/changeset/48646.

[James Wheare]

Oh... does this patch not include the HTTP version number? 1.0 vs 1.1 etc?

[Brian Weinstein]

(In reply to comment #24)
> Oh... does this patch not include the HTTP version number? 1.0 vs 1.1 etc?

It does not, this data is not easy to get from either a ResourceResponse or a ResourceRequest, so it would take some big changes, and we would need some information that I don't know if we have an API to get. If you want to file a bug about it, feel free. There probably should be one.

[James Wheare]

(In reply to comment #25)
> If you want to file a bug about it, feel free. There probably should be one.

Done: Bug 29687