Summary: | Web Inspector: Entering ":n" in Open Resource Dialog, where n > number of lines, should jump to the last line | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, commit-queue, hi, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | EasyFix, GoodFirstBug, InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2016-08-12 22:29:50 PDT
Created attachment 286440 [details]
Patch
Comment on attachment 286440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286440&action=review > Source/WebInspectorUI/UserInterface/Views/TextEditor.js:435 > + let line = Number.constrain(position.lineNumber, 0, this._codeMirror.lineCount() - 1); > + let column = Number.constrain(position.columnNumber, 0, this._codeMirror.getLine(line).length - 1); > + > + let lineHandle = this._codeMirror.getLineHandle(line); > if (!lineHandle || !this._visible || this._initialStringNotSet || this._deferReveal) { How does this handle editors that haven't yet loaded content (initial string not set, defer reveal)? Is lineCount() zero, and so line would be -1, and lineHandle would not exist? On the surface this patch seems fine, but I worry that if "deferReveal" is true, we would have just lost the line number. The test scenario for this is click a link to reveal a position in a resource that will be automatically pretty printed. Created attachment 286530 [details]
Patch
Comment on attachment 286530 [details]
Patch
This feels right! I'll review more closely when at a computer.
Comment on attachment 286530 [details]
Patch
r=me, I like this much better. Thanks for addressing the comments.
To be sure, did you test with a an auto-formatted resource? Let me know if you have questions.
Comment on attachment 286530 [details] Patch Clearing flags on attachment: 286530 Committed r204755: <http://trac.webkit.org/changeset/204755> All reviewed patches have been landed. Closing bug. This isn't working for me on Safari Technology Preview 16. Created attachment 293245 [details]
Patch
Comment on attachment 293245 [details] Patch Clearing flags on attachment: 293245 Committed r208188: <http://trac.webkit.org/changeset/208188> All reviewed patches have been landed. Closing bug. |