Closed
Bug 1312571
Opened 8 years ago
Closed 8 years ago
Implement SessionStore.lastClosedObjectType
Categories
(Firefox :: Session Restore, defect, P2)
Firefox
Session Restore
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 hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Comment 4•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
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
Comment 10•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/75db19443bc054c4babd9567f837c2618947cc6c because I'm backing out bug 1309880.
Assignee | ||
Updated•8 years ago
|
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.
Description
•