Closed Bug 1176112 Opened 4 years ago Closed 4 years ago

A/B test for default browser setting UI on Windows 10

Categories

(Firefox :: Shell Integration, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.3 - Jun 29
Tracking Status
firefox41 --- fixed

People

(Reporter: emk, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
Depends on the patch in bug 1165321.
Attachment #8624866 - Flags: review?(gijskruitbosch+bugs)
Benjamin, the attached patch adds a telemetry probe to help us determine which is the better default-browser dialog on Windows 10. We track the result of the default-browser setting and combine it with which dialog the user would have seen. The dialog that the user sees is chosen randomly, once per profile.

We should only need to collect this data until Firefox 45, at which point we should have enough data to make a concrete decision and can remove the usage of the less successful dialog.

Can you take a look at the patch and give approval for the privacy implications of the new telemetry probe?
Flags: needinfo?(benjamin)
Iteration: --- → 41.3 - Jun 29
Points: --- → 3
Flags: qe-verify-
Comment on attachment 8624866 [details] [diff] [review]
Patch

The description of the histogram is confusing to me. Is this histogram recorded when the user makes a choice in the default-browser dialog? And "openas" and "modernsettings" are the two branches of the experiment?

The code in question is going to run for release users, but as written the histogram here will only be recorded when telemetry is enabled. Is that what you intend? f+ if so
Flags: needinfo?(benjamin)
Attachment #8624866 - Flags: feedback+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> The description of the histogram is confusing to me. Is this histogram
> recorded when the user makes a choice in the default-browser dialog? And
> "openas" and "modernsettings" are the two branches of the experiment?

This histogram is recorded on startup around the same time that we record the telemetry histogram for BROWSER_IS_USER_DEFAULT. Setting the default browser is asynchronous and we don't get a notification when the process has completed successfully, so in turn we just check during startup (it may be default during next startup if not now). "openas" and "modernsettings" are the two branches of the experiment, and this records if the user has set the browser as a default paired with which branch they are using.

With this, we should know if there is a non-negligible difference between the users who got "openas" and "modernsettings" with respect to setting the default browser.

> The code in question is going to run for release users, but as written the
> histogram here will only be recorded when telemetry is enabled. Is that what
> you intend? f+ if so

Yes, it is fine that it doesn't run for release users. We don't currently do anything special with the BROWSER_IS_USER_DEFAULT probe. It would be good to be consistent with that probe due to how closely related they are.

Thanks for the feedback.
Comment on attachment 8624866 [details] [diff] [review]
Patch

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

::: browser/components/nsBrowserGlue.js
@@ +1132,5 @@
> +        let abTest = Math.round(Math.random());
> +        Services.prefs.setIntPref("browser.shell.windows10DefaultBrowserABTest", abTest);
> +      }
> +
> +      if (AppConstants.isPlatformAndVersionAtLeast("win", "10")) {

You probably want to import my toolkit/modules/AppConstants changes into this patch if you're landing it, because I'm still blocked on windows build issues for the original patch.

@@ +1137,5 @@
> +        let abTest = Services.prefs.getIntPref("browser.shell.windows10DefaultBrowserABTest");
> +        let result = abTest == 0 && !isDefault ? 0 :
> +                     abTest == 0 && isDefault ?  1 :
> +                     abTest == 1 && !isDefault ? 2 :
> +                     abTest == 1 && isDefault ?  3 : -1;

result = abTest * 2 + Number(isDefault);

If that is unclear, add a comment above it.

@@ +1139,5 @@
> +                     abTest == 0 && isDefault ?  1 :
> +                     abTest == 1 && !isDefault ? 2 :
> +                     abTest == 1 && isDefault ?  3 : -1;
> +        if (result != -1) {
> +          Services.telemetry.getHistogramById("WIN_10_DEFAULT_BROWSER_AB_TEST").add(result);

You're adding this before people have had a chance to change it, which means this is going to be biased towards !isDefault. Not necessarily a problem, but we should take that into account.
Attachment #8624866 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #5)
> You're adding this before people have had a chance to change it, which means
> this is going to be biased towards !isDefault. Not necessarily a problem,
> but we should take that into account.

This is the same behavior that the above BROWSER_IS_USER_DEFAULT probe uses, so it should be fine here.
https://hg.mozilla.org/mozilla-central/rev/8a504c99c379
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
https://hg.mozilla.org/mozilla-central/rev/8a504c99c379#l5.31
I think this goes against the intention of AppConstants.jsm containing only build time defines. Why don't we move this to toolkit/modules/BrowserUtils.jsm ?
(In reply to Philip Chee from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/8a504c99c379#l5.31
> I think this goes against the intention of AppConstants.jsm containing only
> build time defines. Why don't we move this to
> toolkit/modules/BrowserUtils.jsm ?

It's not a browser utility. 

We could have only defined the app version on AppConstants.jsm (that's constant), but the only reason anyone wants it is to ensure that you're on OS <whatever> and version <whatever> or later. We could duplicate that code everywhere, or we could just take as much work out of people's hands as possible.
You need to log in before you can comment on or make changes to this bug.