Closed Bug 1171730 Opened 5 years ago Closed 5 years ago

funnelcake builds should use geo-specific defaults

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: mconnor, Assigned: mconnor)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch funnelcakeFix (obsolete) — Splinter Review
Regression from a partner build fix that shipped in 35.

This is a super naive patch, untested yet, will take a stab at unit tests when I get home.
Attachment #8615630 - Flags: feedback?(florian)
Have we considered whether or not it's possible to fix this in a hotfix? We could ship that much faster than a full release.
Bug 1110420 (a different regression fix) is what regressed this.  We took an approach that was perhaps too simple, and lumped funnelcake builds in with partner builds.  There's a simple fix for now, but we should look at how we do funnelcake type things and ensure that is a robust, and cleanly distinguishable case.
A hotfix is tricky, because of how the geo defaults work.  We could fudge it, but it'd be much more code (and much more risk) to get right.
Attached patch funnelcakeFix (obsolete) — Splinter Review
Cleaned up the code to be easier to follow the underlying logic:

* if there's a distribution ID, and it doesn't start with "mozilla" it's a partner build, and we don't follow geo defaults for any of those.

* otherwise the pref dictates what we do.
Attachment #8615630 - Attachment is obsolete: true
Attachment #8615630 - Flags: feedback?(florian)
Attachment #8615707 - Flags: feedback?(florian)
The new patch is the same as the old one ?
Attached patch funnelcakeFix (obsolete) — Splinter Review
Attachment #8615707 - Attachment is obsolete: true
Attachment #8615707 - Flags: feedback?(florian)
Attached patch funnelcakeFix w/xpcshell tests (obsolete) — Splinter Review
All tests pass, we get the correct values based on the distroID.  There's a dependency on /browser for the values, not sure what the "right" answer is here, but I'm pretty confident this works correctly for the cases we know about.
Attachment #8615748 - Attachment is obsolete: true
Comment on attachment 8615758 [details] [diff] [review]
funnelcakeFix w/xpcshell tests

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

This seems reasonable. I'm not really comfortable with the hardcoded "Yahoo" and "Google" in the tests. Shouldn't we instead have the tests add 2 test engines, and set the values of browser.search.defaultenginename and browser.search.defaultenginename.US? Is this what you meant with:
> There's a dependency on /browser for the values,
> not sure what the "right" answer is here
?

