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

[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 39929: Fix Take 2
https://bugs.webkit.org/attachment.cgi?id=39929&action=review

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