Closed Bug 1388552 Opened 7 years ago Closed 7 years ago

Restore the Browser Console window

Categories

(Firefox :: Session Restore, enhancement)

Product:
Firefox
Component:
Session Restore
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1388552 - Save Browser Console state in session store
59 bytes, text/x-review-board-request
nchevobbe
: review+
mikedeboer
: review+
Details
Bug 1388552 - Export the HUDService object directly instead of individual methods and properties;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
It would be great to have the Browser Console reopen when the session is restored, similar to how Scratchpad windows get restored
See Also: → 1385452
Requesting review from Mike for the Session Store parts and Nicolas for the DevTools / HUDService parts. I followed the pattern used by Scratchpad for listening to shutdown and storing session state at that time. This is because the final call to getBrowserConsoleSessionState() that comes from Session Store happens after the BC window is closed so HUDService.getBrowserConsole() is always false at that time.

The easiest way I've found to run this locally is to use the shortcut from Bug 1385452, so:

1) Open Browser Console
2) Switch back to main window and do cmd+opt+r / ctrl+alt+r

In this case the session will be restored with the Browser Console.

Alternatively if you restart with the Browser Console closed, it should not be reopened.
Comment on attachment 8895566 [details]
Bug 1388552 - Export the HUDService object directly instead of individual methods and properties;

https://reviewboard.mozilla.org/r/166772/#review172114

::: devtools/client/webconsole/hudservice.js:696
(Diff revision 1)
>  const HUDService = new HUD_SERVICE();
> -
> +exports.HUDService = HUDService;

nit: Can we make this a single line ? `exports.HUDService = new HUD_SERVICE();`
Attachment #8895566 - Flags: review?(nchevobbe) → review+
Comment on attachment 8895122 [details]
Bug 1388552 - Save Browser Console state in session store

https://reviewboard.mozilla.org/r/166230/#review172126

This is looking good overall and working well !
I have a handful of comments, almost all about syntax/naming, and one about an optional saner future proof way to handle restorable tools.

