Closed Bug 872012 Opened 7 years ago Closed 7 years ago

Work - Non-Metro-enabled Firefox cannot take the default browser from Metro-enabled Nightly

Categories

(Firefox :: Shell Integration, defect)

20 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(1 file, 3 obsolete files)

Steps to reproduce:
1. Install a release version of Firefox.
2. Install Nightly.
3. Try to set the release Firefox to default from [Options]-[Advanced]-[System Defaults]-[Make Firefox the default browser].
4. Double click a .url shortcut or type a URL into Start Screen.

Actual result:
Nightly will run.

Expected result:
The release firefox should start.

Perhaps we need to remove the "DelegateExecute" named value even if the Metro support is disabled.
Summary: Non-Metro-enabled Firefox cannot take the default browser from Metro-enabled Nightly → Defect - Non-Metro-enabled Firefox cannot take the default browser from Metro-enabled Nightly
Whiteboard: feature=defect c=tbd u=tbd p=0
Blocks: 855311
No longer blocks: metrov1defect&change
Summary: Defect - Non-Metro-enabled Firefox cannot take the default browser from Metro-enabled Nightly → Work - Non-Metro-enabled Firefox cannot take the default browser from Metro-enabled Nightly
Whiteboard: feature=defect c=tbd u=tbd p=0
I would like to backport this because this is obviously useless unless it is fixed before enabling Metro.
Removing "DelegateExecute" didn't work somehow.
At step 3.5 are you going to control panel and finishing the default setup by selecting Firefox and making it the default?
I didn't have to control panel myself. [Make Firefox the default browser] brought the [Default Programs] panel for me. Then I clicked the panel to try to make the release Firefox default, of course.
Ya I meant [Default Programs] by [Control Panel] thx.  Just making sure since it wasn't specified :D
This patch worked for me.
I had to remove AppUserModelID either.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #750409 - Flags: review?(netzen)
Comment on attachment 750409 [details] [diff] [review]
Remove Metro registrations when Metro support is disabled

Review of attachment 750409 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should be just calling CleanupMetroBrowserHandlerValues before setting the default when MOZ_METRO is not defined.  The appusermmodelid is used for a lot of things and shouldn't be removed.
Attachment #750409 - Flags: review?(netzen)
(In reply to Brian R. Bondy [:bbondy] from comment #7)
> I think we should be just calling CleanupMetroBrowserHandlerValues before
> setting the default when MOZ_METRO is not defined.  The appusermmodelid is
> used for a lot of things and shouldn't be removed.

That sounds like a good idea except that it didn't work at all. CleanupMetroBrowserHandlerValues did nothing for other installations.
Assignee: VYV03354 → nobody
Status: ASSIGNED → NEW
I think we have to call it after setting the default since that function aborts if you aren't the default, did you try it before or after the default was set?
Attached patch Non working patch (obsolete) — Splinter Review
If this code does nothing, even the current code will do nothing. Because I just moved CleanupMetroBrowserHandlerValues out from MOZ_METRO ifdefs.
(In reply to Brian R. Bondy [:bbondy] from comment #7)
> The appusermmodelid is
> used for a lot of things and shouldn't be removed.

The AppUserModelID will be overwritten with a different value anyway if another Metro-enabled installation try to take the default browser. I don't understand why we shouldn't remove it for a non-Metro-enabled installation.
Even if we don't have a Metro browser we still want to have the AppUserModelID, it is used for other OS integration points like the taskbar and the jumplists.
Seems like we could update the CEH such that when installed it's smarter about choosing the right browser. More generally I think we want to migrate toward using the CEH for launching since it gives us more control.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/
Like this?
Attachment #750409 - Attachment is obsolete: true
Attachment #750456 - Attachment is obsolete: true
Attachment #751053 - Flags: review?(jmathies)
The new patch will not need uplifting. The problem will be fixed solely inside the Metro-enabled browser.
If non-Metro-enabled browser is default, tapping the Nightly tile will fail to launch the Metro browser. We need to fallback to launching the desktop browser in this case.
Assignee: nobody → VYV03354
Attachment #751324 - Flags: review?(jmathies)
Comment on attachment 751053 [details] [diff] [review]
Get default browser's path from the association when launching on the desktop

Forgot to obsolete the previous patch.
Attachment #751053 - Attachment is obsolete: true
Attachment #751053 - Flags: review?(jmathies)
Comment on attachment 751324 [details] [diff] [review]
Get default browser's path from the association when launching on the desktop, v2

Review of attachment 751324 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +293,2 @@
>  
> +    BOOL res;

Just noticed this, we should set a default here in case QueryAppIsDefaultAll fails - 

BOOL res = FALSE;

@@ +311,5 @@
> +
> +    Log(L"registeredApp=%s", registeredApp);
> +    bool result = !wcsicmp(registeredApp, kDefaultMetroBrowserIDPathKey);
> +    if (!result) {
> +      result = !wcsicmp(registeredApp, kDemoMetroBrowserIDPathKey);

kDemoMetroBrowserIDPathKey is no longer in use, you need to merge to mc.
Attachment #751324 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/0d177dbf23f4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.