Closed Bug 639836 Opened 11 years ago Closed 11 years ago

Port Bug 581937 ["Recently closed tabs" list keep blank tabs], also add observer for browser.sessionstore.max_tabs_undo

Categories

(SeaMonkey :: Session Restore, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch patch plus test (obsolete) — Splinter Review
From parent bug:

Recently closed tabs list keep blank tabs

See http://forums.mozillazine.org/viewtopic.php?p=9676103#p9676103

Reproducible: Always

Steps to Reproduce:
1. Start Minefield
2. Open New Tab
3. Close the New Tab
4. History > Recently Closed Tabs

Actual Results:
 "Recently closed tabs" listed New Tab 

Expected Results:
 Should not

Also i've added missing pref observer for  browser.sessionstore.max_tabs_undo. Observer for  browser.tabs.max_tabs_undo is needed too, but it's little bit trickier, so i used workaround to make test pass.
Attachment #517746 - Flags: review?(neil)
Attached patch Tabbrowser observer (untested) (obsolete) — Splinter Review
Rather conveniently, the tabbrowser already has an observer all set to delete saved browsers, so all I had to do was to limit it to the new pref value.
Attachment #519179 - Flags: feedback?(misak.bugzilla)
This piece of test  still failing:

  Services.prefs.setIntPref("browser.sessionstore.max_tabs_undo", 0);
  Services.prefs.setIntPref("browser.tabs.max_tabs_undo", 0);
  Services.prefs.clearUserPref("browser.sessionstore.max_tabs_undo");
  Services.prefs.clearUserPref("browser.tabs.max_tabs_undo");

  is(ss.getClosedTabCount(window), 0, "should be no closed tabs");

It returns 3 instead of 0, though patch looks good to me ...
Attached patch Tabbrowser observer (obsolete) — Splinter Review
Previous patch had a silly typo. (Whose idea was it to put the parameters to prefs.addObserver in a different order to obs.addObserver?)
Attachment #519179 - Attachment is obsolete: true
Attachment #519179 - Flags: feedback?(misak.bugzilla)
Attachment #519856 - Flags: review?(misak.bugzilla)
Comment on attachment 519856 [details] [diff] [review]
Tabbrowser observer

For me two patches are identical ...
Third time lucky?
Attachment #519856 - Attachment is obsolete: true
Attachment #519856 - Flags: review?(misak.bugzilla)
Attachment #520234 - Flags: review?(misak.bugzilla)
Attachment #520234 - Flags: review?(misak.bugzilla) → review+
Updated test after observer patch, now i don't need to forgetSavedBrowsers.
Attachment #517746 - Attachment is obsolete: true
Attachment #517746 - Flags: review?(neil)
Attachment #520449 - Flags: review?(neil)
Hmm... we should ensure that tabbrowser doesn't try to undo the tab either.
Sure, sorry. Hope this is the proper fix.
Assignee: nobody → misak.bugzilla
Attachment #520449 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #520449 - Flags: review?(neil)
Attachment #520467 - Flags: review?(neil)
Comment on attachment 520467 [details] [diff] [review]
same patch, with updated test, also uses same logic for saved tabbrowser

>-            if (maxUndoDepth <= 0 || oldSH.count == 0 || aDisableUndo || inOnLoad || isPopup) {
Looks like it used to avoid closing blank tabs, but nowadays the count is 1 for a blank tab. I'll have to come up with a better test.
Attachment #520467 - Flags: review?(neil)
Bug 642998 has the blank tab fix.
Depends on: 642998
Attachment #520449 - Attachment is obsolete: false
Attachment #520449 - Flags: review+
Attachment #520467 - Attachment is obsolete: true
Attachment #520467 - Flags: review-
Pushed: http://hg.mozilla.org/comm-central/rev/9e342d08c55f
http://hg.mozilla.org/comm-central/rev/41b75a0080fd
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.