Setting the default application on Vista doesn't seam to work.

not finish. still includes MessageBox's for debuggin.

Do we really need this for 2.0, or can we fit it into a security and stability release? If we can, I would vote to make it non-blocking, though we'd take a patch if it makes it before freeze or if we have to respin.

I think i would agree with your vote -- this patch isn't ready just yet. the patch needs to be tested against rob strong's installer changes.

*** Bug 339306 has been marked as a duplicate of this bug. ***

A couple of notes: It appears that our app still has a ddeexec hack where it removes the ddeexec key on shutdown and adds it back on startup... this is bad / wrong on so many levels. If the user is not admin it can't do this, if the app crashes it can't remove it, etc. This really should be removed if for no other reason than consistent behavior for admin and non-admin users. I suppose it could be doing this under HKCU for non-admin users but iirc it doesn't. Currently the app adds the protocol handlers which doesn't make sense in Vista so I've done this in the installer. I've tested this patch along with the patch in bug 336469 and it worked well. I think we are going to have to come up with a better string to use to register than brandFullName but I may be incorrect in this... it depends on how Vista handles the StartMenuInternet shortcut. Also, the app should not be touching the HKEY_CLASSES_ROOT keys on Vista since Vista handles this for the app.

cleaned up version of patch v.1. I did not include the removal of the DDE stuff. I think we can do this later since we want to try to make the 2.0 train.

I recant the statement about not setting keys under classes since we need to do so for side by side installs though they are different keys. Turns out that the protocol and file handlers we use will need to be checked to verify that it refers to this install location and if not set to this install location either in HKEY_CURRENT_USER or HKEY_LOCAL_MACHINE. Otherwise, when set as default is selected we will just fail and prompt again on next launch.

The app is going to need to set new reg keys for protocol handlers. IE has one for each file and protocol handler so we may want to bite the bullet and add new reg keys for individual file handlers instead of just reusing FirefoxHTML. Suggested names that I already have in the installer FirefoxGopher FirefoxFTP FirefoxHTTP FirefoxHTTPS This is inline with our current FirefxHTML but we could choose to abandon it and use something mroe similar to what IE uses. The value in using new names for each file handler is that we can specify individual names to be displayed in Set User Defaults (e.g. SUD)... for example IE uses HTML Document and without having individual keys we would display HTML File, HTM File, XHTML File, etc. for the file handlers. A couple of the names IE is using IE.AssocFile.HTM IE.HTTP and so on...

this should be done by the installer, right?

No, per comment #7 If one version of the app is installed and then another version of the app is installed the second install will take over those keys. Then if the second app is uninstalled there is no way to set the original install as default without re-installing. Also, if the first install is intended to be the default the second install is going to make itself the default. There are other scenarios as well but the gist is the app is going to have to set these keys as well.

Comment on attachment 238625 [details] [diff] [review] patch v.2 this doesn't work for side by side.

Doug, the following is the action plan after talking with Schrep... no support for side by side on Vista for now. The installer will alway force to install into the location of an existing install on Vista. This should lessen the complexity quite a bit and mitigate risk. We will still need to be able to write to HKCU\Software\Classes when we can't write to HKLM\Software\Classes. I'll send you more details later when the details become more clear

The main thing that needs to be changed in this patch is the use of Firefox instead of brandFullName for the Vista API calls. Also, the protocol DefaultIcon reg values should specify 0 instead of 1 for the resource since we don't want to show the file icon for protocols

the protocol DefaultIcon reg values should just be for vista right? The other thing is that if we don't use brandFullName, then using a bon echo build will not work (this side by side case will not work, right) Doug

