Bug 1809941 - [devtools] Add line break after parens when content is long. r=devtools-reviewers,bomsy.

When dealing with a `(` token, we check the next tokens to find the matching
closing paren. In the same time, we compute the length of all the tokens inside
the paren.
If we reach a given length (60 at the moment) without finding the closing paren,
we consider it "long" and will treat it (and the closing paren) as line delimiter.
On top of that, we expand line delimiters to `||` and `&&` when there not in a
"short" paren, so we can split up long lines.

Differential Revision: https://phabricator.services.mozilla.com/D175477

devtools/client/debugger/dist/pretty-print-worker.js

@@ -9427,6 +9427,10 @@
       #currentCode = "";
       #currentLine = 1;
       #currentColumn = 0;
+      // The tokens parsed by acorn.
+      #tokenQueue;
+      // The index of the current token in this.#tokenQueue.
+      #currentTokenIndex;
       // The last token we added to the pretty printed code.
       #lastToken;
       // Stack of token types/keywords that can affect whether we want to add a
@@ -9440,6 +9444,7 @@
       //   - "{"
       //   - "{\n"
       //   - "("
+      //   - "(\n"
       //   - "["
       //   - "[\n"
       //   - "do"
@@ -9448,11 +9453,12 @@
       //   - "case"
       //   - "default"
       //
-      // The difference between "[" and "[\n" (as well as "{" and "{\n") is that "\n" is used
-      // when we are treating (curly) brackets as line delimiters and should increment and
-      // decrement the indent level when we find them.
+      // The difference between "[" and "[\n" (as well as "{" and "{\n", and "(" and "(\n")
+      // is that "\n" is used when we are treating (curly) brackets/parens as line delimiters
+      // and should increment and decrement the indent level when we find them.
       // "[" can represent either a property access (e.g. `x["hi"]`), or an empty array literal
       // "{" only represents an empty object literals
