Closed Bug 1308058 Opened 8 years ago Closed 8 years ago

Implement sessions.getRecentlyClosed WebExtensions API

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Iteration:
52.2 - Oct 17
Tracking Status
firefox52 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

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

Attachments

(1 file)

Implement browser.sessions.getRecentlyClosed method, which is documented at https://developer.chrome.com/extensions/sessions#method-getRecentlyClosed.
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review83844

::: browser/components/extensions/ext-sessions.js:8
(Diff revision 3)
> +"use strict";
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "SessionStore",
> +                                  "resource:///modules/sessionstore/SessionStore.jsm");
> +
> +// TODO: Note function is extracted because it will be used by restore in the future.

can you clean up the comment before landing

::: browser/components/extensions/ext-sessions.js:13
(Diff revision 3)
> +// TODO: Note function is extracted because it will be used by restore in the future.
> +function getRecentlyClosed(maxResults, extension) {
> +  let recentlyClosed = [];
> +
> +  // Get closed windows
> +  let closedWindowData = JSON.parse(SessionStore.getClosedWindowData());

This isn't great, have you talked to the session store maintainers about having an alternative interface here that doesn't require serializing and deserializing json?

::: browser/components/extensions/ext-sessions.js:20
(Diff revision 3)
> +    recentlyClosed.push({
> +      lastModified: window.closedAt,
> +      window: WindowManager.convertClosed(window, extension)});
> +  }
> +
> +  //Get closed tabs

add a space before the text please

::: browser/components/extensions/ext-sessions.js:21
(Diff revision 3)
> +      lastModified: window.closedAt,
> +      window: WindowManager.convertClosed(window, extension)});
> +  }
> +
> +  //Get closed tabs
> +  let windows = Array.from(WindowListManager.browserWindows());

you don't need an array here, just `for (let window of WindowListManager.browserWindows()) {...}`