(In reply to comment #14) > the protocol DefaultIcon reg values should just be for vista right? No, they should be for all but hold off on it... they just aren't shown except in Vista > The other thing is that if we don't use brandFullName, then using a bon echo > build will not work (this side by side case will not work, right) Per our discussion last Friday it should use DDE_NAME though you can use anything that has the string Firefox... we can't use brandFullName because when using a bon echo or minefield build it will not work since brandFullName will be Bon Echo or Minefield... so, this has nothing to do with side by side installs.

How about the following? (In reply to comment #5) ... > Currently the app adds the protocol handlers which doesn't make sense in Vista > so I've done this in the installer. > > I've tested this patch along with the patch in bug 336469 and it worked well. I > think we are going to have to come up with a better string to use to register > than brandFullName but I may be incorrect in this... it depends on how Vista > handles the StartMenuInternet shortcut. Also, the app should not be touching > the HKEY_CLASSES_ROOT keys on Vista since Vista handles this for the app.

Ok - we are cutting 1.8.1 in minutes so this will have to defer for a 2.0.0.1 branch.

Uses DDE_NAME.

Comment on attachment 239188 [details] [diff] [review] patch v.3 >Index: src/nsWindowsShellService.cpp ... >+ nsCOMPtr<nsIStringBundleService> bundleService(do_GetService("@mozilla.org/intl/stringbundle;1")); >+ if (!bundleService) >+ return NS_ERROR_FAILURE; >+ >+ nsCOMPtr<nsIStringBundle> brandBundle; >+ nsresult rv = bundleService->CreateBundle(BRAND_PROPERTIES, getter_AddRefs(brandBundle)); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ nsXPIDLString brandFullName; >+ brandBundle->GetStringFromName(NS_LITERAL_STRING("brandFullName").get(), getter_Copies(brandFullName)); >+ >+ hr = pAAR->QueryAppIsDefaultAll(AL_EFFECTIVE, >+ brandFullName.get(), >+ aIsDefaultBrowser); That isn't DDE_NAME

wrong patch.

Comment on attachment 239189 [details] [diff] [review] the real patch v.3 >? x >Index: nsWindowsShellService.cpp >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.cpp,v >retrieving revision 1.21.2.2 >diff -u -1 -0 -r1.21.2.2 nsWindowsShellService.cpp >--- nsWindowsShellService.cpp 28 Jun 2006 23:44:17 -0000 1.21.2.2 >+++ nsWindowsShellService.cpp 19 Sep 2006 16:45:51 -0000 >@@ -185,20 +185,21 @@ > > PRInt32 flags; > } SETTING; > > #define SMI "SOFTWARE\\Clients\\StartMenuInternet\\" > #define CLS "SOFTWARE\\Classes\\" > #define DI "\\DefaultIcon" > #define SOP "\\shell\\open\\command" > #define DDE "\\shell\\open\\ddeexec\\" > #define DDE_NAME "Firefox" // This must be kept in sync with ID_DDE_APPLICATION_NAME as defined in splash.rc >+#define DDE_NAME_W L"Firefox" You defined this and then didn't use it

Comment on attachment 239189 [details] [diff] [review] the real patch v.3 okay. if that is the only change, r= and I will land it on the trunk.

removing unused wide define.

Comment on attachment 239193 [details] [diff] [review] patch v.4 Doug, this won't work... you need a LPCWSTR. Also, now that the fix for the ddeexec hack you no longer need to protect in Observe. It looks like that would have also prevented IsDefaultBrowser in Observe from being called on Vista.

trunk patch including wide string.

Thanks Doug... the patch now compiles. I did a real quick review and it looks fine but you might want to get another set of eyes on this due to my lack of sleep. Since DDE_NAME is not going to be reused the name should probably reflect its actual use. Perhaps APP_REG_NAME?

doug / robert. here come questions. please forgive my Vista / COM ignorance in advance! 1) for non-Vista, in nsWindowsShellService::SetDefaultBrowser(), we write the existing settings to MOZ_BACK_REGISTRY, and then in nsWindowsShellService::RestoreFileSettings(), we restore them. So for Vista, shouldn't that mean we should be calling ClearUserAssociations() in nsWindowsShellService::RestoreFileSettingsVista()? 2) nsWindowsShellService::SetDefaultBrowser() takes a bool, aForAllUsers. Shouldn't this be passed through to SetDefaultBrowserVista(), and if false, shouldn't we calling SetAppAsDefault() with the appropriate ASSOCIATIONLEVEL? 3) I am guess thing that the IApplicationAssociationRegistration interface is duplicated in nsWindowsShellService.h so that will compile on versions of versions of windows without shobjidl.h. is that right? But I think we should move it to nsWindowsShellService.cpp (instead of having it in nsWindowsShellService.h) 4) Any reason not to use CComPtr?

1. I think we are going to end the ablity to roll back (RestoreFileSettings). Rob knows more about this. 2. yes, we should be doing that, but we should also attempt to write all of the defaults to the registry just like the installer (or at least update the paths). This will allow side-by-side installs. Basically for this patch, since we were shooting for 2.0, we dropped the side-by-side case. 3. yes, this interface is not present on any of the current development environments. when we migrate to newer versions of the SDK, we will be able to remove this stuff. I have no problem moving it to the .cpp if you think it is a better place.

