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

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

Attachment: patch plus test

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.

Comment 1

[[e-mail]]

Attachment: Tabbrowser observer (untested)

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.

Comment 2

[Misak Khachatryan]

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 ...

Comment 3

[[e-mail]]

Attachment: Tabbrowser observer

Previous patch had a silly typo. (Whose idea was it to put the parameters to prefs.addObserver in a different order to obs.addObserver?)

Comment 4

[Misak Khachatryan]

Comment on attachment 519856 [details] [diff] [review]
Tabbrowser observer

For me two patches are identical ...

Comment 5

[[e-mail]]

Attachment: Tabbrowser observer

Third time lucky?

Comment 6

[Misak Khachatryan]

Attachment: same patch, with updated test

Updated test after observer patch, now i don't need to forgetSavedBrowsers.

Comment 7

[[e-mail]]

Hmm... we should ensure that tabbrowser doesn't try to undo the tab either.

Comment 8

[Misak Khachatryan]

Attachment: same patch, with updated test, also uses same logic for saved tabbrowser

Sure, sorry. Hope this is the proper fix.

Comment 9

[[e-mail]]

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.

Comment 10

[[e-mail]]

Bug 642998 has the blank tab fix.

Comment 11

[Misak Khachatryan]

Pushed: http://hg.mozilla.org/comm-central/rev/9e342d08c55f
http://hg.mozilla.org/comm-central/rev/41b75a0080fd