Closed Bug 1183682 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox40 --- unaffected
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

[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.
Attached patch Patch (obsolete) — Splinter Review
Benjamin, do we need to do anything special to remove a telemetry probe?
Attachment #8633514 - Flags: review?(gijskruitbosch+bugs)
Attachment #8633514 - Flags: feedback?(benjamin)
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.
Attached patch Patch v1.1Splinter Review
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 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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
The A/B test wasn't uplifted to 40, removing tracking.
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.