Summary: | Web Inspector: Remove largest common indentation spacing in debugger popover | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, commit-queue, hi, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | GoodFirstBug, InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2016-08-30 17:25:22 PDT
Created attachment 287463 [details]
[Image] Bug
We could just pretty print it. Which I believe has been suggested numerous times in the past. Created attachment 287481 [details]
Patch
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. 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" 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. Created attachment 287487 [details]
Patch
Comment on attachment 287487 [details] Patch Clearing flags on attachment: 287487 Committed r205223: <http://trac.webkit.org/changeset/205223> All reviewed patches have been landed. Closing bug. *** Bug 151865 has been marked as a duplicate of this bug. *** |