1460843 - Console clean up: split hudservice.js

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

hudservice.js can be split into more modules:

1) hudservice.js
- should contain HUD_SERVICE object
- we can also rename HUD_SERVICE => HudService (or HUDService)

2) webconsole.js
- should contain WebConsole object

3) browser-console.js
- should contain BrowserConsole and ShutdownObserver objects

Honza

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #0)
> 2) webconsole.js
> - should contain WebConsole object

Do you mean export this from the existing new-webconsole.js (to be renamed to webconsole.js) file, or have a new file? If the latter, that could be confusing to have two webconsole.js files in the client dir.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> (In reply to Jan Honza Odvarko [:Honza] from comment #0)
> > 2) webconsole.js
> > - should contain WebConsole object
> 
> Do you mean export this from the existing new-webconsole.js (to be renamed
> to webconsole.js) file, or have a new file? If the latter, that could be
> confusing to have two webconsole.js files in the client dir.
Ah, sorry for the delay I missed the question in my bugmail!

Since new-webconsole.js implements `NewWebConsoleFrame` object I was thinking about the following:
new-webconsole.js => webconsole-frame.js
NewWebConsoleFrame => WebConsoleFrame
This should be done in bug 1460841.

This bug is only related to hudservice.js file and I was thinking about extracting WebConsole and BrowserConsole objects out of it. So, there would be two new files:
webconsole.js - containing WebConsole object
browser-console.js - containing BrowserConsole and ShutdownObserver objects

This way there is just one webconsole.js file

Honza

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1460843 - Console clean up: split hudservice.js;

Review commit: https://reviewboard.mozilla.org/r/254242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254242/

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8989182 [details]
Bug 1460843 - Console clean up: split hudservice.js;

https://reviewboard.mozilla.org/r/254242/#review261034

Thanks Honza, this feels a lot cleaner to me.
I have a few small comments, but feel free to land this with a green TRY

::: devtools/client/webconsole/browser-console.js:16
(Diff revision 1)
>   * A BrowserConsole instance is an interactive console initialized *per target*
>   * that displays console log data as well as provides an interactive terminal to
>   * manipulate the target's document content.
>   *
>   * This object only wraps the iframe that holds the Browser Console UI. This is
>   * meant to be an integration point between the Firefox UI and the Browser Console
>   * UI and features.

should we add a sentence to indicate that it extends the WebConsole object located in webconsole.js ?

::: devtools/client/webconsole/browser-console.js:31
(Diff revision 1)
>   *        The target that the browser console will connect to.
>   * @param nsIDOMWindow iframeWindow
>   *        The window where the browser console UI is already loaded.
>   * @param nsIDOMWindow chromeWindow
>   *        The window of the browser console owner.
> + * @param object chromeWindow

s/chromeWindow/hudService

::: devtools/client/webconsole/browser-console.js:34
(Diff revision 1)
>  function BrowserConsole() {
>    WebConsole.apply(this, arguments);

should we explicitely declare the parameters here instead of relying on arguments ?
It would be a bit cleaner IMO but I don't feel to strong about it.

::: devtools/client/webconsole/browser-console.js:58
(Diff revision 1)
>      if (this._bcInit) {
>        return this._bcInit;
>      }
>  
>      // Only add the shutdown observer if we've opened a Browser Console window.
>      ShutdownObserver.init();

I guess we should pass this.hudService here

::: devtools/client/webconsole/browser-console.js:120
(Diff revision 1)
>      this._initialized = true;
>    },
>  
>    observe(message, topic) {
>      if (topic == "quit-application-granted") {
> -      HUDService.storeBrowserConsoleSessionState();
> +      this.hudService.storeBrowserConsoleSessionState();

where do we get this.hudService from ?

::: devtools/client/webconsole/webconsole.js:32
(Diff revision 1)
>   *        The target that the web console will connect to.
>   * @param nsIDOMWindow iframeWindow
>   *        The window where the web console UI is already loaded.
>   * @param nsIDOMWindow chromeWindow
>   *        The window of the web console owner.
> + * @param object chromeWindow

s/chromeWindow/hudService

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)
> ::: devtools/client/webconsole/browser-console.js:16
> (Diff revision 1)
> >   * A BrowserConsole instance is an interactive console initialized *per target*
> >   * that displays console log data as well as provides an interactive terminal to
> >   * manipulate the target's document content.
> >   *
> >   * This object only wraps the iframe that holds the Browser Console UI. This is
> >   * meant to be an integration point between the Firefox UI and the Browser Console
> >   * UI and features.
> 
> should we add a sentence to indicate that it extends the WebConsole object
> located in webconsole.js ?
Done

> ::: devtools/client/webconsole/browser-console.js:31
> (Diff revision 1)
> >   *        The target that the browser console will connect to.
> >   * @param nsIDOMWindow iframeWindow
> >   *        The window where the browser console UI is already loaded.
> >   * @param nsIDOMWindow chromeWindow
> >   *        The window of the browser console owner.
> > + * @param object chromeWindow
> 
> s/chromeWindow/hudService
Done

> ::: devtools/client/webconsole/browser-console.js:34
> (Diff revision 1)
> >  function BrowserConsole() {
> >    WebConsole.apply(this, arguments);
> 
> should we explicitely declare the parameters here instead of relying on
> arguments ?
> It would be a bit cleaner IMO but I don't feel to strong about it.
Yes, I also prefer explicit list of args. Done

> ::: devtools/client/webconsole/browser-console.js:58
> (Diff revision 1)
> >      if (this._bcInit) {
> >        return this._bcInit;
> >      }
> >  
> >      // Only add the shutdown observer if we've opened a Browser Console window.
> >      ShutdownObserver.init();
> 
> I guess we should pass this.hudService here
Ah, yes, Fixed.

> ::: devtools/client/webconsole/webconsole.js:32
> (Diff revision 1)
> >   *        The target that the web console will connect to.
> >   * @param nsIDOMWindow iframeWindow
> >   *        The window where the web console UI is already loaded.
> >   * @param nsIDOMWindow chromeWindow
> >   *        The window of the web console owner.
> > + * @param object chromeWindow
> 
> s/chromeWindow/hudService
Done

Thanks Nicolas!
Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8989182 [details]
Bug 1460843 - Console clean up: split hudservice.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254242/diff/1-2/

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=471fdddd9b303f20b085b3e620f29163f6781221

Honza

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a62423eaaa9
Console clean up: split hudservice.js; r=nchevobbe

Comment 9

[Stefan Hindli [:stefan_hindli]]

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