Extensions desiring to selectively forget information about closed tabs/windows have to be quite inventive. What about a new API: void setClosedTabData(in nsIDOMWindow aWindow, in AString aData); or void forgetClosedTab(in nsIDOMWindow aWindow, in unsigned long aIndex); and the same for closed windows once bug 394759 is fixed?

Can this get in for 3.6? Before 3.6a1pre, this an extension could remove a closed tab, by simply reading the closed tab list, splicing out the tab and then setting it using the SetWindowState() function with a state variable as such: {"windows":["_closedTabs":[...]}]} Now this throws an exception, because there must be a tabs[0].boxObject (in restoreHistoryPrecursor). So there no longer is any real work around other than saving then restoring the entire window.

(In reply to comment #1) Doesn't the work-around from the URL work anymore?

It probably does, I never tried that. Though wouldn't that method cause the user to see the tab open and then close? In any case, I was using the work around that oneman (Tab Mix Plus) came up with which was basically to do a SetWindowState with an overwrite parameter of false and with the following state: '{"windows":["_closedTabs":[...]}], "_firstTabs": "true" }' This basically tricked Session Store into solely updating the closedTabs for the window and without changing anything else. In Minefield 3.6a1pre this will throw an exception since SessionStore does not expect the overwrite parameter to be true when firstTabs is true. More specifically, it never expects the tabs array to be empty in the restoreHistoryPrecursor function. I would write a bug against SessionStore for the exception, but I'm pretty the SetWindowState API function was never intended to be used in such a manner even though it worked.

