Closed Bug 1724960 Opened 3 years ago Closed 3 years ago

Add one-time infobar that indicates how to restore the previous session on the next session startup if automatic session restore is disabled

Categories

(Firefox :: Session Restore, enhancement, P3)

Desktop
All
enhancement
Points:
1

Tracking

()

VERIFIED FIXED
94 Branch
Tracking Status
firefox93 --- wontfix
firefox94 --- verified
firefox95 --- verified

People

(Reporter: Gijs, Assigned: sfoster)

References

Details

(Whiteboard: [fidefe-mr11-close-tabs])

Attachments

(2 files)

No description provided.
Blocks: 1724962

We can have the infobar button do nothing or just open the hamburger panel, and leave the rest of the implementation of the highlighting etc. to bug 1724962.

The Bugbug bot thinks this bug should belong to the 'Firefox::Session Restore' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Component: General → Session Restore
Points: --- → 1
Assignee: nobody → sfoster
Status: NEW → ASSIGNED

Updating the bug title since we had decided to not restrict to restarts within 5 minutes of closing Firefox.

Summary: Add one-time infobar that indicates how to restore the previous session if the user restarts within 5 minutes of the last shutdown without automatic session restore → Add one-time infobar that indicates how to restore the previous session on the next session startup if automatic session restore is disabled

Note: Firefox 94 will have a spotlight modal on first session start-up. We want to avoid conflict with the infobar and also display the inforbar on the first session post not seeing the close tab modal. We need to show the infobar on the second session of running Firefox 94 for this reason.

I've got a WIP on here. It has some placeholder logic earlier during startup - before the SessionStartup updates the prefs we'll need to know if this is a restart etc. It has the new strings and creates the infobar at what I think is the right moment during startup, and the button opens the application menu.
So, the work remaining is really about determining when to show the infobar. And do I need to manually remove it if this browser navigates away from the start/home page?

Attachment #9241693 - Attachment description: WIP: Bug 1724960 - WIP Add a one-time infobar to explain session restore → Bug 1724960 - Add a one-time infobar to explain session restore. r?Gijs!
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/563ff2a2a021
Add a one-time infobar to explain session restore. r=Gijs,fluent-reviewers

Backed out for causing talos failures

Backout link

Push with failures

Failure log

Flags: needinfo?(sfoster)

Ok thanks for the backout. Of all the things I considered, unexpectedly loading the Tahoma font was not one of them!
I'll have to investigate on Monday, as at first blush I have no idea what is going on there or why my patch would trigger this.

I see tahoma used in this rule for window and maybe the infobar ends up getting its font-family from there? Seems unlikely that this infobar would use a font not already loaded in rendering the rest of our chrome, but its a place to start.

Flags: needinfo?(sfoster)
Attached file xperf log
(edit: bugzilla was weird about the attachment upload, see next comment)

(In reply to Sam Foster [:sfoster] (he/him) from comment #9)

Ok thanks for the backout. Of all the things I considered, unexpectedly loading the Tahoma font was not one of them!
I'll have to investigate on Monday, as at first blush I have no idea what is going on there or why my patch would trigger this.

I see tahoma used in this rule for window and maybe the infobar ends up getting its font-family from there? Seems unlikely that this infobar would use a font not already loaded in rendering the rest of our chrome, but its a place to start.

I checked but at least for me, I don't see us using Tahoma as a font for the infobar when inspecting with devtools, so I also don't understand why we'd hit this in xperf. :jfkthame, could you help investigate? I tried reproducing locally but xperf just fails miserably on my machine (see attached log)

That said, I think we should just set a pref in https://searchfox.org/mozilla-central/source/testing/profiles/perf/user.js to disable the infobar; we don't want it to show up in the test or any other perf tests. It does this because xperf first does a sanity run to initialize the profile, and then a "real" run, and so we end up showing it in the "real" run, so it shows up for the first page or two that get loaded in rapid succession (because of the timeout). That should work around the issue and allow us to land. It'll only show up on runs where we open the notification bar and anyway is likely pre-existing / applies to other notification bars, so not a "real" performance regression introduced by this patch in that sense. :sfoster, could you try to update the patch w/ that and trypush and/or reland?

Flags: needinfo?(sfoster)
Flags: needinfo?(jfkthame)

Thanks, Gijs. I added support for an explicit opt-out value of -1 (as setting to '2' to opt-out seemed a bit odd/arbitrary) and set that pref for the testing user. AFAICT this try push ran the tests that previously failed and they don't fail any more: https://treeherder.mozilla.org/jobs?repo=try&revision=1eea02094084a3a7dbd504ff979ef3b3fc49a89c

Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/613de0c29596
Add a one-time infobar to explain session restore. r=Gijs,fluent-reviewers
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch
Depends on: 1733111
Depends on: 1734246
Flags: needinfo?(jfkthame)
Depends on: 1736536

Hello! I can confirm that the issue is fixed with firefox 94.0b8 and 95.0a1(2021-10-20) on all OS's.

I will update the flags and mark this issue as VERIFIED->FIXED. If the issue is still valid please feel free to revert the resolution.

Thank you!

Status: RESOLVED → VERIFIED
Depends on: 1737754
Depends on: 1738264
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: