943306 - Allow persisting console input history between sessions

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[:Gijs (he/him)]

Because typing in the same thing in the console after restarting to check what is going on with this bug you're trying to fix is annoying.

Comment 1

[Paul Rouget [:paul]]

This is how it was handled in JSTerm:

https://github.com/paulrouget/firefox-jsterm/blob/master/chrome/jsterm.js#L794
https://github.com/paulrouget/firefox-jsterm/blob/master/bootstrap.js#L50

Comment 2

[Mihai Sucan [:msucan]]

I suggest we dupe this bug to bug 1016452 (or vice-versa). We should decide on what kind of persistence we want: for a single session or do we want the commands history to outlive the browser session?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

I think keeping the console history around until shutdown is a no-brainer, but I'd also like it to be remembered across browser sessions. Maybe Jeff has some thoughts here.

In addition to that we should either make clear() empty the history buffer as well, or provide a new clearHistory() helper if we are concerned about breaking POLA.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I'm a +1, I think this was suggested already in feedback ( but with few votes ):

http://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/5895002-keeps-console-history-after-closing-dev-tools

I think clear() and clearHistory() are both needed - I think there is a strong use case for wanting to clear() out a cluttered view without destructively removing other data.

Some additional questions:

* How much history do we want to keep? ( ~100 items? ) 
* Should we expose console history to add-ons?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Panos Astithas [:past] from comment #3)
> I think keeping the console history around until shutdown is a no-brainer,
> but I'd also like it to be remembered across browser sessions. Maybe Jeff
> has some thoughts here.

Here is how I think it should work:

* Each toolbox instance keeps it's own stack of recent commands (as it currently behaves)
* When a command is run in any instance, it gets pushed onto a persisted (profile-level) stack of commands.  This would persist across sessions.
* When opening a new toolbox, the persisted stack of commands is copied into this instance

> In addition to that we should either make clear() empty the history buffer
> as well, or provide a new clearHistory() helper if we are concerned about
> breaking POLA.

I agree with Jeff about keeping clear() and clearHistory() separate

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: console-history-WIP.patch

A work in progress of behavior described in comment 8

Comment 11

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Created attachment 8549963 [details] [diff] [review]
> console-history-WIP.patch
> 
> A work in progress of behavior described in comment 8

Nice to see this getting worked on.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: console-history-WIP.patch

Updated code and added test.  Unfortunately the simple-storage API doesn't seem to flush on browser closing because the unload() function within the addon sdk [0] never fires for devtools.  Irakli, do you have any ideas how to make this work?

[0]: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/simple-storage.js#125

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Created attachment 8559488 [details] [diff] [review]
> console-history-WIP.patch
> 
> Updated code and added test.  Unfortunately the simple-storage API doesn't
> seem to flush on browser closing because the unload() function within the
> addon sdk [0] never fires for devtools.  Irakli, do you have any ideas how
> to make this work?

I believe at least part of the issue is:

* Unlike the add-on loader, the DevTools loader's |unload| (in Loader.jsm) is never called unless you do it manually to switch loaders
* For an add-on, |unload| is called when the add-on gets a shutdown event[1]
* Loader.jsm would need some technique to call |unload| before final shutdown, similar to AddonManager[2]

[1]: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/app-extension/bootstrap.js#316
[2]: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#833

Comment 14

[Brian Grinstead [:bgrins]]

Ideas for how to store this data:

* Fix simple-storage to work with devtools (at least implementing jryans' idea in Comment 13, maybe more work would need to be done)
* Use another storage API that was mentioned as a possibility: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/async_storage.js.  Wraps indexeddb functionality into an async localStorage API.
* Also IndexedDBHelper.jsm, although it feels like overkill for storing an array of strings.
* There is talk about adding something a simple key value store in Bug 866238, could pick up where that was left off.

Comment 15

[Brian Grinstead [:bgrins]]

jryans mentioned that there are major issues with migrating IndexedDB storage to older versions of Firefox within the same profile.  So, if you open the profile in Nightly, then save a value into the idb store, then open Release with the same profile there can be problems.  This kind of backwards movement is not supported [0].  See also: bug 1117129, bug 1096310, bug 1100663.

There are lots of places that use OS.File to serialize and deserialize data [1].  It would also be possible to add a new module that uses this and wrap it into an API like async_storage provides.  Here are a couple [2][3] of examples that could be relevant.

[0]: https://bugzilla.mozilla.org/show_bug.cgi?id=1117129#c2
[1]: https://dxr.mozilla.org/mozilla-central/search?q=path%3Abrowser+os.file&redirect=true
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/modules/DirectoryLinksProvider.jsm#277
[3]: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionFile.jsm#9

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: console-history.patch

This depends on the key value store from Bug 1134265, but I believe that it is ready for review anyway, as that part isn't super important.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8566329 [details] [diff] [review]
console-history.patch

Review of attachment 8566329 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't see an implementation of the clearHistory() console helper that we discuss above. Will this be a followup? I'm sure it would be a pretty simple change. clearHistory and setting the buffer length from the options panel is all that seem missing, but could be done in followups.

::: browser/devtools/webconsole/test/browser_console_history_persist.js
@@ +39,5 @@
> +    "Third tab has populated history");
> +
> +  hud3.jsterm.INPUT_HISTORY_COUNT = INPUT_HISTORY_COUNT;
> +  hud3.jsterm.setInputValue('"hello from third tab"');
> +  hud3.jsterm.execute();

