We want to get the original default engine from a Mozilla server instead of shipping it as a localized preference (we'll keep the localized preference as a fallback though). The server side for this is being developed in https://github.com/mozilla-services/searchab

I think this works, I want to test it some more, and write tests for it, but attaching now as a WIP so that I can get feedback if anybody feels like having a look. Mike, there's a telemetry probe for the time it takes to do the geo-ip request; do you think we should add one for the time it takes to do the second http request?

Mark, I no longer really expect to land this before Whistler as some small details of the server API spec are still being discussed, but I figured it could be useful to put the patch up for review now so that you can become familiar with it sooner. I've hesitated a bit about whether the sync init code should trigger the ensureKnownCountryCode code path. I tend to think it would be more correct to do so, but that it also wouldn't make any real difference, because it seems extremely unlikely that some code does a sync init of the search service before the async init has started. We need to record the search cohort id in the telemetry environment. I think I would prefer to do this in a separate bug, as new telemetry probes require a separate data review for privacy. I'm hoping to write tests for this tomorrow.

Comment on attachment 8624500 [details] [diff] [review] WIP2 Review of attachment 8624500 [details] [diff] [review]: ----------------------------------------------------------------- I suspect this is the wrong patch :)

(In reply to Mark Hammond [:markh] from comment #3) > Comment on attachment 8624500 [details] [diff] [review] > WIP2 > > Review of attachment 8624500 [details] [diff] [review]: > ----------------------------------------------------------------- > > I suspect this is the wrong patch :) Indeed! Attaching now the file I meant to attach last week so that you can have a look if you want to. Note: A v3 should arrive relatively soon, as I wrote the tests in my flight today, but still need to figure out why they pass reliably when running only my new test file, and cause timeouts 90% of the time (but NOT all the time) when running the whole xpcshell test folder.

Comment on attachment 8625371 [details] [diff] [review] actual WIP2 Review of attachment 8625371 [details] [diff] [review]: ----------------------------------------------------------------- It looks a little like en-US builds get special cased here - that may turn out to be wrong as I look deeper (I've only has a quick look) but that worries me if it is true (eg, many Aussies will use en-US). But in general, I think the code needs some comments with an overview of how this all works, and particularly what a "cohort" is - I've still no idea even after a quick look at the patch). I'll try and catch you this week to chat about it and/or have a more detailed look. ::: browser/app/profile/firefox.js @@ +399,5 @@ > pref("browser.search.order.3", "chrome://browser-region/locale/region.properties"); > > +// Market-specific search defaults > +pref("browser.search.geoSpecificDefaults", false); > +pref("browser.search.geoSpecificDefaults.url", "http://localhost:8080/%APP%/%VERSION%/%CHANNEL%/%LOCALE%/%REGION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%"); I assume this will be changed :)

Comment on attachment 8625371 [details] [diff] [review] actual WIP2 Review of attachment 8625371 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/nsSearchService.js @@ +557,5 @@ > + if (expir > Date.now()) { > + // The territory default we have already fetched hasn't expired yet. > + return; > + } > + fetchCountryCode would always resolve in 2 seconds, even if the geoip request hadn't completed in that time. It seems that fetchRegionDefault doesn't have that same functionality, so a worst-case is that this function might take 100 seconds to resolve. Given this blocks async initialization I'm not sure if that is acceptable here. @@ +712,5 @@ > request.send("{}"); > }); > } > > +function fetchRegionDefault(resolve) { wouldn't it be a little clearer and less error prone to have this return a promise (probably via Task.async)? @@ +3496,5 @@ > return this._buildSortedEngineList(); > return this.__sortedEngines; > }, > > + // Get the original Engine:. Use first the engine name we got based on the This comment looks a bit strange (the "Engine:." part) - we probably just want something like "Get the original Engine object that is the default for this region, ignoring changes the user may have subsequently made" - or something :) ::: toolkit/components/urlformatter/nsURLFormatter.js @@ +93,5 @@ > + REGION: function() { > + try { > + return Services.prefs.getCharPref("browser.search.region"); > + } > + catch(e) { This should be on the same line as the closing "}". I also wonder if this should be more explicit as SEARCHREGION? @@ +94,5 @@ > + try { > + return Services.prefs.getCharPref("browser.search.region"); > + } > + catch(e) { > + return "ZZ"; maybe add a comment that ZZ is the "official" value for unknown

Now with tests, handling of the Retry-After header, and using only the value from the default pref branch for the url template. Comment 6 addressed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee12e2c82e4

(In reply to Florian Quèze [:florian] [:flo] from comment #7) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee12e2c82e4 With a one line change I got test_PlacesSearchAutocompleteProvider.js to pass, but there are still some failures on Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=833afdb649a7 I'll be debugging that tomorrow.

Comment on attachment 8627290 [details] [diff] [review] Patch v3 Review of attachment 8627290 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I didn't get very far again on this, but publishing the few comments I have anyway. ::: browser/app/profile/firefox.js @@ +401,5 @@ > +// Market-specific search defaults > +pref("browser.search.geoSpecificDefaults", false); > +pref("browser.search.geoSpecificDefaults.url", "http://localhost:8080/%APP%/%VERSION%/%CHANNEL%/%LOCALE%/%REGION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%"); > + > +// US specific default (used as a fallback if the geoSpecificDefaults request fails). At face value this comment seems confusing - the geoSpecificDefaults pref is false, so it's not clear why a "geoSpecificDefaults request" would be made. ::: toolkit/components/search/nsSearchService.js @@ +268,5 @@ > /** > * Otherwise, don't log at all by default. This can be overridden at startup > * by the pref, see SearchService's _init method. > */ > +var LOG = DO_LOG;//function(){}; can you get rid of this yet? @@ +505,5 @@ > } > > // Helper method to modify preference keys with geo-specific modifiers, if needed. > function getGeoSpecificPrefName(basepref) { > + if (!geoSpecificDefaultsEnabled() || isPartnerBuild()) I'm afraid I still don't understand this bit - firefox.js is toggling browser.search.geoSpecificDefaults to false, so it seems this will early-return |basepref| in en.us builds - so I don't understand when it is expected to return |basepref + ".US"| @@ +729,5 @@ > request.send("{}"); > }); > } > > +let fetchRegionDefault = () => new Promise(resolve => { I this function needs some comments that describes what it does, what a "cohort" is, etc. I know you explained this to me in person, but I don't think it will be particularly obvious to the next poor sucker reading this.

Comment on attachment 8627290 [details] [diff] [review] Patch v3 Review of attachment 8627290 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/nsSearchService.js @@ +730,5 @@ > }); > } > > +let fetchRegionDefault = () => new Promise(resolve => { > + let urlTemplate = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF) isn't this the function that we decided should also use a timer?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9a7ba31aa8d (In reply to Mark Hammond [:markh] from comment #9) > ::: browser/app/profile/firefox.js > @@ +401,5 @@ > > +// Market-specific search defaults > > +pref("browser.search.geoSpecificDefaults", false); [...] > > +// US specific default (used as a fallback if the geoSpecificDefaults request fails). > > At face value this comment seems confusing - the geoSpecificDefaults pref is > false, so it's not clear why a "geoSpecificDefaults request" would be made. > @@ +505,5 @@ > > } > > > > // Helper method to modify preference keys with geo-specific modifiers, if needed. > > function getGeoSpecificPrefName(basepref) { > > + if (!geoSpecificDefaultsEnabled() || isPartnerBuild()) > > I'm afraid I still don't understand this bit - firefox.js is toggling > browser.search.geoSpecificDefaults to false, so it seems this will > early-return |basepref| in en.us builds - so I don't understand when it is > expected to return |basepref + ".US"| I think what confuses you is that the pref is set to false in firefox.js, and then overridden to true in firefox-l10n.js. I added a comment to explain that in firefox.js About the .US thing. I wonder if we should eventually remove the en-US specific aspect of it, and have a fallback with a defaultenginename.<region code> pref for all locales that have region-specific defaults. I think currently all the use cases for region-specific defaults are on the en-US locale, so probably not much benefit in the short term, except for making the code less confusing. We could do this in a follow-up. (In reply to Mark Hammond [:markh] from comment #10) > > +let fetchRegionDefault = () => new Promise(resolve => { > > + let urlTemplate = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF) > > isn't this the function that we decided should also use a timer? Yes and no. I want the 2s time to apply to both the country lookup and the default engine lookup together. If I added a timer in fetchRegionDefault, we would end up taking up to 4s. What I did in the patch is that I added a timer in the ensureKnownCountryCode code path that doesn't call fetchCountryCode. It would probably have been a bit cleaner to move the fetchCountryCode timer up to ensureKnownCountryCode instead of duplicating it, but that would have involved more code changes due to tasks becoming functions with callbacks and vice versa.

