803585 - Default client checking should be done from nsSuiteGlue, not in navigator->Startup()

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Philip Chee]

From Bug 311605 Comment 0:
> 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

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

This patch:
1. Moves the check from navigator.js to nsSuiteGlue.js
2. Removes the try/catch block since we now have the shell service for all three supported platforms.

Questions:
1. Currently the check is called from "sessionstore-windows-restored". Should this be called from "final-ui-startup" instead?

2. In nsWindowsShellService.cpp->IsDefaultClient()/IsDefaultClientVista() why don't we check for RSS feeds?

Comment 2

[[e-mail]]

(In reply to Philip Chee from comment #1)
> 2. Removes the try/catch block since we now have the shell service for all
> three supported platforms.
That's no excuse to explicitly desupport other platforms :-P

> Questions:
> 1. Currently the check is called from "sessionstore-windows-restored".
> Should this be called from "final-ui-startup" instead?
This happens very early on, before any main windows are opened.
Do we want to do it before or after the master password check?

> 2. In nsWindowsShellService.cpp->IsDefaultClient()/IsDefaultClientVista()
> why don't we check for RSS feeds?
Because historically we didn't know how to set as the default RSS reader?
(You'd have to ask mcsmurf whether our helper can do this.)

Comment 3

[Philip Chee]

Attachment: Patch v1.1 Put back the try/catch block.

>> 2. Removes the try/catch block since we now have the shell service for all
>> three supported platforms.
> That's no excuse to explicitly desupport other platforms :-P
OK restored the try/catch block.

>> Questions:
>> 1. Currently the check is called from "sessionstore-windows-restored".
>> Should this be called from "final-ui-startup" instead?
> This happens very early on, before any main windows are opened.
> Do we want to do it before or after the master password check?
I've decided that if it ain't broke don't fix it.

Comment 4

[[e-mail]]

Comment on attachment 673573 [details] [diff] [review]
Patch v1.1 Put back the try/catch block.

>-        this._onBrowserStartup(subject);
>+        this._onWindowsRestored(subject);
I don't actually agree with bug 719254; this notification only applies when the browser is opened, not when any other window is opened. So it really is browser startup.

>+    Services.tm.mainThread
>+               .dispatch(this._checkForDefaultClient.bind(this, aWindow),
>+                         Components.interfaces.nsIThread.DISPATCH_NORMAL);
I see Firefox does this as well, because the old code ran on a timeout. But as it happens, some of the sessionstore code also runs on a timeout. I need to check to see whether we need a second timeout.

>-// This function gets the shell service and has it check its setting
>-...
>-function checkForDefaultBrowser()
...
>+  // This method gets the shell service and has it check its settings.
>+...
>+  _checkForDefaultClient: function checkForDefaultClient(aWindow)
;-)

>-    var shellService = Components.classes["@mozilla.org/suite/shell-service;1"]
>+      let shellService = Components.classes[NS_SHELLSERVICE_CID]
let :-( NS_SHELL_SERVICE_CID :-)

>+      // show the default client dialog only if we should check for the default
>+      // client and we aren't already the default for the stored app types in
>+      // shell.checkDefaultApps.
Sentence needs capital letter.

Comment 5

[[e-mail]]

Oh, and I guess we need a followup bug to remove the mCheckedThisSessionClient variable from all of the shell services.

Comment 6

[[e-mail]]

Comment on attachment 673573 [details] [diff] [review]
Patch v1.1 Put back the try/catch block.

>-  const NS_SHELLSERVICE_CID = "@mozilla.org/suite/shell-service;1";
>-
>-  if (NS_SHELLSERVICE_CID in Components.classes) try {
>-    const nsIShellService = Components.interfaces.nsIShellService;
>-    var shellService = Components.classes["@mozilla.org/suite/shell-service;1"]
Actually it was the removal of the duplication that I was getting at.

>     // Load the "more info" page for a locked places.sqlite
>     // This property is set earlier by places-database-locked topic.
>     if (this._isPlacesDatabaseLocked)
>       notifyBox.showPlacesLockedWarning();
> 
>     // Detect if updates are off and warn for outdated builds.
>     if (this._shouldShowUpdateWarning())
>       notifyBox.showUpdateWarning();
>+
>+    Services.tm.mainThread
>+               .dispatch(this._checkForDefaultClient.bind(this, aWindow),
Sigh, how did the jump list code land in between the rights notification and the update warning? Anyway, the _checkForDefaultClient function belongs after the _shouldShowUpdateWarning function for consistency.

I've had a look at the timing of the dialog.

Without the patch, the dialog actually appears before Session Restore has had a chance to kick in, so you still only have a blank tab at that point.
With the patch, Session Restore manages to restore all the tabs in all background windows. However, in the active window, only the current tab and one other tab get to restore, the others have to wait until you close the dialog.
With the patch, but without the async dispatch, only the current tab gets restored until you close the dialog.
So to avoid the oddity of having one other tab restore, I'd prefer it if you removed the async dispatch.

Comment 7

[Philip Chee]

Attachment: Patch v1.2 Fix review issues.

Changes in this patch:
1. _onWindowsRestored() changed back to _onBrowserStartup().
2. Made the call to _checkForDefaultClient() sync instead of async.
3. Start comment sentence with a capital letter.
4. Moved _checkForDefaultClient() to after _shouldShowUpdateWarning().

Comment 8

[[e-mail]]

Comment on attachment 673884 [details] [diff] [review]
Patch v1.2 Fix review issues.

>+      let shellService = Components.classes[NS_SHELLSERVICE_CID]
>+                                   .getService(nsIShellService);
>+      let appTypes = shellService.shouldBeDefaultClientFor;
[sadface is still there]

>+        // Force the sidebar to build since the windows integration dialog has
>+        // come up.
>+        aWindow.SidebarRebuild();
Bah, I knew there was something I forgot to check - this might not be necessary now that the dialog happens at a different time.

Comment 9

[[e-mail]]

Comment on attachment 673884 [details] [diff] [review]
Patch v1.2 Fix review issues.

>+        aWindow.SidebarRebuild();
Well, I couldn't even make it necessary without the patch, let alone with the patch, so we might as well remove it. I'll let (pun intended) you decide whether or not to remove the {}s.

Comment 10

[Philip Chee]

Attachment: Patch v1.3 as checked in with nits fixed. r=Neil.

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/b9dbdc680a9f

Changes in this patch:
1. s/let/var
2. Remove call to aWindow.SidebarRebuild() as it is not needed.
2a. Left the {} around the if block.