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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

[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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Benjamin, do we need to do anything special to remove a telemetry probe?

Comment 2

[Benjamin Smedberg]

No

Comment 3

[Masatoshi Kimura [:emk]]

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())|.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Comment 6

[Masatoshi Kimura [:emk]]

(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

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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

[:Gijs (he/him)]

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.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4fc84033bdcf

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fc84033bdcf

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

The A/B test wasn't uplifted to 40, removing tracking.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/514ef1c61d6f