Closed Bug 1312571 Opened 8 years ago Closed 8 years ago

Implement SessionStore.lastClosedObjectType

Categories

(Firefox :: Session Restore, defect, P2)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1309880
Iteration:
52.3 - Nov 14

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

Attachments

(1 file)

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 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?
Attachment #8804082 - Flags: review?(mdeboer)
Depends on: 1309880
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.
Attachment #8804082 - Flags: review?(mdeboer) → review+
This is ready to be checked-in, but please land it after bug 1309880.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4537bb4cfb82
Implement SessionStore.lastClosedObjectType, r=mikedeboer
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.