Attachment 342597 Details for Bug 186453 – WIP

[patch] WIP

bug-186453-20180612144842.patch (text/plain), 9.96 KB, created by Matt Baker on 2018-06-12 14:48:42 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Matt Baker

Created: 2018-06-12 14:48:42 PDT

Size: 9.96 KB

patch

obsolete

>Subversion Revision: 232776
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 97bb0f138372cd9ceca521005b47af3c98df688f..bb6bbe49665e4b706fd47b564b18b9e2fbcd850d 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,41 @@
>+2018-06-12  Matt Baker  <mattbaker@apple.com>
>+
>+        Web Inspector: REGRESSION (r195723): CodeMirrorEditor shouldn't force line endings to "\n"
>+        https://bugs.webkit.org/show_bug.cgi?id=186453
>+        <rdar://problem/39689180>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Esprima token locations should be identified by line and column number,
>+        instead of character offset, since the former is line-ending agnostic.
>+
>+        * UserInterface/Controllers/TypeTokenAnnotator.js:
>+        (WI.TypeTokenAnnotator.prototype.insertAnnotations):
>+        (WI.TypeTokenAnnotator.prototype._insertTypeToken):
>+        (WI.TypeTokenAnnotator.prototype._insertToken):
>+
>+        * UserInterface/Models/ScriptSyntaxTree.js:
>+        (WI.ScriptSyntaxTree):
>+        (WI.ScriptSyntaxTree.prototype.filterByLines.filterForNodesInRange):
>+        (WI.ScriptSyntaxTree.prototype.filterByLines):
>+        (WI.ScriptSyntaxTree.prototype._createInternalSyntaxTree):
>+        Request per-token location (line, column) from Esprima. Esprima
>+        locations are converted to the format CodeMirror expects before
>+        being added to the AST node.
>+
>+        (WI.ScriptSyntaxTree.prototype.filterByRange.filterForNodesInRange): Deleted.
>+        (WI.ScriptSyntaxTree.prototype.filterByRange): Deleted.
>+
>+        * UserInterface/Views/CodeMirrorEditor.js:
>+        (WI.CodeMirrorEditor.create):
>+        (WI.CodeMirrorEditor):
>+        Let CodeMirror auto-detect line endings.
>+
>+        * UserInterface/Views/TextEditor.js:
>+        (WI.TextEditor.set string.update):
>+        (WI.TextEditor.prototype.set string):
>+        Remove assertion, as it will no longer be true.
>+
> 2018-06-09  Dan Bernstein  <mitz@apple.com>
> 
>         [Xcode] Clean up and modernize some build setting definitions
>diff --git a/Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js b/Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js
>index bf1cc019ae87b8b28dcc3e98c0e1d5bf301da7cf..22754c5c8a39903480c2c09f881a1572abdb7ca2 100644
>--- a/Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js
>+++ b/Source/WebInspectorUI/UserInterface/Controllers/TypeTokenAnnotator.js
>@@ -56,9 +56,11 @@ WI.TypeTokenAnnotator = class TypeTokenAnnotator extends WI.Annotator
>             return;
> 
>         var {startOffset, endOffset} = this.sourceCodeTextEditor.visibleRangeOffsets();
>+        let startLine = this.sourceCodeTextEditor.currentOffsetToCurrentPosition(startOffset).line;
>+        let endLine = this.sourceCodeTextEditor.currentOffsetToCurrentPosition(endOffset).line;
>+        let allNodesInRange = scriptSyntaxTree.filterByLines(startLine, endLine);
> 
>         var startTime = Date.now();
>-        var allNodesInRange = scriptSyntaxTree.filterByRange(startOffset, endOffset);
>         scriptSyntaxTree.updateTypes(allNodesInRange, (nodesWithUpdatedTypes) => {
>             // Because this is an asynchronous call, we could have been deactivated before the callback function is called.
>             if (!this.isActive())
>@@ -86,7 +88,7 @@ WI.TypeTokenAnnotator = class TypeTokenAnnotator extends WI.Annotator
>     {
>         if (node.type === WI.ScriptSyntaxTree.NodeType.Identifier) {
>             if (!node.attachments.__typeToken && node.attachments.types && node.attachments.types.valid)
>-                this._insertToken(node.range[0], node, false, WI.TypeTokenView.TitleType.Variable, node.name);
>+                this._insertToken(node, false, WI.TypeTokenView.TitleType.Variable, node.name);
> 
>             if (node.attachments.__typeToken)
>                 node.attachments.__typeToken.update(node.attachments.types);
>@@ -105,16 +107,16 @@ WI.TypeTokenAnnotator = class TypeTokenAnnotator extends WI.Annotator
>         var scriptSyntaxTree = this._script._scriptSyntaxTree;
>         if (!node.attachments.__typeToken && (scriptSyntaxTree.containsNonEmptyReturnStatement(node.body) || !functionReturnType.typeSet.isContainedIn(WI.TypeSet.TypeBit.Undefined))) {
>             var functionName = node.id ? node.id.name : null;
>-            this._insertToken(node.typeProfilingReturnDivot, node, true, WI.TypeTokenView.TitleType.ReturnStatement, functionName);
>+            this._insertToken(node, true, WI.TypeTokenView.TitleType.ReturnStatement, functionName);
>         }
> 
>         if (node.attachments.__typeToken)
>             node.attachments.__typeToken.update(node.attachments.returnTypes);
>     }
> 
>-    _insertToken(originalOffset, node, shouldTranslateOffsetToAfterParameterList, typeTokenTitleType, functionOrVariableName)
>+    _insertToken(node, shouldTranslateOffsetToAfterParameterList, typeTokenTitleType, functionOrVariableName)
>     {
>-        var tokenPosition = this.sourceCodeTextEditor.originalOffsetToCurrentPosition(originalOffset);
>+        var tokenPosition = node.startPosition;
>         var currentOffset = this.sourceCodeTextEditor.currentPositionToCurrentOffset(tokenPosition);
>         var sourceString = this.sourceCodeTextEditor.string;
> 
>diff --git a/Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js b/Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js
>index 5de64e8c81e223ba97941d1c27cafa8f97059df1..f2c3ebdc99bc29da4a1a1062565e96c9c3ff5e03 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js
>@@ -33,7 +33,7 @@ WI.ScriptSyntaxTree = class ScriptSyntaxTree
> 
>         try {
>             let sourceType = this._script.sourceType === WI.Script.SourceType.Module ? "module" : "script";
>-            let esprimaSyntaxTree = esprima.parse(sourceText, {range: true, sourceType});
>+            let esprimaSyntaxTree = esprima.parse(sourceText, {loc: true, range: true, sourceType});
>             this._syntaxTree = this._createInternalSyntaxTree(esprimaSyntaxTree);
>             this._parsedSuccessfully = true;
>         } catch (error) {
>@@ -101,7 +101,7 @@ WI.ScriptSyntaxTree = class ScriptSyntaxTree
>         return allNodes;
>     }
> 
>-    filterByRange(startOffset, endOffset)
>+    filterByLines(startLine, endLine)
>     {
>         console.assert(this._parsedSuccessfully);
>         if (!this._parsedSuccessfully)
>@@ -118,17 +118,17 @@ WI.ScriptSyntaxTree = class ScriptSyntaxTree
> 
>             // If a node's range ends before the range we're interested in starts, we don't need to search any of its
>             // enclosing ranges, because, by definition, those enclosing ranges are contained within this node's range.
>-            if (node.range[end] < startOffset)
>+            if (node.endPosition.line < startLine)
>                 state.skipChildNodes = true;
> 
>             // We are only interested in nodes whose start position is within our range.
>-            if (startOffset <= node.range[start] && node.range[start] <= endOffset)
>+            if (startLine <= node.startPosition.line && node.startPosition.line <= endLine)
>                 allNodes.push(node);
> 
>             // Once we see nodes that start beyond our range, we can quit traversing the AST. We can do this safely
>             // because we know the AST is traversed using depth first search, so it will traverse into enclosing ranges
>             // before it traverses into adjacent ranges.
>-            if (node.range[start] > endOffset)
>+            if (node.startPosition.line > endLine)
>                 state.shouldStopEarly = true;
>         }
> 
>@@ -1058,6 +1058,16 @@ WI.ScriptSyntaxTree = class ScriptSyntaxTree
>             return null;
>         }
> 
>+        let {start, end} = node.loc;
>+        result.startPosition = {
>+            line: start.line - 1,
>+            ch: start.column
>+        };
>+        result.endPosition = {
>+            line: end.line - 1,
>+            ch: end.column
>+        };
>+
>         result.range = node.range;
>         // This is an object for which you can add fields to an AST node without worrying about polluting the syntax-related fields of the node.
>         result.attachments = {};
>diff --git a/Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js b/Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js
>index a0338c2cb4d87f030ee04fba5151b5ac34ea2356..66e47eeb01d562c493c53d0a3679c49544a71dab 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js
>@@ -27,9 +27,6 @@ WI.CodeMirrorEditor = class CodeMirrorEditor
> {
>     static create(element, options)
>     {
>-        if (options.lineSeparator === undefined)
>-            options.lineSeparator = "\n";
>-
>         // CodeMirror's manual scrollbar positioning results in double scrollbars,
>         // nor does it handle braces and brackets well, so default to using LTR.
>         // Clients can override this if custom layout for RTL is available.
>diff --git a/Source/WebInspectorUI/UserInterface/Views/TextEditor.js b/Source/WebInspectorUI/UserInterface/Views/TextEditor.js
>index 5f6e24f00dc9509aa86588f66555b8eb811a26e5..7da80933f0285fc41940fbc5ae6eabecd0c83935 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/TextEditor.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/TextEditor.js
>@@ -118,10 +118,9 @@ WI.TextEditor = class TextEditor extends WI.View
>             if (this._initialStringNotSet)
>                 this._codeMirror.removeLineClass(0, "wrap");
> 
>-            if (this._codeMirror.getValue() !== newString) {
>+            if (this._codeMirror.getValue() !== newString)
>                 this._codeMirror.setValue(newString);
>-                console.assert(this.string.length === newString.length, "A lot of our code depends on precise text offsets, so the string should remain the same.");
>-            } else {
>+            else {
>                 // Ensure we at display content even if the value did not change. This often happens when auto formatting.
>                 this.layout();
>             }

Actions: View | Formatted Diff | Diff

Attachments on bug 186453: 342354 | 342358 | 342492 | 342597 | 342645 | 344956