1312571 - Implement SessionStore.lastClosedObjectType

Status: RESOLVED DUPLICATE of bug 1309880

(Firefox :: Session Restore, defect, P2)

Description

[Bob Silverberg [:bsilverberg]]

For the WebExtensions sessions API, in the case where a user does not pass a sessionId to the restore() method, we need to restore the most recently closed item, which may be a tab or a window.

My initial take at this code was using the same routine as I used for getRecentlyClosed in order to figure out whether the most recently closed item was either a tab or a window, but getRecentlyClosed does a lot more than that, calling both SessionStore.getClosedTabData and SessionStore.getClosedWindowData.

Because we only need to know which type of object was most recently closed, this seemed like a good candidate to add to SessionStore. This property will simply return either "tab" or "window", depending on which type of object was most recently closed.

I have already put together a patch for this for review, but am open to suggestions for other ways of efficiently giving the WebExtensions API access to this piece of information.

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1312571 - Implement SessionStore.lastClosedObjectType,

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

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8804082 [details]
Bug 1312571 - Implement SessionStore.lastClosedObjectType,

https://reviewboard.mozilla.org/r/88226/#review87328

The code looks good, so r=me, but the test can be folded into the one to be added in bug 1309880, so let's wait for that.

::: browser/components/sessionstore/SessionStore.jsm:504
(Diff revision 1)
>        LastSession.clear();
>      }
>    },
>  
>    /**
> +   * Returns a string describing the last closed object, either "tab" or "window".

Can you add a sentence that this method was created to support WebExtensions?

::: browser/components/sessionstore/SessionStore.jsm:507
(Diff revision 1)
>  
>    /**
> +   * Returns a string describing the last closed object, either "tab" or "window".
> +   */
> +  get lastClosedObjectType() {
> +    if (this._closedWindows.length != 0) {

nit: `if (this._closedWindows.length) {`

::: browser/components/sessionstore/SessionStore.jsm:508
(Diff revision 1)
>    /**
> +   * Returns a string describing the last closed object, either "tab" or "window".
> +   */
> +  get lastClosedObjectType() {
> +    if (this._closedWindows.length != 0) {
> +      let tabTimestamps = [];

Please add a comment here that explains what you're about to do, like: "Since there are closed windows, we need to check if there's a closed tab in one of the currently open windows that was closed after the last-closed window."

::: browser/components/sessionstore/SessionStore.jsm:517
(Diff revision 1)
> +        let windowState = this._windows[window.__SSi];
> +        if (windowState && windowState._closedTabs[0]) {
> +          tabTimestamps.push(windowState._closedTabs[0].closedAt);
> +        }
> +      }
> +      if (tabTimestamps.length == 0 ||

nit: `!tabTimestamps.length ||`

::: browser/components/sessionstore/test/browser_lastClosedObjectType.js:7
(Diff revision 1)
> +
> +/**
> + * This test is for the lastClosedObjectType property.
> + */
> +
> +Cu.import("resource:///modules/sessionstore/SessionStore.jsm");

To me this looks like you can fold the assertions into the test you're creating for bug 1309880. What do you think?

Comment 3

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8804082 [details]
Bug 1312571 - Implement SessionStore.lastClosedObjectType,

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

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8804082 [details]
Bug 1312571 - Implement SessionStore.lastClosedObjectType,

https://reviewboard.mozilla.org/r/88226/#review88032

Nice! Thanks for the changes, they look really good.

::: browser/components/sessionstore/SessionStore.jsm:513
(Diff revision 2)
>    },
>  
>    /**
> +   * Returns a string describing the last closed object, either "tab" or "window".
> +   *
> +   * This is being added to support the sessions.restore WebExtensions API.

nit: "This was added..."

::: browser/components/sessionstore/SessionStore.jsm:530
(Diff revision 2)
> +        if (windowState && windowState._closedTabs[0]) {
> +          tabTimestamps.push(windowState._closedTabs[0].closedAt);
> +        }
> +      }
> +      if (!tabTimestamps.length ||
> +        (tabTimestamps.sort((a, b) => b - a)[0] < this._closedWindows[0].closedAt)) {

nit: if you give this line two more spaces, it'll align nicely with the first clause.

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8804082 [details]
Bug 1312571 - Implement SessionStore.lastClosedObjectType,

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

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8804082 [details]
Bug 1312571 - Implement SessionStore.lastClosedObjectType,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88226/diff/3-4/

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8804082 [details]
Bug 1312571 - Implement SessionStore.lastClosedObjectType,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88226/diff/4-5/

Comment 8

[Bob Silverberg [:bsilverberg]]

This is ready to be checked-in, but please land it after bug 1309880.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4537bb4cfb82
Implement SessionStore.lastClosedObjectType, r=mikedeboer

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/75db19443bc054c4babd9567f837c2618947cc6c because I'm backing out bug 1309880.