DevTools: "Jump between editing locations" experiment

DevTools: "Jump between editing locations" experiment

This patch introduces "Jump between editing locations" experiment. It
adds "Alt+" and "Alt-" shortcuts to the scripts panel to navigate between
editing locations. An editing location is considered to be switched if
one of the following happens:
- A user clicks with his mouse and changes cursor position
- Position in editor was highlighted and cursor was moved there (e.g.
due to "Goto function" outline dialog)
- A new file was opened

BUG=281507
R=vsevik@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165395

[lushnikov, 2013-08-28 18:01:58]

Please take a look

[vsevik, 2013-08-29 10:46:16]

https://chromiumcodereview.appspot.com/23474010/diff/7001/Source/devtools/fro...
File Source/devtools/front_end/ScriptsPanel.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/7001/Source/devtools/fro...
Source/devtools/front_end/ScriptsPanel.js:369:
this._jumpHistoryManager.dropHistoryEntries(project.id(), null);
 move this to _removeUISourceCodes

https://chromiumcodereview.appspot.com/23474010/diff/7001/Source/devtools/fro...
Source/devtools/front_end/ScriptsPanel.js:638:
this._jumpHistoryManager.dropHistoryEntries(uiSourceCode.project().id(),
uiSourceCode.path());
So you don't need to listen for uiSourceCodeRemoved/projectWillReset anymore?

