The default bug view has changed. See this FAQ.

default browser checking should be done from nsBrowserGlue, not in delayedStartup

RESOLVED FIXED in Firefox 13

Status

()

Firefox
General
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: Gavin, Assigned: Alastair Robertson)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

5.65 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
There's no reason to check the default browser status each time a window is
opened, it should only be done once at startup.
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox1.6-
Created attachment 200035 [details] [diff] [review]
patch

I think this is complete, will check before asking for review.
Comment on attachment 200035 [details] [diff] [review]
patch

not going to change nsIShellService.idl
Attachment #200035 - Attachment is obsolete: true
Priority: -- → P1
Created attachment 201221 [details] [diff] [review]
patch v2
Attachment #201221 - Flags: review?(bugs.mano)
Created attachment 201224 [details] [diff] [review]
patch v2 (that works!)
Attachment #201221 - Attachment is obsolete: true
Attachment #201224 - Flags: review?(bugs.mano)
Attachment #201221 - Flags: review?(bugs.mano)
Whiteboard: [patch-r?]
Comment on attachment 201224 [details] [diff] [review]
patch v2 (that works!)

I don't see the value of keeping shouldCheckDefaultBrowser as an attribute of nsIShellService, you can check the pref in nsIBrowserGlue.

Also, checkDefaultBrowser should check for the default browser regardless of the pref state, check the pref in the startup method instead.

>Index: browser/components/shell/public/nsIShellService.idl
>===================================================================

>@@ -40,41 +40,36 @@
> interface nsIDOMElement;
> 
> [scriptable, uuid(d6f62053-3769-46f6-bd2b-0a1440d6c394)]

bump the UUID.
Attachment #201224 - Flags: review?(bugs.mano) → review-
(In reply to comment #5)
> Also, checkDefaultBrowser should check for the default browser regardless of
> the pref state, check the pref in the startup method instead.

I initially attempted to share the code for startup and the options dialog, but since the dialogs behaved differently (checkbox vs. no checkbox, the latter always checks regardless of pref) I just kept them seperate. If I make the pref check seperate then maybe it should be public with a param to indicate whether to show the "Always remember" checkbox? That way it could be used by the options dialog too.
Whiteboard: [patch-r?] → [patch-r-]
Created attachment 201716 [details] [diff] [review]
patch v3

Do you prefer this approach? I'm too tired to be attaching patches, so I'll wait before asking for review, but if you want to take a look go ahead! :)
Attachment #201224 - Attachment is obsolete: true
+    if (!aForceCheck && !this.prefService.getBoolPref(kCheckEveryTimePref))
+      return;

Calling this method should _alwasys_ check the default browser, any other check should be done in the caller.
Created attachment 201961 [details] [diff] [review]
patch v4
Attachment #201716 - Attachment is obsolete: true
Attachment #201961 - Flags: review?(bugs.mano)
The latest patch fixes your comment about the pref checking, and changes isDefaultBrowser from a method to an attribute. The interface is already changing enough, I don't see any reason to try and avoid making that change.
Whiteboard: [patch-r-] → [patch-r?]
Comment on attachment 201961 [details] [diff] [review]
patch v4

I'm still against a property for reading, and a method for setting (or, a property which does a lot when setting it); if you disagree, ask mike to review.
Attachment #201961 - Flags: review?(bugs.mano) → review-
Created attachment 203333 [details] [diff] [review]
patch v5

Same, unbitrotted and without method->attribute change.
Attachment #201961 - Attachment is obsolete: true
Attachment #203333 - Flags: review?(bugs.mano)
Comment on attachment 203333 [details] [diff] [review]
patch v5

>Index: browser/components/Makefile.in
>===================================================================

>+ifneq (,$(filter windows gtk2 mac cocoa, $(MOZ_WIDGET_TOOLKIT)))
>+DEFINES += -DHAVE_SHELL_SERVICE=1
>+endif

I'm wondering if we should continue to #ifdef it, all tier1 platforms are supported and you're doing runtime checks anyway.

>Index: browser/components/nsBrowserGlue.js
>===================================================================

>+    var sURL = "chrome://branding/locale/brand.properties";
>+    var brandBundle = bundleSvc.createBundle(sURL);
>+    sURL = "chrome://browser/locale/shellservice.properties";
>+    var shellBundle = bundleSvc.createBundle(sURL);

Make them consts.

