Bug 160840

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 InspectorAssignee: Devin Rousso <drousso>
Severity: Normal CC: bburg, commit-queue, drousso, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: EasyFix, GoodFirstBug, InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Description Flags
Patch none

Description Nikita Vasilyev 2016-08-12 22:29:50 PDT
1. Open Inspector on this page.
2. Press Command-Shift-O
3. Type "global.js:99999999"

global.js opens and focused on the last line.

global.js opens and focuses on the previously focused line (or just scrolls to the top of the resource).

This is how it works in Sublime Text and Chrome DevTools.
Comment 1 Radar WebKit Bug Importer 2016-08-12 22:30:15 PDT
Comment 2 Devin Rousso 2016-08-18 22:59:09 PDT
Created attachment 286440 [details]
Comment 3 Joseph Pecoraro 2016-08-19 13:09:09 PDT
Comment on attachment 286440 [details]

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.
Comment 4 Devin Rousso 2016-08-19 22:41:47 PDT
Created attachment 286530 [details]
Comment 5 Joseph Pecoraro 2016-08-20 02:40:47 PDT
Comment on attachment 286530 [details]

This feels right! I'll review more closely when at a computer.
Comment 6 Joseph Pecoraro 2016-08-22 15:29:54 PDT
Comment on attachment 286530 [details]

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 7 WebKit Commit Bot 2016-08-22 22:14:18 PDT
Comment on attachment 286530 [details]

Clearing flags on attachment: 286530

Committed r204755: <http://trac.webkit.org/changeset/204755>
Comment 8 WebKit Commit Bot 2016-08-22 22:14:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Nikita Vasilyev 2016-10-28 15:48:54 PDT
This isn't working for me on Safari Technology Preview 16.
Comment 10 Devin Rousso 2016-10-28 16:48:20 PDT
Created attachment 293245 [details]
Comment 11 WebKit Commit Bot 2016-10-31 16:19:58 PDT
Comment on attachment 293245 [details]

Clearing flags on attachment: 293245

Committed r208188: <http://trac.webkit.org/changeset/208188>
Comment 12 WebKit Commit Bot 2016-10-31 16:20:02 PDT
All reviewed patches have been landed.  Closing bug.