996778 - Remember split console height

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P3)

Description

[Jonathan Allard]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140411004002

Steps to reproduce:

Open dev tools in inspector in a page, and open split console. Notice the split is too large, reduce the size.

Open another tab and inspector, open split console.


Actual results:

Split console is a default height


Expected results:

Preferred height is recalled

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: split-console-height.patch

This patch remembers the split console height while resizing (so it will be set when opening toolbox in a new tab), and when destroying the toolbox.

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: split-console-height.patch

Same patch, but with a Math.round() call in the test.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=01e3bc6c4ca3

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 8464822 [details] [diff] [review]
split-console-height.patch

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

This is looking good. Thanks. Just some minor comments:

::: browser/devtools/framework/toolbox.js
@@ +257,5 @@
>          this._loadInitialZoom();
>  
> +        this.webconsolePanel = this.doc.getElementById("toolbox-panel-webconsole");
> +        this.webconsolePanel.height =
> +          Services.prefs.getIntPref("devtools.toolbox.splitconsoleHeight");

Might want to add a PREF_... constant for the pref name.

@@ +355,5 @@
>    },
>  
> +  _saveSplitConsoleHeight: function() {
> +    Services.prefs.setIntPref("devtools.toolbox.splitconsoleHeight",
> +      this.doc.getElementById("toolbox-panel-webconsole").height);

Why not use this.webconsolePanel directly?

::: browser/devtools/webconsole/test/browser_webconsole_split_persist.js
@@ +21,5 @@
>      yield toggleSplitConsoleWithEscape();
>      ok(toolbox.splitConsole, "Split console is now visible.");
> +    ok(getVisiblePrefValue(), "Visibility pref is true");
> +
> +    let splitConsolePanel = toolbox.doc.getElementById("toolbox-panel-webconsole");

Why not use toolbox.webconsolePanel?

@@ +35,5 @@
>  
>      ok(toolbox.splitConsole, "Split console is visible by default.");
> +    is(getHeightPrefValue(), 200, "Height is set based on panel height after closing");
> +
> +    let splitConsolePanel = toolbox.doc.getElementById("toolbox-panel-webconsole");

Same here. Also the variable is already defined - you might want to remove |let| here.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: split-console-height.patch

Thanks for the feedback - this patch addresses your comments.  Try push: https://tbpl.mozilla.org/?tree=Try&rev=7f73dea49e37

Comment 5

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/071ac392b4fb

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/071ac392b4fb