This was called out in bug 1102416 comment 3, and then punted on because Firefox didn't need it in bug 1102416 comment 5. This temporarily caused bug 1108627 for Fennec until we just disabled geo-specific defaults entirely there. It's likely that Fennec will want geo-specific defaults eventually (maybe soon), so we should fix this properly.

Does this mean we just need to make the defaultEngine setter be region-aware?

WIP: This changes the nsSearchService.defaultEngine getter and setter to be aware of the regional preference. Try (for Android): https://treeherder.mozilla.org/#/jobs?repo=try&revision=db493d7f6b75

Update the preferences so we use the US-only search defaults when appropriate.

Comment on attachment 8551991 [details] [diff] [review] regional-default-search v0.1 Review of attachment 8551991 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, although I think it would be nicer to abstract this into a single place - something like a getGeoSpecificPrefName() as mentioned in bug 1108627 comment 10.

Comment on attachment 8551991 [details] [diff] [review] regional-default-search v0.1 Review of attachment 8551991 [details] [diff] [review]: ----------------------------------------------------------------- Also, it could also be argued that we should remove the check for browser.search.geoSpecificDefaults from getIsUS() and have it inside that new getGeoSpecificPrefName() helper - that would make getIsUS() accurate even when that pref disables geo specific defaults. It probably doesn't matter much though while getIsUS() isn't exported and is only used in the context of geo specific defaults, but it sure *sounds* cleaner and more future-proof.

We've got bug 1109118 on file to clean this stuff up - I'd love to see it on an upcoming desktop priority backlog!

I made both of Mark's suggestions in this patch. I could also just use the simple patch if Desktop doesn't want this much churn. Also, we should probably consider just the simple patch for uplifting to beta.

Fixed a typo in this patch

