Bug 102098 – Web Inspector: TextEditor works incorrectly on files having both \r\n and 'n as line breaks.

• Status: RESOLVED
• Resolution: INVALID
• Component: Web Inspector (Deprecated)

[Vsevolod Vlasov]

Patch to follow

[Vsevolod Vlasov]

Created attachment 173910 [details]
WIP

[Build Bot]

Comment on attachment 173910 [details]
WIP

Attachment 173910 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14811974

New failing tests:
inspector/editor/text-editor-line-breaks.html
inspector/editor/text-editor-model.html

[WebKit Review Bot]

Comment on attachment 173910 [details]
WIP

Attachment 173910 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14823743

New failing tests:
inspector/editor/text-editor-line-breaks.html
inspector/editor/text-editor-model.html

[Andrey Adaikin]

Comment on attachment 173910 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=173910&action=review

> Source/WebCore/ChangeLog:3
> +        Web Inspector: TextEditor works incorrectly on files having both \r\n and 'n as line breaks.

what does "incorrectly" mean? FYI, i have no clue what you are fixing here.

> Source/WebCore/inspector/front-end/TextEditorModel.js:212
> +        return line.replace(/\n|\r\n$/, "");

this regex is equivalent to "(\n)|(\r\n$)". did you mean "(\r?\n)+$" ?

> Source/WebCore/inspector/front-end/TextEditorModel.js:264
> +        var newLines = text.split(/\n/);

so, what do you do with \r ? do you assume there will be never a \r in the text?
i suggest leaving the existing regex as is.

> Source/WebCore/inspector/front-end/TextEditorModel.js:388
>          var clip = [];

move this line below the if- check.

[Vsevolod Vlasov]

Created attachment 175683 [details]
Patch

[Vsevolod Vlasov]

> what does "incorrectly" mean? FYI, i have no clue what you are fixing here.
E.g. currently when you open file having both \n and \r\n as a line breaks, following function returns false.

function testTextEditorModel()
{
    var text = ...;
    textModel.setText(text)
    return textModel.text() === text
}

[Pavel Feldman]

Comment on attachment 175683 [details]
Patch

Do you think this is necessary / ready for landing?

[Vsevolod Vlasov]

(In reply to comment #7)
> (From update of attachment 175683 [details])
> Do you think this is necessary / ready for landing?

I think so.
Andrey, would you mind looking at it once again?

[Andrey Adaikin]

Comment on attachment 175683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175683&action=review

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1931
> +     */

@return {{line: number, column: number}}

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1960
> +                offset += this._textModel.lineLength(i) + 1; // lineBreak

this._textModel.lineBreak.length ?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2252
> +        function normalizeLineBreaks(text)

not used, remove

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2256
> +        console.assert(this._normalizeLineBreaks(this.element.innerText) === this._normalizeLineBreaks(this._textModel.text()) + "\n", "DOM does not match model.");

remove? expensive assert

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2260
> +                console.assert("No line number on line row");

console.error

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2268
> +                    console.assert("Chunk is not matching: %d %O", lineNumber, lineRow);

console.error

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2270
> +                console.assert("Line is not matching: %d %O", lineNumber, lineRow);

console.error

> Source/WebCore/inspector/front-end/TextEditorModel.js:233
> +        if (line.endsWith("\r\n"))

charAt?