Depends on the patch in bug 1165321.

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?

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

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

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