Pretty Print HTML resources (including inline <script>/<style>) Some pretty printing opportunities: - Clean up element attributes to always be quoted - Clean up indenting to always be consistent After pretty printing it would be important to: - Set breakpoints in inline <script> - Step through pretty printed inline <script> - Ensure Style sidebar links to <style> go to the right place
<rdar://problem/29119232>
Created attachment 378160 [details] [PATCH] WIP - Pretty Printing This gets pretty-printing itself working but doesn't have the SourceMap indexing implemented. So it doesn't know how to take you to the right place.
Created attachment 378161 [details] [IMAGE] HTMLFormatter Tool
Created attachment 378272 [details] [IMAGE] SourceMap Tool
Created attachment 378273 [details] [PATCH] WIP - Pretty Printing and Stepping Through Formatted <script> Next thing that needs to be added is the ability to add breakpoints in inline scripts. But stepping through code works just fine!!
Created attachment 378274 [details] [TEST] Simple Test Page with Inline <script>
Comment on attachment 378273 [details] [PATCH] WIP - Pretty Printing and Stepping Through Formatted <script> View in context: https://bugs.webkit.org/attachment.cgi?id=378273&action=review > Source/WebInspectorUI/Tools/SourceMaps/External:1 > +../../UserInterface/External This is a symlink, I probably shouldn't include this. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:26 > +// FIXME: Map line/column location information into the builder. Fixed! > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:109 > + // FIXME: XML mode, self-closing should be disabled to allow for simplicity. For now this is "HTMLFormatter" so I'll remove this comment. But we could have a mode of basic XML pretty printing.
Created attachment 378423 [details] [PATCH] Proposed Fix
Hmm one case is: > <meta http-equiv="Content-type" content="text/html; charset=utf-8" /> This currently pretty prints to: > <meta http-equiv="Content-type" content="text/html; charset=utf-8"> But it should probably keep the self-close and be: > <meta http-equiv="Content-type" content="text/html; charset=utf-8"/>
Created attachment 378429 [details] [PATCH] Proposed Fix Keep a self closing tag "<br />" outputting as "<br/>" instead of just "<br>". Since we want to be a whitespace only pretty printer.
Created attachment 378471 [details] [PATCH] Proposed Fix Fix broken tests, and add new tests for pause locations in inline <script>.
Created attachment 378472 [details] [PATCH] Proposed Fix
Oops, easy test fixes. Apparently WI.Resource get scripts() is not in a sorted order!
Created attachment 378503 [details] [PATCH] Proposed Fix
Comment on attachment 378472 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378472&action=review Nice work! This is really awesome =D As is, I think this is fantastic, but some of the tests do strip some trailing "invalid" content, which I don't think we should do. My "ideal" would be for the original and formatted content to be completely equal if all whitespace was removed from both. > LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-inline-script-pause-locations-expected.txt:178 > +-=> 3 | <script>console.log("here 1");document.getElementById("one").addEventListener("click", (event) => { console.log("one"); console.log(event); });</script> Why does it dump this <script> after the second <script>? Shouldn't it go before? I think we need to do some sort somewhere, perhaps for `WI.Resource.get scripts`, based on the id/line/column. > LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-inline-script-pause-locations.html:11 > + let suite = InspectorTest.createAsyncSuite("Debugger.resolvedBreakpoint.dumpAllLocations.InlineScript"); NIT: I personally prefer to have all "similar" tests share the same suite name, and then each test case have a sub-"classed" name off of that, so you can better relate tests to each other (especially if some sub-test needs to have a special configuration for a particular platform). > LayoutTests/inspector/debugger/breakpoints/resources/dump.js:27 > let script = window.findScript(scriptRegex); You already have a `script` argument, so you can remove this (it would also throw an error, since `scriptRegex` isn't defined). > LayoutTests/inspector/debugger/breakpoints/resources/dump.js:67 > + name, test(resolve, reject) { Style: please put `name` and `test` on separate lines. > LayoutTests/inspector/debugger/breakpoints/resources/dump.js:68 > + window.loadLinesFromSourceCode(resource).then((lines) => { Can we make this test `async` instead of having this giant indent? > LayoutTests/inspector/debugger/breakpoints/resources/dump.js:73 > + InspectorTest.debug(); Oops. > LayoutTests/inspector/debugger/breakpoints/resources/dump.js:77 > + for (let line = script.range.startLine; line <= script.range.endLine; ++line) { NIT: you could call this `lineNumber` and make the object construction simpler. > LayoutTests/inspector/debugger/breakpoints/resources/dump.js:79 > + for (let column = 0; column <= max; ++column) { Ditto (77). > LayoutTests/inspector/debugger/resources/log-pause-location.js:21 > + for (let resource of frame.resourceCollection) { I realize this wouldn't affect any tests, but what about subframes? > LayoutTests/inspector/formatting/resources/formatting-utilities.js:25 > + InspectorTest.log("Input:"); Aside: I wish we could actually diff the outputs :( > LayoutTests/inspector/formatting/resources/formatting-utilities.js:64 > + timeout: -1, Style: I typically put `timeout` after the `test`. > LayoutTests/inspector/formatting/resources/html-tests/attributes.html:17 > +<p a=1 b=2></p> I realize that what I'm about to suggest isn't really "valid" HTML, but it may be something we'll want to support anyways, just so we can pretty-print the rest of the content around it. What about some tests for attributes that don't have a name (`<p "value"></p>`) or attributes that have a numeric name (`<p 1></p>`) or attributes in the closing tag (`<p></p attr="value">`)? > LayoutTests/inspector/formatting/resources/html-tests/attributes.html:19 > +<p lol, nice test :P > LayoutTests/inspector/formatting/resources/html-tests/eof-1-expected.html:1 > +<p>foo This is a bit odd. At the very least, I'd expect the "remainder" text to be added as a newline to the very end of the pretty printed output. > LayoutTests/inspector/formatting/resources/html-tests/inline-script-expected.html:5 > +(function(a, b, c) { Part of me thinks that we should indent inline script/stylesheet contents by one additional level, so it's easier to distinguish script/stylesheet content from HTML content. > LayoutTests/inspector/formatting/resources/html-tests/inline-script-expected.html:37 > +<!-- Script is: `<a><b></a></b>` --> > +<script><a><b></a></b></script> > + > +<!-- Script is: `let str = "` --> > +<script>let str = "</script> > +"; Nice! > LayoutTests/inspector/formatting/resources/html-tests/list-expected.html:17 > + <li> > + One > + <li> > + Two > + <li>Three I think most developers would treat these as siblings, not descendants. ``` <ul> <li>one <li>two <li>three </ul> ``` I realize that that's a "detail" of HTML, but I think it's valid to make our pretty printer be as close to actual HTML output as possible without modifying anything other than whitespace. > LayoutTests/inspector/formatting/resources/html-tests/not-well-formed-1-expected.html:2 > + </br><p>Test</p> Along the lines of inspector/formatting/resources/html-tests/list-expected.html:13, this should probably do a similar thing: ``` <p> </br> <p>Test</p> ``` > LayoutTests/inspector/formatting/resources/html-tests/not-well-formed-2-expected.html:2 > + </z><b> This should probably still be on a newline, even though `</z>` is crazy wrong. > LayoutTests/inspector/formatting/resources/html-tests/not-well-formed-2.html:1 > +<a></z><b></z><c></z></c></b></a> Good thinking! Along these lines, what about something like "<<a>><<b>><</b>><</a>>" or anything as equally gross? > LayoutTests/inspector/formatting/resources/source-map-utilities.js:6 > + let startLine = Math.max(line - linesOfContext, 0); Style: I prefer to put the "lower bound" number first, so it's more logical to read. > LayoutTests/inspector/formatting/resources/source-map-utilities.js:15 > + output += caret; Perhaps as a sort of "quick sanity check", we should assert that the character before/after the caret (ignoring any whitespace) is the same between the original and formatted strings? That would make it _really_ obvious where there are issues. > LayoutTests/inspector/formatting/resources/source-map-utilities.js:54 > + InspectorTest.log(`Original Position: ${i} (${line}, ${column})`); For the sake of brevity, we could drop the " Position". > LayoutTests/inspector/formatting/resources/source-map-utilities.js:89 > + async function loadFormattedContentAndSourceMap(mode, testText, testName, testURL) { Style: please declare this above where it's used. > LayoutTests/inspector/formatting/resources/source-map-utilities.js:114 > + async function loadSourceMapTestResource(testURL) { Ditto (89). > LayoutTests/inspector/formatting/source-map-css-1-expected.txt:10 > + 0| @#media(max-device-width:480px){body{color:red;background:blue}} This test format is awesome :D > LayoutTests/inspector/formatting/source-map-html-1.html:11 > + addSourceMapTest(suite, "text/html", "1", "resources/html-source-map-tests/1.html"); Where's inspector/formatting/source-map-html-1-expected.txt? > Source/WebInspectorUI/ChangeLog:28 > + - renames "EsprimaFormatter" to "JSFormatter" Yay! > Source/WebInspectorUI/ChangeLog:29 > + - eliminates an extra trailing newline in CSSFormatter output +1 > Source/WebInspectorUI/ChangeLog:30 > + - renames "Tools/PrettyPrinting" to "Tools/LegacyPrettyPrinting" which will be removed soon. \(0.0)/ > Source/WebInspectorUI/ChangeLog:53 > + Renamed this tool. We should just wait till <https://webkit.org/b/201624> lands and remove all of this from this patch. > Source/WebInspectorUI/Tools/HTMLFormatter/index.html:39 > + <option value="svg-simple">Simple SVG Document</option> Should we have an `xml-simple` for debugging? > Source/WebInspectorUI/Tools/HTMLFormatter/index.html:197 > +<!-- Copyright © 2014 Apple Inc. All rights reserved. --> I'm somewhat surprised you didn't use Source.svg :P > Source/WebInspectorUI/Tools/HTMLFormatter/styles.css:2 > + font-family: Arial, sans-serif; Style: I'd reorder most of these properties :( > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1599 > + if (!selectedTreeElement) You mention that this avoids an exception. Do you have a backtrace? This shouldn't be possible :( > Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1763 > + if (!selectedTreeElement) Ditto. > Source/WebInspectorUI/UserInterface/Views/TextEditor.js:185 > + return mode === "javascript" || mode === "css" || mode === "htmlmixed"; What about "xml"? If anything, I'd expect that to be easier to pretty print than HTML, since more structured (e.g. required closing tags). Perhaps a followup? > Source/WebInspectorUI/UserInterface/Views/TextEditor.js:910 > + else if (mode === "htmlmixed") NIT: make this into a switch-case? > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:93 > get indented() I just realized, we could probably use this to replace the `depth` value inside `CSSFormatter`. > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:143 > + console.assert(!string.includes("\n"), "Appended a string with newlines. This breaks the source map."); Nice! This would've helped avoid <https://webkit.org/b/201559>. > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:145 > this._append(string); What about also asserting/early-returning if the string is empty? > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:151 > + appendTokenWithPossibleNewlines(string, originalPosition) Given that you use `string` as the parameter name, perhaps this should be `appendStringWithPossibleNewlines`? Either that or we should rename `string` to `token`. > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:157 > + let line = lines[i]; If there are multiple newlines in a row, `line` could be an empty string, which would muck with some of this object's state. I'd either early-return inside `appendToken` or wrap the call in an `if` here. > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:163 > + currentPosition += 1; ++currentPosition > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:256 > + indentToLevel(level) Why isn't this just a setter? It seems way more complex than it needs to be, unless you're trying to future-proof if `indent()`/`dedent()` add more functionality? > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:275 > + if (this.lastNewlineAppendWasMultiple) > + this.removeLastNewline(); This should probably be a loop of `removeLastNewline`, as the multiple could be more than 2. ``` while (this.lastTokenWasNewline) this.removeLastnewline(); this.appendNewline(); ``` > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:28 > + constructor(sourceText, indentString = " ") If we wanted to add XML pretty printing, I'd probably include the `sourceType`. Alternatively, you could probably pretty print XML with regular expressions, so maybe we should just have a different class. NIT: for the sake of consistency, we could also allow this to take a `builder` :P > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:63 > + get success() Style: this can be one-lined. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:96 > + if (node.children) { NIT: we could invert this and just early return `!node.children` rather than indent all the content. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:122 > + Style: unnecessary newline. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:146 > + let q; Style: please give this a default value, like `""`, or add a `default` to the switch-case. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:174 > + if (node.type === HTMLTreeBuilderFormatter.NodeType.Node) { switch-case? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:190 > + if (node.__shouldHaveNoChildren) { Style: unnecessary braces. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:219 > + if (this._hasMultipleNewLines(textString)) This could use a comment, like "if the content has multiple newlines, we should put each line on its own line, so add a newline between the opening tag and the first line of content". Do we also add a newline after the text to ensure that "<p>a\nb\nc</p>` renders with each on their own line? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:229 > + let openerString = node.opener ? node.opener : "<!--"; If only we could use `??` :( > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:258 > + if (node.type === HTMLTreeBuilderFormatter.NodeType.Node) { Ditto (174). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:280 > + let closingCommentString = node.opener ? ">" : "-->"; Interesting. What is the situation where we'd only want to output `>` as the closing tag of a comment? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:322 > + Style: unnecessary newline. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:355 > + "area", "base", "basefont", "br", "canvas", "col", "command", "embed", "frame", NIT: I personally hate this type of formatting, as there's basically no situation where you get a clean diff for any non-minor change. I'd prefer if you either made it all one line (yuck) or separated each onto their own line (yay). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:357 > + "wbr", "track", "menuitem" Style: missing trailing comma. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:59 > + _peek(n = 1) This isn't used. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:64 > + _peekChararacter(c) I think a more accurate name would be `_atCharacter` or `_isCurrentCharacter`. Furthermore, isn't this the same as `_peekString`? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:69 > + _peekRegex(regex) In my mind, `_peek*` means to "get me the next * characters", which is not what these `_peek*` functions do. I'd prefer you name them `_match*` or `_currentIs*` or `_frontMatches` or something else. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:77 > + let c = str[i]; You could inline this. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:87 > + console.assert(str.toLowerCase() === str, "Pass value in as lowercase..."); This is an odd assertion message. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:90 > + let c = str[i]; Ditto (77). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:131 > + _consumeUntilRegex(regex, newMode) This is never used. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:197 > + let text = this._consumeUntilString("<"); > + scriptText += text; Inline this? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:223 > + } > + // DOCTYPE Style: missing newline. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:231 > + } > + // CDATA Ditto (222). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:237 > + } > + // Bogus Comment. Ditto (222). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:268 > + } > + if (this._peekChararacter("/")) { Ditto (222). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:276 > + } > + if (this._peekChararacter(">")) { Ditto (222). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:297 > + if (this._peek("/>")) { Do you mean `_peekString("/>")`? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:304 > + } > + if (this._peekChararacter(">")) { Ditto (222). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:335 > + } > + if (this._peekChararacter(`'`)) { Ditto (222). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:340 > + } > + if (this._peekChararacter(">")) { Ditto (222). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:405 > + this._pushNode(node); > + } > + > + _pushNode(node) Why not merge these functions? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:64 > + if (parserNode.type === HTMLParser.NodeType.OpenTag) { switch-case? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:84 > + if (parserNode.type === HTMLParser.NodeType.OpenTag) { Ditto (64). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:177 > +HTMLTreeBuilderFormatter.TagsWithoutChildren = new Set([ NIT: perhaps this should be `TagNamesWithoutChildren`?
Comment on attachment 378472 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378472&action=review >> LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-inline-script-pause-locations-expected.txt:178 >> +-=> 3 | <script>console.log("here 1");document.getElementById("one").addEventListener("click", (event) => { console.log("one"); console.log(event); });</script> > > Why does it dump this <script> after the second <script>? Shouldn't it go before? I think we need to do some sort somewhere, perhaps for `WI.Resource.get scripts`, based on the id/line/column. This was fixed in a follow-up. The scripts are now sorted. >> LayoutTests/inspector/debugger/breakpoints/resources/dump.js:27 >> let script = window.findScript(scriptRegex); > > You already have a `script` argument, so you can remove this (it would also throw an error, since `scriptRegex` isn't defined). This was fixed in a follow-up patch. >> LayoutTests/inspector/debugger/breakpoints/resources/dump.js:79 >> + for (let column = 0; column <= max; ++column) { > > Ditto (77). All of this code was pre-existing and just ported to resources. >> LayoutTests/inspector/debugger/resources/log-pause-location.js:21 >> + for (let resource of frame.resourceCollection) { > > I realize this wouldn't affect any tests, but what about subframes? I think that is handled by the `frame of WI.networkManager.frames` loop. In fact this resource is a subframe main resource. >> LayoutTests/inspector/formatting/resources/html-tests/attributes.html:17 >> +<p a=1 b=2></p> > > I realize that what I'm about to suggest isn't really "valid" HTML, but it may be something we'll want to support anyways, just so we can pretty-print the rest of the content around it. > > What about some tests for attributes that don't have a name (`<p "value"></p>`) or attributes that have a numeric name (`<p 1></p>`) or attributes in the closing tag (`<p></p attr="value">`)? Good idea, added. >> LayoutTests/inspector/formatting/resources/html-tests/eof-1-expected.html:1 >> +<p>foo > > This is a bit odd. At the very least, I'd expect the "remainder" text to be added as a newline to the very end of the pretty printed output. I handled unexpected EOF all over the place to append the final incomplete token as ErrorText which the formatter just outputs like a Text node. >> LayoutTests/inspector/formatting/resources/html-tests/list-expected.html:17 >> + <li>Three > > I think most developers would treat these as siblings, not descendants. > > ``` > <ul> > <li>one > <li>two > <li>three > </ul> > ``` > > I realize that that's a "detail" of HTML, but I think it's valid to make our pretty printer be as close to actual HTML output as possible without modifying anything other than whitespace. This is an enhancement we can do later. I agree this would be nice to do. >> LayoutTests/inspector/formatting/resources/html-tests/not-well-formed-1-expected.html:2 >> + </br><p>Test</p> > > Along the lines of inspector/formatting/resources/html-tests/list-expected.html:13, this should probably do a similar thing: > ``` > <p> > </br> > <p>Test</p> > ``` Sure. The </br> is error text so the tree doesn't pop. I've added a newline after any Error text, so this becomes: <p> </br> <p>Test</p> >> LayoutTests/inspector/formatting/resources/html-tests/not-well-formed-2.html:1 >> +<a></z><b></z><c></z></c></b></a> > > Good thinking! > > Along these lines, what about something like "<<a>><<b>><</b>><</a>>" or anything as equally gross? Good test. The current formatter hit some assertions. I missed handling non-ascii in parseOpenTag. That now becomes a text node. So this becomes: Output: < <a> >< <b> >< </b> >< </a> > >> LayoutTests/inspector/formatting/resources/source-map-utilities.js:15 >> + output += caret; > > Perhaps as a sort of "quick sanity check", we should assert that the character before/after the caret (ignoring any whitespace) is the same between the original and formatted strings? That would make it _really_ obvious where there are issues. I like that! >> LayoutTests/inspector/formatting/source-map-html-1.html:11 >> + addSourceMapTest(suite, "text/html", "1", "resources/html-source-map-tests/1.html"); > > Where's inspector/formatting/source-map-html-1-expected.txt? The test and output are in this diff: inspector/formatting/source-map-html-1.html inspector/formatting/source-map-html-1-expected.txt >> Source/WebInspectorUI/Tools/HTMLFormatter/index.html:39 >> + <option value="svg-simple">Simple SVG Document</option> > > Should we have an `xml-simple` for debugging? That is what this amounts to. >> Source/WebInspectorUI/Tools/HTMLFormatter/styles.css:2 >> + font-family: Arial, sans-serif; > > Style: I'd reorder most of these properties :( All of these Tools just copy the styles. I find its easier to just copy them as is instead of making it different. >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1599 >> + if (!selectedTreeElement) > > You mention that this avoids an exception. Do you have a backtrace? This shouldn't be possible :( This is happening all over the place in trunk. Reloads. Drag a breakpoint off the gutter to delete it. >> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:185 >> + return mode === "javascript" || mode === "css" || mode === "htmlmixed"; > > What about "xml"? If anything, I'd expect that to be easier to pretty print than HTML, since more structured (e.g. required closing tags). Perhaps a followup? Lets do xml separately, since it will be slightly different. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:157 >> + let line = lines[i]; > > If there are multiple newlines in a row, `line` could be an empty string, which would muck with some of this object's state. I'd either early-return inside `appendToken` or wrap the call in an `if` here. I added an `if (line)`. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:256 >> + indentToLevel(level) > > Why isn't this just a setter? It seems way more complex than it needs to be, unless you're trying to future-proof if `indent()`/`dedent()` add more functionality? Precisely. I want this to be calling indent/dedent in case those have extra logic. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:275 >> + this.removeLastNewline(); > > This should probably be a loop of `removeLastNewline`, as the multiple could be more than 2. > ``` > while (this.lastTokenWasNewline) > this.removeLastnewline(); > this.appendNewline(); > ``` Hmm, I'm not sure we will expect a appendMultipleNewlines more than 2. I'll try this out though. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:219 >> + if (this._hasMultipleNewLines(textString)) > > This could use a comment, like "if the content has multiple newlines, we should put each line on its own line, so add a newline between the opening tag and the first line of content". > > Do we also add a newline after the text to ensure that "<p>a\nb\nc</p>` renders with each on their own line? This is specifically for whitespace only text nodes, if the whitespace has multiple newlines we include an empty line in the output. So this document: <!-- Title --> <title>Test</title> <!-- Styles --> <link rel="stylesheet" href="foo.css"> Will keep the blank line above `<!-- Styles -->` since it was a whitespace text node with multiple newlines. Note it collapses multiple blank lines to a single blank line. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:280 >> + let closingCommentString = node.opener ? ">" : "-->"; > > Interesting. What is the situation where we'd only want to output `>` as the closing tag of a comment? In the case of bogus comments. For example: <?xml version="1.0" encoding="UTF-8"?> Will have a opener of "<?" and text until the ending "?". So we end it with a ">". >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:59 >> + _peek(n = 1) > > This isn't used. This is for debugging. So you can pause in the formatter and peek and the next few characters. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:69 >> + _peekRegex(regex) > > In my mind, `_peek*` means to "get me the next * characters", which is not what these `_peek*` functions do. I'd prefer you name them `_match*` or `_currentIs*` or `_frontMatches` or something else. `peek` and `consume` are a common tokenizer / parser naming convention. As is the `expect` / `match`. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:131 >> + _consumeUntilRegex(regex, newMode) > > This is never used. Lol, thanks! >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:297 >> + if (this._peek("/>")) { > > Do you mean `_peekString("/>")`? Oops, yes! >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:405 >> + _pushNode(node) > > Why not merge these functions? Done. Originally I was going to put the tree builder in this class and _pushNode was the starting point. I then moved it out.
Created attachment 378600 [details] [PATCH] Proposed Fix
Comment on attachment 378600 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378600&action=review > LayoutTests/inspector/formatting/resources/source-map-utilities.js:92 > + let nextOriginalChar = originalLines[line][column]; > + let nextFormattedChar = formattedLines[formattedLine][formattedColumn]; > + if (nextOriginalChar !== undefined) > + InspectorTest.assert(nextOriginalChar === nextFormattedChar, `FAIL: Mapping appears to be to a different token. ${JSON.stringify(nextOriginalChar)} => ${JSON.stringify(nextFormattedChar)}`); This assertion a few legit issues. - The closing tag for comments (and doctype, cdata, etc) need to have their position in case the inner text ended with a newline. - One of the attribute cases was not outputting the `namePos`. Also now we only have to search the output for "FAIL" for the obvious bad cases (of which there should be none right now!). > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:475 > + ErrorText: "error-text", This new node type is for EOF remaining text. Our TreeBuilder formats passes this through as text and HTMLFormatter outputs it as text.
I can mark these tests as SLOW. They indeed have a lot of output and timed out on debug WK1 bots: inspector/formatting/source-map-html-1.html inspector/formatting/source-map-html-2.html inspector/formatting/source-map-javascript-1.html inspector/formatting/source-map-css-1.html
Not sure why the wincairo both failed to apply. This file is in the patch: LayoutTests/inspector/formatting/resources/html-tests/not-well-formed-3.html
Created attachment 378658 [details] [PATCH] Proposed Fix Lets see if the bots do better on this patch.
Comment on attachment 378658 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378658&action=review r=me, given how large this patch is, it's getting very difficult to track differences between each iteration, but the tests look great and I think I've looked at enough of the code enough times to think that this is good to go. Please run this through ESLint once before landing, as I did notice some style issues (I pointed out the ones I saw, but there may be others). Also, it looks like inspector/formatting/source-map-html-1.html is still too slow for some reason, so that needs to be fixed as well. Fantastic work! Please make some followup bugs for some of the other things we've discussed: - ensure that `<p><p>` are siblings, not parent-child - add XML/SVG support > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:120 > + appendNonToken(string) Why does this need to exist? Shouldn't every character that isn't whitespace be considered a token? > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:143 > + console.assert(!string.includes("\n"), "Appended a string with newlines. This breaks the source map."); We should also assert that `!/\s$/.test(string)`, since any trailing whitespace should really be handled by `appendSpace()`. > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:155 > + Style: extra newline > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:178 > + this.lastTokenWasWhitespace = false; Shouldn't this be `true` if we just added an indent? > Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:303 > + let indent = this._indentCache[this._indent]; > + if (indent) This seems wrong. If it doesn't exist in `this._indentCache`, shouldn't we add it? Is it even possible to hit this? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:100 > + for (let childNode of children) { NIT: this could just be `child` > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:123 > + Style: extra newline > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:144 > + let {name, value, quote, namePos, valuePos} = attr; Why not just destructure this in the parameters list? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:147 > + let q; Style: this needs a default value > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:293 > + let closingDoctypeString = ">"; Style: `const` > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:300 > + let closingCDataString = "]]>"; Ditto (293) > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:32 > + console.assert(treeBuilder); What about an assertion for `sourceText`? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:54 > + _isEOF() Aside: we could totally make a base `Parser` class that holds all of these functions, as I'm sure they'd also be useful elsewhere > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:64 > + _peekChararacter(c) This is spelled very wrong lol. Can we merge this with `_peekString`? Since there's no real difference between a character and a string in JS, we could just have `_peekString` handle single character peeks. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:69 > + _peekRegex(regex) This name may be a bit misleading, because it only peeks a single character, and `RegExp` can handle more than that. Perhaps this should be `_peekCharacterRegex`? Or even better, assert that `regex.source.startsWith("^")` and allow it to test `this._data.substring(this._pos)`. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:90 > + let c = str[i]; This should be moved lower (or inlined) so that it isn't evaluated if we take the `!d` branch. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:106 > + Style: extra whitespace > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:276 > + if (this._peekChararacter("/")) { > + if (this._peekString("/>")) { Why are we doing double the effort and peeking for "/" twice? Can we not just `this._peekString("/>")`? > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:308 > + if (this._peekChararacter("/")) { > + if (this._peekString("/>")) { Ditto (275) > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:102 > + nodesToPop++; Please move this above the `if` since it's called in both branches. That way, we only need one call. > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:153 > + } Style: missing semicolon > Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:164 > + } Ditto (153)
Comment on attachment 378658 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378658&action=review >> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:120 >> + appendNonToken(string) > > Why does this need to exist? Shouldn't every character that isn't whitespace be considered a token? This does not need to exist. We can aim to phase it out. This saves a little work: - for output characters that immediately follow tokens such that we don't need to provide an additional position mapping which will just ignored because it is in the right position - avoid additional original position calculations with every appendToken I think for now the '=' might need to carry over a position from the HTMLParser if we wanted to drop this entirely. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:143 >> + console.assert(!string.includes("\n"), "Appended a string with newlines. This breaks the source map."); > > We should also assert that `!/\s$/.test(string)`, since any trailing whitespace should really be handled by `appendSpace()`. Hmm, I'm not sure that matters in all cases. I can imagine a TextNode in HTML may end with whitespace. You could argue that we could strip that at any point, but I'm going to leave this as is for now. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:178 >> + this.lastTokenWasWhitespace = false; > > Shouldn't this be `true` if we just added an indent? Sounds good. I also dropped the `_startOfLine = false`. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:303 >> + if (indent) > > This seems wrong. If it doesn't exist in `this._indentCache`, shouldn't we add it? Is it even possible to hit this? The check above this ensures something is in the cache or not already. This check avoids empty string appends (when indent is 0). >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:144 >> + let {name, value, quote, namePos, valuePos} = attr; > > Why not just destructure this in the parameters list? Readability. Not all things that are possible should be done. It is easier to read and understand this code when the method signature shows it takes an Attr node. If the destructure were to happen in the parameter list it is not necessarily clear that those properties are the properties on an Attr node unless you already knew what the properties were on an Attr node. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:147 >> + let q; > > Style: this needs a default value I don't see anything wrong with this. It is a valid initialization and we do this in hundreds of places in our code base. It would be considered a "dead store" in most static analyzers unless we were to rewrite the following code just to make the initial assignment do something useful. I will make the default case assign an empty string to ensure all cases provide a value. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:293 >> + let closingDoctypeString = ">"; > > Style: `const` FWIW these comments don't appear to add any value. `let` or `const` here isn't going to change the meaning of this code, runtime behavior of the code, or make it more or less readable. I realize these values can be inlined but I pulled them out into a variable to enhance readability ("why this string, oh the variable name"). Do you see an advantage to const here other than it could be const? >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:32 >> + console.assert(treeBuilder); > > What about an assertion for `sourceText`? Good idea! >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:64 >> + _peekChararacter(c) > > This is spelled very wrong lol. > > Can we merge this with `_peekString`? Since there's no real difference between a character and a string in JS, we could just have `_peekString` handle single character peeks. Hah, how have we missed that until now! I've switched to using _peekString everywhere. My original concern was performance but this performs well always using _peekString. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:69 >> + _peekRegex(regex) > > This name may be a bit misleading, because it only peeks a single character, and `RegExp` can handle more than that. Perhaps this should be `_peekCharacterRegex`? Or even better, assert that `regex.source.startsWith("^")` and allow it to test `this._data.substring(this._pos)`. I want to avoid creating substrings. I've named it _peekCharacterRegex. >> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:276 >> + if (this._peekString("/>")) { > > Why are we doing double the effort and peeking for "/" twice? Can we not just `this._peekString("/>")`? Good catch.
Comment on attachment 378658 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378658&action=review >>> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:178 >>> + this.lastTokenWasWhitespace = false; >> >> Shouldn't this be `true` if we just added an indent? > > Sounds good. I also dropped the `_startOfLine = false`. Hmm, changing these hurt tests in a bad way (closing </style> / </script>). I've reverted to what was in the patch.
Created attachment 378719 [details] [PATCH] For Landing
Comment on attachment 378719 [details] [PATCH] For Landing Clearing flags on attachment: 378719 Committed r249831: <https://trac.webkit.org/changeset/249831>
Comment on attachment 378658 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=378658&action=review >>> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:120 >>> + appendNonToken(string) >> >> Why does this need to exist? Shouldn't every character that isn't whitespace be considered a token? > > This does not need to exist. We can aim to phase it out. > > This saves a little work: > > - for output characters that immediately follow tokens such that we don't need to provide an additional position mapping which will just ignored because it is in the right position > - avoid additional original position calculations with every appendToken > > I think for now the '=' might need to carry over a position from the HTMLParser if we wanted to drop this entirely. Ah, I see. The "don't add a mapping" part wasn't immediately obvious, but that does make sense. It seems like the difference between `appendToken` and `appendNonToken` is worth a comment, or perhaps a more explicit name. >>> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:143 >>> + console.assert(!string.includes("\n"), "Appended a string with newlines. This breaks the source map."); >> >> We should also assert that `!/\s$/.test(string)`, since any trailing whitespace should really be handled by `appendSpace()`. > > Hmm, I'm not sure that matters in all cases. I can imagine a TextNode in HTML may end with whitespace. You could argue that we could strip that at any point, but I'm going to leave this as is for now. Really, the only reason I suggested this is so that `lastTokenWasWhitespace` is kept as accurate as possible. If `string` ends with a whitespace character, as it is now, `lastTokenWasWhitespace` would be `false`, which isn't really accurate. >>>> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:178 >>>> + this.lastTokenWasWhitespace = false; >>> >>> Shouldn't this be `true` if we just added an indent? >> >> Sounds good. I also dropped the `_startOfLine = false`. > > Hmm, changing these hurt tests in a bad way (closing </style> / </script>). I've reverted to what was in the patch. We likely still need `this._startOfLine = false;` given that we would've just indented. I think the only thing we need to do is check whether the last character added was a whitespace, and if so set `lastTokenWasWhitespace` accordingly. >>> Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js:303 >>> + if (indent) >> >> This seems wrong. If it doesn't exist in `this._indentCache`, shouldn't we add it? Is it even possible to hit this? > > The check above this ensures something is in the cache or not already. This check avoids empty string appends (when indent is 0). Ah, I see. That is very subtle :( >>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:147 >>> + let q; >> >> Style: this needs a default value > > I don't see anything wrong with this. It is a valid initialization and we do this in hundreds of places in our code base. It would be considered a "dead store" in most static analyzers unless we were to rewrite the following code just to make the initial assignment do something useful. I will make the default case assign an empty string to ensure all cases provide a value. It's not that this is "wrong", just that I've thought we always preferred to have a variable have a default value, ensuring that we never tried to do anything with an uninitialized `let`/`const`. That's why I put this as a "Style", since it's really just a stylistic difference. >>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:293 >>> + let closingDoctypeString = ">"; >> >> Style: `const` > > FWIW these comments don't appear to add any value. `let` or `const` here isn't going to change the meaning of this code, runtime behavior of the code, or make it more or less readable. I realize these values can be inlined but I pulled them out into a variable to enhance readability ("why this string, oh the variable name"). > > Do you see an advantage to const here other than it could be const? AFAIU, our usual style was to use `const` for things that wouldn't change between invocations of the program. Since this is effectively a constant value, it should use `const` instead of a let. Again, this was just a "Style", so not a functionality issue.
> > - for output characters that immediately follow tokens such that we don't need to provide an additional position mapping which will just ignored because it is in the right position > > - avoid additional original position calculations with every appendToken > > > > I think for now the '=' might need to carry over a position from the HTMLParser if we wanted to drop this entirely. > > Ah, I see. The "don't add a mapping" part wasn't immediately obvious, but > that does make sense. It seems like the difference between `appendToken` > and `appendNonToken` is worth a comment, or perhaps a more explicit name. Yeah, I originally added this for when I was adding tokens that did not exist in the original source. For example converting `<input type=text>` to `<input type="text>`. But we abandoned that approach in favor of pure whitespace changes, so really we shouldn't have any such new tokens in the output now. > >>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:147 > >>> + let q; > >> > >> Style: this needs a default value > > > > I don't see anything wrong with this. It is a valid initialization and we do this in hundreds of places in our code base. It would be considered a "dead store" in most static analyzers unless we were to rewrite the following code just to make the initial assignment do something useful. I will make the default case assign an empty string to ensure all cases provide a value. > > It's not that this is "wrong", just that I've thought we always preferred to > have a variable have a default value, ensuring that we never tried to do > anything with an uninitialized `let`/`const`. That's why I put this as a > "Style", since it's really just a stylistic difference. That has never been a style rule that I'm aware of. I seem to think it is normally done to denote initialization is immediately following in a conditional. For example: let value; if (...) value = ...; else value = ...; Fortunately we can use a ternary when the "..." sections are short enough, but if not then I think we've used the above pattern and it has always been clear to me.
*** Bug 156082 has been marked as a duplicate of this bug. ***