https://chromiumcodereview.appspot.com/23474010/diff/7001/Source/devtools/fro...
Source/devtools/front_end/ScriptsPanel.js:1545:
WebInspector.ScriptsPanel.JumpHistoryManager.prototype = {
Let's extract it to a separate file and delegate registerShortcuts, editorClosed
and sourceFrameCreated calls to it.

[vsevik, 2013-08-29 10:49:29]

https://chromiumcodereview.appspot.com/23474010/diff/7001/Source/devtools/fro...
File Source/devtools/front_end/ScriptsPanel.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/7001/Source/devtools/fro...
Source/devtools/front_end/ScriptsPanel.js:1636: rollover: function() {
{ on the next line, here and below and above.

[pfeldman, 2013-08-29 11:25:13]

Please file a bug for each feature. How is this working in case I was on
Timeline and then pressed Alt-? I think history should be devtools-wide, not
editor-wide. I tend to not lgtm due to not being thought through as is.

[lushnikov, 2013-08-29 16:13:20]

Addressed everything

[lushnikov, 2013-08-30 08:31:45]

On 2013/08/29 16:13:20, lushnikov wrote:
> Addressed everything

So the latest version of patch generalizes the concept of jump positions.
Everyone is now capable of creating its own implementation
of JumpHistoryEntry interface to support history navigation between its states.
Two implementations of this interface are introduced in this patch:
- ScriptsPanelJumpHistoryEntry, which navigates between different editing
locations inside opened tabs
- PanelJumpHistoryEntry, which adds panel openings to the history and makes it
possible to navigate between them 

JumpHistoryManager instance resides on WebInspector global object.

[lushnikov, 2013-08-30 10:35:07]

The update adds additional logic to the "reveal()" method:
no it should return a boolean value to determine if it did something or not. In
case it returns false, JumpHistoryManager will go on to the next entry until one
will be successfully revealed.

[vsevik, 2013-09-04 09:46:22]

Please add a fix and test for the case where we first edit something then switch
to timeline panel and then navigate back to sources from it.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
File Source/devtools/front_end/CodeMirrorTextEditor.js (right):

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/CodeMirrorTextEditor.js:156:
this.element.addEventListener("mousedown", function() {
FWIW We agreed to allow anonymous function only when the whole function is on
one line.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/CodeMirrorTextEditor.js:159:
this.element.addEventListener("mousedown", function() {
ditto.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/CodeMirrorTextEditor.js:997:
this._reportJump(codeMirror.getCursor("head"), selection.head)
; is missing

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/CodeMirrorTextEditor.js:1240: return
positionHandle._lineHandle === this._lineHandle && positionHandle._columnNumber
== this._columnNumber &&
No need to split this line, but I'd name the parameter "handle" for the sake of
brevity.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
File Source/devtools/front_end/ScriptsPanel.js (right):

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/ScriptsPanel.js:469: if (this._searchQuery)
Please extract all jump specific logic to a separate
ScriptsPanelJumpHistorySupport object, so that ScriptsPanel could only delegate
some events to it.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
File Source/devtools/front_end/SourceFrame.js (right):

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/SourceFrame.js:104: this._jumpToPositionDelegate =
jumpToPositionDelegate;
Please make it an event instead

[lushnikov, 2014-01-02 14:06:09]

@pfeldman: The functionality doesn't allow beyond-panel navigation, because
"editor-in-drawer" satisfies all the needs we had for now.

@vsevik: this is a revised implementation which addressed all your previous
comments.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
File Source/devtools/front_end/CodeMirrorTextEditor.js (right):

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/CodeMirrorTextEditor.js:156:
this.element.addEventListener("mousedown", function() {
On 2013/09/04 09:46:22, vsevik wrote:
> FWIW We agreed to allow anonymous function only when the whole function is on
> one line.

Done.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/CodeMirrorTextEditor.js:159:
this.element.addEventListener("mousedown", function() {
On 2013/09/04 09:46:22, vsevik wrote:
> ditto.

Done.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/CodeMirrorTextEditor.js:997:
this._reportJump(codeMirror.getCursor("head"), selection.head)
On 2013/09/04 09:46:22, vsevik wrote:
> ; is missing

Done.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/CodeMirrorTextEditor.js:1240: return
positionHandle._lineHandle === this._lineHandle && positionHandle._columnNumber
== this._columnNumber &&
On 2013/09/04 09:46:22, vsevik wrote:
> No need to split this line, but I'd name the parameter "handle" for the sake
of
> brevity.

I love "_lineHandle" more - that's not too verbose.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
File Source/devtools/front_end/ScriptsPanel.js (right):

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/ScriptsPanel.js:469: if (this._searchQuery)
On 2013/09/04 09:46:22, vsevik wrote:
> Please extract all jump specific logic to a separate
> ScriptsPanelJumpHistorySupport object, so that ScriptsPanel could only
delegate
> some events to it.

Done.

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
File Source/devtools/front_end/SourceFrame.js (right):

https://codereview.chromium.org/23474010/diff/22001/Source/devtools/front_end...
Source/devtools/front_end/SourceFrame.js:104: this._jumpToPositionDelegate =
jumpToPositionDelegate;
On 2013/09/04 09:46:22, vsevik wrote:
> Please make it an event instead

Done.

[lushnikov, 2014-01-13 09:52:47]

ping?

[lushnikov, 2014-01-14 17:12:14]

On 2014/01/13 09:52:47, lushnikov wrote:
> ping?

ping ping?!

[vsevik, 2014-01-15 16:19:05]

https://chromiumcodereview.appspot.com/23474010/diff/28001/LayoutTests/inspec...
File LayoutTests/inspector/jump-to-previous-editing-location.html (right):

https://chromiumcodereview.appspot.com/23474010/diff/28001/LayoutTests/inspec...
LayoutTests/inspector/jump-to-previous-editing-location.html:81: var
uiSourceCode = findSourceCode("blink-fs");
use InspectorTest.showScriptSource from debugger-test.js

https://chromiumcodereview.appspot.com/23474010/diff/28001/LayoutTests/inspec...
LayoutTests/inspector/jump-to-previous-editing-location.html:239: var anchorUri
= uiSourceCode.uri();
anchorURI

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
File Source/devtools/front_end/SourcesPanel.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:531: if (uiSourceCode ===
this._currentUISourceCode && typeof lineNumber === "number") {
This looks redundant.
Please remove these lines unless you could show a test that fails without them!

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:558:
this._historyManager.updateCurrentState();
You wrap incorrect line with your calls here and this would not work with
switching files through navigator (test).

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:1196: handleShortcut: function(event)
Please register and handle this shortcuts on the panel (just like we do for
other sources panel shortcuts)

[vsevik, 2014-01-16 12:24:25]

lgtm

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
File Source/devtools/front_end/SourcesPanel.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:552: _showFile: function(uiSourceCode)
I think you should move your calls out of this method and make them one level
above instead.
This would better address your needs.

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:729:
this._historyManager.readOnlyLock();
Can we remove these?

[vsevik, 2014-01-16 12:24:52]

not lgtm, this was a mistake :)

[lushnikov, 2014-01-16 17:10:28]

Addressed your comments in the latest CL. There should be no more mistakes this
time :)

https://chromiumcodereview.appspot.com/23474010/diff/28001/LayoutTests/inspec...
File LayoutTests/inspector/jump-to-previous-editing-location.html (right):

https://chromiumcodereview.appspot.com/23474010/diff/28001/LayoutTests/inspec...
LayoutTests/inspector/jump-to-previous-editing-location.html:81: var
uiSourceCode = findSourceCode("blink-fs");
On 2014/01/15 16:19:05, vsevik wrote:
> use InspectorTest.showScriptSource from debugger-test.js

Done.

https://chromiumcodereview.appspot.com/23474010/diff/28001/LayoutTests/inspec...
LayoutTests/inspector/jump-to-previous-editing-location.html:239: var anchorUri
= uiSourceCode.uri();
On 2014/01/15 16:19:05, vsevik wrote:
> anchorURI

Done.

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
File Source/devtools/front_end/SourcesPanel.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:531: if (uiSourceCode ===
this._currentUISourceCode && typeof lineNumber === "number") {
On 2014/01/15 16:19:05, vsevik wrote:
> This looks redundant.
> Please remove these lines unless you could show a test that fails without
them!

Done.

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:552: _showFile: function(uiSourceCode)
On 2014/01/16 12:24:26, vsevik wrote:
> I think you should move your calls out of this method and make them one level
> above instead.
> This would better address your needs.

Done.

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:558:
this._historyManager.updateCurrentState();
On 2014/01/15 16:19:05, vsevik wrote:
> You wrap incorrect line with your calls here and this would not work with
> switching files through navigator (test).

Moved these lines out of the method

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:729:
this._historyManager.readOnlyLock();
On 2014/01/16 12:24:26, vsevik wrote:
> Can we remove these?

Not really; test fails.

https://chromiumcodereview.appspot.com/23474010/diff/28001/Source/devtools/fr...
Source/devtools/front_end/SourcesPanel.js:1196: handleShortcut: function(event)
On 2014/01/15 16:19:05, vsevik wrote:
> Please register and handle this shortcuts on the panel (just like we do for
> other sources panel shortcuts)

Done.

[vsevik, 2014-01-17 13:00:22]

https://chromiumcodereview.appspot.com/23474010/diff/168001/Source/devtools/f...
File Source/devtools/front_end/SourcesPanel.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/168001/Source/devtools/f...
Source/devtools/front_end/SourcesPanel.js:553:
this.highlightPosition(lineNumber, columnNumber);
remove this!

https://chromiumcodereview.appspot.com/23474010/diff/168001/Source/devtools/f...
Source/devtools/front_end/SourcesPanel.js:714:
this._historyManager.pushNewState();
one line below

[lushnikov, 2014-01-17 13:28:44]

Addressed comments; please take a look

https://codereview.chromium.org/23474010/diff/168001/Source/devtools/front_en...
File Source/devtools/front_end/SourcesPanel.js (right):

https://codereview.chromium.org/23474010/diff/168001/Source/devtools/front_en...
Source/devtools/front_end/SourcesPanel.js:553:
this.highlightPosition(lineNumber, columnNumber);
On 2014/01/17 13:00:23, vsevik wrote:
> remove this!

Done.

https://codereview.chromium.org/23474010/diff/168001/Source/devtools/front_en...
Source/devtools/front_end/SourcesPanel.js:714:
this._historyManager.pushNewState();
On 2014/01/17 13:00:23, vsevik wrote:
> one line below

Moved to correct places

[vsevik, 2014-01-17 16:30:00]

Let's move this out of experiment and add a shortcut description to shortcut
screen

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
File Source/devtools/front_end/CodeMirrorTextEditor.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/CodeMirrorTextEditor.js:164: this._anticipateJump =
value;
isHandlingMouseDownEvent

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/CodeMirrorTextEditor.js:342: if
(!this._presearchSelection)
selectionBeforeSearch

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
File Source/devtools/front_end/EditingLocationHistoryManager.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/EditingLocationHistoryManager.js:75: readOnlyLock:
function()
Unused

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/EditingLocationHistoryManager.js:80:
releaseReadOnlyLock: function()
Unused

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/EditingLocationHistoryManager.js:204:
this._sourcesPanel.showUISourceCode(uiSourceCode);
showSourceLocation instead?

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
File Source/devtools/front_end/UISourceCodeFrame.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/UISourceCodeFrame.js:51: sourceCode: function()
uiSourceCode

[lushnikov, 2014-01-17 17:33:52]

got it out of experiment; take a look!

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
File Source/devtools/front_end/CodeMirrorTextEditor.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/CodeMirrorTextEditor.js:164: this._anticipateJump =
value;
On 2014/01/17 16:30:01, vsevik wrote:
> isHandlingMouseDownEvent

Done.

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/CodeMirrorTextEditor.js:342: if
(!this._presearchSelection)
On 2014/01/17 16:30:01, vsevik wrote:
> selectionBeforeSearch

Done.

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
File Source/devtools/front_end/EditingLocationHistoryManager.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/EditingLocationHistoryManager.js:75: readOnlyLock:
function()
On 2014/01/17 16:30:01, vsevik wrote:
> Unused

Done.

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/EditingLocationHistoryManager.js:80:
releaseReadOnlyLock: function()
On 2014/01/17 16:30:01, vsevik wrote:
> Unused

Done.

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/EditingLocationHistoryManager.js:204:
this._sourcesPanel.showUISourceCode(uiSourceCode);
On 2014/01/17 16:30:01, vsevik wrote:
> showSourceLocation instead?

Done.

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
File Source/devtools/front_end/UISourceCodeFrame.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/238001/Source/devtools/f...
Source/devtools/front_end/UISourceCodeFrame.js:51: sourceCode: function()
On 2014/01/17 16:30:01, vsevik wrote:
> uiSourceCode

Done.

[vsevik, 2014-01-17 17:38:58]

lgtm

https://chromiumcodereview.appspot.com/23474010/diff/338001/Source/devtools/f...
File Source/devtools/front_end/CodeMirrorTextEditor.js (right):

https://chromiumcodereview.appspot.com/23474010/diff/338001/Source/devtools/f...
Source/devtools/front_end/CodeMirrorTextEditor.js:162: function
updateAnticipateJumpFlag(value)
please update the name appropriately.

[commit-bot: I haz the power, 2014-01-17 17:46:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23474010/338001

[commit-bot: I haz the power, 2014-01-17 17:46:50]

Failed to apply patch for Source/devtools/front_end/SourcesPanelDescriptor.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/devtools/front_end/SourcesPanelDescriptor.js
  Hunk #1 FAILED at 77.
  Hunk #2 FAILED at 154.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/devtools/front_end/SourcesPanelDescriptor.js.rej

Patch:       Source/devtools/front_end/SourcesPanelDescriptor.js
Index: Source/devtools/front_end/SourcesPanelDescriptor.js
diff --git a/Source/devtools/front_end/SourcesPanelDescriptor.js
b/Source/devtools/front_end/SourcesPanelDescriptor.js
index
b1bd0dc736758702099a7cd367dfa0d568c26962..683f6488c9ba35ea26093cb96c9b84a8b66ca0ff
100644
--- a/Source/devtools/front_end/SourcesPanelDescriptor.js
+++ b/Source/devtools/front_end/SourcesPanelDescriptor.js
@@ -77,6 +77,8 @@ WebInspector.SourcesPanelDescriptor.prototype = {
        
section.addAlternateKeys(WebInspector.SourcesPanelDescriptor.ShortcutKeys.DecreaseCSSUnitByOne,
WebInspector.UIString("Decrement CSS unit by 1"));
        
section.addAlternateKeys(WebInspector.SourcesPanelDescriptor.ShortcutKeys.IncreaseCSSUnitByTen,
WebInspector.UIString("Increment CSS unit by 10"));
        
section.addAlternateKeys(WebInspector.SourcesPanelDescriptor.ShortcutKeys.DecreaseCSSUnitByTen,
WebInspector.UIString("Decrement CSS unit by 10"));
+       
section.addAlternateKeys(WebInspector.SourcesPanelDescriptor.ShortcutKeys.JumpToPreviousLocation,
WebInspector.UIString("Jump to previous editing location"));
+       
section.addAlternateKeys(WebInspector.SourcesPanelDescriptor.ShortcutKeys.JumpToNextLocation,
WebInspector.UIString("Jump to next editing location"));
     },
 
     __proto__: WebInspector.PanelDescriptor.prototype
@@ -154,6 +156,13 @@ WebInspector.SourcesPanelDescriptor.ShortcutKeys = {
 
     ToggleComment: [
        
WebInspector.KeyboardShortcut.makeDescriptor(WebInspector.KeyboardShortcut.Keys.Slash,
WebInspector.KeyboardShortcut.Modifiers.CtrlOrMeta)
+    ],
+
+    JumpToPreviousLocation: [
+       
WebInspector.KeyboardShortcut.makeDescriptor(WebInspector.KeyboardShortcut.Keys.Minus,
WebInspector.KeyboardShortcut.Modifiers.Alt)
+    ],
 
+    JumpToNextLocation: [
+       
WebInspector.KeyboardShortcut.makeDescriptor(WebInspector.KeyboardShortcut.Keys.Plus,
WebInspector.KeyboardShortcut.Modifiers.Alt)
     ]
 };

[commit-bot: I haz the power, 2014-01-20 09:29:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/23474010/468001

[commit-bot: I haz the power, 2014-01-20 10:28:14]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[lushnikov, 2014-01-20 10:31:56]

Committed patchset #12 manually as r165395 (presubmit successful).