Closed Bug 1308060 Opened 8 years ago Closed 8 years ago

Implement sessions.restore WebExtensions API

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [sessions]triaged)

Attachments

(1 file)

Implement browser.sessions.restore method, which is documented at https://developer.chrome.com/extensions/sessions#method-restore.
Depends on: 1309880
Comment on attachment 8802264 [details]
Bug 1308060 - Implement sessions.restore WebExtensions API,

https://reviewboard.mozilla.org/r/86706/#review86864

Just a few minor things but lets get these sorted out before landing...

::: browser/components/extensions/ext-sessions.js:68
(Diff revision 4)
> +        if (sessionId) {
> +          return Promise.resolve(createSession(SessionStore.undoCloseById(sessionId), extension, sessionId));
> +        }
> +
> +        // Need to find the most recently closed tab or window and restore it.
> +        let recentlyClosed = getRecentlyClosed(1, extension)[0];

This seems unnecessarily expensive, getRecentlyClosed() does a whole bunch of formatting of objects (everything available from SessionStore, regardless of maxResults!) and all you use the result for here is to check if the most recent think is a window or a tab...  Is there not a quicker way to get that information from SessionStore?

::: browser/components/extensions/test/browser/browser_ext_sessions.js:200
(Diff revision 4)
>    yield extension.unload();
>  });
> +
> +add_task(function* test_sessions_restore() {
> +  function checkLocalTab(tab, expectedUrl) {
> +    let {Management: {global: {TabManager}}} = Cu.import("resource://gre/modules/Extension.jsm", {});

you do this (plus more) again on line 265, why don't you just hoist this to the top of this fucntion or even the whole file?

::: browser/components/extensions/test/browser/browser_ext_sessions.js:203
(Diff revision 4)
> +add_task(function* test_sessions_restore() {
> +  function checkLocalTab(tab, expectedUrl) {
> +    let {Management: {global: {TabManager}}} = Cu.import("resource://gre/modules/Extension.jsm", {});
> +
> +    let realTab = TabManager.getTab(tab.id);
> +    let tabState = JSON.parse(SessionStore.getTabState(realTab));

why are you checking what SessionStore thinks about the tab instead of checking the tab's url directly?
Attachment #8802264 - Flags: review?(aswan)
Depends on: 1312571
Comment on attachment 8802264 [details]
Bug 1308060 - Implement sessions.restore WebExtensions API,

https://reviewboard.mozilla.org/r/86706/#review86864

> This seems unnecessarily expensive, getRecentlyClosed() does a whole bunch of formatting of objects (everything available from SessionStore, regardless of maxResults!) and all you use the result for here is to check if the most recent think is a window or a tab...  Is there not a quicker way to get that information from SessionStore?

Good point. There is not anything currently in SessionStore that would give us just the information we need, other than using getClosedWindowData and getClosedTabData, which is pretty much what I'm doing now (although you are correct that I am doing even more). I have proposed an addition to SessionStore.jsm and have created a patch for it, both of which can be seen at bug 1312571. I am awaiting feedback and/or review from Mike DeBoer for that bug. My hope is that it will be accepted and I can then use that single method to determine if the most recentlty closed item is a tab or a window.

> why are you checking what SessionStore thinks about the tab instead of checking the tab's url directly?

The tab's url has not been populated at this point. I asked Kris about what I could wait for in order to know that the tab's url would be available, and he suggested taking this approach instead. I.e., asking SessionStore for the tab's url rather than waiting for it to be populated.
Comment on attachment 8802264 [details]
Bug 1308060 - Implement sessions.restore WebExtensions API,

https://reviewboard.mozilla.org/r/86706/#review86864

> Good point. There is not anything currently in SessionStore that would give us just the information we need, other than using getClosedWindowData and getClosedTabData, which is pretty much what I'm doing now (although you are correct that I am doing even more). I have proposed an addition to SessionStore.jsm and have created a patch for it, both of which can be seen at bug 1312571. I am awaiting feedback and/or review from Mike DeBoer for that bug. My hope is that it will be accepted and I can then use that single method to determine if the most recentlty closed item is a tab or a window.

Bug 1312571 has landed (on autoland, thus far) which implements `SessionStore.lastClosedObjectType` which allows me to check whether the last closed object was a window or a tab. It turns out, however, that that only simplifies things completely if it is a window. If the last closed object is a tab I still need to get the window that the tab belongs to, in order to restore it, and I cannot get that information without querying SessionStore again. It's true that I don't need to do any conversion on the data I retrieve, so I just added a few lines to the body of the `restore` function which gets the most recently closed tab from SessionStore and then reopens it using its id. I think this is about as efficient as it's going to get. Even if I were to add another method to SessionStore.jsm to give me only the most recently closed tab, it would still be doing pretty much the same thing.

> The tab's url has not been populated at this point. I asked Kris about what I could wait for in order to know that the tab's url would be available, and he suggested taking this approach instead. I.e., asking SessionStore for the tab's url rather than waiting for it to be populated.

We were hoping to land this in 52, which gives us tomorrow, so my plan is to leave this as is. I tried a bunch of other ways of waiting for the tab to be populated, but none of them seem to work, and this suggestion of Kris' does.
Comment on attachment 8802264 [details]
Bug 1308060 - Implement sessions.restore WebExtensions API,

https://reviewboard.mozilla.org/r/86706/#review88568

Just a nit, looks good to me.

::: browser/components/extensions/ext-sessions.js:64
(Diff revision 7)
>          let maxResults = filter.maxResults == undefined ? this.MAX_SESSION_RESULTS : filter.maxResults;
>          return Promise.resolve(getRecentlyClosed(maxResults, extension));
>        },
> +      restore: function(sessionId) {
> +        if (sessionId) {
> +          return Promise.resolve(createSession(SessionStore.undoCloseById(sessionId), extension, sessionId));

Can you make createSession() return a Promise consistently then you don't need to call Promise.resolve() here.
Also, your choice here but this line is repeated several times here, I think it would be more readable if you had an if..elseif..elseif block that chooses which SessionStore method to call, then the final conversion and return statement once at the end.
Attachment #8802264 - Flags: review?(aswan) → review+
Comment on attachment 8802264 [details]
Bug 1308060 - Implement sessions.restore WebExtensions API,

https://reviewboard.mozilla.org/r/86706/#review88572

just nits, looks good to go when you get the tests passing.
Comment on attachment 8802264 [details]
Bug 1308060 - Implement sessions.restore WebExtensions API,

https://reviewboard.mozilla.org/r/86706/#review88568

> Can you make createSession() return a Promise consistently then you don't need to call Promise.resolve() here.
> Also, your choice here but this line is repeated several times here, I think it would be more readable if you had an if..elseif..elseif block that chooses which SessionStore method to call, then the final conversion and return statement once at the end.

Thanks Andrew. The code does indeed look better after making those changes. :)
This seems fine on try. I did see an intermittent on browser_ext_sessions_getRecentlyClosed.js, but that was introduced in bug 1308058 and is unaffected by this commit, so should be dealt with separately.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d680b3accfc5
Implement sessions.restore WebExtensions API, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d680b3accfc5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Keywords: dev-doc-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.