[webkit-reviews] review denied: [Bug 19945] List HTTP status code with response headers in resources tab of Web Inspector : [Attachment 39903] Fix

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 19945: List HTTP status code with response headers in resources tab of Web
Inspector
https://bugs.webkit.org/show_bug.cgi?id=19945

Attachment 39903: Fix
https://bugs.webkit.org/attachment.cgi?id=39903&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +    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.