(In reply to comment #3) > It probably does, I never tried that. Though wouldn't that method cause the > user to see the tab open and then close? That's indeed a known issue with that work-around. (In reply to comment #1) > Can this get in for 3.6? Sure, as soon as somebody volunteers a patch and ideally some tests and the patch passes review. Have you ever considered trying to do this yourself? I'd of course help you along with any issue you might encounter...

(In reply to comment #4) > Sure, as soon as somebody volunteers a patch and ideally some tests and the > patch passes review. Have you ever considered trying to do this yourself? I'd > of course help you along with any issue you might encounter... Sure why not. I don't think the patch itself should be that difficult since it looks like the tab can just be spliced out of his._windows[aWindow.__SSi]._closedTabs._closedTabs, basically the first half of undoCloseTab (and a lot simpler than it was in SeaMonkey bug 479448). I think the hardest part, at least for me, would be the automated test since I'm not very familiar with them.

(In reply to comment #5) > I think the hardest part, at least for me, would be the automated test since > I'm not very familiar with them. For the test, you can follow the lead of what we've already got at <http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser/>. Your test might actually look quite similar to the one for bug 464199 which is also about selectively forgetting closed tabs.

Okay, I came up with the patch and the test. I'm not sure who is supposed to review these so I just set you as the reviewer. Please tell me if the test is good enough.

Comment on attachment 369936 [details] [diff] [review] Patch to add forgetClosedTab function to SessionStore and test file Thanks for giving this a shot, Michael. Your patch looks good, save a few details: >+ nsIDOMNode forgetClosedTab(in nsIDOMWindow aWindow, in unsigned long aIndex); When you change an interface, you always have to change its IID as well (search for uuid). >+ * Contributor(s): >+ * You've certainly contributed to this file. >+ ss.forgetClosedTab(newWin, 2); >+ ss.forgetClosedTab(newWin, null); Nit: You're using a tab instead of spaces for indentation. And I'd like you to test invalid conditions as well, i.e. make sure that passing -1 or (test_state.windows[0]._closedTabs.length + 1) actually throws. See e.g. the test for bug 345898 for how we do this for the rest of our API. >+ gPrefService.setIntPref("browser.sessionstore.max_tabs_undo", max_tabs_undo); We're just about to change to using gPrefService.clearUserPref instead (see the patch waiting for check-in in bug 485088). Please do so as well. Finally, please make sure that all files in your patch are UTF-8 encoded.

(In reply to comment #8) > When you change an interface, you always have to change its IID as well (search > for uuid). > I didn't know that. Okay, done > >+ * Contributor(s): > >+ * > > You've certainly contributed to this file. > Oops. Okay I added myself. > Nit: You're using a tab instead of spaces for indentation. > Converted tab to spaces. > And I'd like you to test invalid conditions as well, i.e. make sure that > passing -1 or (test_state.windows[0]._closedTabs.length + 1) actually throws. Okay I added some invalid conditions to the test. > We're just about to change to using gPrefService.clearUserPref instead (see the > patch waiting for check-in in bug 485088). Please do so as well. Done > Finally, please make sure that all files in your patch are UTF-8 encoded. I checked and they already are UTF-8 (technically "UTF-8 without BOM"). I'm going to recompile and retest and I'll resubmit in a few.

Implemented requested changes

Comment on attachment 370136 [details] [diff] [review] Patch to add forgetClosedTab function to SessionStore and test file - take 2 >+ function test(aLambda) { I know that this function isn't too descriptively named in the other tests, either, but "testForError" or something similar would make the actual tests below easier to read. >+ // all of the following calls with illegal arguments should throw NS_ERROR_ILLEGAL_VALUE Please move these tests above the check for how many closed tabs remain, so that you can verify that not only they do throw but don't accidentally delete closed tab data. r+=me with these two fixed, thanks.

Added 2 changes specified by Zeniko.

Comment on attachment 370201 [details] [diff] [review] Patch to add forgetClosedTab function to SessionStore and test file - take 3 Zeniko, there's no way to r+ you directly so I made it r?.

Comment on attachment 370201 [details] [diff] [review] Patch to add forgetClosedTab function to SessionStore and test file - take 3 You can carry over the r+ yourself (as long as it's got my own name attached above). BTW: I didn't mean to rename the argument to the test function but its name ("test") so that below it reads more to the point ok(testForError(function() ...),

> BTW: I didn't mean to rename the argument to the test function but its name > ("test") so that below it reads more to the point Sigh, okay fixed.

Comment on attachment 370273 [details] [diff] [review] Patch to add forgetClosedTab function to SessionStore and test file - take 4 >+ }, >+ >+ forgetClosedTab: function sss_undoCloseTab(aWindow, aIndex) { >+ if (!aWindow.__SSi) >+ throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG); sss_forgetClosedTab r=me with this fixed, thanks!

> sss_forgetClosedTab Oops. Okay fixed. I can't set superreview+, so I just set review+. Hopefully that's sufficient. Can someone add a "checkin-needed" keyword?

Thanks for the patch, Michael! Looking forward to your next patch... ;-)

I don't suppose there's any possible way to get this "wanted+" for Firefox 3.5? The reason I ask, is that since bug 480148 is now in Firefox 3.5, without the fix for this bug both Session Manager and Tab Mix Plus will need to use the workaround from the URL which is less than ideal. As is, the latest development version of TMP breaks the closed tab list in the nightly Firefox 3.5 loads and there's really no good work around for that without this fix.

Comment on attachment 370457 [details] [diff] [review] Patch to add forgetClosedTab function to SessionStore and test file - take 5 Drivers: This additional API would help our privacy efforts for 3.5 without having late-compat effects.

Is it possible to make forgetClosedTab return the closed tab status?

(In reply to comment #22) > Is it possible to make forgetClosedTab return the closed tab status? I'm not sure what you mean by "the closed tab status". There is no status for the removed closed tab, as it's gone after returning from the call to forgetClosedTab. The call will always be successful as long as the parameters are valid, if they are not an exception will be thrown. If you're referring to the number of closed tabs after the tab is removed, that can be retrieved via the getClosedTabCount function. If you aren't referring to any of the above, then I'm not sure what you are looking for.

Comment on attachment 370457 [details] [diff] [review] Patch to add forgetClosedTab function to SessionStore and test file - take 5 This changes the vtable for binary compatibility - can we get a patch that doesn't? Otherwise the costs don't seem to bear out the benefits.

I think it's pretty unlikely that this interface is being used by binary components, but since there also doesn't appear to be a strong need for the feature, it's probably best to avoid landing it at this late stage anyways. Feel free to renominate if you disagree...

I'd argue for taking this on the branch if bug 488930 gets fixed in time. This API is quite simple, reasonably tested and would fit into Firefox 3.5 offering improved privacy (extensions currently have to employ one of several hacks to purge individual tabs from the list of recently closed ones).

Comment on attachment 370457 [details] [diff] [review] Patch to add forgetClosedTab function to SessionStore and test file - take 5 >+ nsIDOMNode forgetClosedTab(in nsIDOMWindow aWindow, in unsigned long aIndex); Have you considered adding a helper API to tabbrowser to this? Benefits: 1. Adding, removing and restoring tabs are already exposed on tabbrowser 2. Independent of any backend differences you have to take for FF3.5 3. SeaMonkey wants to implement this on tabbrowser ;-)

Personally I haven't really thought about it since there are some tricks that can be done using using the setWindowState to do all of that. It's probably a good idea though as it would be a way to unify SeaMonkey's and Firefox's closed tab API interfaces since the current sessionstore APIs don't work all that well (or in one case, at all) on closed tabs in SeaMonkey.