>Index: browser/components/nsIBrowserGlue.idl
>===================================================================
>+
>+  /** 
>+   * Checks whether the browser is set as the system default, and if not,
>+   * prompts the user to make it the default.
>+   *
>+   * @param aForceCheck a boolean value indicating whether an alert should be 
>+   *        displayed if the browser is already registered as the system
>+   *        default. Also controls whether or not the "Should check at startup"
>+   *        checkbox appears in the confirmation prompt.

aSilentMode (or, aInteractiveMode if you don't wish to reverse it) would be cleaner.

>+   * @param aParentWindow an optionally null window which is the parent of the 
>+   *        prompts and alerts

Make that "@param aParentWindow Optional:".

>Index: browser/components/shell/src/nsGNOMEShellService.h
>===================================================================
> 
> private:
>   ~nsGNOMEShellService() {}
> 
>   NS_HIDDEN_(PRBool) KeyMatchesAppName(const char *aKeyValue) const;
> 
>-  PRPackedBool mCheckedThisSession;
>-  PRPackedBool mUseLocaleFilenames;
>+  PRBool mUseLocaleFilenames;

Why are you changing mUseLocaleFilenames to PRBool (I don't see you playing with its bitfields ;) ).

>Index: browser/components/shell/src/nsWindowsShellService.h
>===================================================================
> private:
>-  PRBool    mCheckedThisSession;
>+

Remove the private block.
Attachment #203333 - Flags: review?(bugs.mano) → review-
(In reply to comment #13)
> I'm wondering if we should continue to #ifdef it, all tier1 platforms are
> supported and you're doing runtime checks anyway.

It's still ifdeffed for components/prefs and base, I won't change that, but I'll remove the one I'm adding.

> >Index: browser/components/nsBrowserGlue.js
> >===================================================================
> 
> >+    var sURL = "chrome://branding/locale/brand.properties";
> >+    var brandBundle = bundleSvc.createBundle(sURL);
> >+    sURL = "chrome://browser/locale/shellservice.properties";
> >+    var shellBundle = bundleSvc.createBundle(sURL);
> 
> Make them consts.

Ok.

> >Index: browser/components/nsIBrowserGlue.idl
> >+   * @param aForceCheck a boolean value indicating whether an alert should be 
> >+   *        displayed if the browser is already registered as the system
> >+   *        default. Also controls whether or not the "Should check at startup"
> >+   *        checkbox appears in the confirmation prompt.
> 
> aSilentMode (or, aInteractiveMode if you don't wish to reverse it) would be
> cleaner.

It's not really "interactive" or "silent", though... in both cases a dialog appears if it's not default.

> >+   * @param aParentWindow an optionally null window which is the parent of the 
> >+   *        prompts and alerts
> 
> Make that "@param aParentWindow Optional:".

Ok.

> >Index: browser/components/shell/src/nsWindowsShellService.h
> >===================================================================
> > private:
> >-  PRBool    mCheckedThisSession;
> >+
> 
> Remove the private block.

Ok.
(In reply to comment #13)
> Make that "@param aParentWindow Optional:".

Actually, I did it that way to match the one for "Sanitize" just above it.
Created attachment 204638 [details] [diff] [review]
patch v6

address comments (other than the ones mentioned above)
Attachment #203333 - Attachment is obsolete: true
Attachment #204638 - Flags: review?(bugs.mano)
Comment on attachment 204638 [details] [diff] [review]
patch v6

As discussed on IRC:

* s/aForceCheck/aUserInitiated  
* fix both comments (regarding optional parameters).

r=mano.
Attachment #204638 - Flags: review?(bugs.mano) → review+
Created attachment 204940 [details] [diff] [review]
for checkin
Attachment #204638 - Attachment is obsolete: true
Whiteboard: [patch-r?] → [checkin needed]
mozilla/browser/base/content/browser.js 1.535
mozilla/browser/components/nsBrowserGlue.js 1.9
mozilla/browser/components/nsIBrowserGlue.idl 1.2
mozilla/browser/components/preferences/general.js 1.5
mozilla/browser/components/shell/public/nsIShellService.idl 1.6
mozilla/browser/components/shell/src/nsGNOMEShellService.cpp 1.11
mozilla/browser/components/shell/src/nsGNOMEShellService.h 1.4
mozilla/browser/components/shell/src/nsMacShellService.cpp 1.8
mozilla/browser/components/shell/src/nsMacShellService.h 1.4
mozilla/browser/components/shell/src/nsShellService.h 1.4
mozilla/browser/components/shell/src/nsWindowsShellService.cpp 1.22
mozilla/browser/components/shell/src/nsWindowsShellService.h 1.5
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed] → [checkin needed
Whiteboard: [checkin needed
I've backed this out because it hasn't been tested with the profile migrator.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Also because it makes the dialog appear before the initial browser window, something I'd considered but didn't think was an issue.
Status: REOPENED → NEW
Whiteboard: [wontfix?]
Priority: P1 → P5
Target Milestone: Firefox 2 → Firefox 3
We can probably just use sessionstore-windows-restored for this, to address the issues in comment 20/comment 21.
This would be feasible now that we have _onBrowserStartup and this.getMostRecentBrowserWindow(), I think.
Assignee: gavin.sharp → nobody
Priority: P5 → --
Whiteboard: [wontfix?]
Target Milestone: Firefox 3 → ---
(Assignee)

Comment 24

5 years ago
Created attachment 587063 [details] [diff] [review]
Patch 1 (alastair)

(In reply to Mano from comment #13)
> Comment on attachment 203333 [details] [diff] [review]
> patch v5
> 
> >Index: browser/components/Makefile.in
> >===================================================================
> 
> >+ifneq (,$(filter windows gtk2 mac cocoa, $(MOZ_WIDGET_TOOLKIT)))
> >+DEFINES += -DHAVE_SHELL_SERVICE=1
> >+endif
> 
> I'm wondering if we should continue to #ifdef it, all tier1 platforms are
> supported and you're doing runtime checks anyway.

I included the #ifdef in my patch as it was still being used in browser.js and I wasn't sure if this 6 year old comment was still relevant. Do we still need this?

I removed the setTimeout() from the default browser check, as it doesn't work in nsBrowserGlue.js. The default browser prompt will block javascript execution until an option is selected (like was happening in bug 654388). Is it possible that this could cause problems here?
Attachment #587063 - Flags: feedback?(gavin.sharp)
Comment on attachment 587063 [details] [diff] [review]
Patch 1 (alastair)

This looks good. You can get rid of the ifdef, and you can have this continue to by async by using Services.tm.mainThread.dispatch, as in e.g. TelemetryPing.js.

Thanks for looking into this!
Attachment #587063 - Flags: feedback?(gavin.sharp) → feedback+
(Assignee)

Comment 26

5 years ago
Created attachment 588572 [details] [diff] [review]
Patch 2 (alastair) - remove #ifdef, use Services.tm.mainThread.dispatch()

Addressed the points made in comment 25.

I'm happy to help :)
Attachment #587063 - Attachment is obsolete: true
Attachment #588572 - Flags: review?(gavin.sharp)
(Assignee)

Comment 27

5 years ago
Created attachment 588740 [details] [diff] [review]
Patch 2.1 (alastair) - removed .bind(this)
Attachment #588572 - Attachment is obsolete: true
Attachment #588740 - Flags: review?(gavin.sharp)
Attachment #588572 - Flags: review?(gavin.sharp)
Comment on attachment 588740 [details] [diff] [review]
Patch 2.1 (alastair) - removed .bind(this)

Sorry, I missed something when I looked at this earlier: moving this code to _onBrowserStartup will break the willRecoverSession check (it will always be false, I think, since sessionstore will have already kicked in and restored stuff). To solve that, we'll probably want to change the two NOTIFY_WINDOWS_RESTORED notifyObservers calls in nsSessionStore to pass different final arguments (rather than "") so that they're differentiable, propagate that through to _onBrowserStartup (to be renamed in bug 719254!), and then only show this prompt if a session hasn't been restored.
Attachment #588740 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 29

5 years ago
I might not be understanding you correctly, but it appears to me that willRecoverSession is still being set ok:

With "When Firefox starts: Show my windows and tabs from last time" set:
[alastair@arch bin]$ ./firefox
ss.sessionType: 2 (RESUME_SESSION)
Ci.nsISessionStartup.RECOVER_SESSION: 1
willRecoverSession: false

With "When Firefox starts: Show my home page" set:
[alastair@arch bin]$ ./firefox
ss.sessionType: 3 (DEFER_SESSION)
Ci.nsISessionStartup.RECOVER_SESSION: 1
willRecoverSession: false
^C                                   (Force Firefox to "crash")
[alastair@arch bin]$ ./firefox
ss.sessionType: 1 (RECOVER_SESSION)
Ci.nsISessionStartup.RECOVER_SESSION: 1
willRecoverSession: true
[alastair@arch bin]$

I get the same results with and without the patch applied. The prompt is only hidden if Firefox crashed and needs to recover the previous session.
Comment on attachment 588740 [details] [diff] [review]
Patch 2.1 (alastair) - removed .bind(this)

Yep, you're right - sessionType never gets reset (unless history is cleared), so this shouldn't be a problem. Sorry about that, and thanks for following up!
Attachment #588740 - Flags: review- → review+
(Assignee)

Comment 31

5 years ago
Created attachment 602534 [details] [diff] [review]
Patch 2.2 (alastair) - updated patch

Updated to account for other changes in browser.js since Patch 2.1 was created.
Attachment #588740 - Attachment is obsolete: true
Attachment #602534 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/163f88cb1413
Assignee: nobody → alastair
Keywords: checkin-needed
Target Milestone: --- → Firefox 13

Updated

5 years ago
Attachment #204940 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/163f88cb1413
Status: NEW → RESOLVED
Last Resolved: 12 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 737830
You need to log in before you can comment on or make changes to this bug.