[webkit-reviews] review denied: [Bug 22920] Inspector Request Headers Should Show Data/Variables/Parameters Sent With Request : [Attachment 38550] proposed patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Patrick Mueller
<pmuellr at yahoo.com>'s request for review:
Bug 22920: Inspector Request Headers Should Show Data/Variables/Parameters Sent
With Request
https://bugs.webkit.org/show_bug.cgi?id=22920

Attachment 38550: proposed patch
https://bugs.webkit.org/attachment.cgi?id=38550&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> +    this.requestMethod = requestMethod || "";
> +    this.requestFormData = requestFormData || "";

Is this needed, can they be null/undefined?

> +	   var parmString = url.split("?",2)[1];

Need a space after the comma (a few places like this in the patch.)


> +	       if (testKey.toLowerCase() == lowerKey) {

Triple equals would be best. No need for the braces.

> +	   return undefined;

Just remove this and let the default undefined return happen.

> +	   title += "<div style='white-space:pre-wrap' class='header-value'>" +
formData.escapeHTML() + "</div>";

You should add a style class to the inspector.css and not have this inline
style. Use double quotes for the HTML too.

> +	   for (var i = 0; i < parms.length; i++) {

Use ++i.

> +	       parm = parm.split("=",2);

Space after the comma.

> +	       if (parm.length == 1) parm.push("");

Should be on two lines.

> +	       var tooltip;
> +	       if (val.indexOf("%") >= 0) {
> +		   tooltip = decodeURIComponent(val).replace("+"," ");
> +	       }

No need for the braces. I think we should show decoded by default. I like Joe's
idea of a way to toggle the representations inline so they can be copied. I
don't think a tooltip is useful, but maybe a tooltip showing the size in
bytes/KB/MB is.

> +	       if (tooltip) parmTreeElement.tooltip = tooltip;

Should be on two lines, if we need it.