::: toolkit/components/search/nsSearchService.js
@@ +412,5 @@
> +  // check to see if this is a partner build.  Partner builds should not use geo-specific defaults.
> +  let distroID;
> +  try {
> +    distroID = Services.prefs.getCharPref("distribution.id");
> +    

nit: trailing whitespace.

@@ +425,5 @@
>    try {
>      geoSpecificDefaults = Services.prefs.getBoolPref("browser.search.geoSpecificDefaults");
>    } catch(e) {}
>  
> +  return geoSpecificDefaults;  

trailing whitespace.
Attachment #8615758 - Flags: feedback+
Comment on attachment 8615758 [details] [diff] [review]
funnelcakeFix w/xpcshell tests

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+    // Mozilla-provided builds (i.e. funnelcake) are not partner builds
>+    if (!distroID.startsWith("mozilla")) {

Shouldn't this condition be (distroID && !distroID.startsWith("mozilla"))? There may not be empty pref values in practice, but I think the code should handle that case anyways.
Attached patch funnelcakeFixSplinter Review
for review and checkin.  comments addressed, tests now don't depend on /browser settings, we should be good to go here.
Attachment #8615758 - Attachment is obsolete: true
Attachment #8616022 - Flags: review?(florian)
Attachment #8616022 - Flags: review?(florian) → review+
Mike, could you fill the uplift request? thanks
Flags: needinfo?(mconnor)
Comment on attachment 8616022 [details] [diff] [review]
funnelcakeFix

Approval Request Comment
[Feature/regressing bug #]: bug 1110420
[User impact if declined]: incorrect search default for all funnelcake users
[Describe test coverage new/current, TreeHerder]: xpcshell tests added to ensure bug 1110420 doesn't regress, and this functions correctly.
[Risks and why]: low risk, very small logic tweak to exclude a subset of distribution IDs
[String/UUID change made/needed]: none
Flags: needinfo?(mconnor)
Attachment #8616022 - Flags: approval-mozilla-release?
Attachment #8616022 - Flags: approval-mozilla-beta?
Attachment #8616022 - Flags: approval-mozilla-aurora?
Comment on attachment 8616022 [details] [diff] [review]
funnelcakeFix

Taking it as it can impacts our partner repacks.

FYI, we won't be doing a 38.0.6 releases, we will just publish 38.0.6 for some repacks.
Attachment #8616022 - Flags: approval-mozilla-release?
Attachment #8616022 - Flags: approval-mozilla-release+
Attachment #8616022 - Flags: approval-mozilla-beta?
Attachment #8616022 - Flags: approval-mozilla-beta+
Attachment #8616022 - Flags: approval-mozilla-aurora?
Attachment #8616022 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/c557991689fb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
For QA:

Install a previous funnelcake build. Ideally VPN to US, or follow whatever process was used to verify the location-specific defaults in Fx35.  The default selection should be verified to be Google, despite "US" as the saved countryCode and browser.search.defaultenginename.US being set to Yahoo.

Then update to 38.0.6 and verify that the Yahoo default is correctly applied.  Also, if we have built new 38.0.6 funnelcake installers, please verify that Yahoo is correctly set for those users as well.
https://wiki.mozilla.org/Releases/Firefox_38.0.6/BuildNotes#Manually_verify_updates has some quick links and expected behaviour (from a whatsnew point of view). 

(In reply to Mike Connor [:mconnor] from comment #19)
> Then update to 38.0.6 and verify that the Yahoo default is correctly
> applied.

This didn't work for me, but I don't have a VPN to the US to test with.

> Also, if we have built new 38.0.6 funnelcake installers, please
> verify that Yahoo is correctly set for those users as well.

We haven't created any new funnelcake installers yet, the growth team is holding off on making sure this is fixed. Would creating a new profile with an older build updated to 38.0.6 be equivalent ?
We have tested build 38.0.6 today, following indications from Nick Thomas and from https://wiki.mozilla.org/Releases/Firefox_38.0.6/BuildNotes#Manually_verify_updates. All scenarios covered worked as expected, so this build looks good to go.

Scenarios covered by setting a US proxy (verified that US is correctly set for the proxy used) and performing various update scenarios via the release-localtest channel:
1. Update from 35.0.1 funnelcake build to 38.0.6 funnelcake (en-US and Windows only)
- Google was the default engine on 35.0.1 (despite detected country being US) – after update: Yahoo was the default search engine, and a what’s new page was displayed, as expected
2. Update from 38.0.5 funnelcake build to 38.0.6 funnelcake (en-US and Windows only)
- Google was the default engine on 38.0.5 (despite detected country being US) – after update: Yahoo was the default search engine, and NO what’s new page was displayed, as expected
3. Update from older funnelcake build with custom engine (e.g. Bing) to 38.0.6 funnelcake (en-US and Windows only)
- Custom engine was kept as expected (what’s new page displayed only for update from 35.0.1 funnelcake, as expected)
4. Update from regular 38.0.5 build
- NO update was provided, as expected
5. Update from regular 38.0 build
- Update performed to 38.0.5, as expected (what’s new page provided only for supported builds – e.g. en-US)

More testing details can be found at: https://etherpad.mozilla.org/Fx38-0-6.
(In reply to Nick Thomas [:nthomas] from comment #20)
> > Also, if we have built new 38.0.6 funnelcake installers, please
> > verify that Yahoo is correctly set for those users as well.
> 
> We haven't created any new funnelcake installers yet, the growth team is
> holding off on making sure this is fixed. Would creating a new profile with
> an older build updated to 38.0.6 be equivalent ?

Alternatively, could one of the partner builds that did get created as part of 38.0.6 be used to verify this instead?
You need to log in before you can comment on or make changes to this bug.