1463672 - Add history support in CodeMirror-JsTerm

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

Bug 983473 will land without the ability to navigate through the JsTerm history, so let's deal with it here.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1463672 - Add history support in CodeMirror-JsTerm; .

This implements history navigation support in CodeMirror-JsTerm
using Arrow keys (and Ctrl-N/Ctrl-P on OSX).
The tests covering that feature now runs with both the old and the
new JsTerm so we can be confident about the CodeMirror-JsTerm.

Review commit: https://reviewboard.mozilla.org/r/248492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248492/

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Nicolas, I am seeing collisions when applying the patch. Are any other patches required?

Honza


patching file devtools/client/webconsole/components/JSTerm.js
Hunk #1 FAILED at 208
Hunk #3 FAILED at 894
2 out of 3 hunks FAILED -- saving rejects to file devtools/client/webconsole/components/JSTerm.js.rej
patching file devtools/client/webconsole/test/mochitest/browser_jsterm_history.js
Hunk #1 FAILED at 10
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/webconsole/test/mochitest/browser_jsterm_history.js.rej
patching file devtools/client/webconsole/test/mochitest/browser_jsterm_history_arrow_keys.js
Hunk #1 FAILED at 13
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/webconsole/test/mochitest/browser_jsterm_history_arrow_keys.js.rej
patching file devtools/client/webconsole/test/mochitest/browser_jsterm_history_nav.js
Hunk #1 FAILED at 6
1 out of 2 hunks FAILED -- saving rejects to file devtools/client/webconsole/test/mochitest/browser_jsterm_history_nav.js.rej
patching file devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js
Hunk #1 FAILED at 13
Hunk #2 FAILED at 95
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh new-history

Comment 3

[Brian Grinstead [:bgrins]]

I think it'll need to be rebased around the let->const migration

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yep, Bug 1454696, I had to rebase my patches too.
Honza

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1463672 - Destroy editor when destroying JSTerm; .

We need to call editor.destroy in order to not leak windows.

Review commit: https://reviewboard.mozilla.org/r/250780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250780/

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8982520 [details]
Bug 1463672 - Add history support in CodeMirror-JsTerm; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248492/diff/1-2/

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Everything should be good now

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8982520 [details]
Bug 1463672 - Add history support in CodeMirror-JsTerm; .

https://reviewboard.mozilla.org/r/248492/#review257940

Looks good to me!
R+
Honza

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8985059 [details]
Bug 1463672 - Destroy editor when destroying JSTerm; .

https://reviewboard.mozilla.org/r/250780/#review257942

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8982520 [details]
Bug 1463672 - Add history support in CodeMirror-JsTerm; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248492/diff/2-3/

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8985059 [details]
Bug 1463672 - Destroy editor when destroying JSTerm; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250780/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f998954c5390
Add history support in CodeMirror-JsTerm; r=Honza.
https://hg.mozilla.org/integration/autoland/rev/4a7b85481dcf
Destroy editor when destroying JSTerm; r=Honza.

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f998954c5390
https://hg.mozilla.org/mozilla-central/rev/4a7b85481dcf