[webkit-reviews] review denied: [Bug 107276] Web Inspector:=?UTF-8?Q?=20Show=20requests=20in=20=60curl=60=20syntax=20in=20DevTools=20=E2=86=92=20Network=20=E2=86=92=20Headers=20?=: [Attachment 183447] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Sergey Ryazanov
<serya at chromium.org>'s request for review:
Bug 107276: Web Inspector: Show requests in `curl` syntax in DevTools → Network
→ Headers
https://bugs.webkit.org/show_bug.cgi?id=107276

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183447&action=review


Mostly looks good. Few nits inline.

> Source/WebCore/English.lproj/localizedStrings.js:89
> +localizedStrings["Copy `curl` command"] = "Copy `curl` command";

Consider renaming to "Copy as curl"?

> Source/WebCore/inspector/front-end/RequestHeadersView.js:341
> +	   var copyCurlButton =
this._createToggleButton(WebInspector.UIString("Copy `curl` command"));

I think this should be exposed as a context menu item on the network panel
items.

> Source/WebCore/inspector/front-end/RequestHeadersView.js:408
> +	   function escape(str) {

keep { on the next line.

> Source/WebCore/inspector/front-end/RequestHeadersView.js:417
> +	   var supposedMethod = "GET";

inferredMethod?

> Source/WebCore/inspector/front-end/RequestHeadersView.js:419
> +	   if (this._request.requestContentType() ==
"application/x-www-form-urlencoded" && this._request.requestFormData) {

===

> Source/WebCore/inspector/front-end/RequestHeadersView.js:423
> +	   } if (this._request.requestFormData) {

else if ?

> Source/WebCore/inspector/front-end/RequestHeadersView.js:429
> +	   if (this._request.requestMethod != supposedMethod)

!==

> Source/WebCore/inspector/front-end/RequestHeadersView.js:438
> +	   command += data;

A good pattern is to make command an array, push tokens into it and then return
command.join(""); You don't need "data" as well.