::: browser/components/extensions/ext-sessions.js:41
(Diff revision 3)
> +extensions.registerSchemaAPI("sessions", "addon_parent", context => {
> +  let {extension} = context;
> +  return {
> +    sessions: {
> +      getRecentlyClosed: function(filter) {
> +        let maxResults = filter ? filter.maxResults : this.MAX_SESSION_RESULTS;

didn't somebody recently add a default value thing to schemas that you could use to avoid the condition here? :-)

::: browser/components/extensions/ext-utils.js:745
(Diff revision 3)
>      }
>  
>      return result;
>    },
>  
> +  // Converts tabs returned from SessionStore.getClosedTabData and

why is this here?  if its about a data type that's specific to the sessions api, ext-sessions.js seems like the right place.
(same for the window bit below)

::: browser/components/extensions/ext-utils.js:749
(Diff revision 3)
>  
> +  // Converts tabs returned from SessionStore.getClosedTabData and
> +  // SessionStore.getClosedWindowData into API tab objects
> +  convertClosed(tab, window) {
> +    let result = {
> +      sessionId: String(tab.closedAt),

This looks dubious.  It looks like we don't actually have a concept of ids?

::: browser/components/extensions/ext-utils.js:756
(Diff revision 3)
> +      windowId: WindowManager.getId(window),
> +      selected: false,
> +      highlighted: false,
> +      active: false,
> +      pinned: false,
> +      incognito: false,

This looks like a placeholder?  Is there a bug to actually fill it in correctly?
Attachment #8798422 - Flags: review?(aswan)
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review83844

> can you clean up the comment before landing

Will do. I put this in here to pre-answer a question I anticipated during the review.

> This isn't great, have you talked to the session store maintainers about having an alternative interface here that doesn't require serializing and deserializing json?

Looking at the code [1], it is specifically designed to deliver JSON serialized data. There is only one consumer that I can see, and it too (like my code) deserializes the JSON [2], so I'm not sure why it was decided to do it that way. I'm guessing that the original developer imagined that there would be other consumers, and some of them would prefer JSON, but that not being the case, maybe we can just change it to return an object rather than JSON. I will ask the developer who has already answered some of my other questions if there is a specific reason to return JSON and if he's be in favour of changing it, or adding another method or argument that would allow us to get an object back instead.

[1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2254
[2] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm#102

> didn't somebody recently add a default value thing to schemas that you could use to avoid the condition here? :-)

Good point. :)  I added this, but I had to hardcode in the `25`. It would be nice to be able to use the `MAX_SESSION_RESULTS` property that is also declared in the schema, but I couldn't figure out how to ref it. Do you have a suggestion?

> why is this here?  if its about a data type that's specific to the sessions api, ext-sessions.js seems like the right place.
> (same for the window bit below)

I discussed this with Kris on IRC a week or two ago and asked him whether he thought it should be in ext-sessions.js or in here. He said he preferred that code that related to windows and code that related to tabs be in ext-utils.js close to the other code that deals with windows and tabs. If you strongly disagree then we can ask him to take a look at this patch and maybe, in context, he would change his mind. I can see virtues in either approach.

> This looks dubious.  It looks like we don't actually have a concept of ids?

That is correct. The sessionId is really only used in order to restore a tab or a window, and the methods we need to use to do that (undoCloseTab [1] and undoCloseWindow [2]) do not have a concept of ids. They only accept an index, which is the index of the tab or window in the current list of closed tabs and windows that is maintained by the browser.

So, when a user wants to restore a window, we need to first get the data about closed windows, and then look up the window the user meant in order to determine its current index. I am using `closedAt` for that as it should be unique for any given tab or window. I realize that this is far from ideal, but it is the way the entire SessionStore restore mechanism is currently architected.

[1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#253
[2] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#269

> This looks like a placeholder?  Is there a bug to actually fill it in correctly?

I knew that `incognito` will always be false for windows, because Firefox does not keep a record of closed private windows, but I just checked and it does keep track of closed tabs in private windows, so I have updated this to use the data that is available for that. I will need to add a test for this.
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review83844

> That is correct. The sessionId is really only used in order to restore a tab or a window, and the methods we need to use to do that (undoCloseTab [1] and undoCloseWindow [2]) do not have a concept of ids. They only accept an index, which is the index of the tab or window in the current list of closed tabs and windows that is maintained by the browser.
> 
> So, when a user wants to restore a window, we need to first get the data about closed windows, and then look up the window the user meant in order to determine its current index. I am using `closedAt` for that as it should be unique for any given tab or window. I realize that this is far from ideal, but it is the way the entire SessionStore restore mechanism is currently architected.
> 
> [1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#253
> [2] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#269

FYI, I sent another note to the maintainer of SessionStore to ask if he'd be in favour of a scheme that would allow us to re-open tabs and windows based on a known id, rather than having to rely on their current index position. I will let you know what he says when he replies.
Depends on: 1309702
Depends on: 1309880
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

This has multiple open dependencies so clearing the review flag for now.  If they're not actually dependencies and you want to land this as-is, please go ahead and re-set the flag.
Attachment #8798422 - Flags: review?(aswan)
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review83844

> FYI, I sent another note to the maintainer of SessionStore to ask if he'd be in favour of a scheme that would allow us to re-open tabs and windows based on a known id, rather than having to rely on their current index position. I will let you know what he says when he replies.

I have fixed this in this patch, but it depends on bug 1309880 landing, which is currently in review. You can review the code, but if you want to run the actual test you'd need to rebase this patch on top of the patch for bug 1309880.
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review83844

> I knew that `incognito` will always be false for windows, because Firefox does not keep a record of closed private windows, but I just checked and it does keep track of closed tabs in private windows, so I have updated this to use the data that is available for that. I will need to add a test for this.

I added a new test for private browsing windows. This should now be fully ready for the next review.
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review85682

::: browser/components/extensions/ext-utils.js:747
(Diff revision 7)
>      return result;
>    },
>  
> +  // Converts tabs returned from SessionStore.getClosedTabData and
> +  // SessionStore.getClosedWindowData into API tab objects
> +  convertClosed(tab, window) {

I still think this would make more sense in ext-sessions.js but if its going to stay here, could you make the name more descriptive, something like `fromClosedTabData`.  It can also be static...

::: browser/components/extensions/ext-utils.js:1058
(Diff revision 7)
>      return result;
>    },
> +
> +  // Converts windows returned from SessionStore.getClosedWindowData
> +  // into API window objects
> +  convertClosed(window, extension) {

Same comment above about location and naming.

::: browser/components/extensions/schemas/sessions.json:68
(Diff revision 7)
> +        "parameters": [
> +          {
> +            "$ref": "Filter",
> +            "name": "filter",
> +            "optional": true,
> +            "default": {"maxResults": 25}

This isn't quite what I meant, I think you've now broken the case where somebody passes an empty object.
I was thinking you would just make the default an empty object then your code doesn't actually need to test for the presence of an object, it just needs to test for the presence of the property.

::: browser/components/extensions/test/browser/browser_ext_sessions.js:6
(Diff revision 7)
> +let createdWindowProps = [];
> +let sessionIds = new Set();

Can you make the functions that populate these just return them and manage them locally?  Using global variables is fragile and difficult to read.

::: browser/components/extensions/test/browser/browser_ext_sessions.js:9
(Diff revision 7)
> +
> +let initialTimestamps = [];
> +let createdWindowProps = [];
> +let sessionIds = new Set();
> +
> +function removeOldItems(item) {

The name of this is confusing since it doesn't actually remove anything.

::: browser/components/extensions/test/browser/browser_ext_sessions.js:13
(Diff revision 7)
> +
> +function removeOldItems(item) {
> +  return !initialTimestamps.includes(item.lastModified);
> +}
> +
> +function checkWindow(window, localCreatedWindowProps) {

Having this code both do checking and have side effects (changing sessionIds and localCreatedWindowProps) feels clumsy.  Since checkWindow() and checkTab() are both only called from one place, I think it would be easier to follow if you moved any side effects out to checkRecentlyClosed()

::: browser/components/extensions/test/browser/browser_ext_sessions.js:73
(Diff revision 7)
> +    browser.tabs.query({active: true, currentWindow: true}).then(tabs => {
> +      currentWindowId = tabs[0].windowId;
> +      return browser.sessions.getRecentlyClosed();
> +    }).then(recentlyClosed => {
> +      browser.test.sendMessage("initialData", {recentlyClosed, currentWindowId});
> +    });

I think this would be easier to follow if written something like:
```js
Promise.all([browser.tabs.query(...), browser.sessions.getRecentlyClosed()])
  .then([windowid, recentlyClosed] => { browser.test.sendMessage(...);
  });
```

::: browser/components/extensions/test/browser/browser_ext_sessions.js:96
(Diff revision 7)
> +      permissions: ["sessions", "tabs"],
> +    },
> +    background,
> +  });
> +
> +  // Open and close a window that will be ignored, to prove that we are removing previous entries

I don't understand this comment?

::: browser/components/extensions/test/browser/browser_ext_sessions.js:127
(Diff revision 7)
> +  extension.sendMessage("check-sessions");
> +  recentlyClosed = yield extension.awaitMessage("recentlyClosed");
> +  let finalResult = recentlyClosed.filter(removeOldItems);
> +  checkRecentlyClosed(finalResult, 5, currentWindowId);
> +
> +  ok(finalResult[0].window, "first item is a window");

I think that in the event these fail, you would get slightly better messages with `is(result.window, undefined, "...")` and `isnot(result.tab, undefined, "...")` (or vice versa depending on the case of course)

::: browser/components/extensions/test/browser/browser_ext_sessions.js:131
(Diff revision 7)
> +  ok(finalResult[2].tab, "third item is a tab");
> +  ok(finalResult[3].window, "fourth item is a window");
> +  ok(finalResult[4].window, "fifth item is a window");

Why don't these have the negative tests like the first 2 cases above?
Attachment #8798422 - Flags: review?(aswan)
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review85682

> I still think this would make more sense in ext-sessions.js but if its going to stay here, could you make the name more descriptive, something like `fromClosedTabData`.  It can also be static...

I checked again with Kris and he still feels it belongs in ext-utils.js, but I did rename the functions. I'm not sure what you mean by making them static though, could you be a bit more explicit?

> Same comment above about location and naming.

Same response. I renamed the function.

> Can you make the functions that populate these just return them and manage them locally?  Using global variables is fragile and difficult to read.

I'm not sure if I've done exactly what you wanted, but I did get rid of the global variables and the code does seem better for it.

> I don't understand this comment?

I could probably remove the comment and the code that follows it, but here's what it means: 

The test includes code that ensures that any closed window data that existed prior to running the test will be ignored, i.e., it won't affect the test to have extra closed window data exist prior to the test being run. In order to prove that that is in fact the case, I open and close a window at the beginning, which will be considered to be part of that "old closed window data". That window is ignored and not considered as part of the data for the test. If my code that ignores "old closed window data" doesn't work, or stops working, doing this extra open/close of a window at the beginning of the test will find that regression.

I don't know if what I just wrote about is more understandable than the original comment, but at least I know what it means. ;)  If it's still not clear let me know.

> Why don't these have the negative tests like the first 2 cases above?

It didn't seem necessary. The negative tests are just to verify that if a session has a tab that it does not have a window and vice-versa, so it seemed that having one of each case was sufficient. It certainly doesn't cost much to add the rest of them so I have done that in the latest commit.
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review85682

> I checked again with Kris and he still feels it belongs in ext-utils.js, but I did rename the functions. I'm not sure what you mean by making them static though, could you be a bit more explicit?

I mean that the code to convert a tab doesn't actually rely on any of the state in an instance of TabManager so you could just make the function be a property of the class instead of part of the prototype.  That's a minor thing though, you don't need to change it if you prefer it the way it is.
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review85958

We're down to little nitpicks on the test code.  I think they're things that might make the code more readable to people looking at it for the first time, but feel free to land when you're happy with it.

::: browser/components/extensions/ext-sessions.js:39
(Diff revisions 7 - 9)
>  extensions.registerSchemaAPI("sessions", "addon_parent", context => {
>    let {extension} = context;
>    return {
>      sessions: {
>        getRecentlyClosed: function(filter) {
> -        return Promise.resolve(getRecentlyClosed(filter.maxResults, extension));
> +        let maxResults = filter.maxResults || this.MAX_SESSION_RESULTS;

nit, this will convert 0 to MAX_SESSION_RESULTS, though i don't know why anybody would ever pass 0 in on purpose...

::: browser/components/extensions/test/browser/browser_ext_sessions.js:58
(Diff revision 9)
> +    yield BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser);
> +    let props = {};
> +    for (let prop of ["screenY", "screenX", "outerWidth", "outerHeight"]) {
> +      props[prop] = win[prop];
> +    }
> +    createdWindowProps.push(props);

This is confusing to me, why not just have this function return `props` and let the caller worry about where to keep that?
Attachment #8798422 - Flags: review?(aswan) → review+
Comment on attachment 8798422 [details]
Bug 1308058 - Implement sessions.getRecentlyClosed WebExtensions API,

https://reviewboard.mozilla.org/r/83932/#review85958

> nit, this will convert 0 to MAX_SESSION_RESULTS, though i don't know why anybody would ever pass 0 in on purpose...

According to the schema 0 is legal, which does seem odd, but I've fixed this.

> This is confusing to me, why not just have this function return `props` and let the caller worry about where to keep that?

Yes, that's *much* better. Thanks!
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/0fd006bf6f9d
Implement sessions.getRecentlyClosed WebExtensions API, r=aswan
Keywords: checkin-needed
backed this out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5822489&repo=autoland
Flags: needinfo?(bob.silverberg)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb7b2d806384
Backed out changeset 0fd006bf6f9d for timeouts in browser_ext_sessions.js
This seems to be exhibiting the same behaviour as the failing test for bug 1309880, which also caused it to be backed out. 

The details are in that bug, but in summary, all of the assertions in the test are passing, but it is failing due to it exceeding the timeout threshold. Looking at the timestamps in the log (a snippet of which is available at [1]), it appears that something is happening between the time the test file starts and the first test in the file is executed. There is a lot of debug output in the log, but I do not know what is happening during that time.

[1] https://gist.github.com/bobsilverberg/8dbec2c875d802a0c17dca09ffb4bae9
Flags: needinfo?(bob.silverberg)
It looks like this is safe to land again. The latest try run looks fine.
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15760cbfa77a
Implement sessions.getRecentlyClosed WebExtensions API, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/15760cbfa77a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Backed out for almost permafailing browser_ext_sessions.js on Linux debug in e10s mode:

https://hg.mozilla.org/mozilla-central/rev/73b00825762607f642563256fdae2ce66e883d0e

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=15760cbfa77a3810d587efae976cd4a02b7b598c
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=6135211&repo=autoland

22:08:27     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_sessions.js | closed tab has the expected value for incognito - 
22:08:27     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_sessions.js | each item has a unique sessionId - 
22:08:27     INFO - --DOCSHELL 0x81208800 == 27 [pid = 2273] [id = 387]
22:08:27     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_sessions.js | the closed private window info was not found in recently closed data - 
22:08:28     INFO - Leaving test bound test_sessions_get_recently_closed_private
22:08:28     INFO - --DOCSHELL 0x8823a800 == 26 [pid = 2273] [id = 390]
22:08:28     INFO - TEST-INFO | started process screentopng
22:08:30     INFO - TEST-INFO | screentopng: exit 0
22:08:30     INFO - Buffered messages logged at 22:08:28
22:08:30     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/ExtensionContent.jsm" line: 853}]
22:08:30     INFO - Buffered messages finished
22:08:30     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_sessions.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -
Status: RESOLVED → REOPENED
Flags: needinfo?(bob.silverberg)
Resolution: FIXED → ---
I split browser_ext_sessions.js into two files for the two tests, submitted to try and retriggered bc2 (which is what was failing above) on linux debug a bunch of times and see no failures, so let's try landing this again.
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c1c87e1bcbe9
Implement sessions.getRecentlyClosed WebExtensions API, r=aswan
Keywords: checkin-needed
(In reply to Bob Silverberg [:bsilverberg] from comment #38)
> I split browser_ext_sessions.js into two files for the two tests, submitted
> to try and retriggered bc2 (which is what was failing above) on linux debug
> a bunch of times and see no failures, so let's try landing this again.
Unfortunately, the webextensions tests are now part of bc5: https://archive.mozilla.org/pub/firefox/try-builds/bsilverberg@mozilla.com-3079871cfb04d5db52198e2b499c366ec03e19b7/try-linux-debug/try_ubuntu32_vm-debug_test-mochitest-e10s-browser-chrome-5-bm03-tests1-linux32-build694.txt.gz
This happened because the tests get rechunked very often and one has to check in which chunk the tests got put.
Thanks aryx. I retriggered bc5 on my try run a few times to see what happens. Is there a quick way to see in which chunk a particular test got put, other than looking at the logs for each chunk?
Flags: needinfo?(aryx.bugmail)
You have to search each of the at the moment 7 logs :-|
Flags: needinfo?(aryx.bugmail)
https://hg.mozilla.org/mozilla-central/rev/c1c87e1bcbe9
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
I'm not seeing `url` in the results, when the sessions are tabs. Code like this:

function gotSessions(sessionInfos) {
  for (sessionInfo of sessionInfos) {
    if (sessionInfo.tab) {
      console.log(sessionInfo.tab.url);
    }      
  }
}

chrome.browserAction.onClicked.addListener(function() {
  chrome.sessions.getRecentlyClosed(gotSessions);
});

...gives me "undefined" in Firefox 52, but lists actual URLs in Chrome. Permissions: ["sessions", "tabs"].

What am I missing?
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.