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

VERIFIED FIXED in Firefox 3 beta4

Status

()

VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: tuukka.tolvanen, Assigned: mozbugs)

Tracking

Trunk
Firefox 3 beta4
Points:
---
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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

11 years ago
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.
Component: General → Session Restore
OS: Linux → All
QA Contact: general → session.restore
Hardware: PC → All
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.
No longer blocks: 342020
Severity: normal → major
Depends on: 342020
Keywords: regression

Comment 3

11 years ago
No regression. Still hoping for a generally improved UX WRT that prompt...
Severity: major → normal
Keywords: regression
Duplicate of this bug: 405046
Duplicate of this bug: 411987
(Assignee)

Comment 6

11 years ago
Created attachment 298874 [details] [diff] [review]
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.
Attachment #298874 - Flags: review?(zeniko)

Comment 7

11 years ago
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.
Attachment #298874 - Flags: review?(zeniko) → review-
(Assignee)

Comment 8

11 years ago
Created attachment 299084 [details] [diff] [review]
Addressed review comments
Assignee: nobody → manish
Attachment #298874 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #299084 - Flags: review?(zeniko)

Comment 9

11 years ago
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.
Attachment #299084 - Flags: review?(zeniko) → review+
(Assignee)

Comment 10

11 years ago
Created attachment 299239 [details] [diff] [review]
Addressed final review comment

Carrying forward r+, asking for ui-r
Attachment #299084 - Attachment is obsolete: true
Attachment #299239 - Flags: ui-review?(beltzner)
Attachment #299239 - Flags: review+
Comment on attachment 299239 [details] [diff] [review]
Addressed final review comment

ui-r+a=beltzner, yay retention
Attachment #299239 - Flags: ui-review?(beltzner)
Attachment #299239 - Flags: ui-review+
Attachment #299239 - Flags: approval1.9+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 405359
(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?
Flags: in-litmus?

Comment 16

11 years ago
(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. 

https://litmus.mozilla.org/show_test.cgi?id=5304 added to Litmus.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.