1425521 - Put the JSTerm into the Console React app

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

At the moment, the jsterm (console input), lives as a separate vanilla JS app, which makes it disconnected from the console. 
Since we are planning to work on having the jsterm rewritten to react, we should go through a b

The first step of the rewrite of the jsterm to React/Redux is to integrate the current jsterm code into the console React app (i.e. in https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/devtools/client/webconsole/new-console-output/new-console-output-wrapper.js#197-206).

To do that, we can take some inspiration from https://github.com/ReactTraining/react-workshop/blob/master/subjects/ImperativeToDeclarative/solution.js to wrap the current code in a React element, so we don't have to work behind a pref to do all the jsterm work.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Here's an happy TRY push https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ce33352c016b6009378f0904326ca6597998e29.

I already removed some of the unnecessary function and properties in the new jsterm (everything related to the old sidebar/variableview).
Also, the fact that we return early for the browser console when `devtools.chrome.enabled` is false had some impact on some methods where we assumed that `this.inputNode` was always available. 
I think the patch makes sense and isn't too huge, but if you want to, I may put back the methods and properties like before and do some of those cleanups in follow ups.

The next move after this patch will be to put the execute function on the hud and pass everything that is needed by the jsterm as props, which should make the component easier to understand and refactor.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425521 - Wrap JsTerm in a React component in new frontend; .

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

Comment 3

[Brian Grinstead [:bgrins]]

Can you do a talos comparison with/without the patch?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

sure, let's see what damp says https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2fbdbe167eee67466250df0a131df2309709f628&newProject=try&newRevision=e50681f9c5a9010fbdfd6cd4b308520c15c0fda0&framework=1

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Looks like g2 jobs have frequent intermittent on linux since yesterday.
Here's a new try push on windows https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=eb7dadf028abe348fbb345db2de2a0836f7c5f86&newProject=try&newRevision=cd6aefc03692f9201cf81202f6ca38835fd714c2&framework=1

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Managed to have (hopefully) meaningful numbers on DAMP: 
- https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=a357d9aabdf5736e5693589f0631b0bc42285426&newProject=try&newRevision=987ee463126a0c0b4562908b770897e235179525&originalSignature=389a5b1ef37772bad22611e4d6237fffea35c9cb&newSignature=389a5b1ef37772bad22611e4d6237fffea35c9cb&showOnlyImportant=1&framework=1
- https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=a357d9aabdf5736e5693589f0631b0bc42285426&newProject=try&newRevision=987ee463126a0c0b4562908b770897e235179525&originalSignature=2fa549258445b035f7463afad97852e46b399a6d&newSignature=2fa549258445b035f7463afad97852e46b399a6d&showOnlyImportant=1&framework=1

Looks like the patch has some benefits on webconsole close, for some reason.
At least, it doesn't seem to cause console regression, so the patch should be safe to land.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8964962 [details]
Bug 1425521 - Wrap JsTerm in a React component in new frontend; .

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425521 - Move old jsterm file into webconsole/old; .

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

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8964962 [details]
Bug 1425521 - Wrap JsTerm in a React component in new frontend; .

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

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8964962 [details]
Bug 1425521 - Wrap JsTerm in a React component in new frontend; .

https://reviewboard.mozilla.org/r/233580/#review241634

This looks like a nice start! If you have a green try, then let's get it landed and iterate on it in follow ups

::: devtools/client/webconsole/webconsole.html
(Diff revision 3)
>              src="resource://devtools/client/webconsole/main.js"></script>
>    </head>
>    <body class="theme-sidebar" role="application">
>      <div id="app-wrapper" class="theme-body">
>        <div id="output-container" role="document" aria-live="polite"></div>
> -      <div id="jsterm-wrapper">

This node doesn't get added with the new component, but there's one reference to it in HTML docs in CSS:

- https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#356

Unless if I'm missing something, that rule should be removed.

::: devtools/client/webconsole/webconsole.html
(Diff revision 3)
>      <div id="app-wrapper" class="theme-body">
>        <div id="output-container" role="document" aria-live="polite"></div>
> -      <div id="jsterm-wrapper">
> -        <div id="webconsole-notificationbox"></div>
> -        <div class="jsterm-input-container" style="direction:ltr">
> -          <div class="jsterm-stack-node">

This node doesn't get added with the new component, but there are two references to it in HTML docs in CSS:

- https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#357
- https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#394

Unless if I'm missing something, those should be removed.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8966474 [details]
Bug 1425521 - Move old jsterm file into webconsole/old; .

https://reviewboard.mozilla.org/r/235160/#review241638

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8964962 [details]
Bug 1425521 - Wrap JsTerm in a React component in new frontend; .

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

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8966474 [details]
Bug 1425521 - Move old jsterm file into webconsole/old; .

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18c6e8809c2b
Wrap JsTerm in a React component in new frontend; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/06f67baf2aed
Move old jsterm file into webconsole/old; r=bgrins.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/18c6e8809c2b
https://hg.mozilla.org/mozilla-central/rev/06f67baf2aed