+      // "(" can represent lots of different things (wrapping expression, if/loop condition, function call, …)
       #stack = [];
 
       /**
@@ -9479,11 +9485,12 @@
         // After this process, tokenQueue has the following token stream:
         //
         //     [ foo, '// a', '// b', bar]
-        const tokenQueue = this.#getTokens(input);
+        this.#tokenQueue = this.#getTokens(input);
 
-        for (let i = 0, len = tokenQueue.length; i < len; i++) {
-          const token = tokenQueue[i];
-          const nextToken = tokenQueue[i + 1];
+        for (let i = 0, len = this.#tokenQueue.length; i < len; i++) {
+          this.#currentTokenIndex = i;
+          const token = this.#tokenQueue[i];
+          const nextToken = this.#tokenQueue[i + 1];
           this.#handleToken(token, nextToken);
 
           // Acorn's tokenizer re-uses tokens, so we have to copy the last token on
@@ -9691,6 +9698,14 @@
           } else if (isObjectLiteral(token, this.#lastToken)) {
             // Don't add new lines for empty object literals
             stackEntry = nextToken?.type?.label === "}" ? "{" : "{\n";
+          } else if (
+            isRoundBracketStartingLongParenthesis(
+              token,
+              this.#tokenQueue,
+              this.#currentTokenIndex
+            )
+          ) {
+            stackEntry = "(\n";
           } else if (ttl == "{") {
             // We need to add a line break for "{" which are not empty object literals
             stackEntry = "{\n";
@@ -9748,7 +9763,8 @@
           (token.type.label == "{" && this.#stack.at(-1) === "{\n") ||
           // Don't increment indent for empty array literals
           (token.type.label == "[" && this.#stack.at(-1) === "[\n") ||
-          token.type.keyword == "switch"
+          token.type.keyword == "switch" ||
+          (token.type.label == "(" && this.#stack.at(-1) === "(\n")
         ) {
           this.#indentLevel++;
         }
@@ -9757,7 +9773,11 @@
       #shouldDecrementIndent(token) {
         const top = this.#stack.at(-1);
         const ttl = token.type.label;
-        return (ttl == "}" && top == "{\n") || (ttl == "]" && top == "[\n");
+        return (
+          (ttl == "}" && top == "{\n") ||
+          (ttl == "]" && top == "[\n") ||
+          (ttl == ")" && top == "(\n")
+        );
       }
 
       #maybeDecrementIndent(token) {
@@ -9952,6 +9972,75 @@
       );
     }
 
+    /**
+     * Determines if we think that the given token starts a long parenthesis
+     *
+     * @param {Object} token
+     *        The token we want to determine if it is the beginning of a long paren.
+     * @param {Array<Object>} tokenQueue
+     *        The whole list of tokens parsed by acorn
+     * @param {Integer} currentTokenIndex
+     *        The index of `token` in `tokenQueue`
+     * @returns
+     */
+    function isRoundBracketStartingLongParenthesis(
+      token,
+      tokenQueue,
+      currentTokenIndex
+    ) {
+      if (token.type.label !== "(") {
+        return false;
+      }
+
+      // If we're just wrapping an object, we'll have a new line right after
+      if (tokenQueue[currentTokenIndex + 1].type.label == "{") {
+        return false;
+      }
+
+      // We're going to iterate through the following tokens until :
+      // - we find the closing parent
+      // - or we reached the maximum character we think should be in parenthesis
+      const longParentContentLength = 60;
+
+      // Keep track of other parens so we know when we get the closing one for `token`
+      let parenCount = 0;
+      let parenContentLength = 0;
+      for (let i = currentTokenIndex + 1, len = tokenQueue.length; i < len; i++) {
+        const currToken = tokenQueue[i];
+        const ttl = currToken.type.label;
+
+        if (ttl == "(") {
+          parenCount++;
+        } else if (ttl == ")") {
+          if (parenCount == 0) {
+            // Matching closing paren, if we got here, we didn't reach the length limit,
+            // as we return when parenContentLength is greater than the limit.
+            return false;
+          }
+          parenCount--;
+        }
+
+        // Aside block comments, all tokens start and end location are on the same line, so
+        // we can use `start` and `end` to deduce the token length.
+        const tokenLength = currToken.comment
+          ? currToken.text.length
+          : currToken.end - currToken.start;
+        parenContentLength += tokenLength;
+
+        // If we didn't find the matching closing paren yet and the characters from the
+        // tokens we evaluated so far are longer than the limit, so consider the token
+        // a long paren.
+        if (parenContentLength > longParentContentLength) {
+          return true;
+        }
+      }
+
+      // if we get to here, we didn't found a closing paren, which shouldn't happen
+      // (scripts with syntax error are not displayed in the debugger), but just to
+      // be safe, return false.
+      return false;
+    }
+
     // If any of these tokens are followed by a token on a new line, we know that
     // ASI cannot happen.
     const PREVENT_ASI_AFTER_TOKENS = new Set([
@@ -10122,8 +10211,9 @@
         (ttl == "{" && top == "{\n") ||
         // Don't add a new line for empty array literals
         (ttl == "[" && top == "[\n") ||
-        (ttl == "," && top != "(") ||
-        (ttl == ":" && (top == "case" || top == "default"))
+        ((ttl == "," || ttl == "||" || ttl == "&&") && top != "(") ||
+        (ttl == ":" && (top == "case" || top == "default")) ||
+        (ttl == "(" && top == "(\n")
       );
     }
 

devtools/client/debugger/src/workers/pretty-print/pretty-fast.js

