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>
Status: RESOLVED FIXED    
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   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

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

Expected:
global.js opens and focused on the last line.

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

Notes:
This is how it works in Sublime Text and Chrome DevTools.
Comment 1 Radar WebKit Bug Importer 2016-08-12 22:30:15 PDT
<rdar://problem/27835331>
Comment 2 Devin Rousso 2016-08-18 22:59:09 PDT
Created attachment 286440 [details]
Patch
Comment 3 Joseph Pecoraro 2016-08-19 13:09:09 PDT
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.
Comment 4 Devin Rousso 2016-08-19 22:41:47 PDT
Created attachment 286530 [details]
Patch
Comment 5 Joseph Pecoraro 2016-08-20 02:40:47 PDT
Comment on attachment 286530 [details]
Patch

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

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]
Patch
Comment 11 WebKit Commit Bot 2016-10-31 16:19:58 PDT
Comment on attachment 293245 [details]
Patch

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.