Last Comment Bug 311605 - default browser checking should be done from nsBrowserGlue, not in delayedStartup
: default browser checking should be done from nsBrowserGlue, not in delayedSta...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Alastair Robertson
:
Mentors:
Depends on: 737830
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-07 18:49 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-03-21 06:36 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (24.97 KB, patch)
2005-10-18 19:51 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch v2 (24.98 KB, patch)
2005-10-28 18:39 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch v2 (that works!) (26.13 KB, patch)
2005-10-28 19:30 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
asaf: review-
Details | Diff | Splinter Review
patch v3 (32.64 KB, patch)
2005-11-02 20:10 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch v4 (33.72 KB, patch)
2005-11-05 14:52 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
asaf: review-
Details | Diff | Splinter Review
patch v5 (33.15 KB, patch)
2005-11-16 19:12 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
asaf: review-
Details | Diff | Splinter Review
patch v6 (32.69 KB, patch)
2005-11-30 19:46 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
asaf: review+
Details | Diff | Splinter Review
for checkin (32.70 KB, patch)
2005-12-04 03:15 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
Patch 1 (alastair) (5.89 KB, patch)
2012-01-09 11:49 PST, Alastair Robertson
gavin.sharp: feedback+
Details | Diff | Splinter Review
Patch 2 (alastair) - remove #ifdef, use Services.tm.mainThread.dispatch() (5.58 KB, patch)
2012-01-13 17:47 PST, Alastair Robertson
no flags Details | Diff | Splinter Review
Patch 2.1 (alastair) - removed .bind(this) (5.57 KB, patch)
2012-01-15 07:45 PST, Alastair Robertson
gavin.sharp: review+
Details | Diff | Splinter Review
Patch 2.2 (alastair) - updated patch (5.65 KB, patch)
2012-03-02 16:33 PST, Alastair Robertson
gavin.sharp: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2005-10-07 18:49:34 PDT
There's no reason to check the default browser status each time a window is
opened, it should only be done once at startup.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-10-18 19:51:49 PDT
Created attachment 200035 [details] [diff] [review]
patch

I think this is complete, will check before asking for review.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-10-26 09:25:39 PDT
Comment on attachment 200035 [details] [diff] [review]
patch

not going to change nsIShellService.idl
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-10-28 18:39:12 PDT
Created attachment 201221 [details] [diff] [review]
patch v2
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-10-28 19:30:03 PDT
Created attachment 201224 [details] [diff] [review]
patch v2 (that works!)
Comment 5 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-11-02 06:37:26 PST
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.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-02 07:17:12 PST
(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.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-02 20:10:43 PST
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! :)
Comment 8 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-11-02 22:17:31 PST
+    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.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-05 14:52:22 PST
Created attachment 201961 [details] [diff] [review]
patch v4
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-05 15:03:49 PST
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.
Comment 11 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-11-05 19:03:34 PST
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.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-16 19:12:29 PST
Created attachment 203333 [details] [diff] [review]
patch v5

Same, unbitrotted and without method->attribute change.
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-11-30 00:16:27 PST
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.
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-30 17:34:17 PST
(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.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-30 19:45:19 PST
(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.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-30 19:46:26 PST
Created attachment 204638 [details] [diff] [review]
patch v6

address comments (other than the ones mentioned above)
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-12-04 03:13:30 PST
Comment on attachment 204638 [details] [diff] [review]
patch v6

As discussed on IRC:

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

r=mano.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-04 03:15:32 PST
Created attachment 204940 [details] [diff] [review]
for checkin
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-04 10:37:04 PST
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
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-04 13:06:06 PST
I've backed this out because it hasn't been tested with the profile migrator.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-04 13:09:13 PST
Also because it makes the dialog appear before the initial browser window, something I'd considered but didn't think was an issue.
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-07 13:48:17 PST
We can probably just use sessionstore-windows-restored for this, to address the issues in comment 20/comment 21.
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-09 10:50:49 PST
This would be feasible now that we have _onBrowserStartup and this.getMostRecentBrowserWindow(), I think.
Comment 24 Alastair Robertson 2012-01-09 11:49:00 PST
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?
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-12 10:44:13 PST
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!
Comment 26 Alastair Robertson 2012-01-13 17:47:57 PST
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 :)
Comment 27 Alastair Robertson 2012-01-15 07:45:39 PST
Created attachment 588740 [details] [diff] [review]
Patch 2.1 (alastair) - removed .bind(this)
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-18 15:27:01 PST
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.
Comment 29 Alastair Robertson 2012-02-25 17:52:41 PST
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 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-29 17:21:47 PST
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!
Comment 31 Alastair Robertson 2012-03-02 16:33:41 PST
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.
Comment 33 Marco Bonardo [::mak] 2012-03-07 02:49:41 PST
https://hg.mozilla.org/mozilla-central/rev/163f88cb1413

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