Allow persisting console input history between sessions

RESOLVED FIXED in Firefox 39

Status

()

Firefox
Developer Tools: Console
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Gijs, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed, relnote-firefox 39+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
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

Updated

4 years ago
Priority: -- → P3
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?
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.
Flags: needinfo?(jgriffiths)
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?
Flags: needinfo?(jgriffiths)
Duplicate of this bug: 1042639
Duplicate of this bug: 1042644

Updated

3 years ago
Duplicate of this bug: 997628
(Assignee)

Comment 8

3 years ago
(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
(Assignee)

Comment 9

3 years ago
Created attachment 8549963 [details] [diff] [review]
console-history-WIP.patch

A work in progress of behavior described in comment 8
Duplicate of this bug: 1016452
(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.
(Assignee)

Comment 12

3 years ago
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?

[0]: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/simple-storage.js#125
Flags: needinfo?(rFobic)
(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
(Assignee)

Comment 14

3 years ago
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.
(Assignee)

Comment 15

3 years ago
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
(Assignee)

Updated

3 years ago
Depends on: 1134265
(Assignee)

Comment 16

3 years ago
Created attachment 8566329 [details] [diff] [review]
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.
Assignee: nobody → bgrinstead
Attachment #8549963 - Attachment is obsolete: true
Attachment #8559488 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8566329 - Flags: review?(past)
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.
Attachment #8566329 - Flags: review?(past) → review+
(Assignee)

Updated

3 years ago
Summary: Allow persisting (browser) console input history between sessions → Allow persisting console input history between sessions
(Assignee)

Updated

3 years ago
Blocks: 1134845
(Assignee)

Updated

3 years ago
Flags: needinfo?(rFobic)
(Assignee)

Comment 18

3 years ago
Created attachment 8567357 [details] [diff] [review]
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.
Attachment #8567357 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8566329 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e2e9f88b88
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

3 years ago
Created attachment 8569608 [details] [diff] [review]
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
Attachment #8567357 - Attachment is obsolete: true
Attachment #8569608 - Flags: review?(past)
(Assignee)

Comment 21

3 years ago
Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8566329&action=interdiff&newid=8569608&headers=1
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!
Attachment #8569608 - Flags: review?(past) → review+
(Assignee)

Comment 23

3 years ago
(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
(Assignee)

Comment 24

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/8bccd5a79c1a
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

3 years ago
Blocks: 1137448
https://hg.mozilla.org/mozilla-central/rev/8bccd5a79c1a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
(Assignee)

Comment 26

3 years ago
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)]:
relnote-firefox: --- → ?
(Assignee)

Updated

3 years ago
Blocks: 1070469
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.
Flags: needinfo?(bgrinstead)
Relnoted for 39 as: Webconsole input history is persistent even after closing the toolbox
relnote-firefox: ? → 39+
(Assignee)

Comment 29

3 years ago
(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).
Flags: needinfo?(bgrinstead)
You need to log in before you can comment on or make changes to this bug.