Web Inspector: JavaScript formatting of single statement arrow function can be poor

[Joseph Pecoraro]

JavaScript formatting of single statement arrow function can be poor

Test:
```
() => { 1 }
() => { for(x of []); }
() => { try{1}catch{2} }
```

Expected:
```
() => {1}
() => {
    for (x of [])
        ;
}
() => {
    try {
        1
    } catch {
        2
    }
}
```

Actual:
```
() => {1}
() => {for (x of [])
        ;
}
() => {try {
        1
    } catch {
        2
    }
}
```

[Joseph Pecoraro]

Created attachment 376447 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 376447 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:309
> +            let isSingleStatementArrowFunctionWithUnlikelyNewlineContent = node.parent.type === "ArrowFunctionExpression" && node.body.length === 1 && !this._isLikelyToHaveNewline(node.body[0]);

Perhaps a better name is "WithUnlikelyMultilineContent"

[Joseph Pecoraro]

Created attachment 376448 [details]
[PATCH] Proposed Fix

[Ross Kirsling]

Comment on attachment 376448 [details]
[PATCH] Proposed Fix

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

Seems like a reasonable improvement. I agree with your renaming suggestion too.

> Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:196
> +        if (nodeType === "ExpressionStatement")
> +            return false;

This shouldn't be necessary. (Unless it's meant as an optimization? But in that case it'd be incomplete, and you'd probably want to opt for a switch...)

> Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:206
> +            || nodeType === "FunctionDeclaration"

"ClassDeclaration" too, right?

[Joseph Pecoraro]

(In reply to Ross Kirsling from comment #4)
> Comment on attachment 376448 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376448&action=review
> 
> Seems like a reasonable improvement. I agree with your renaming suggestion
> too.
> 
> > Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:196
> > +        if (nodeType === "ExpressionStatement")
> > +            return false;
> 
> This shouldn't be necessary. (Unless it's meant as an optimization? But in
> that case it'd be incomplete, and you'd probably want to opt for a switch...)

Yeah, I'll just remove this. I assumed the common case would be Expression and no need to check 10 things if its almost _always_ this. But I didn't measure and it seems unlikely to be important.

> 
> > Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:206
> > +            || nodeType === "FunctionDeclaration"
> 
> "ClassDeclaration" too, right?

Yep! Thanks

[Joseph Pecoraro]

Created attachment 376515 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 376515 [details]
[PATCH] For Landing

Clearing flags on attachment: 376515

Committed r248785: <https://trac.webkit.org/changeset/248785>

[Radar WebKit Bug Importer]

<rdar://problem/54417189>