Comment on attachment 8628360 [details] [diff] [review] Patch v4 Review of attachment 8628360 [details] [diff] [review]: ----------------------------------------------------------------- I think this is looking fine, but do think we need to think a little more about the telemetry, at least to avoid false-positives about the geoip lookup timing out, but probably to collect similar telemetry for just the region defaults. I also ran out of time again, but think I need to look at the tests a little more closely, so only f+ for now - I'll get back to it ASAP. ::: browser/locales/en-US/firefox-l10n.js @@ +3,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > #filter substitution > > +pref("browser.search.geoSpecificDefaults", true); I think a comment here would also be useful and may avoid localizers being confused and enabling it in locales where it should not be. ::: mobile/android/app/mobile.js @@ +264,5 @@ > pref("browser.search.order.1", "chrome://browser/locale/region.properties"); > pref("browser.search.order.2", "chrome://browser/locale/region.properties"); > pref("browser.search.order.3", "chrome://browser/locale/region.properties"); > > +// Market-specific search defaults let's add the same comment here as in firefox.js (and ditto for mobile-l10n.js) ::: toolkit/components/search/nsSearchService.js @@ +690,5 @@ > + // so there's nothing else to do. > + if (timerId == null) { > + return; > + } > + Services.telemetry.getHistogramById("SEARCH_SERVICE_COUNTRY_TIMEOUT").add(0); I'm not sure about this telemetry - I suspect we will find we suddenly start reporting timeouts - we've only a few seconds as it is, and now we are adding a new xhr request. Sadly I think we might want telemetry for both the old and new xhr. @@ +696,5 @@ > + resolve(); > + }; > + > + if (result && geoSpecificDefaultsEnabled()) { > + fetchRegionDefault().then(callback); I think we need a .catch here just incase it fails. @@ +745,5 @@ > + let urlTemplate = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF) > + .getCharPref("geoSpecificDefaults.url"); > + let endpoint = Services.urlFormatter.formatURL(urlTemplate); > + > + // As an escape hatch, no endpoint means no geoip. s/geoip/region defaults/ or something ::: toolkit/components/search/tests/xpcshell/head_search.js @@ +259,5 @@ > + const nsIPLS = Ci.nsIPrefLocalizedString; > + // Copy the logic from nsSearchService > + let pref = kDefaultenginenamePref; > + if (isUS === undefined) > + isUS = Services.prefs.getCharPref(kLocalePref) == "en-US" && isUSTimezone(); it seems a shame that these tests now seem to behave differently based on the timezone of where the test is being run. Is this necessary, or can we pretend to always (or never) be in the US?

