Bug 159380 - Web Inspector: ER: Copy as cURL
Summary: Web Inspector: ER: Copy as cURL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Johan K. Jensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-01 18:30 PDT by Joseph Pecoraro
Modified: 2022-06-02 00:01 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.70 KB, patch)
2016-07-05 21:12 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff
Patch (16.76 KB, patch)
2016-07-05 21:47 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff
Patch (16.65 KB, patch)
2016-07-05 21:53 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff
Patch (16.70 KB, patch)
2016-07-08 13:59 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff
Patch (16.70 KB, patch)
2016-07-12 11:17 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-07-01 18:30:56 PDT
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.
Comment 1 Joseph Pecoraro 2016-07-01 18:31:16 PDT
<rdar://problem/17508156>
Comment 2 Johan K. Jensen 2016-07-05 21:12:34 PDT
Created attachment 282849 [details]
Patch
Comment 3 Joseph Pecoraro 2016-07-05 21:47:40 PDT
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(); });
Comment 4 Johan K. Jensen 2016-07-05 21:47:49 PDT
Created attachment 282850 [details]
Patch

New patch, now with tests
Comment 5 Johan K. Jensen 2016-07-05 21:53:45 PDT
Created attachment 282851 [details]
Patch

Updated with feedback from review.
Comment 6 WebKit Commit Bot 2016-07-05 21:54:47 PDT
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.
Comment 7 Joseph Pecoraro 2016-07-07 15:28:44 PDT
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!
Comment 8 Joseph Pecoraro 2016-07-07 15:41:01 PDT
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");
Comment 9 Johan K. Jensen 2016-07-08 13:59:03 PDT
Created attachment 283197 [details]
Patch
Comment 10 WebKit Commit Bot 2016-07-08 14:00:43 PDT
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.
Comment 11 Joseph Pecoraro 2016-07-11 11:33:45 PDT
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.
Comment 12 Johan K. Jensen 2016-07-12 11:17:36 PDT
Created attachment 283430 [details]
Patch
Comment 13 WebKit Commit Bot 2016-07-12 11:19:55 PDT
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.
Comment 14 Joseph Pecoraro 2016-07-12 15:00:34 PDT
Comment on attachment 283430 [details]
Patch

r=me
Comment 15 WebKit Commit Bot 2016-07-12 15:22:18 PDT
Comment on attachment 283430 [details]
Patch

Clearing flags on attachment: 283430

Committed r203132: <http://trac.webkit.org/changeset/203132>
Comment 16 WebKit Commit Bot 2016-07-12 15:22:23 PDT
All reviewed patches have been landed.  Closing bug.