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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

Attachment: patch

I think this is complete, will check before asking for review.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 200035 [details] [diff] [review]
patch

not going to change nsIShellService.idl

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch v2

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch v2 (that works!)

Comment 5

[Asaf Romano (gone)]

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: [e-mail]]]

(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: [e-mail]]]

Attachment: 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

[Asaf Romano (gone)]

+    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: [e-mail]]]

Attachment: patch v4

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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

[Asaf Romano (gone)]

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: [e-mail]]]

Attachment: patch v5

Same, unbitrotted and without method->attribute change.

Comment 13

[Asaf Romano (gone)]

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: [e-mail]]]

(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: [e-mail]]]

(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: [e-mail]]]

Attachment: patch v6

address comments (other than the ones mentioned above)

Comment 17

[Asaf Romano (gone)]

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: [e-mail]]]

Attachment: for checkin

Comment 19

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

I've backed this out because it hasn't been tested with the profile migrator.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

We can probably just use sessionstore-windows-restored for this, to address the issues in comment 20/comment 21.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

This would be feasible now that we have _onBrowserStartup and this.getMostRecentBrowserWindow(), I think.

Comment 24

[Alastair Robertson]

Attachment: 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: [e-mail]]]

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]

Attachment: 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]

Attachment: Patch 2.1 (alastair) - removed .bind(this)

Comment 28

[:Gavin Sharp [email: [e-mail]]]

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]

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: [e-mail]]]

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]

Attachment: Patch 2.2 (alastair) - updated patch

Updated to account for other changes in browser.js since Patch 2.1 was created.

Comment 32

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/163f88cb1413

Comment 33

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/163f88cb1413