Comment on attachment 8628360 [details] [diff] [review] Patch v4 Review of attachment 8628360 [details] [diff] [review]: ----------------------------------------------------------------- The tests look good to me. I think the only other discussion we need to have is around the telemetry. ::: toolkit/components/search/tests/xpcshell/test_geodefaults.js @@ +116,5 @@ > + // (Re)initializing the search service should trigger a request, > + // and set the default engine based on it. > + let commitPromise = promiseAfterCommit(); > + // Due to the previous initialization, we expect the countryCode to already be set. > + do_check_true(Services.prefs.prefHasUserValue("browser.search.countryCode")); let's check this is exactly what you expect (ie, FR) @@ +157,5 @@ > + > + // Check that the expiration timestamp has been updated. > + let metadata = yield promiseGlobalMetadata(); > + do_check_eq(typeof metadata.searchdefaultexpir, "number"); > + do_check_true(metadata.searchdefaultexpir >= date + 31536000 * 1000); it mike make sense to have this magic number be a constant, and check here that the new expiry is withing (say) an hour (or even a day!) of the expected time. @@ +182,5 @@ > + // XXX For some reason forcing a sync init while already asynchronously > + // reinitializing causes a shutdown warning related to engineMetadataService's > + // finalize method having already been called. Seems harmless for the purpose > + // of this test. > + do_check_eq(Services.search.currentEngine.name, getDefaultEngineName(false)); We should probably check gInitialized is false before this line to ensure we are actually checking before the async init completed, and that it is true after to check we did perform a sync init. @@ +267,5 @@ > + > + // Check that the expiration timestamp has been updated. > + let metadata = yield promiseGlobalMetadata(); > + do_check_eq(typeof metadata.searchdefaultexpir, "number"); > + do_check_true(metadata.searchdefaultexpir >= date + 86400 * 1000); let's also check we are in a sane range here (ie, within a few hours or so of what we expect) to prevent a future us confusing seconds with ms or similar. ::: toolkit/components/search/tests/xpcshell/test_location.js @@ +5,5 @@ > installTestEngine(); > > Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "AU"}'); > + Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF) > + .setCharPref("geoSpecificDefaults.url", 'data:application/json,{}'); I wonder if we could just nuke this pref here - the code lists no pref as an escape-hatch so that should work OK and helps test that no pref *actually* works :) Ditto for the next few tests

