390060 - with session restore enabled, default browser startup check doesn't

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Tuukka Tolvanen (sp3000)]

1. options -> main, set
     * when firefox starts, show my windows and tabs from last time
     * always check to see if firefox is default browser on startup
 2. exit and launch a fx build that is not default (the same one will do if not default)

expected: set as default browser query on startup
result: no default browser query on startup

I'll guess this would be due to bug 342020. After a crash might be a bad time to be forward with suggestions toward the user, but the session restore on normal exit pref entirely undoing the effect of the startup default check pref is a bit unexpected.

Comment 1

[Simon Bünzli]

Well, I'd rather have that prompt be replaced with a better UX (see e.g. http://wiki.mozilla.org/images/4/4e/Retention_Plan_2.010.png ) than to try to figure out whether startup is following a clean shutdown or rather a crash or a restart -- where you wouldn't want the prompt, either, and which we currently don't really distinguish from a clean shutdown.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is a really annoying issue for Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9pre) Gecko/20071029 BonEcho/2.0.0.9pre

An external application which is installed on my pc at work uses IE exclusively. During startup I got ask if IE should be the default browser. I was too quick and answered with yes. Now IE is set as my default browser and due to my homepage setting to display my last opened tabs, Firefox doesn't check if it's the default browser anymore. You have to do it manually.

This way of browsing is widely accepted by our users. It's great to have the last opened tabs open after a restart. Everyone could step into this issue when having other applications installed which uses a hardcoded browser.

Comment 3

[Simon Bünzli]

No regression. Still hoping for a generally improved UX WRT that prompt...

Comment 6

[Manish Singh]

Attachment: Ask default browser question on session resume

This patch is based on how I solved it for Flock.

Since there doesn't seem to be any movement on a better UX, and now that Firefox prompts you to save your session when you shutdown, this seems reasonable to go in. The patch is pretty straightforward.

Comment 7

[Simon Bünzli]

Comment on attachment 298874 [details] [diff] [review]
Ask default browser question on session resume

>+  long getSessionType();

Why not use a read-only attribute (named sessionType or similar)? FWIW: doRestore should've been one as well. And nit: Most IDLs I've seen use unsigned long for constants.

>+    if (this._lastSessionCrashed) {
>+      if (this._doRecoverSession()) {
>+        this._sessionType = Ci.nsISessionStartup.RECOVER_SESSION;
>+      }

Nit: Drop the inner parens (though I'm not fond of it, that's our coding style).

>+    } else {
>+      if (this._doResumeSession()) {

I'd prefer seeing an |else if {| instead of another pair of parens. And nit: else should go on a new line.

>+    return Boolean(this.getSessionType());

That doesn't read very well. What's wrong with
>+    return this._sessionType != Ci.nsISessionStartup.NO_SESSION;

>+    return this._iniString ? this._sessionType
>+                           : Ci.nsISessionStartup.NO_SESSION;

That's not too obvious, either. I know that you've just copied to logic over from doRestore, but while you're at it, please move this condition to sss_init so that we can just return _sessionType here.

Finally: This'll need ui-r from beltzner as well.

Comment 8

[Manish Singh]

Attachment: Addressed review comments

Comment 9

[Simon Bünzli]

Comment on attachment 299084 [details] [diff] [review]
Addressed review comments

Hm, I think the following would be even clearer to read:

>+    if (this._iniString) {
>+      if (this._lastSessionCrashed && this._doRecoverSession())
>+        this._sessionType = Ci.nsISessionStartup.RECOVER_SESSION;
>+      else if (!this._lastSessionCrashed && this._doResumeSession())
>+        this._sessionType = Ci.nsISessionStartup.RESUME_SESSION;
>+      else
>+        this._iniString = null; // reset the state string
>     }

With that change r+. Thanks, Manish.

Comment 10

[Manish Singh]

Attachment: Addressed final review comment

Carrying forward r+, asking for ui-r

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 299239 [details] [diff] [review]
Addressed final review comment

ui-r+a=beltzner, yay retention

Comment 12

[Matthew (lilmatt) Willis]

Attachment 299239 [details] [diff] landed on trunk

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That's working fine now. Verified with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre ID:2008021504

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre ID:2008021504

One bad thing happens for me on OS X. The dialog is placed half-outside of the screen. I'll file a new bug.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #13)
> One bad thing happens for me on OS X. The dialog is placed half-outside of the
> screen. I'll file a new bug.

That would be an invalid one. This behavior can be seen when you have Firebug 1.1.0b10 installed and enabled. John, could you track this please?

Comment 16

[John J. Barton]

(In reply to comment #15)
> That would be an invalid one. This behavior can be seen when you have Firebug
> 1.1.0b10 installed and enabled. John, could you track this please?

I'm not sure what your asking. Are you saying:
  For FF3b4/OSX + firebug the restore dialog comes up off screen and the cause seems to be firebug?
Seems pretty unlikely unless firebug's window tracking somehow interferes with FF window placement. 

Comment 17

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5304 added to Litmus.