Bug 159380 – Web Inspector: ER: Copy as cURL

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Summary:
This is a frequent request for developers that want to test network / server side issues.

Other browser tools, like Firefox and Chrome have "Copy as cURL" functionality.

[Joseph Pecoraro]

<rdar://problem/17508156>

[Johan K. Jensen]

Created attachment 282849 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 282849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282849&action=review

I know you wrote tests for this! So r- until we see the tests. And try to rebaseline (update your repository) so the patch applies on the bots.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:687
> +         * Credit: Google DevTools

The Copyright at the top should be enough.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:709
> +                return "'" + str + "'";

Style: Using template strings this reads better:

    return `'${str}'`;

> Source/WebInspectorUI/UserInterface/Views/ResourceTimelineDataGridNode.js:154
> +        let disableFocus = false;
> +        contextMenu.appendItem(WebInspector.UIString("Copy as cURL"), () => {
> +            this._resource.generateCURLCommand();
> +        }, disableFocus);

You can leave off the boolean, enabled is the default. This could just be:

    contextMenu.appendItem(WebInspector.UIString("Copy as cURL"), () => { this._resource.generateCURLCommand(); });

[Johan K. Jensen]

Created attachment 282850 [details]
Patch

New patch, now with tests

[Johan K. Jensen]

Created attachment 282851 [details]
Patch

Updated with feedback from review.

[WebKit Commit Bot]

Attachment 282851 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Models/Resource.js:708:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 282851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282851&action=review

Nice! r=me

> Source/WebInspectorUI/ChangeLog:6
> +        Reviewed by Joseph Pecoraro.

This is not yet reviewed by me, so it should have the NOBODY OOPS line here.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:687
> +        /**
> +         * Escape util function for POSIX oriented operating systems.
> +         */

This is just repeating the name of the function, no need of this comment.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:696
> +            function escapeCharacter(x) {
> +                let code = x.charCodeAt(0);
> +                if (code < 256) {
> +                    // Add leading zero when needed to not care about the next character.
> +                    return code < 16 ? "\\x0" + code.toString(16) : "\\x" + code.toString(16);
> +                }
> +                code = code.toString(16);
> +                return "\\u" + ("0000" + code).substr(code.length, 4);

This could reach much nicer as:

    let code = x.charCodeAt(0);
    let hex = code.toString(16);
    if (code < 16)
        return "\\x0" + hex;
    if (code < 256)
        return "\\x" + hex;
    return "\\u" + ("0000" + hex).substr(hex.length, 4);

And it might be even better with a leftPad like function.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:699
> +            if (/[^\x20-\x7E]|\'/.test(str)) {

Should be no need to escape this single quote.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:701
> +                return "$\'" + str.replace(/\\/g, "\\\\")

Should be no need to escape this single quote.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:702
> +                                  .replace(/\'/g, "\\\'")

This could be:

    .replace(/'/g, "\\'")

Escaping a single quote in the JavaScript RegExp or string does nothing.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:712
> +        var command = ["curl " + escapeStringPosix(this.url).replace(/[[{}\]]/g, "\\$&")];

Style: `let` instead of `var`

> LayoutTests/http/tests/inspector/network/copy-as-curl.html:33
> +    request.open("GET", "resources/url?utf8=ð", true);

This html file should probably have the appropriate meta tag if you are embedding UTF8 content in it:

  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">

> LayoutTests/http/tests/inspector/network/copy-as-curl.html:108
> +                InspectorTest.expectThat(curl.find((cmd) => cmd.includes('X-Custom1')) === "-H 'X-Custom1: test1'", "Command should have correct custom header 1.");
> +                InspectorTest.expectThat(curl.find((cmd) => cmd.includes('X-Custom2')) === "-H $'X-Custom2\\'%: \\'Test\\'\\'1\\\\\\'2'", "Command should have correct custom header 2.");
> +                InspectorTest.expectThat(curl.find((cmd) => cmd.includes('X-Custom3')) === "-H $'X-Custom3: \\'${PWD}'", "Command should have correct custom header 3.");

Nice! This is easy to read!

[Joseph Pecoraro]

Comment on attachment 282851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282851&action=review

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:696
>> +                return "\\u" + ("0000" + code).substr(code.length, 4);
> 
> This could reach much nicer as:
> 
>     let code = x.charCodeAt(0);
>     let hex = code.toString(16);
>     if (code < 16)
>         return "\\x0" + hex;
>     if (code < 256)
>         return "\\x" + hex;
>     return "\\u" + ("0000" + hex).substr(hex.length, 4);
> 
> And it might be even better with a leftPad like function.

Even better, String.prototype.padStart exists!

    let code = x.charCodeAt(0);
    let hex = code.toString(16);
    if (code < 256)
        return "\\x" + hex.padStart(2, "0");
    return "\\u" + hax.padStart(4, "0");

[Johan K. Jensen]

Created attachment 283197 [details]
Patch

[WebKit Commit Bot]

Attachment 283197 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Models/Resource.js:703:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 283197 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283197&action=review

r=me

> Source/WebInspectorUI/UserInterface/Models/Resource.js:696
> +                return "$\'" + str.replace(/\\/g, "\\\\")

Can still remove the \ in \' here.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:720
> +        let curlCommand = command.join(" \\\n");

I wonder if we should have some extra whitespace here.

So instead of:

    curl 'foo' \
    -XGET \
    -H 'Accept: */*' \
    ...

We would get:

    curl 'foo' \
        -XGET \
        -H 'Accept: */*' \
        ...

Not too important though.

[Johan K. Jensen]

Created attachment 283430 [details]
Patch

[WebKit Commit Bot]

Attachment 283430 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Models/Resource.js:703:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 283430 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 283430 [details]
Patch

Clearing flags on attachment: 283430

Committed r203132: <http://trac.webkit.org/changeset/203132>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.