::: browser/components/sessionstore/SessionStore.jsm:2768
(Diff revision 2)
> -    if (lastSessionState.scratchpads) {
> -      DevToolsShim.restoreScratchpadSession(lastSessionState.scratchpads);
> +    if (lastSessionState.scratchpads || lastSessionState.browserConsole) {
> +      DevToolsShim.restoreDevToolsSession({
> +        scratchpads: lastSessionState.scratchpads,
> +        browserConsole: lastSessionState.browserConsole,
> +      });
>      }

This start to feels tedious. If we want to add another tool later (i.e. browser toolbox), we'll have to add another case.
Could this "restorable" tools be in an array or something so we only have to check if the session state contains one of the tools we need to restore, and then passing the arrau to restoreDevToolsSession

::: browser/components/sessionstore/SessionStore.jsm:3142
(Diff revision 2)
>      // Collect and store session cookies.
>      state.cookies = SessionCookies.collect();
>  
> -    let scratchpads = DevToolsShim.getOpenedScratchpads();
> +    let {scratchpads, browserConsole} = DevToolsShim.getDevToolsSession();
> +    state.browserConsole = browserConsole;
>      if (scratchpads && scratchpads.length) {

At l.2768 we call restoreDevToolsSession only if `lastSessionState.scratchpads` is truthy. Does is needs to be updated to mach this line, or should we modify this test to look like the one at l.2768 ?

::: devtools/client/framework/devtools.js:401
(Diff revision 2)
>     * Get the array of currently opened scratchpad windows.
>     *
>     * @return {Array} array of currently opened scratchpad windows.
>     *         Empty array if the scratchpad manager is not loaded.

Update the jsdoc to match what the function does

::: devtools/client/framework/devtools.js:418
(Diff revision 2)
>    /**
>     * Restore the provided array of scratchpad window states.
>     */

Update JSDoc

::: devtools/client/scratchpad/scratchpad-manager.jsm:171
(Diff revision 2)
>      Services.obs.addObserver(this, "quit-application-granted");
>  
>      this._initialized = true;
>    },
>  
> -  observe: function SDO_observe(aMessage, aTopic, aData)
> +  observe(aMessage, aTopic, aData) {

out of curiosity, why we have such patterns (i.e. named functions for properties) ? I see that from time to time in the codebase.

::: devtools/client/webconsole/hudservice.js:52
(Diff revision 2)
>     * @type Map
>     */
>    consoles: null,
>  
> +  _browerConsoleSessionState: false,
> +  setBrowserConsoleSessionState() {

It looks weird to have a setter without parameters. Could we have one, or rename the function to toggleBrowserConsoleSessionState so it better conveys its meaning ? (I was confused by the usage in devtools/client/webconsole/test/browser_console_restore.js)

::: devtools/client/webconsole/hudservice.js:726
(Diff revision 2)
> +    Services.obs.addObserver(this, "quit-application-granted");
> +
> +    this._initialized = true;
> +  },
> +
> +  observe(aMessage, aTopic, aData) {

nit: eslint prevents hungarian notation, could we have the arguments renamed so we save some work when we make this file eslint compliant (message, topic, data would do I think)

::: devtools/client/webconsole/test/head.js:1812
(Diff revision 2)
>      column: location.getAttribute("data-column"),
>    } : null;
>  }
> +
> +function waitForBrowserConsole() {
> +  let deferred = defer();

Could we use plain new Promise here ? We were talking about killing defer in this week devtools central (and I think there are some bugs filed to do that across the codebase).

::: devtools/client/webconsole/test/head.js:1814
(Diff revision 2)
>  }
> +
> +function waitForBrowserConsole() {
> +  let deferred = defer();
> +
> +  Services.obs.addObserver(function observer(aSubject) {

s/aSubject/subject
Attachment #8895122 - Flags: review?(nchevobbe) → review+
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment on attachment 8895122 [details]
Bug 1388552 - Save Browser Console state in session store

https://reviewboard.mozilla.org/r/166230/#review172310

Cool! I think the next patch will hit the mark, but I'd still like to see it before landing.

::: browser/components/sessionstore/SessionStore.jsm:2772
(Diff revision 2)
>  
> -    if (lastSessionState.scratchpads) {
> -      DevToolsShim.restoreScratchpadSession(lastSessionState.scratchpads);
> +    if (lastSessionState.scratchpads || lastSessionState.browserConsole) {
> +      DevToolsShim.restoreDevToolsSession({
> +        scratchpads: lastSessionState.scratchpads,
> +        browserConsole: lastSessionState.browserConsole,
> +      });

I'd recommend obsoleting the conditional and move it to the DevToolsShim, so that one statement is all that remains here.

::: browser/components/sessionstore/SessionStore.jsm:3140
(Diff revision 2)
>      };
>  
>      // Collect and store session cookies.
>      state.cookies = SessionCookies.collect();
>  
> -    let scratchpads = DevToolsShim.getOpenedScratchpads();
> +    let {scratchpads, browserConsole} = DevToolsShim.getDevToolsSession();

I'd change this to:
```js
DevToolsShim.addCurrentSession(state);
```
And move the other code into the shim as well.

::: browser/components/sessionstore/SessionStore.jsm:3537
(Diff revision 2)
>        }
>      }
>  
>      this.restoreWindow(aWindow, root.windows[0], aOptions);
>  
> -    if (aState.scratchpads) {
> +    if (aState.scratchpads || aState.browserConsole) {

I'd recommend changing this to:
```js
DevToolsShim.restoreDevToolsSession(aState);
```
Attachment #8895122 - Flags: review?(mdeboer)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6)
> Comment on attachment 8895566 [details]
> Bug 1388552 - Export the HUDService object directly instead of individual
> methods and properties;
> 
> https://reviewboard.mozilla.org/r/166772/#review172114
> 
> ::: devtools/client/webconsole/hudservice.js:696
> (Diff revision 1)
> >  const HUDService = new HUD_SERVICE();
> > -
> > +exports.HUDService = HUDService;
> 
> nit: Can we make this a single line ? `exports.HUDService = new
> HUD_SERVICE();`

We can't without making other changes in the module, since the WebConsole and BrowserConsole functions in this file reference the HUDService instance.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> ::: devtools/client/scratchpad/scratchpad-manager.jsm:171
> (Diff revision 2)
> >      Services.obs.addObserver(this, "quit-application-granted");
> >  
> >      this._initialized = true;
> >    },
> >  
> > -  observe: function SDO_observe(aMessage, aTopic, aData)
> > +  observe(aMessage, aTopic, aData) {
> 
> out of curiosity, why we have such patterns (i.e. named functions for
> properties) ? I see that from time to time in the codebase.

I can't remember exact details but I believe it used to be needed to display a function name in console.trace(). But now we print out the property name so it isn't needed anymore. So in this particular case with the name it would show `SDO_observe` with the name but wouldn't display a name otherwise, but now we show `observe` if there's no name.

If there was an automated way to switch all of our syntax from `foo: function() {}` and `foo: function bar() {}` to `foo() {}` I can't think of a reason not to do it.
(In reply to Brian Grinstead [:bgrins] from comment #10)
> > out of curiosity, why we have such patterns (i.e. named functions for
> > properties) ? I see that from time to time in the codebase.
> 
> I can't remember exact details but I believe it used to be needed to display
> a function name in console.trace(). But now we print out the property name
> so it isn't needed anymore. So in this particular case with the name it
> would show `SDO_observe` with the name but wouldn't display a name
> otherwise, but now we show `observe` if there's no name.
> 
> If there was an automated way to switch all of our syntax from `foo:
> function() {}` and `foo: function bar() {}` to `foo() {}` I can't think of a
> reason not to do it.

In fact, this is a remnant of the days where thrown errors could not reliably infer the function name from functions as object properties and outside proper stack-traces. So a thrown stack trace would contain a whole bunch of 'Anonymous function' entries, which were _very_ helpful. This is past tense, however, because this was fixed in SpiderMonkey several years ago.

+1 for changing left-overs.
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> outside proper stack-traces. So a thrown stack trace would contain a whole

*output
Blocks: 1389169
Thank you both for the reviews! The latest version should address the comments.
Comment on attachment 8895122 [details]
Bug 1388552 - Save Browser Console state in session store

https://reviewboard.mozilla.org/r/166230/#review172620

One question about naming, bit this looks goof for me as is. r+

::: devtools/client/framework/devtools.js:406
(Diff revision 5)
> -   * Get the array of currently opened scratchpad windows.
> +   * Called from SessionStore.jsm in mozilla-central when saving the current state.
>     *
> -   * @return {Array} array of currently opened scratchpad windows.
> -   *         Empty array if the scratchpad manager is not loaded.
> +   * @param {Object} state
> +   *                 A SessionStore state object that gets modified by reference
>     */
> -  getOpenedScratchpads: function () {
> +  addCurrentSession: function (state) {

Is there a reason not to call it `saveCurrentDevToolsSession` ? It would be consistent with the `restoreDevToolsSession` function.
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > > out of curiosity, why we have such patterns (i.e. named functions for
> > > properties) ? I see that from time to time in the codebase.
> > 
> > I can't remember exact details but I believe it used to be needed to display
> > a function name in console.trace(). But now we print out the property name
> > so it isn't needed anymore. So in this particular case with the name it
> > would show `SDO_observe` with the name but wouldn't display a name
> > otherwise, but now we show `observe` if there's no name.
> > 
> > If there was an automated way to switch all of our syntax from `foo:
> > function() {}` and `foo: function bar() {}` to `foo() {}` I can't think of a
> > reason not to do it.
> 
> In fact, this is a remnant of the days where thrown errors could not
> reliably infer the function name from functions as object properties and
> outside proper stack-traces. So a thrown stack trace would contain a whole
> bunch of 'Anonymous function' entries, which were _very_ helpful. This is
> past tense, however, because this was fixed in SpiderMonkey several years
> ago.
> 
> +1 for changing left-overs.

Thanks for the explanation !
Comment on attachment 8895122 [details]
Bug 1388552 - Save Browser Console state in session store

https://reviewboard.mozilla.org/r/166230/#review172666

Looks very clean, thanks! I'd also be fine with `saveCurrentSession`... I'll leave the bikeshedding to you two! :-P
Attachment #8895122 - Flags: review?(mdeboer) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b54b0c24d18f
Export the HUDService object directly instead of individual methods and properties;r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d33c5f14a37d
Save Browser Console state in session store;r=mikedeboer,nchevobbe
https://hg.mozilla.org/mozilla-central/rev/b54b0c24d18f
https://hg.mozilla.org/mozilla-central/rev/d33c5f14a37d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: