Use the Vista Default Application API

RESOLVED FIXED in Firefox 2

Status

()

Firefox
General
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: dougt, Assigned: rstrong)

Tracking

({verified1.8.1.2})

2.0 Branch
Firefox 2
x86
Windows XP
verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.1 -
blocking1.8.1.2 +
blocking-firefox2 -
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [vista])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

11 years ago
Setting the default application on Vista doesn't seam to work.
(Reporter)

Updated

11 years ago
Blocks: 352420
(Reporter)

Comment 1

11 years ago
Created attachment 238356 [details] [diff] [review]
patch v.1

not finish.  still includes MessageBox's for debuggin.

Updated

11 years ago
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2
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.
(Reporter)

Comment 3

11 years ago
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.
(Reporter)

Comment 6

11 years ago
Created attachment 238625 [details] [diff] [review]
patch v.2

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.
Attachment #238356 - Attachment is obsolete: true
Attachment #238625 - Flags: review?(robert.bugzilla)
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...
(Reporter)

Comment 9

11 years ago
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.
(Reporter)

Comment 11

11 years ago
Comment on attachment 238625 [details] [diff] [review]
patch v.2

this doesn't work for side by side.
Attachment #238625 - Flags: review?(robert.bugzilla) → review-
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
(Reporter)

Comment 14

11 years ago
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.

Comment 17

11 years ago
Ok - we are cutting 1.8.1 in minutes so this will have to defer for a 2.0.0.1 branch.
Flags: blocking-firefox2? → blocking-firefox2-
(Reporter)

Comment 18

11 years ago
Created attachment 239188 [details] [diff] [review]
patch v.3

Uses DDE_NAME.
Attachment #238625 - Attachment is obsolete: true
Attachment #239188 - Flags: review?
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
Attachment #239188 - Flags: review? → review-
(Reporter)

Comment 20

11 years ago
Created attachment 239189 [details] [diff] [review]
the real patch v.3

wrong patch.
Attachment #239188 - Attachment is obsolete: true
Attachment #239189 - Flags: review?
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
Attachment #239189 - Flags: review? → review-
(Reporter)

Comment 22

11 years ago
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.
(Reporter)

Updated

11 years ago
Attachment #239189 - Flags: superreview?
(Reporter)

Comment 23

11 years ago
Created attachment 239193 [details] [diff] [review]
patch v.4

removing unused wide define.
Attachment #239189 - Attachment is obsolete: true
Attachment #239193 - Flags: superreview?
Attachment #239193 - Flags: review?
Attachment #239189 - Flags: superreview?
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.
(Reporter)

Comment 25

11 years ago
Created attachment 239239 [details] [diff] [review]
patch v.5

trunk patch including wide string.
Attachment #239193 - Attachment is obsolete: true
Attachment #239239 - Flags: review?
Attachment #239193 - Flags: superreview?
Attachment #239193 - Flags: review?
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?
(Reporter)

Comment 28

11 years ago
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.
(Reporter)

Comment 31

11 years ago
Created attachment 239432 [details] [diff] [review]
patch v.6

renames DDE_NAME_W to rob's suggestion.
moves vista defines from .h to .cpp.
Attachment #239239 - Attachment is obsolete: true
Attachment #239432 - Flags: review?
Attachment #239239 - Flags: review?
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.
(Reporter)

Comment 33

11 years ago
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
Attachment #239432 - Flags: review? → review+
(Reporter)

Comment 36

11 years ago
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.
Whiteboard: [Fx 2.0.0.1]
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?
Flags: blocking1.8.1.1?
Wanted ASAP on the 1.8.1 branch, not a strict blocker for 1.8.1.1
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1.2+

Comment 39

11 years ago
This still happens. So fix it please. 

Comment 40

11 years ago
Even happens on 2.0.0.1. 

Comment 41

11 years ago
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.

Comment 43

11 years ago
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.
Summary: Default Application on Vista not working → Use the Vista Default Application API
Whiteboard: [Fx 2.0.0.1]
If this patch is good for the branch please ask for approval, or create an appropriate branch-merged patch for this blocking bug.

Updated

11 years ago
Whiteboard: [vista]
(Reporter)

Comment 47

11 years ago
Created attachment 251226 [details] [diff] [review]
patch v.6 (For Branch)

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?)
Attachment #251226 - Flags: review?
I already have a patch that includes the changes in v.6 along with the additional changes required.

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Assignee: dougt → robert.bugzilla
Status: REOPENED → NEW

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Whiteboard: [vista] → [vista] :rs has v.6 patch somewhere, needs review?
fixed on the branch.
Keywords: fixed1.8.1.2
Blocks: 369465
No longer blocks: 352420
Whiteboard: [vista] :rs has v.6 patch somewhere, needs review? → [vista]
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)
Keywords: fixed1.8.1.2 → verified1.8.1.2

Comment 52

11 years ago
(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.
(Reporter)

Comment 53

10 years ago
Comment on attachment 251226 [details] [diff] [review]
patch v.6 (For Branch)

clearing flag.
Attachment #251226 - Flags: review?
You need to log in before you can comment on or make changes to this bug.