Summary: | Web Inspector: TextViewer and TextEditorModel must support both \n and \r\n as line separators | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Adaikin <aandrey> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aandrey, apavlov, bweinstein, commit-queue, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2011-04-13 09:30:38 PDT
Created attachment 89560 [details]
Patch
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. Created attachment 89573 [details]
Patch
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. 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... (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 on attachment 89573 [details] Patch Clearing flags on attachment: 89573 Committed r83983: <http://trac.webkit.org/changeset/83983> All reviewed patches have been landed. Closing bug. |