Comment on attachment 8552658 [details] [diff] [review] regional-default-search v0.2 Nice improvement. getIsGeoSpecificDefaults is a weird name, how about geoSpecificDefaultsEnabled? >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js > for (var j = 1; ; j++) { >- var prefName = BROWSER_SEARCH_PREF + "order."; >- if (getIsUS()) { >- prefName += "US."; >- } >- prefName += j; >+ var prefName = getGeoSpecificPrefName(BROWSER_SEARCH_PREF + "order"); >+ prefName += "." + j; You can hoist the getGeoSpecificPrefName call outside of the loop (optionally do some s/var/let/s here too).

Comment on attachment 8552659 [details] [diff] [review] regional-search-properties v0.2 The "North America" comment in region.properties isn't quite right since bug 1109120, so I would adjust that to match the "US market only" comment in mobile.js.

Comment on attachment 8552658 [details] [diff] [review] regional-default-search v0.2 Review of attachment 8552658 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with Gavin's comments addressed. ::: toolkit/components/search/nsSearchService.js @@ +445,5 @@ > + if (!getIsGeoSpecificDefaults()) > + return basepref; > + if (getIsUS()) > + return basepref + ".US"; > + return basepref; nit: trailing whitespace

Try run is looking green for desktop and android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4af35dedefa

Landed with Gavin and Mark comments addressed https://hg.mozilla.org/integration/fx-team/rev/1f51eb900e57 Leaving open to land the second patch after bug 1123980 lands

Updated the patch with gavin's comment about fixing the comment

Comment on attachment 8553994 [details] [diff] [review] regional-search-properties v0.3 Review of attachment 8553994 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me. Did you sort out the search activity issues?

(In reply to :Margaret Leibovic from comment #16) > Looks fine to me. Did you sort out the search activity issues? No. That's the next step... But before that, I see Try fialures for testSearchSuggestion: And trying a build with everything in place does not seem to be working as expected: 1. I don't see a browser.search.countryCode preference 2. I see a browser.search.isUS = false 3. The awesomescreen is still using Yahoo as the default, and showing search suggestions. If iUS = false, we should be using Google. 4. Looking at Settings > Customize > Search, I see the correct order: Google (default), Yahoo, Bing Here is a snippet of my logcat: I/Gecko (18024): *** Search: SearchService.init I/Gecko (18024): *** Search: metadata init: starting I/Gecko (18024): *** Search: metadata init: could not load JSON file Unix error 2 during operation open on file /data/data/org.mozilla.fennec_mfinkle/files/mozilla/zregh9y1.default/search-metadata.json (No such file or directory) I/Gecko (18024): *** Search: metadata init: complete I/Gecko (18024): *** Search: _asyncInit start I/Gecko (18024): *** Search: _asyncLoadEngines: start I/Gecko (18024): *** Search: _asyncFindJAREngines: looking for engines in JARs I/Gecko (18024): *** Search: _asyncLoadEngines: loading from cache directories I/Gecko (18024): *** Search: _loadEnginesFromCache: Loading from cache. 7 engines to load. I/Gecko (18024): *** Search: _addEngineToStore: Adding engine: "Amazon.com" I/Gecko (18024): *** Search: _addEngineToStore: Adding engine: "Google" I/Gecko (18024): *** Search: _addEngineToStore: Adding engine: "Twitter" I/Gecko (18024): *** Search: _addEngineToStore: Adding engine: "Wikipedia" I/Gecko (18024): *** Search: _addEngineToStore: Adding engine: "Yahoo" I/Gecko (18024): *** Search: _addEngineToStore: Adding engine: "Bing" I/Gecko (18024): *** Search: _addEngineToStore: Adding engine: "DuckDuckGo" I/Gecko (18024): *** Search: _asyncLoadEngines: done I/Gecko (18024): *** Search: _asyncInit: Completed _asyncInit I/Gecko (18024): *** Search: getVisibleEngines: getting all visible engines I/Gecko (18024): *** Search: _buildSortedEngineList: building list I/Gecko (18024): *** Search: getSubmission: In data: "__searchTerms__"; Purpose: "null" I/Gecko (18024): *** Search: getSubmission: Out data: "__searchTerms__" I/Gecko (18024): *** Search: getSubmission: In data: "dummy"; Purpose: "undefined" I/Gecko (18024): *** Search: getSubmission: Out data: "dummy" I/Gecko (18024): *** Search: getSubmission: In data: "dummy"; Purpose: "undefined" I/Gecko (18024): *** Search: getSubmission: Out data: "dummy" More eyes would be helpful

Oops. That link to the Try failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab61bcbd166

Comment on attachment 8553994 [details] [diff] [review] regional-search-properties v0.3 >diff --git a/mobile/locales/en-US/chrome/region.properties b/mobile/locales/en-US/chrome/region.properties > # Default search engine > browser.search.defaultenginename=Yahoo Did you intend to change this? >-browser.search.order.1=Yahoo >+browser.search.order.1=Google To match this? Not sure if that's related to your failures but it seems odd.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #19) > Not sure if that's related to your failures but it seems odd. Good catch. I am updating and will make a new Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1f3e261914a

We found a few problems in the previous patch: * defaultengine was still Yahoo and not Google * I copied pasted the ".US" overrides but failed to change the chrome:// URI to the correct location for mobile. * We had disabled the geoip.url which stopped fetching the countrycode. These are all corrected here and this seems to work correctly on my test devices. Let's see how the Try build works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=106a2f71142d

Comment on attachment 8556556 [details] [diff] [review] regional-search-properties v0.4 Review of attachment 8556556 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I missed those mistakes in the last review!

Try build looks OK https://hg.mozilla.org/integration/fx-team/rev/c28ca398479c

Unfortunately this creates issues: These 4 new keys are reported as missing by compare-locales, and the last thing we want/need is localizers trying to add them in their locales. We need to patch both mobile's filter.py to ignore these keys. Recent example: http://hg.mozilla.org/mozilla-central/rev/9b7ba1b01c72

Comment on attachment 8552658 [details] [diff] [review] regional-default-search v0.2 Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: Unable to ship Yahoo as default engine [Describe test coverage new/current, TreeHerder]: Manual QA turned up a few issues that have been patched. [Risks and why]: Working well on Nightly for a while. No search engine issues. [String/UUID change made/needed]: None

Comment on attachment 8556556 [details] [diff] [review] regional-search-properties v0.4 Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: Unable to ship Yahoo as default engine [Describe test coverage new/current, TreeHerder]: Manual QA turned up a few issues that have been patched. [Risks and why]: Working well on Nightly for a while. No search engine issues. [String/UUID change made/needed]: None

https://hg.mozilla.org/releases/mozilla-aurora/rev/eba7d8f6993a https://hg.mozilla.org/releases/mozilla-aurora/rev/c02f3626cfb9 https://hg.mozilla.org/releases/mozilla-beta/rev/a9eec576bb2c https://hg.mozilla.org/releases/mozilla-beta/rev/e1faea5d0942

Looks like I had a bad merge and missed removing a critical preference: https://hg.mozilla.org/releases/mozilla-beta/rev/ff6bff2f5694