(In reply to Mark Hammond [:markh] from comment #12) > ::: toolkit/components/search/tests/xpcshell/head_search.js > @@ +259,5 @@ > > + const nsIPLS = Ci.nsIPrefLocalizedString; > > + // Copy the logic from nsSearchService > > + let pref = kDefaultenginenamePref; > > + if (isUS === undefined) > > + isUS = Services.prefs.getCharPref(kLocalePref) == "en-US" && isUSTimezone(); > > it seems a shame that these tests now seem to behave differently based on > the timezone of where the test is being run. Is this necessary, or can we > pretend to always (or never) be in the US? I haven't changed this behavior, just factored it out from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_selectedEngine.js?rev=862529bd3b1f#17 My new test always pretends to not be in the US, by giving false as a parameter to the getDefaultEngineName function.

comment 12 and comment 13 addressed. The url is now set to the final one. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b635ac246a4

Changed 2 bc tests to take into account browser.search.geoSpecificDefaults now being false for mochitests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7e75cd7c4b6

(In reply to Pulsebot from comment #17) > https://hg.mozilla.org/integration/fx-team/rev/2ae760290f8c Note: I changed the URL in the pref files before pushing. The actual url is on the mozilla.com domain, not .org.

(In reply to Pulsebot from comment #19) > https://hg.mozilla.org/integration/fx-team/rev/912519e2d0df Disabled by making the URL empty, because of Talos and Crashtest bustage.

This also busted Autophone. Will these urls be re-instated at some point and the test suites have to adapt by setting these urls to empty strings?

(In reply to Bob Clary [:bc:] from comment #21) > This also busted Autophone. Where is this visible on the treeherder output? Can I verify I'm fixing it on try? > Will these urls be re-instated at some point Yes, as soon as I can do so without breaking the tree. > the test suites have to adapt by setting these urls to empty strings? In most cases it would be preferable if test suites didn't touch the url prefs and instead just set user_pref("browser.search.geoSpecificDefaults", false); That's what we did for mochitests.

(In reply to Florian Quèze [:florian] [:flo] from comment #22) > (In reply to Bob Clary [:bc:] from comment #21) > > This also busted Autophone. > > Where is this visible on the treeherder output? Can I verify I'm fixing it > on try? > Autophone is a so-called Tier 3 job which isn't visible on Treeherder by default. You need to show the hidden jobs. This will show Autophone on inbound: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&exclusion_profile=false&filter-searchStr=autophone Yes, for your case the following should be sufficient: try: -b o -p android-api-9,android-api-11 -u autophone-smoketest,autophone-s1s2,autophone-webapp -t none > > Will these urls be re-instated at some point > > Yes, as soon as I can do so without breaking the tree. > > > the test suites have to adapt by setting these urls to empty strings? > > In most cases it would be preferable if test suites didn't touch the url > prefs and instead just set user_pref("browser.search.geoSpecificDefaults", > false); > That's what we did for mochitests. Filed bug 1182802 for me to add the pref. I'll do that this weekend. Thanks.

https://hg.mozilla.org/mozilla-central/rev/2ae760290f8c https://hg.mozilla.org/mozilla-central/rev/912519e2d0df

Getting stuff right in prefs file is rare, and it seems that the localized pref is just an optimization? As such, I don't think we should bother much about ensuring that the pref is actually correct, at least on the l10n side.

(In reply to Axel Hecht [:Pike] from comment #25) > Getting stuff right in prefs file is rare, and it seems that the localized > pref is just an optimization? The pref being false prevents pointless HTTP requests to a Mozilla server; so yes, it's fair to say it's an optimization. > As such, I don't think we should bother much about ensuring that the pref is > actually correct, at least on the l10n side. It would be good to have it correct on large locales, and probably doesn't matter much for smaller ones.

Try seems OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=732bc58269bb Approval Request Comment [Feature/regressing bug #]: a/b testing of search defaults. [User impact if declined]: none, but it's wanted ASAP for business reasons. [Describe test coverage new/current, TreeHerder]: the patch contains automated tests. I'm hoping QA will also chime in. [Risks and why]: non-trivial, but the sooner we expose this to aurora/beta testing, the more we can reduce risks. [String/UUID change made/needed]: none

Try looks OK on beta too: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52c56aed5837 Approval Request Comment: see comment 27. Note: the uplift will require pushing https://hg.mozilla.org/build/talos/rev/8a61f8d21fc0 on the talos repository with 4a8d22dd38c4 as the parent. I did this in a user repository for my try push.

Comment on attachment 8635323 [details] [diff] [review] Patch for aurora This is a huge code change with some reftests and unit tests added. The try push looks clean. The sooner we uplift to Aurora we will find out any ripple effects.

Requesting QE team to verify this feature/bug fix.

Tracking 40 as we need to ship this fix.

I've performed some tests using latest Developer Edition 41.0a2 2015-07-20 under Win 7 x64, Ubuntu 14.04 x86 and Mac OSX 10.9.5 and ensured that: - Basic search operations still work - No new regressions were introduced by this patch - Google remains the default search engine for RO region - For US builds, I manually added the region and countryCode prefs for a profile not opened before - the default search engine in this case was Yahoo - There are no issues when there's no internet connection at the time a profile is created - The previous user choices are not overridden - There are no issues when opening two locales with the same profile, one having browser.search.geoSpecificDefaults set to true and the other one having the pref set to false (or when I manually changed the pref's value) - There are no issues if the internet connection is slow (obtained the same results as for Firefox 40 beta) Please let me know if there are some other cases that I should cover. For now, marking as verified.

Looks like this broke geo specific defaults on Fennec. The default for US should be "Yahoo" but is "Google".

https://hg.mozilla.org/releases/mozilla-beta/rev/37ea9e22ac42 (approved by lmandel over IRC).

Comment on attachment 8635326 [details] [diff] [review] Patch for beta Approved for Beta (as Florian mentioned). Note that Florian landed a patch that differs from this one in that it includes the mobile fix.

Verified as fixed with Firefox 40 beta 7 under Win 7 64-bit and Mac OS X. I'm removing the qe-verify+ flag since this verification should suffice.