User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090220 SeaMonkey/2.0b1pre Since closed tabs in SeaMonkey are tied to the browser history, if the user uses the Tools->Clear Private Data... menu and then clears the "Browsing History", SeaMonkey needs to clear the stored closed tabs. Otherwise, if the user uses the "Undo Close Tab" tab bar content menu entry, a blank tab is opened that cannot be closed. Reproducible: Always Steps to Reproduce: 1. Close one or more tabs 2. Click the Tools->Clear Private Data... menu item. 3. In the window that opens check the "Browsing History" and click "Clear Private Data "Now" 4. Right click tab bar and select "Undo Close Tab" Actual Results: A blank tab opens. This tab cannot be closed. Expected Results: The "Undo Close Tab" open should be grayed out as there should not be any closed tabs to restore. Something in SeaMonkey needs to observe the "browser:purge-session-history" notification and wipe out each window's savedBrowsers array when observed. I would suggest either browser.xml or tabbrowser.xml do this. I think the later would make more sense.

I'm trying to submit a patch and request a review. Hopefully I did it correctly.

Comment on attachment 363332 [details] [diff] [review] Clear close tabs when browser history is cleared 2.0b1 doesn't need approval yet.

Comment on attachment 363332 [details] [diff] [review] Clear close tabs when browser history is cleared >--- tabbrowser.xml.1cba9a3b59cb 2009-02-20 11:53:51.679822200 -0500 >+++ tabbrowser.xml 2009-02-20 12:54:29.515292300 -0500 We normally prefer hg-style diffs i.e. a/suite/browser/tabbrowser.xml format. >+ <implementation type="application/javascript" implements="nsIObserver"> Nit: don't bother with type="application/javascript" >+ Nit: spaces on blank line >+ // Wipe out savedBrowsers since history is gone >+ this.savedBrowsers = new Array(); Unfortunately they still need to be properly destroyed. See removeTab. >+ try { >+ var os = Components.classes["@mozilla.org/observer-service;1"] >+ .getService(Components.interfaces.nsIObserverService); >+ os.addObserver(this, "browser:purge-session-history", false); >+ } >+ catch (e) { >+ Components.utils.reportError(e); >+ } Does this one need try/catch? >+ More superfluous spaces. >+ // It's not clear why this sometimes throws an exception. Do you have reproducible steps?

(In reply to comment #3) > We normally prefer hg-style diffs i.e. a/suite/browser/tabbrowser.xml format. > I don't have hg installed on the machine I wrote the patch on (I just used diff). I corrected by hand. > Nit: don't bother with type="application/javascript" > Okay. I got that by looking at browser.xml. I thought it was required. > >+ > Nit: spaces on blank line Fixed. > > >+ // Wipe out savedBrowsers since history is gone > >+ this.savedBrowsers = new Array(); > Unfortunately they still need to be properly destroyed. See removeTab. I was originally going to do a splice but I figured the garbage collection routine would clean up the replaced array since it wouldn't be referenced by anything. I'll change it though since any speed gains by just replacing the array will be negligible. > Does this one need try/catch? > I have no idea, I copied the code from browser.xml which has try/catch logic so I figured it couldn't hurt. I removed it though since I'm not sure how this could cause an exception. > >+ > More superfluous spaces. > Fixed. > >+ // It's not clear why this sometimes throws an exception. > Do you have reproducible steps? Once again, this came from a copy/paste from browser.xml. I took out the entire try/catch clause.

>> Unfortunately they still need to be properly destroyed. See removeTab. > I was originally going to do a splice but I figured the garbage collection GC won't properly destroy the browsers (splice won't help). See http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#1469

Comment on attachment 363402 [details] [diff] [review] Update patch with changes based on Neil's comments. Cancelling as per comment #5.

(In reply to comment #5) >>>Unfortunately they still need to be properly destroyed. See removeTab. >>I was originally going to do a splice but I figured the garbage collection >GC won't properly destroy the browsers (splice won't help). See >http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#1469 That version isn't right either :-( You want line 1439 instead.

Before I go ahead and submit the change. I have a question. How come if the undoing of closed tabs is disabled, the code calls oldBrowser.parentNode.destroy() to destroy the browser's notification box, but when a closed tab gets pushed off the closed tab it does not? See: http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#1439 http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#1469 Should the browser's notification box be destroyed when it's removed from the closed tab list or not?

Oops I misread comment #7 when I posted commment #8 so just ignore the entire comment. I made the changes to destroy the browser as it is done in removeTab on line #1472, but added a call to destroy the notification-box. I figured while I was adding code to destroy the browsers on a session clear, I might as well correct the code that does it when a closed tab is pushed off the end of the closed tab queue, i.e. add a call to destroy the notification box. See comment #7. If you don't want the change to removeTab and would rather file a separate bug (though I think the problems are similar enough to fix it here), tell me and I'll remove the one line I added in removeTab.

Sorry for late comment, but i think maybe better to remove savedBrowsers from tabbrowser.xml and move all the code with them to nsSessionstore.js, which i'm planning to do. See bug 478707, comment #1. Fixing that bug in the way i described should also fix this bug.

I'm not against that personally, but based on bug 36810 comment #163, it seems pretty clear that moving the closed tab functionality from tabbrowser.xml to nssessionstore.js was something that wasn't wanted.

What SeaMonkey drivers don't want is the way FF sesssionstore does undoclosetab and idea to have both savedBrowsers and _closedTabs, but they shouldn't be against moving existing restoretab algorithm to SM sessionstore (maybe FF people will want it too, because our is better ;) ) and eliminate savedBrowsers. This will keep sessionstore more general and help port features/patches from FF with less pain.

Like I said, I'm not against it. If this issue gets fixed in bug 478707, then everything's good. Is there an ETA for the patch for bug 36810? The main reason I ask, is that users can currently end up with uncloseable tabs which require shutting down the browser to get rid of, if bug 36810 won't be fixed for a while, this could be put in as a stop-gap measure. Otherwise it would probably be a good idea to somehow link this bug to bug 478707. This isn't technically a dupe, but it's not really a dependency either.

Bug 36810 is fixed. I think you mean bug 478707. To be honest, i'm actually little bit scared to begin work on it :), because don't understand things well. But i try to get it done before beta 1.

Comment on attachment 363438 [details] [diff] [review] Actually destroy the browsers and notification boxes being removed. [Checkin: Comment 20] >diff --git a/suite/common/tabbrowser.xml b/suite/common/tabbrowser.xml >--- a/suite/common/tabbrowser.xml >+++ b/suite/common/tabbrowser.xml suite/browser/tabbrowser.xml I think you'll find ;-)

So do we want this or do we hold off for the fix to bug 478707?

Still waiting to here what's happening with bug 478707. If it's not going to make SeaMonkey 2.0, this should be checked in.

Well, i'm stuck there, lacking knowledge. If you can help me there ... There is not so much to do for man who know things better.

Well I'm going to request this be added for now since otherwise the browser can end up in a bad state. If and when bug 478707 is fixed something better can be done.

Comment on attachment 363438 [details] [diff] [review] Actually destroy the browsers and notification boxes being removed. [Checkin: Comment 20] http://hg.mozilla.org/comm-central/rev/cc4c93a723de