1959616 - Move browser-window UI functionality for session restore from browser.js and BrowserGlue.sys.mjs to a session restore module

Status: NEW

(Firefox :: Session Restore, task, P1)

Description

[:Gijs (he/him)]

browser.js defines (among much else) the following globals:

restoreLastClosedTabOrWindowOrSession
function undoCloseTab(aIndex, sourceWindowSSId)
function undoCloseWindow(aIndex)
var RestoreLastSessionObserver

Additionally, BrowserGlue has _maybeShowRestoreSessionInfoBar().

This is all basically session restore interacting with individual windows and either modifying UI in them, or handling UI clicks and then calling into the main SessionStore module.

I think these could live in a new module, browser/components/sessionstore/SessionWindowUI.sys.mjs or similar.

Shane or Jason, would you like to pick this up?

Comment 1

[Jason Jones]

Absolutely! We'll have a patch ready for feedback soon.

Comment 2

[Jason Jones]

Attachment: Bug 1959616 - Move browser-window UI functionality for session restore from browser.js and BrowserGlue.sys.mjs into SessionWindowUI.sys.mjs r?Gijs!

Comment 3

[Jason Jones]

Hi Gijs, I've put together a WIP patch I'd like your feedback on. The approach moves the mentioned globals into a new browser/components/sessionstore/SessionWindowUI.sys.mjs module with two exports. Also, I could use some guidance in honing in on appropriate tests to run. So far, the following were checked/updated, but I'm a bit lost on coverage for RestoreLastSessionObserver and BrowserGlue/maybeShowRestoreSessionInfoBar() in particular.

browser/components/extensions/test/browser/browser_ext_tabs_remove.js
browser/components/sessionstore/test/browser_restoreLastActionCorrectOrder.js
browser/components/sessionstore/test/browser_restoreLastClosedTabOrWindowOrSession.js
browser/components/tabbrowser/test/browser/tabs/browser_undo_close_tabs.js
browser/components/tabbrowser/test/browser/tabs/browser_undo_close_tabs_at_start.js
browser/components/sessionstore/test/browser_undoCloseById.js
browser/components/sessionstore/test/browser_undoCloseById_targetWindow.js
browser/components/sessionstore/test/browser_aboutSessionRestore.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutSessionRestore.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_noSessionRestoreMenuOption.js

As mentioned in 1959618, browser/components/sessionstore/test/browser_replace_load.js hangs but doesn't appear to be caused by our code changes, so I haven't run a full /sessionstore/ test suite.

Comment 4

[:Gijs (he/him)]

(In reply to Jason Jones from comment #3)

Hi Gijs, I've put together a WIP patch I'd like your feedback on. The approach moves the mentioned globals into a new browser/components/sessionstore/SessionWindowUI.sys.mjs module with two exports.

Nice! Looks like the linter has noticed the removal of the undoCloseTab and undoCloseWindow global and has flagged up some more files that need updating to talk to the new module. :-)

Also, I could use some guidance in honing in on appropriate tests to run. So far, the following were checked/updated, but I'm a bit lost on coverage for RestoreLastSessionObserver and BrowserGlue/maybeShowRestoreSessionInfoBar() in particular.

Looks like for RestoreLastSessionObserver, we want to check what tests check for the command in the window being enabled/disabled. That's at least browser/components/privatebrowsing/test/browser/browser_privatebrowsing_noSessionRestoreMenuOption.js. But the command also affects the app menu and menubar entries, and the former has an automated test at https://searchfox.org/mozilla-central/source/browser/components/customizableui/test/browser_history_restore_session.js .

For maybeShowRestoreSessionInfoBar, unfortunately I can't see any tests. However, you could probably test manually fairly easily, by using ./mach run -P with a new profile (see https://support.mozilla.org/en-US/kb/profile-manager-create-remove-switch-firefox-profiles for how to set up "old school" profiles), and then closing it and running the same ./mach run command with the same profile, and checking you get the infobar.

As mentioned in 1959618, browser/components/sessionstore/test/browser_replace_load.js hangs but doesn't appear to be caused by our code changes, so I haven't run a full /sessionstore/ test suite.

That test is skipped (if you look at https://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_replace_load.js it says "This test gets skipped with pattern: true"), so if you run the entire directory it won't run and you can check the results of the other tests.

Comment 5

[Jason Jones]

(In reply to :Gijs (he/him) from comment #4)

Nice! Looks like the linter has noticed the removal of the undoCloseTab and undoCloseWindow global and has flagged up some more files that need updating to talk to the new module. :-)

Updated and tested successfully!

Looks like for RestoreLastSessionObserver, we want to check what tests check for the command in the window being enabled/disabled. That's at least browser/components/privatebrowsing/test/browser/browser_privatebrowsing_noSessionRestoreMenuOption.js. But the command also affects the app menu and menubar entries, and the former has an automated test at https://searchfox.org/mozilla-central/source/browser/components/customizableui/test/browser_history_restore_session.js .

Manually verified Browser:RestoreLastSession was being toggled appropriately, and both these tests checked out successfully.

For maybeShowRestoreSessionInfoBar, unfortunately I can't see any tests. However, you could probably test manually fairly easily, by using ./mach run -P with a new profile (see https://support.mozilla.org/en-US/kb/profile-manager-create-remove-switch-firefox-profiles for how to set up "old school" profiles), and then closing it and running the same ./mach run command with the same profile, and checking you get the infobar.

Verified, along with the below directories.

browser/components/sessionstore/test
browser/components/tabbrowser/test/browser/tabs