Bug 58449 - Web Inspector: TextViewer and TextEditorModel must support both \n and \r\n as line separators
Summary: Web Inspector: TextViewer and TextEditorModel must support both \n and \r\n a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Adaikin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-13 09:30 PDT by Alexander Pavlov (apavlov)
Modified: 2011-04-15 09:30 PDT (History)
12 users (show)

See Also:


Attachments
Patch (6.63 KB, patch)
2011-04-14 05:26 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (5.72 KB, patch)
2011-04-14 06:28 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2011-04-13 09:30:38 PDT
Currently the text editor relies on \n being the only line separator. DOS files have \r\n as line separators, and they should be preserved when the text is retrieved from the text editor (e.g. when saving edited stylesheets).
Comment 1 Andrey Adaikin 2011-04-14 05:26:02 PDT
Created attachment 89560 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2011-04-14 05:34:17 PDT
Comment on attachment 89560 [details]
Patch

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

> Source/WebCore/inspector/front-end/SourceFrame.js:40
> +    this._textModel.useWindowsLineBreaks = (WebInspector.platform === "windows");

You seem to detect the line break model based on the client OS, while a browser can load pages authored on any OS. You should analyze the input to detect the input document line break model instead.
Comment 3 Andrey Adaikin 2011-04-14 06:28:51 PDT
Created attachment 89573 [details]
Patch
Comment 4 Alexander Pavlov (apavlov) 2011-04-14 06:58:19 PDT
Comment on attachment 89573 [details]
Patch

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

Looks good, with one comment.

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

Why not this._lineBreak ? It would be consistent with the "join" counterpart calls below.
Comment 5 Andrey Adaikin 2011-04-14 07:12:11 PDT
Just to deal with weird cases of mixed line breaks, if any. We never want invisible chars (like \r) to appear in the text editor, because there will be editing artefacts, like deleting zero-width invisible characters...
Comment 6 Alexander Pavlov (apavlov) 2011-04-14 07:18:02 PDT
(In reply to comment #5)
> Just to deal with weird cases of mixed line breaks, if any. We never want invisible chars (like \r) to appear in the text editor, because there will be editing artefacts, like deleting zero-width invisible characters...

I get it. Fair point. Waiting for someone to r+.
Comment 7 WebKit Commit Bot 2011-04-15 09:30:01 PDT
Comment on attachment 89573 [details]
Patch

Clearing flags on attachment: 89573

Committed r83983: <http://trac.webkit.org/changeset/83983>
Comment 8 WebKit Commit Bot 2011-04-15 09:30:12 PDT
All reviewed patches have been landed.  Closing bug.