Nit: I think you could combine these two in a single jsterm.execute(...);

@@ +58,5 @@
> +});
> +
> +// Populate the history by running the following commands
> +//   [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
> +function* populateInputHistory(hud) {

Nit: why not use the more common /* */ for function comments?

@@ +65,5 @@
> +  jsterm.INPUT_HISTORY_COUNT = INPUT_HISTORY_COUNT;
> +
> +  for (let i = 0; i < INPUT_HISTORY_COUNT; i++) {
> +    jsterm.setInputValue(i);
> +    jsterm.execute();

Nit: I think you could combine these two in a single jsterm.execute(i);

::: browser/devtools/webconsole/webconsole.js
@@ +56,5 @@
>  
>  const IGNORED_SOURCE_URLS = ["debugger eval code"];
>  
> +// The number of entries to store in input history
> +const INPUT_HISTORY_COUNT = 50;

This seems like the kind of thing a user would want to tweak via a pref. Preferably in the options panel.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: console-history.patch

Updated from comments - going to follow up with clearHistory() in bug 1134845.  Going to wait to land to make sure they make it into the same release.

Comment 19

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e2e9f88b88

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: console-history.patch

Panos, I had to spend some time tracking down test failures and I'd like you to take a look at the changes needed in the init function for the webconsoleframe just to double check my plan.

When I yielded on the history being loaded there were issues with tests that were listening for the 'web-console-created' event [0]. This is because it was firing once initConnection was called, but sometimes this would happen before the history was loaded and the panel wouldn't be created in time.  If I didn't wait at all for the history to be loaded, then there was an issue on windows 7 where the history would be perused before it was loaded in the test added in this patch [1].

So, the change here waits for both things to be finished before emitting the event.  I'll send over an interdiff shortly.

[0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c43e95deefa0
[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46f6ebc325d4

Comment 21

[Brian Grinstead [:bgrins]]

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8566329&action=interdiff&newid=8569608&headers=1

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8569608 [details] [diff] [review]
console-history.patch

Review of attachment 8569608 [details] [diff] [review]:
-----------------------------------------------------------------

The new test now fails on Windows, but the other changes look good to me. Thanks for the interdiff!

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Panos Astithas [:past] from comment #22)
> The new test now fails on Windows

That was from a try push during some debugging.  Here is a try push with the current patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29036862ddd0

Comment 24

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/8bccd5a79c1a

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8bccd5a79c1a

Comment 26

[Brian Grinstead [:bgrins]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Webconsole input history used to be limited to a single toolbox instance, so it would never be remembered outside of that toolbox.  But now it is saved to your profile so it can be restored when a new toolbox is opened.
[Suggested wording]: Webconsole input will be remembered even after closing the toolbox
[Links (documentation, blog post, etc)]:

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Brian, is the console input history persistent across Firefox sessions, i.e. if I quit Firefox and reopen it? Or just persistent across closing and reopening developer tools? Thanks.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Relnoted for 39 as: Webconsole input history is persistent even after closing the toolbox

Comment 29

[Brian Grinstead [:bgrins]]

(In reply to Liz Henry (:lizzard) from comment #27)
> Brian, is the console input history persistent across Firefox sessions, i.e.
> if I quit Firefox and reopen it? Or just persistent across closing and
> reopening developer tools? Thanks.

It persists in both cases (it's stored per-profile).  We also have the clearHistory() method available if someone wants to get rid of it (Bug 1134845).