Bug 161417 – Web Inspector: Remove largest common indentation spacing in debugger popover

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Steps:
1. Open http://nv.github.io/webkit-inspector-bugs/debugger-popover-indentation/
2. Set breakpoint on line 19.
3. Reload the page.
4. Hover on MyApp.foo.bar.buz.

Expected popover content:
function(a) {
    var x = 42;
    return a + x;
}

Actual popover content:
function(a) {
                var x = 42;
                return a + x;
            }

[Radar WebKit Bug Importer]

<rdar://problem/28087730>

[Nikita Vasilyev]

Created attachment 287463 [details]
[Image] Bug

[Joseph Pecoraro]

We could just pretty print it. Which I believe has been suggested numerous times in the past.

[Devin Rousso]

Created attachment 287481 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 287481 [details]
Patch

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

r=me but with a bunch of nits. Thanks for looking into this! Sounds like it also produces syntax highlighted output? Awesome

> Source/WebInspectorUI/ChangeLog:18
> +        Removed check for invalid JSON to attempt formatting by wrapping content in "(...)".  This

This did not remove the check for invalid JSON, it loosed then conditions by which it will attempt its fuzzy formatting.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.css:178
> +    background-color: inherit

Style: Semicolon

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1479
> +            let title = content.createChild("div", "title");

Nit: I find just using the DOM APIs clearer then using these "helpers".

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1488
> +            let wrapper = content.createChild("div", "body");

Nit: I find it clearer to just use direct DOM APIs instead of these magic APIs.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1492
> +                readOnly: "nocursor",

Nice, I did not know about "nocursor!

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1500
> +            // <rdar://problem/10593948> Provide a way to change the tab width in the Web Inspector

Nit: Include a FIXME with this. And might as well group the lines pleasantly:

    // FIXME:
    let workerProxy = ...
    workerProxy.formatJavaScript(...)

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1502
> +                codeMirror.setValue(formattedText || data.description);

I assume the || lets this handle unparsable code like a "function() { [native code] }" properly?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterWorker.js:73
>          // Format invalid JSON.
>          // Some applications do not use JSON.parse but eval on JSON content. That is more permissive
>          // so try to format invalid JSON. Again no source map data since it is not code.

This comment should now be extended now that this covers more cases. Mentioning anonymous/unnamed functions, and maybe even arrow functions would be useful for future JoePeck:

    // Format invalid JSON and anonymous functions
    // Some applications do not use JSON.parse but eval on JSON content. That is more permissive
    // so try to format invalid JSON. Again no source map data since it is not code.
    // Likewise, an unnamed function's toString() produces a "function() { ... }" or even "x=>x" which
    // on its own is not a valid program. Wrap it in parenthesis to make it a function expression which is.

I wish there was a good term for this. Partial statement? Incomplete expression? Those both sound wrong.

[Joseph Pecoraro]

Comment on attachment 287481 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:18
>> +        Removed check for invalid JSON to attempt formatting by wrapping content in "(...)".  This
> 
> This did not remove the check for invalid JSON, it loosed then conditions by which it will attempt its fuzzy formatting.

"loosed then" => "loosened the"

[Joseph Pecoraro]

Comment on attachment 287481 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterWorker.js:73
>>          // so try to format invalid JSON. Again no source map data since it is not code.
> 
> This comment should now be extended now that this covers more cases. Mentioning anonymous/unnamed functions, and maybe even arrow functions would be useful for future JoePeck:
> 
>     // Format invalid JSON and anonymous functions
>     // Some applications do not use JSON.parse but eval on JSON content. That is more permissive
>     // so try to format invalid JSON. Again no source map data since it is not code.
>     // Likewise, an unnamed function's toString() produces a "function() { ... }" or even "x=>x" which
>     // on its own is not a valid program. Wrap it in parenthesis to make it a function expression which is.
> 
> I wish there was a good term for this. Partial statement? Incomplete expression? Those both sound wrong.

Actually arrow functions are valid on their own without names. So this really does apply to just unnamed function(){...}. Clip the "x=>x" text.

This means we could have gone with the /^\s*function\b/ check. But I kind of like the idea of always doing this lenient check in case something comes up in the future, or if there is something we haven't thought of.

[Devin Rousso]

Created attachment 287487 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 287487 [details]
Patch

Clearing flags on attachment: 287487

Committed r205223: <http://trac.webkit.org/changeset/205223>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Devin Rousso]

*** Bug 151865 has been marked as a duplicate of this bug. ***