1. The reg key backup / restore is broken under several conditions and is something we should not do on Vista anyways. I haven't tested ClearUserAssociations yet and it is what we want to use. We would only want to call that when we are default and we would need to check if additional logic to handle the individual associations is needed when we are only default for a subset of the associations. 2. By default we don't want to do this. Perhaps it could be added it as a command line handler for now.

robert / dougt, thanks for the quick responses. I've provided the other set of eyes, and I think we should that code from the .h to the .cpp. I defer to (a rested) robert to give the official review for this patch, as he understands this code the best.

renames DDE_NAME_W to rob's suggestion. moves vista defines from .h to .cpp.

robert asked if I had any other concerns about this patch. I don't assumming that we are ok with this: from http://windowssdk.msdn.microsoft.com/en-us/library/aa360791.aspx IApplicationAssociationRegistration::SetAppAsDefaultAll "Sets an application as the default for all of the registered assocations of any type and level for that application." If we are ok with that, no matter what that params are for SetDefaultBrowser(PRBool aClaimAllTypes, PRBool aForAllUsers), then I'm ok.

This isn't what it does on RC1. Even with an admin account, i can not change the defaults of new or existing users of the system. I think the documentation you are reading might need to be updated as the vista application cookbook simply says: "Pass in the name of the registered app and it will set all the defaults registered to the application." I wonder if this is because UAC protects different users. Yet another thing to ask Microsoft about.

The All in SetAppAsDefaultAll I believe refers to all of the defaults for the app and not for all users. It doesn't specifically state whether it should be for all users or only the current user

Comment on attachment 239432 [details] [diff] [review] patch v.6 Thanks Doug

Comment on attachment 239432 [details] [diff] [review] patch v.6 Fixed on Trunk Checking in nsWindowsShellService.cpp; /cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.cpp,v <-- nsWindowsShellService.cpp new revision: 1.33; previous revision: 1.32 done Checking in nsWindowsShellService.h; /cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.h,v <-- nsWindowsShellService.h new revision: 1.9; previous revision: 1.8 This does not address side-by-side installs. I have opened bug 353814 for this issue.

Doug, since this is fixed on trunk do you want to resolve this as fixed or are you planning on additional work to be done in this bug for the trunk?

Wanted ASAP on the 1.8.1 branch, not a strict blocker for 1.8.1.1

This still happens. So fix it please.

Even happens on 2.0.0.1.

Why not change the registry key value that Windows Vista uses to find the default browser? Instead of pointing to IE, change it to the path that Firefox uses. Its not that hard!!!

We know it isn't fixed yet. There is still lots more to be done. It is planned to be fixed for 2.0.0.2. Also, see bug 352420 for some of the additional bugs that need to be fixed.

It doesnt work from within Firefox, but if you use the Tool Vista gives you, you can set 9 (i donnt know why 9 but it says so) priviledges to firefox. After that it works like a charm. Andi

(In reply to comment #42) > We know it isn't fixed yet. There is still lots more to be done. It is planned > to be fixed for 2.0.0.2. Also, see bug 352420 for some of the additional bugs > that need to be fixed. If that's the case, should this be RESO FIXED as it is right now?

(In reply to comment #44) > If that's the case, should this be RESO FIXED as it is right now? I changed the summary so it doesn't infer this bug covers everything needed.

If this patch is good for the branch please ask for approval, or create an appropriate branch-merged patch for this blocking bug.

This is the untested branch patch based on v.6. These needs to be tested with robert's changes a bit before landing (right robert?)

I already have a patch that includes the changes in v.6 along with the additional changes required.

fixed on the branch.

To verify Start -> Default Programs -> Set your default programs and verify that setting Firefox as the default actually sets Firefox as the default program for you.

verified fixed for 1.8.1.2 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2) Gecko/2007020823 Firefox/2.0.0.2 (RC2) Firefox 2.0.0.2 has all defaults under Start -> Default Programs -> Set your default program (like https, gopher, xht, Start Menu Link, etc)

(In reply to comment #1) > Created an attachment (id=238356) [details] > patch v.1 > > not finish. still includes MessageBox's for debuggin. > (In reply to comment #0) > Setting the default application on Vista doesn't seam to work. > sorry but even if at first sight it seems impossible to set Firefox and Thunderbird as default programs in Vista, the workaround is to disable UAC (user account control first),reboot, set again FF and Thunderbird as default, done. Now you can if you want turn UAC on again.

Comment on attachment 251226 [details] [diff] [review] patch v.6 (For Branch) clearing flag.