Web Inspector: Execution highlighting in the frontend should be line/column-based
authormattbaker@apple.com <mattbaker@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Jul 2018 21:53:14 +0000 (21:53 +0000)
committermattbaker@apple.com <mattbaker@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Jul 2018 21:53:14 +0000 (21:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187532
<rdar://problem/42035580>

Reviewed by Joseph Pecoraro.

Source code offsets from Esprima should not be used to calculate ranges
in CodeMirror for expression highlighting.

This also fixes a long standing bug when adjusting for the starting
position of an inline script. Previously the start offset from the script
TextRange was used for this purpose, but the value is often incorrect (see
https://bugs.webkit.org/show_bug.cgi?id=187532#c5). By using the starting
line/column instead, we avoid the problem.

* UserInterface/Models/ScriptSyntaxTree.js:
(WI.ScriptSyntaxTree.prototype.containersOfPosition):
(WI.ScriptSyntaxTree.prototype.containersOfOffset): Deleted.

* UserInterface/Models/SourceCodePosition.js:
(WI.SourceCodePosition.prototype.offsetColumn):

* UserInterface/Views/SourceCodeTextEditor.js:
(WI.SourceCodeTextEditor.prototype.textEditorExecutionHighlightRange.toInlineScriptPosition):
(WI.SourceCodeTextEditor.prototype.textEditorExecutionHighlightRange.fromInlineScriptPosition):
(WI.SourceCodeTextEditor.prototype.textEditorExecutionHighlightRange):
(WI.SourceCodeTextEditor.prototype.textEditorExecutionHighlightRange.convertRangeOffsetsToSourceCodeOffsets): Deleted.

* UserInterface/Views/TextEditor.js:
(WI.TextEditor.prototype._updateExecutionRangeHighlight):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233817 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js
Source/WebInspectorUI/UserInterface/Models/SourceCodePosition.js
Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js
Source/WebInspectorUI/UserInterface/Views/TextEditor.js

index 35ed3c8..5d5343e 100644 (file)
@@ -1,5 +1,38 @@
 2018-07-13  Matt Baker  <mattbaker@apple.com>
 
+        Web Inspector: Execution highlighting in the frontend should be line/column-based
+        https://bugs.webkit.org/show_bug.cgi?id=187532
+        <rdar://problem/42035580>
+
+        Reviewed by Joseph Pecoraro.
+
+        Source code offsets from Esprima should not be used to calculate ranges
+        in CodeMirror for expression highlighting.
+
+        This also fixes a long standing bug when adjusting for the starting
+        position of an inline script. Previously the start offset from the script
+        TextRange was used for this purpose, but the value is often incorrect (see
+        https://bugs.webkit.org/show_bug.cgi?id=187532#c5). By using the starting
+        line/column instead, we avoid the problem.
+
+        * UserInterface/Models/ScriptSyntaxTree.js:
+        (WI.ScriptSyntaxTree.prototype.containersOfPosition):
+        (WI.ScriptSyntaxTree.prototype.containersOfOffset): Deleted.
+
+        * UserInterface/Models/SourceCodePosition.js:
+        (WI.SourceCodePosition.prototype.offsetColumn):
+
+        * UserInterface/Views/SourceCodeTextEditor.js:
+        (WI.SourceCodeTextEditor.prototype.textEditorExecutionHighlightRange.toInlineScriptPosition):
+        (WI.SourceCodeTextEditor.prototype.textEditorExecutionHighlightRange.fromInlineScriptPosition):
+        (WI.SourceCodeTextEditor.prototype.textEditorExecutionHighlightRange):
+        (WI.SourceCodeTextEditor.prototype.textEditorExecutionHighlightRange.convertRangeOffsetsToSourceCodeOffsets): Deleted.
+
+        * UserInterface/Views/TextEditor.js:
+        (WI.TextEditor.prototype._updateExecutionRangeHighlight):
+
+2018-07-13  Matt Baker  <mattbaker@apple.com>
+
         Web Inspector: SourceCodePosition.js missing from Test.html
         https://bugs.webkit.org/show_bug.cgi?id=187644
 
index 26128c9..8b77c3a 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -79,22 +79,20 @@ WI.ScriptSyntaxTree = class ScriptSyntaxTree
         return nodes;
     }
 
-    containersOfOffset(offset)
+    containersOfPosition(position)
     {
         console.assert(this._parsedSuccessfully);
         if (!this._parsedSuccessfully)
             return [];
 
         let allNodes = [];
-        const start = 0;
-        const end = 1;
 
         this.forEachNode((node, state) => {
-            if (node.range[end] < offset)
+            if (node.endPosition.isBefore(position))
                 state.skipChildNodes = true;
-            if (node.range[start] > offset)
+            else if (node.startPosition.isAfter(position))
                 state.shouldStopEarly = true;
-            if (node.range[start] <= offset && node.range[end] >= offset)
+            else
                 allNodes.push(node);
         });
 
index 55b5e79..17a0a9f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -36,6 +36,12 @@ WI.SourceCodePosition = class SourceCodePosition
     get lineNumber() { return this._lineNumber; }
     get columnNumber() { return this._columnNumber; }
 
+    offsetColumn(delta)
+    {
+        console.assert(this._columnNumber + delta >= 0);
+        return new WI.SourceCodePosition(this._lineNumber, this._columnNumber + delta);
+    }
+
     equals(position)
     {
         return this._lineNumber === position.lineNumber && this._columnNumber === position.columnNumber;
index 7c1ad7b..37d4e4b 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -1411,25 +1411,40 @@ WI.SourceCodeTextEditor = class SourceCodeTextEditor extends WI.TextEditor
         this._reinsertAllThreadIndicators();
     }
 
-    textEditorExecutionHighlightRange(offset, position, characterAtOffset, callback)
+    textEditorExecutionHighlightRange(currentPosition, callback)
     {
+        let {line, ch} = this.currentPositionToOriginalPosition(currentPosition);
+        let position = new WI.SourceCodePosition(line, ch);
+
         let script = this._getAssociatedScript(position);
         if (!script) {
             callback(null);
             return;
         }
 
-        // If this is an inline script, then convert to offset within the inline script.
-        let adjustment = script.range.startOffset || 0;
-        offset = offset - adjustment;
+        let {startLine, startColumn} = script.range;
+
+        function toInlineScriptPosition(position) {
+            let columnNumber = position.lineNumber === startLine ? position.columnNumber - startColumn : position.columnNumber;
+            return new WI.SourceCodePosition(position.lineNumber - startLine, columnNumber);
+        }
+
+        function fromInlineScriptPosition(position) {
+            let columnNumber = position.lineNumber ? position.columnNumber : position.columnNumber + startColumn;
+            return new WI.SourceCodePosition(position.lineNumber + startLine, columnNumber);
+        }
 
         // When returning offsets, convert to offsets within the SourceCode being viewed.
-        function convertRangeOffsetsToSourceCodeOffsets(range) {
-            return range.map((offset) => offset + adjustment);
+        let highlightSourceCodeRange = (startPosition, endPosition) => {
+            startPosition = fromInlineScriptPosition(startPosition).toCodeMirror();
+            endPosition = fromInlineScriptPosition(endPosition).toCodeMirror();
+            callback({startPosition, endPosition});
         }
 
         script.requestScriptSyntaxTree((syntaxTree) => {
-            let nodes = syntaxTree.containersOfOffset(offset);
+            // Convert to the position within the inline script before querying the AST.
+            position = toInlineScriptPosition(position);
+            let nodes = syntaxTree.containersOfPosition(position);
             if (!nodes.length) {
                 callback(null);
                 return;
@@ -1439,18 +1454,17 @@ WI.SourceCodeTextEditor = class SourceCodeTextEditor extends WI.TextEditor
             // Avoid highlighting the entire program if this is the start of the first statement.
             // Special case the assignment expression inside of a for..of and for..in to highlight a larger range.
             for (let node of nodes) {
-                let startOffset = node.range[0];
-                if (startOffset === offset && node.type !== WI.ScriptSyntaxTree.NodeType.Program) {
-                    callback(convertRangeOffsetsToSourceCodeOffsets(node.range));
+                if (node.startPosition.equals(position) && node.type !== WI.ScriptSyntaxTree.NodeType.Program) {
+                    highlightSourceCodeRange(node.startPosition, node.endPosition);
                     return;
                 }
                 if (node.type === WI.ScriptSyntaxTree.NodeType.ForInStatement || node.type === WI.ScriptSyntaxTree.NodeType.ForOfStatement) {
-                    if (node.left.range[0] === offset) {
-                        callback(convertRangeOffsetsToSourceCodeOffsets([node.left.range[0], node.right.range[1]]));
+                    if (node.left.startPosition.equals(position)) {
+                        highlightSourceCodeRange(node.left.startPosition, node.right.endPosition);
                         return;
                     }
                 }
-                if (startOffset > offset)
+                if (node.startPosition.isAfter(position))
                     break;
             }
 
@@ -1458,16 +1472,14 @@ WI.SourceCodeTextEditor = class SourceCodeTextEditor extends WI.TextEditor
             // We check this after ensuring nothing starts with this offset,
             // as that would be more important.
             for (let node of nodes) {
-                let startOffset = node.range[0];
-                let endOffset = node.range[1];
-                if (endOffset === offset) {
+                if (node.endPosition.equals(position)) {
                     if (node.type === WI.ScriptSyntaxTree.NodeType.BlockStatement) {
                         // Closing brace of a block, only highlight the closing brace character.
-                        callback(convertRangeOffsetsToSourceCodeOffsets([offset - 1, offset]));
+                        highlightSourceCodeRange(position.offsetColumn(-1), position);
                         return;
                     }
                 }
-                if (startOffset > offset)
+                if (node.startPosition.isAfter(position))
                     break;
             }
 
@@ -1479,7 +1491,8 @@ WI.SourceCodeTextEditor = class SourceCodeTextEditor extends WI.TextEditor
                 return aLength - bLength;
             });
 
-            let characterAtOffsetIsDotOrBracket = characterAtOffset === "." || characterAtOffset === "[";
+            let characterAtPosition = this.getTextInRange(currentPosition, {line: currentPosition.line, ch: currentPosition.ch + 1});
+            let characterAtPositionIsDotOrBracket = characterAtPosition === "." || characterAtPosition === "[";
 
             for (let i = 0; i < nodes.length; ++i) {
                 let node = nodes[i];
@@ -1488,7 +1501,7 @@ WI.SourceCodeTextEditor = class SourceCodeTextEditor extends WI.TextEditor
                 if (node.type === WI.ScriptSyntaxTree.NodeType.CallExpression
                     || node.type === WI.ScriptSyntaxTree.NodeType.NewExpression
                     || node.type === WI.ScriptSyntaxTree.NodeType.ThrowStatement) {
-                    callback(convertRangeOffsetsToSourceCodeOffsets(node.range));
+                    highlightSourceCodeRange(node.startPosition, node.endPosition);
                     return;
                 }
 
@@ -1507,24 +1520,24 @@ WI.SourceCodeTextEditor = class SourceCodeTextEditor extends WI.TextEditor
                 //     foo["x"]*["y"]["z"] => inside y looking at parent call frame => |foo["x"]["y"]|["z"]
                 //
                 if (node.type === WI.ScriptSyntaxTree.NodeType.ThisExpression
-                    || (characterAtOffsetIsDotOrBracket && (node.type === WI.ScriptSyntaxTree.NodeType.Identifier || node.type === WI.ScriptSyntaxTree.NodeType.MemberExpression))) {
+                    || (characterAtPositionIsDotOrBracket && (node.type === WI.ScriptSyntaxTree.NodeType.Identifier || node.type === WI.ScriptSyntaxTree.NodeType.MemberExpression))) {
                     let memberExpressionNode = null;
                     for (let j = i + 1; j < nodes.length; ++j) {
                         let nextNode = nodes[j];
                         if (nextNode.type === WI.ScriptSyntaxTree.NodeType.MemberExpression) {
                             memberExpressionNode = nextNode;
-                            if (offset === memberExpressionNode.range[1])
+                            if (position.equals(memberExpressionNode.endPosition))
                                 continue;
                         }
                         break;
                     }
 
                     if (memberExpressionNode) {
-                        callback(convertRangeOffsetsToSourceCodeOffsets(memberExpressionNode.range));
+                        highlightSourceCodeRange(memberExpressionNode.startPosition, memberExpressionNode.endPosition);
                         return;
                     }
 
-                    callback(convertRangeOffsetsToSourceCodeOffsets(node.range));
+                    highlightSourceCodeRange(node.startPosition, node.endPosition);
                     return;
                 }
             }
index 6e5e263..f24e8af 100644 (file)
@@ -1324,12 +1324,8 @@ WI.TextEditor = class TextEditor extends WI.View
             return;
 
         let currentPosition = {line: this._executionLineNumber, ch: this._executionColumnNumber};
-        let originalOffset = this.currentPositionToOriginalOffset(currentPosition);
-        let originalCodeMirrorPosition = this.currentPositionToOriginalPosition(currentPosition);
-        let originalPosition = new WI.SourceCodePosition(originalCodeMirrorPosition.line, originalCodeMirrorPosition.ch);
-        let characterAtOffset = this._codeMirror.getRange(currentPosition, {line: this._executionLineNumber, ch: this._executionColumnNumber + 1});
 
-        this._delegate.textEditorExecutionHighlightRange(originalOffset, originalPosition, characterAtOffset, (range) => {
+        this._delegate.textEditorExecutionHighlightRange(currentPosition, (range) => {
             let start, end;
             if (!range) {
                 // Highlight the rest of the line.
@@ -1337,8 +1333,8 @@ WI.TextEditor = class TextEditor extends WI.View
                 end = {line: this._executionLineNumber};
             } else {
                 // Highlight the range.
-                start = this.originalOffsetToCurrentPosition(range[0]);
-                end = this.originalOffsetToCurrentPosition(range[1]);
+                start = range.startPosition;
+                end = range.endPosition;
             }
 
             // Ensure the marker is cleared in case there were multiple updates very quickly.