Remove the default browser A/B test for Windows 10 (keep Settings approach)

RESOLVED FIXED in Firefox 41

Status

()

Firefox
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 42
Points:
---

Firefox Tracking Flags

(firefox40 unaffected, firefox41 fixed, firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

[Tracking Requested - why for this release]:
We need to get this uplifted for Firefox 40 and remove the A/B test before going to release.
Blocks: 1077146
Created attachment 8633514 [details] [diff] [review]
Patch

Benjamin, do we need to do anything special to remove a telemetry probe?
Attachment #8633514 - Flags: review?(gijskruitbosch+bugs)
Attachment #8633514 - Flags: feedback?(benjamin)

Comment 2

3 years ago
No
Attachment #8633514 - Flags: feedback?(benjamin)
Comment on attachment 8633514 [details] [diff] [review]
Patch

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

::: browser/components/shell/nsWindowsShellService.cpp
@@ -768,5 @@
>        // fall back to showing control panel for all defaults
>        if (NS_FAILED(rv)) {
>          if (IsWin10OrLater()) {
> -          rv = LaunchModernSettingsDialogDefaultApps();
> -        } else {

|if (IsWin10OrLater())| should be changed to |if (!IsWin10OrLater())|.
(In reply to Masatoshi Kimura [:emk] from comment #3)
> Comment on attachment 8633514 [details] [diff] [review]
> Patch
> 
> Review of attachment 8633514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/shell/nsWindowsShellService.cpp
> @@ -768,5 @@
> >        // fall back to showing control panel for all defaults
> >        if (NS_FAILED(rv)) {
> >          if (IsWin10OrLater()) {
> > -          rv = LaunchModernSettingsDialogDefaultApps();
> > -        } else {
> 
> |if (IsWin10OrLater())| should be changed to |if (!IsWin10OrLater())|.

Actually, the control panel should work for Windows 10. I'll just remove the conditional around that call. Thanks for catching that.
Created attachment 8633528 [details] [diff] [review]
Patch v1.1
Attachment #8633514 - Attachment is obsolete: true
Attachment #8633514 - Flags: review?(gijskruitbosch+bugs)
Attachment #8633528 - Flags: review?(gijskruitbosch+bugs)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Actually, the control panel should work for Windows 10. I'll just remove the
> conditional around that call. Thanks for catching that.

The control panel worked before bug 791501, but it no longer works because Windows 10 blocks IAARUI::LaunchAdvancedAssociationUI. So you can't remove the version check. (Or do you expect this dialog [1]? This is in the unlikely fallback branch anyway.)

[1] https://bug1167294.bugzilla.mozilla.org/attachment.cgi?id=8608889
Ok, I was unaware of bug 791501.

Since this code path is only hit when LaunchModernSettingsDialogDefaultApps() fails, I think it is fine to show the warning bar/dialog since the user may have to manually make the change themselves (or find out that their account lacks the permissions to do so).

Comment 8

3 years ago
Comment on attachment 8633528 [details] [diff] [review]
Patch v1.1

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

rs=me

::: browser/components/nsBrowserGlue.js
@@ -166,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
>                                    "resource://gre/modules/LightweightThemeManager.jsm");
>  
> -XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
> -                                  "resource://gre/modules/AppConstants.jsm");

Kinda sad we're ditching this, I'm likely to still want to use this in this file, but adding it back is almost free, so let's ditch it for now.
Attachment #8633528 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/4fc84033bdcf
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
The A/B test wasn't uplifted to 40, removing tracking.
status-firefox40: affected → unaffected
tracking-firefox40: ? → ---
Comment on attachment 8633528 [details] [diff] [review]
Patch v1.1

Approval Request Comment
[Feature/regressing bug #]: bug 1176112
[User impact if declined]: different default-browser experience for users given at random
[Describe test coverage new/current, TreeHerder]: manual testing, landed on mozilla-central
[Risks and why]: none expected 
[String/UUID change made/needed]: none
Attachment #8633528 - Flags: approval-mozilla-aurora?
Comment on attachment 8633528 [details] [diff] [review]
Patch v1.1

This patch seems safe and low risk given that it is turning off some settings. Let's land it on Aurora.
Attachment #8633528 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.