Last Comment Bug 654388 - "Restore Previous Session" button is always hidden if "Default Browser" dialog pops
: "Restore Previous Session" button is always hidden if "Default Browser" dialo...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Alastair Robertson
:
:
Mentors:
: 711387 (view as bug list)
Depends on:
Blocks: 555529
  Show dependency treegraph
 
Reported: 2011-05-03 00:45 PDT by Ginn Chen
Modified: 2013-10-01 02:22 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 - use setTimeout (2.91 KB, patch)
2011-09-30 10:09 PDT, Alastair Robertson
paul: feedback-
gavin.sharp: feedback+
Details | Diff | Splinter Review
Patch 1.1 - added comments (3.10 KB, patch)
2011-11-08 11:30 PST, Alastair Robertson
gavin.sharp: review+
Details | Diff | Splinter Review
Patch 1.2 - removed line from comments (3.03 KB, patch)
2011-12-19 14:48 PST, Alastair Robertson
no flags Details | Diff | Splinter Review

Description Ginn Chen 2011-05-03 00:45:22 PDT
Steps:

1) Make sure Firefox is not your default browser and you have default browser check for Firefox start-up. Also "about"home" is your home page.
2) Run Firefox, select "No" for default browser dialog, open several tabs, quit Firefox.
3) Run Firefox, select "No" for default browser dialog.
You can not see the "Restore Previous Session" button in about:home
4) Refresh about:home, now you see the button.
5) Restart Firefox, select "Yes" for default browser dialog.
You can not see the "Restore Previous Session" button in about:home
6) Refresh about:home, now you see the button.
7) Restart Firefox, now you don't have the default browser dialog because Firefox is the default browser. And you can see "Restore Previous Session" button.
Comment 1 Alastair Robertson 2011-09-30 10:09:53 PDT
Created attachment 563767 [details] [diff] [review]
Patch 1 - use setTimeout

The prompt asking whether to set Firefox as the default browser blocks the startup Javascript code until the user clicks one of the options. I've put the prompt in a setTimeout so it should only execute after the rest of the startup code is done.

Another solution would be to just move the code for the prompt to be after the code that initialises the session restore service, although this would mean it still blocks other pieces of startup code (but that might not matter).

--

The tests failed with and without my patch applied, so I'll try testing it again in a few days.
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-05 16:35:21 PDT
Comment on attachment 563767 [details] [diff] [review]
Patch 1 - use setTimeout

Ah I understand the problem here now (I didn't before, but this patch cleared it up). about:home still loads and we check sessionstore.canRestoreLastSession but since we haven't really init'd that's false even if it would become true.

I would actually much prefer just moving the code down since that's effectively what you're doing and setTimeout is misdirection that can lead to hard to track bugs.

I think moving the block down should be ok. We can still figure out if a session is being restored (I would add a comment about the about:home load needing the ss service init'd to work properly).

Gavin, you're more familiar with that whole delayedStartup process, does that sound reasonable?
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-05 17:24:51 PDT
Comment on attachment 563767 [details] [diff] [review]
Patch 1 - use setTimeout

I think this is probably fine, as a spot fix. Would need to audit for any dependencies on when this code runs (don't think there are any), but using a timeout is more reliable than just moving it later (it's annoying and error prone to have to remember to explicitly keep it as the "last step" in startup).

Longer-term, we should get rid of this prompt entirely. A modal dialog on startup isn't a very good place to prompt - I think there are bugs on that?
Comment 4 Alastair Robertson 2011-11-08 11:30:52 PST
Created attachment 572918 [details] [diff] [review]
Patch 1.1 - added comments

(In reply to Alastair Robertson from comment #1)
> The tests failed with and without my patch applied, so I'll try testing it
> again in a few days.

Took slightly longer than I intended (sorry), but the patch does pass the tests!
Comment 5 Alastair Robertson 2011-11-08 11:33:29 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> Would need to audit for any dependencies on when this code runs

Sorry, I didn't notice this before. What would this involve?
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-09 12:42:09 PST
Comment on attachment 572918 [details] [diff] [review]
Patch 1.1 - added comments

Sorry for the unreasonably long delay in response here, Alastair. This is a fine stop-gap for now, but if what the comment says is true ("ss service needs to be running before about:home is loaded"), then this isn't really a sufficient fix, since delayedStartup can still theoretically run after pages load. I think we should just remove that part of the comment. r=me with that.

I'd really like to investigate a few related fixes:

- bug 311605: move this code out of the browser.js window loading path and into nsBrowserGlue (runs once on startup)
- converting this modal dialog to some sort of non-modal notification
- having sessionstore functionality in general not depend on a window's delayedStartup having run (this probably involves re-working how sessionstore startup works, so it's probably not be trivial)

If you're interested in tackling the first one, I can guarantee quick review turnarounds, and eternal gratitude! :)
Comment 7 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2011-12-16 05:09:36 PST
*** Bug 711387 has been marked as a duplicate of this bug. ***
Comment 8 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2011-12-16 05:18:47 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> If you're interested in tackling the first one, I can guarantee quick review
> turnarounds, and eternal gratitude! :)

Gavin, is that a must-have to get this patch landed? Or can the proposed solutions be follow-up bugs?
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-19 10:19:23 PST
It's another bug entirely and I r+ed the patch here, so it's clearly not blocking this patch.
Comment 10 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2011-12-19 12:39:06 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> It's another bug entirely and I r+ed the patch here, so it's clearly not
> blocking this patch.

Which means it can be checked-in? It seems that Alastair can't do it on his own, so can we please set the keyword if it's so? Thanks.
Comment 11 Alastair Robertson 2011-12-19 14:48:21 PST
Created attachment 582969 [details] [diff] [review]
Patch 1.2 - removed line from comments
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-12-21 14:03:39 PST
What's the checkin comment and user line for that patch?
Comment 13 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-12-21 15:43:16 PST
(In reply to Boris Zbarsky (:bz) from comment #12)
> What's the checkin comment and user line for that patch?

Thanks for checking but I took care of it.

https://hg.mozilla.org/integration/fx-team/rev/a5427ddeef60
Comment 14 Tim Taubert [:ttaubert] 2011-12-29 09:17:21 PST
https://hg.mozilla.org/mozilla-central/rev/a5427ddeef60
Comment 15 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-01-02 11:50:08 PST
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a1) Gecko/20120102 Firefox/12.0a1

Note You need to log in before you can comment on or make changes to this bug.