1045333 - Refactor toolbox.toggleSplitConsole

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Brian Grinstead [:bgrins]]

As mentioned in Comments 11-13 in bug 974550, we could simplify the code used inside of toolbox.open by refactoring the toggleSplitConsole function into openSplitConsole/closeSplitConsole/toggleSplitConsole

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: split-console-refactor.patch

What do you think?

Comment 2

[Joe Walker [:jwalker]]

Comment on attachment 8463956 [details] [diff] [review]
split-console-refactor.patch

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

::: browser/devtools/framework/toolbox.js
@@ +227,5 @@
>    get splitConsole() {
>      return this._splitConsole;
>    },
>  
> +  set splitConsole(value) {

Can't we inline this, or at the very least make it clear that it doesn't actually split the console? It's slightly less DRY to inline, but would be fewer lines overall!

@@ +982,5 @@
>     */
> +  openSplitConsole: function() {
> +    this.splitConsole = true;
> +    this._refreshConsoleDisplay();
> +    this.emit("split-console");

I know this is a change, but shouldn't we be doing this inside the then() below?

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Joe Walker [:jwalker] from comment #2)
> Comment on attachment 8463956 [details] [diff] [review]
> split-console-refactor.patch
> 
> Review of attachment 8463956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/toolbox.js
> @@ +227,5 @@
> >    get splitConsole() {
> >      return this._splitConsole;
> >    },
> >  
> > +  set splitConsole(value) {
> 
> Can't we inline this, or at the very least make it clear that it doesn't
> actually split the console? It's slightly less DRY to inline, but would be
> fewer lines overall!

Yes, moving the functionality directly into openSplitConsole/closeSplitConsole.

> @@ +982,5 @@
> >     */
> > +  openSplitConsole: function() {
> > +    this.splitConsole = true;
> > +    this._refreshConsoleDisplay();
> > +    this.emit("split-console");
> 
> I know this is a change, but shouldn't we be doing this inside the then()
> below?

Yeah, makes sense.. this is only used for tests so pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3e19dc74f3a1

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: split-console-refactor.patch

Address comment 2

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: split-console-refactor.patch

Moved the emitted event back to after the call to refreshConsoleDisplay() for tests.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fa1cc76dd784

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/2b9fbe98ce77

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b9fbe98ce77