974550 - Add a preference to optionally persist split console state

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

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

I noticed in Canary that the split console will reappear on toolbox open if it was previously open. This state is persisted across tabs. I suspect this is something I got from tweaking about:flags

I noticed myself missing this recently ( our split console state is not persisted ). What I propose is to expose a new preference:

* if the preference is on, every time the user closes or opens the split console this state is stored, and used the next time the toolbox is opened

* the preference should be off by default

Comment 1

[markus.priewasser]

Also please remember the height of the split console panel after you changed it. Right now the height is reset every time you open devtools to around half of the page height, which is quite big.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to markus.priewasser from comment #1)
> Also please remember the height of the split console panel after you changed
> it. Right now the height is reset every time you open devtools to around
> half of the page height, which is quite big.

You are right, it doesn't remember the height but I think it should.  Of course it should still be constrained to the current toolbox size so as to prevent weirdness from:

1) Make devtools really tall (600px)
2) Make split console really tall (500px)
2) Close split console
3) Make devtools short (200px)
4) Open split console (max should still be constrained within 50px or so of the current toolbox height)

Comment 3

[Brian Grinstead [:bgrins]]

Bug 996778 is opened to remember the height

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: split-console-persist.patch

Persists the split console state in the toolbox and adds a test for it.  Had to change the behavior of toggleSplitConsole a bit to allow changing while console panel is opened to make sure that the state is consistently applied (even if console is the first selected panel).

The difference now is if you open the console panel then click the split console button / press escape then the split console state will be updated.  We may consider stopping any escape keypress from doing this when the jsterm input is focused (although it will already not do this when closing an autocomplete popup or variables view).

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 8461054 [details] [diff] [review]
split-console-persist.patch

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

Patch looks good to me. Thanks a lot for your patch!

I think that we should avoid allowing the state change while the console tab is active, but I wont block the patch for this change. r+

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: split-console-persist.patch

Thought of a better way to handle this that prevents state change when console is active

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 8461593 [details] [diff] [review]
split-console-persist.patch

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

Thanks for the update.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: split-console-persist.patch

I had to make a couple of changes to fix some issues with tests (like https://tbpl.mozilla.org/?tree=Try&rev=69c68ec2baba).  The primary change had to do with a leak that was happening in browser_toolbox_sidebar.js.

What was happening was that the split console was left opened in a previous test, so it was being opened on startup in toolbox.js.  There were two issues factoring into the leak (that I'd like Joe to take a look at):

1) Toolbox.open() was not waiting for toggleSplitConsole to finish (specifically for the webconsole panel to be opened) before resolving.  This left a possibility for a leak if the toolbox was quickly closed before the tool had finished loading.
2) gDevTools.showToolbox() was not waiting for toolbox.open() to finish *if* a toolId was passed in.  I looked further into this logic and realized that by the time open finishes, the event that it was waiting for (toolId + "-selected") would have already fired anyway - open waits for this event as well.  So I simplified this method to always wait for open to finish.

Anyway, I went ahead and cleaned up the split console state in the offending test, but I think we should still go with these changes because it is a leak waiting to happen.

Comment 9

[Brian Grinstead [:bgrins]]

Joe, in case I wasn't clear enough - I just need a review on the changes to gDevTools.jsm and the promise changes in toolbox.open / toolbox.toggleSplitConsole

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: split-console-persist.patch

Pretty much the same patch, but with a few changes so that all tests are passing: https://tbpl.mozilla.org/?tree=Try&rev=c9d1f36ce764

Comment 11

[Joe Walker [:jwalker]]

Comment on attachment 8463343 [details] [diff] [review]
split-console-persist.patch

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

::: browser/devtools/framework/toolbox.js
@@ +983,2 @@
>     */
> +  toggleSplitConsole: function(forceToggle = false) {

I'm not a huge fan of this function name; wouldn't openSplitConsole be a better name. Maybe this isn't the place to change it though.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Joe Walker [:jwalker] from comment #11)
> Comment on attachment 8463343 [details] [diff] [review]
> split-console-persist.patch
> 
> Review of attachment 8463343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/toolbox.js
> @@ +983,2 @@
> >     */
> > +  toggleSplitConsole: function(forceToggle = false) {
> 
> I'm not a huge fan of this function name; wouldn't openSplitConsole be a
> better name. Maybe this isn't the place to change it though.

It does toggle the console state (which is convenient for a few places, like ESC key and the command button).  We could refactor such that there is a closeSplitConsole / openSplitConsole / toggleSplitConsole, where toggleSplitConsole that just does a check on the current state and calls one or the other.  Then in the toolbox.open function we could call openSplitConsole instead of toggleSplitConsole with a bool parameter.  I can tackle this in 996778 or a separate follow up.

Comment 13

[Joe Walker [:jwalker]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> (In reply to Joe Walker [:jwalker] from comment #11)
> > Comment on attachment 8463343 [details] [diff] [review]
> > split-console-persist.patch
> > 
> > Review of attachment 8463343 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/framework/toolbox.js
> > @@ +983,2 @@
> > >     */
> > > +  toggleSplitConsole: function(forceToggle = false) {
> > 
> > I'm not a huge fan of this function name; wouldn't openSplitConsole be a
> > better name. Maybe this isn't the place to change it though.
> 
> It does toggle the console state (which is convenient for a few places, like
> ESC key and the command button).  We could refactor such that there is a
> closeSplitConsole / openSplitConsole / toggleSplitConsole, where
> toggleSplitConsole that just does a check on the current state and calls one
> or the other.  Then in the toolbox.open function we could call
> openSplitConsole instead of toggleSplitConsole with a bool parameter.  I can
> tackle this in 996778 or a separate follow up.

Ug yes. _refreshConsoleDisplay() applies the state that is setup elsewhere. I'll leave it up to you when you do this.

Comment 14

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/571e039d0de0

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/571e039d0de0