Bug 153529

Summary:

CodeMirror will strip out "\r" from files with "\r\n" as newlines causing our offsets into the file to be incorrect

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

Web Inspector

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, benjamin, burg, commit-queue, fpizlo, ggaren, graouts, gskachkov, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, oliver, sukolsak, timothy, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
example file	none
image	none
patch	none

Description Saam Barati 2016-01-26 19:08:11 PST

I'm guessing this is parser related.

Comment 1 Saam Barati 2016-01-26 19:08:55 PST

Created attachment 269972 [details]
example file

Comment 2 Saam Barati 2016-01-26 19:10:02 PST

Created attachment 269973 [details]
image

what that file looks like in inspector

Comment 3 Saam Barati 2016-01-27 14:26:14 PST

This is a bug relating to how we use CodeMirror.

Comment 4 Radar WebKit Bug Importer 2016-01-27 14:26:41 PST

<rdar://problem/24376799>

Comment 5 Radar WebKit Bug Importer 2016-01-27 14:28:17 PST

<rdar://problem/24376840>

Comment 6 Saam Barati 2016-01-27 14:47:52 PST

Created attachment 270047 [details]
patch

Comment 7 BJ Burg 2016-01-27 14:51:10 PST

Comment on attachment 270047 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js:31
> +            options.lineSeparator = "\n";

Why does this make CodeMirror leave the \r alone?

Comment 8 Saam Barati 2016-01-27 15:20:37 PST

(In reply to comment #7)
> Comment on attachment 270047 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270047&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js:31
> > +            options.lineSeparator = "\n";
> 
> Why does this make CodeMirror leave the \r alone?

It will trust us and use "\n" as the newline terminator
instead of its own default thing that takes into account
"\r"

Comment 9 Timothy Hatcher 2016-01-27 15:21:47 PST

Comment on attachment 270047 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js:31
>> +            options.lineSeparator = "\n";
> 
> Why does this make CodeMirror leave the \r alone?

I suspect the \r will be there in the editor on the end of the lines, but will be invisible.

Comment 10 Timothy Hatcher 2016-01-27 15:23:29 PST

Does CodeMirror render the \r? From the code it looks like it might:

 var txt = content.appendChild(elt("span", m[0] == "\r" ? "␍" : "␤", "cm-invalidchar"));
 txt.setAttribute("cm-text", m[0]);
 builder.col += 1;

Comment 11 Saam Barati 2016-01-27 17:20:52 PST

(In reply to comment #10)
> Does CodeMirror render the \r? From the code it looks like it might:
> 
>  var txt = content.appendChild(elt("span", m[0] == "\r" ? "␍" : "␤",
> "cm-invalidchar"));
>  txt.setAttribute("cm-text", m[0]);
>  builder.col += 1;

It does.

Comment 12 WebKit Commit Bot 2016-01-27 18:14:41 PST

Comment on attachment 270047 [details]
patch

Clearing flags on attachment: 270047

Committed r195723: <http://trac.webkit.org/changeset/195723>

Comment 13 WebKit Commit Bot 2016-01-27 18:14:46 PST

All reviewed patches have been landed.  Closing bug.