@@ -165,6 +165,10 @@ class PrettyFast {
   #currentCode = "";
   #currentLine = 1;
   #currentColumn = 0;
+  // The tokens parsed by acorn.
+  #tokenQueue;
+  // The index of the current token in this.#tokenQueue.
+  #currentTokenIndex;
   // The last token we added to the pretty printed code.
   #lastToken;
   // Stack of token types/keywords that can affect whether we want to add a
@@ -178,6 +182,7 @@ class PrettyFast {
   //   - "{"
   //   - "{\n"
   //   - "("
+  //   - "(\n"
   //   - "["
   //   - "[\n"
   //   - "do"
@@ -186,11 +191,12 @@ class PrettyFast {
   //   - "case"
   //   - "default"
   //
-  // The difference between "[" and "[\n" (as well as "{" and "{\n") is that "\n" is used
-  // when we are treating (curly) brackets as line delimiters and should increment and
-  // decrement the indent level when we find them.
+  // The difference between "[" and "[\n" (as well as "{" and "{\n", and "(" and "(\n")
+  // is that "\n" is used when we are treating (curly) brackets/parens as line delimiters
+  // and should increment and decrement the indent level when we find them.
   // "[" can represent either a property access (e.g. `x["hi"]`), or an empty array literal
   // "{" only represents an empty object literals
+  // "(" can represent lots of different things (wrapping expression, if/loop condition, function call, …)
   #stack = [];
 
   /**
@@ -217,11 +223,12 @@ class PrettyFast {
     // After this process, tokenQueue has the following token stream:
     //
     //     [ foo, '// a', '// b', bar]
-    const tokenQueue = this.#getTokens(input);
+    this.#tokenQueue = this.#getTokens(input);
 
-    for (let i = 0, len = tokenQueue.length; i < len; i++) {
-      const token = tokenQueue[i];
-      const nextToken = tokenQueue[i + 1];
+    for (let i = 0, len = this.#tokenQueue.length; i < len; i++) {
+      this.#currentTokenIndex = i;
+      const token = this.#tokenQueue[i];
+      const nextToken = this.#tokenQueue[i + 1];
       this.#handleToken(token, nextToken);
 
       // Acorn's tokenizer re-uses tokens, so we have to copy the last token on
@@ -429,6 +436,14 @@ class PrettyFast {
       } else if (isObjectLiteral(token, this.#lastToken)) {
         // Don't add new lines for empty object literals
         stackEntry = nextToken?.type?.label === "}" ? "{" : "{\n";
+      } else if (
+        isRoundBracketStartingLongParenthesis(
+          token,
+          this.#tokenQueue,
+          this.#currentTokenIndex
+        )
+      ) {
+        stackEntry = "(\n";
       } else if (ttl == "{") {
         // We need to add a line break for "{" which are not empty object literals
         stackEntry = "{\n";
@@ -486,7 +501,8 @@ class PrettyFast {
       (token.type.label == "{" && this.#stack.at(-1) === "{\n") ||
       // Don't increment indent for empty array literals
       (token.type.label == "[" && this.#stack.at(-1) === "[\n") ||
-      token.type.keyword == "switch"
+      token.type.keyword == "switch" ||
+      (token.type.label == "(" && this.#stack.at(-1) === "(\n")
     ) {
       this.#indentLevel++;
     }
@@ -495,7 +511,11 @@ class PrettyFast {
   #shouldDecrementIndent(token) {
     const top = this.#stack.at(-1);
     const ttl = token.type.label;
-    return (ttl == "}" && top == "{\n") || (ttl == "]" && top == "[\n");
+    return (
+      (ttl == "}" && top == "{\n") ||
+      (ttl == "]" && top == "[\n") ||
+      (ttl == ")" && top == "(\n")
+    );
   }
 
   #maybeDecrementIndent(token) {
@@ -690,6 +710,75 @@ function isObjectLiteral(token, lastToken) {
   );
 }
 
+/**
+ * Determines if we think that the given token starts a long parenthesis
+ *
+ * @param {Object} token
+ *        The token we want to determine if it is the beginning of a long paren.
+ * @param {Array<Object>} tokenQueue
+ *        The whole list of tokens parsed by acorn
+ * @param {Integer} currentTokenIndex
+ *        The index of `token` in `tokenQueue`
+ * @returns
+ */
+function isRoundBracketStartingLongParenthesis(
+  token,
+  tokenQueue,
+  currentTokenIndex
+) {
+  if (token.type.label !== "(") {
+    return false;
+  }
+
+  // If we're just wrapping an object, we'll have a new line right after
+  if (tokenQueue[currentTokenIndex + 1].type.label == "{") {
+    return false;
+  }
+
+  // We're going to iterate through the following tokens until :
+  // - we find the closing parent
+  // - or we reached the maximum character we think should be in parenthesis
+  const longParentContentLength = 60;
+
+  // Keep track of other parens so we know when we get the closing one for `token`
+  let parenCount = 0;
+  let parenContentLength = 0;
+  for (let i = currentTokenIndex + 1, len = tokenQueue.length; i < len; i++) {
+    const currToken = tokenQueue[i];
+    const ttl = currToken.type.label;
+
+    if (ttl == "(") {
+      parenCount++;
+    } else if (ttl == ")") {
+      if (parenCount == 0) {
+        // Matching closing paren, if we got here, we didn't reach the length limit,
+        // as we return when parenContentLength is greater than the limit.
+        return false;
+      }
+      parenCount--;
+    }
+
+    // Aside block comments, all tokens start and end location are on the same line, so
+    // we can use `start` and `end` to deduce the token length.
+    const tokenLength = currToken.comment
+      ? currToken.text.length
+      : currToken.end - currToken.start;
+    parenContentLength += tokenLength;
+
+    // If we didn't find the matching closing paren yet and the characters from the
+    // tokens we evaluated so far are longer than the limit, so consider the token
+    // a long paren.
+    if (parenContentLength > longParentContentLength) {
+      return true;
+    }
+  }
+
+  // if we get to here, we didn't found a closing paren, which shouldn't happen
+  // (scripts with syntax error are not displayed in the debugger), but just to
+  // be safe, return false.
+  return false;
+}
+
 // If any of these tokens are followed by a token on a new line, we know that
 // ASI cannot happen.
 const PREVENT_ASI_AFTER_TOKENS = new Set([
@@ -860,8 +949,9 @@ function isLineDelimiter(token, stack) {
     (ttl == "{" && top == "{\n") ||
     // Don't add a new line for empty array literals
     (ttl == "[" && top == "[\n") ||
-    (ttl == "," && top != "(") ||
-    (ttl == ":" && (top == "case" || top == "default"))
+    ((ttl == "," || ttl == "||" || ttl == "&&") && top != "(") ||
+    (ttl == ":" && (top == "case" || top == "default")) ||
+    (ttl == "(" && top == "(\n")
   );
 }
 

devtools/client/debugger/src/workers/pretty-print/tests/__snapshots__/prettyFast.spec.js.snap

@@ -1063,6 +1063,106 @@ Array [
 ]
 `;
 
+exports[`Long parenthesis 1`] = `
+"if (
+  thisIsAVeryLongVariable &&
+  thisIsAnotherOne ||
+  yetAnotherVeryLongVariable
+) {
+  (
+    thisIsAnotherOne = thisMayReturnNull() ||
+    'hi',
+    thisIsAVeryLongVariable = 42,
+    yetAnotherVeryLongVariable &&
+    doSomething(
+      true /* do it well */ ,
+      thisIsAVeryLongVariable,
+      thisIsAnotherOne,
+      yetAnotherVeryLongVariable
+    )
+  )
+}
+for (
+  let thisIsAnotherVeryLongVariable = 0;
+  i < thisIsAnotherVeryLongVariable.length;
+  thisIsAnotherVeryLongVariable++
+) {
+}
+const x = ({
+  thisIsAnotherVeryLongPropertyName: 'but should not cause the paren to be a line delimiter'
+})
+"
+`;
+
+exports[`Long parenthesis 2`] = `
+Array [
+  "(1, 0) -> (2, 6)",
+  "(1, 3) -> (2, 9)",
+  "(2, 2) -> (2, 10)",
+  "(2, 26) -> (2, 34)",
+  "(3, 2) -> (2, 37)",
+  "(3, 19) -> (2, 54)",
+  "(4, 2) -> (2, 57)",
+  "(5, 0) -> (2, 83)",
+  "(5, 2) -> (2, 85)",
+  "(6, 2) -> (3, 8)",
+  "(7, 4) -> (3, 9)",
+  "(7, 21) -> (3, 26)",
+  "(7, 23) -> (3, 28)",
+  "(7, 40) -> (3, 45)",
+  "(7, 41) -> (3, 46)",
+  "(7, 43) -> (3, 48)",
+  "(8, 4) -> (3, 51)",
+  "(8, 8) -> (3, 55)",
+  "(9, 4) -> (3, 57)",
+  "(9, 28) -> (3, 81)",
+  "(9, 30) -> (3, 83)",
+  "(9, 32) -> (3, 85)",
+  "(10, 4) -> (3, 87)",
+  "(10, 31) -> (3, 114)",
+  "(11, 4) -> (3, 117)",
+  "(11, 15) -> (3, 128)",
+  "(12, 6) -> (3, 129)",
+  "(12, 28) -> (3, 150)",
+  "(13, 6) -> (3, 151)",
+  "(13, 29) -> (3, 174)",
+  "(14, 6) -> (3, 176)",
+  "(14, 22) -> (3, 192)",
+  "(15, 6) -> (3, 194)",
+  "(16, 4) -> (3, 220)",
+  "(17, 2) -> (3, 221)",
+  "(18, 0) -> (4, 6)",
+  "(19, 0) -> (5, 6)",
+  "(19, 4) -> (5, 10)",
+  "(20, 2) -> (5, 11)",
+  "(20, 6) -> (5, 15)",
+  "(20, 36) -> (5, 45)",
+  "(20, 38) -> (5, 47)",
+  "(20, 39) -> (5, 48)",
+  "(21, 2) -> (5, 50)",
+  "(21, 4) -> (5, 52)",
+  "(21, 6) -> (5, 54)",
+  "(21, 35) -> (5, 83)",
+  "(21, 36) -> (5, 84)",
+  "(21, 42) -> (5, 90)",
+  "(22, 2) -> (5, 92)",
+  "(22, 31) -> (5, 121)",
+  "(23, 0) -> (5, 123)",
+  "(23, 2) -> (5, 125)",
+  "(24, 0) -> (5, 126)",
+  "(25, 0) -> (6, 6)",
+  "(25, 6) -> (6, 12)",
+  "(25, 8) -> (6, 14)",
+  "(25, 10) -> (6, 16)",
+  "(25, 11) -> (6, 17)",
+  "(26, 2) -> (6, 18)",
+  "(26, 35) -> (6, 51)",
+  "(26, 37) -> (6, 53)",
+  "(27, 0) -> (6, 108)",
+  "(27, 1) -> (6, 109)",
+]
+`;
+
 exports[`Multi line comment 1`] = `
 "/* Comment
     more comment */

devtools/client/debugger/src/workers/pretty-print/tests/prettyFast.spec.js

@@ -390,6 +390,16 @@ const cases = [
       }
     `,
   },
+  {
+    name: "Long parenthesis",
+    input: `
+      if (thisIsAVeryLongVariable && thisIsAnotherOne || yetAnotherVeryLongVariable) {
+        (thisIsAnotherOne = thisMayReturnNull() || "hi", thisIsAVeryLongVariable = 42, yetAnotherVeryLongVariable && doSomething(true /* do it well */,thisIsAVeryLongVariable, thisIsAnotherOne, yetAnotherVeryLongVariable))
+      }
+      for (let thisIsAnotherVeryLongVariable = 0; i < thisIsAnotherVeryLongVariable.length; thisIsAnotherVeryLongVariable++) {}
+      const x = ({thisIsAnotherVeryLongPropertyName: "but should not cause the paren to be a line delimiter"})
+    `,
+  },
 ];
 
 const includesOnly = cases.find(({ only }) => only);

devtools/client/debugger/test/mochitest/browser_dbg-features-breakable-positions.js

@@ -42,12 +42,12 @@ add_task(async function testBreakableLinesOverReloads() {
 
   info("Pretty print first html page load and assert breakable lines");
   await prettyPrint(dbg);
-  await assertBreakablePositions(dbg, "index.html:formatted", 80, [
+  await assertBreakablePositions(dbg, "index.html:formatted", 84, [
     { line: 16, columns: [0, 8] },
     { line: 22, columns: [0, 8, 35] },
     { line: 27, columns: [0, 8] },
     { line: 28, columns: [] },
-    { line: 34, columns: [] },
+    { line: 36, columns: [] },
   ]);
   await closeTab(dbg, "index.html:formatted");
 

devtools/client/debugger/test/mochitest/browser_dbg-pretty-print-console.js

@@ -34,7 +34,7 @@ add_task(async function() {
   const formattedLink = await waitForConsoleLink(
     dbg,
     "arithmetic",
-    "math.min.js:formatted:22:18"
+    "math.min.js:formatted:31:24"
   );
   ok(true, "Message location was updated as expected");
 
@@ -43,7 +43,7 @@ add_task(async function() {
   );
   formattedLink.click();
   await selectSource(dbg, "math.min.js:formatted");
-  await waitForSelectedLocation(dbg, 22);
+  await waitForSelectedLocation(dbg, 31);
 });
 
 async function waitForConsoleLink(dbg, messageText, linkText) {

devtools/client/debugger/test/mochitest/browser_dbg-pretty-print-inline-scripts.js

@@ -92,7 +92,7 @@ add_task(async function() {
   const secondMessageLink = secondMessage.querySelector(".frame-link-source");
   is(
     secondMessageLink.innerText,
-    `${PRETTY_PRINTED_FILENAME}:38:12`,
+    `${PRETTY_PRINTED_FILENAME}:41:12`,
     "second console message has expected location"
   );
   info(
@@ -101,7 +101,7 @@ add_task(async function() {
   secondMessageLink.click();
   await waitForSelectedSource(dbg, PRETTY_PRINTED_FILENAME);
   ok(true, "pretty printed file was selected in debugger…");
-  await waitForSelectedLocation(dbg, 38);
+  await waitForSelectedLocation(dbg, 41);
   ok(true, "…at the expected location");
 
   info("Check that event listener popup is pointing to pretty-printed file");
@@ -132,7 +132,7 @@ add_task(async function() {
   const headerFilename = header.querySelector(".event-tooltip-filename")
     .innerText;
   ok(
-    headerFilename.endsWith(`${PRETTY_PRINTED_FILENAME}:48`),
+    headerFilename.endsWith(`${PRETTY_PRINTED_FILENAME}:51`),
     `Location in event tooltip is the pretty printed one (${headerFilename})`
   );
   info(
@@ -141,7 +141,7 @@ add_task(async function() {
   header.querySelector(".event-tooltip-debugger-icon").click();
   await waitForSelectedSource(dbg, PRETTY_PRINTED_FILENAME);
   ok(true, "pretty printed file was selected in debugger…");
-  await waitForSelectedLocation(dbg, 48);
+  await waitForSelectedLocation(dbg, 51);
   ok(true, "…at the expected location");
 });
 
@@ -199,12 +199,15 @@ function getExpectedPrettyPrintedHtml() {
     <script id="inline" type="application/javascript">
 ➤const userInfo = JSON.parse(document.getElementById('json-data').text);
 ➤console.log('User information: %o', userInfo);
-➤document.addEventListener('click', function onClick(e) {
-➤  console.log('in inline script');
-  // this is
-  // something
-➤  e.target;
-➤});
+➤document.addEventListener(
+  'click',
+  function onClick(e) {
+➤    console.log('in inline script');
+    // this is
+    // something
+➤    e.target;
+➤  }
+);
 </script>
   </head>
 

devtools/client/debugger/test/mochitest/browser_dbg-pretty-print-paused-anonymous.js

@@ -78,15 +78,18 @@ document.addEventListener('click', e=>{
 
   is(
     prettyEvalSourceValue.trim(),
-    `setTimeout(() =>{
-  debugger;
-  document.addEventListener('click', e=>{
+    `
+setTimeout(
+  () =>{
     debugger;
-  }, {
-    once: true
-  })
-}, 100)
-`.trim(),
+    document.addEventListener('click', e=>{
+      debugger;
+    }, {
+      once: true
+    })
+  },
+  100
+)`.trim(),
     "script was pretty printed as expected"
   );
   await resume(dbg);
@@ -96,7 +99,7 @@ document.addEventListener('click', e=>{
     content.document.body.click();
   });
   await waitForPaused(dbg);
-  await assertPausedAtSourceAndLine(dbg, prettyEvalSource.id, 4);
+  await assertPausedAtSourceAndLine(dbg, prettyEvalSource.id, 5);
   await resume(dbg);
 
   info("Check that pretty printing works in `new Function` source");

devtools/client/debugger/test/mochitest/browser_dbg-pretty-print.js

@@ -31,7 +31,7 @@ add_task(async function() {
 
   await stepOver(dbg);
 
-  assertPausedAtSourceAndLine(dbg, ppSrc.id, 27);
+  assertPausedAtSourceAndLine(dbg, ppSrc.id, 39);
 
   await resume(dbg);
 

devtools/client/inspector/markup/test/browser_markup_shadowdom_open_debugger_pretty_printed.js

@@ -49,5 +49,5 @@ add_task(async function() {
   customBadge.click();
 
   await waitForSelectedSource(dbg, "shadowdom_open_debugger.min.js:formatted");
-  await waitForSelectedLocation(dbg, 3);
+  await waitForSelectedLocation(dbg, 5);
 });