Closed Bug 1117186 Opened 9 years ago Closed 9 years ago

fix geo-specific search defaults interaction with the defaultEngine getter

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: Gavin, Assigned: mfinkle)

References

Details

Attachments

(2 files, 4 obsolete files)

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?
Attached patch regional-default-search v0.1 (obsolete) — Splinter Review
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
Assignee: nobody → mark.finkle
Attached patch regional-search-properties v0.1 (obsolete) — Splinter Review
Update the preferences so we use the US-only search defaults when appropriate.
Attachment #8551991 - Flags: feedback?(mhammond)
Blocks: 1123880
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.
Attachment #8551991 - Flags: feedback?(mhammond) → feedback+
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.
Attachment #8551991 - Attachment is obsolete: true
Attachment #8552658 - Flags: review?(mhammond)
Attached patch regional-search-properties v0.2 (obsolete) — Splinter Review
Fixed a typo in this patch
Attachment #8551992 - Attachment is obsolete: true
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).
Attachment #8552658 - Flags: feedback+
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
Attachment #8552658 - Flags: review?(mhammond) → review+
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
Keywords: leave-open
Attached patch regional-search-properties v0.3 (obsolete) — Splinter Review
Updated the patch with gavin's comment about fixing the comment
Attachment #8552659 - Attachment is obsolete: true
Attachment #8553994 - Flags: review?(margaret.leibovic)
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?
Attachment #8553994 - Flags: review?(margaret.leibovic) → review+
(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
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
Attachment #8553994 - Attachment is obsolete: true
Attachment #8556556 - Flags: review?(margaret.leibovic)
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!
Attachment #8556556 - Flags: review?(margaret.leibovic) → review+
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
Depends on: 1128991
Depends on: 1129533
Depends on: 1129554
Depends on: 1129570
Depends on: 1129576
Depends on: 1129607
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
Attachment #8552658 - Flags: approval-mozilla-beta?
Attachment #8552658 - Flags: approval-mozilla-aurora?
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
Attachment #8556556 - Flags: approval-mozilla-beta?
Attachment #8556556 - Flags: approval-mozilla-aurora?
Attachment #8552658 - Flags: approval-mozilla-beta?
Attachment #8552658 - Flags: approval-mozilla-beta+
Attachment #8552658 - Flags: approval-mozilla-aurora?
Attachment #8552658 - Flags: approval-mozilla-aurora+
Attachment #8556556 - Flags: approval-mozilla-beta?
Attachment #8556556 - Flags: approval-mozilla-beta+
Attachment #8556556 - Flags: approval-mozilla-aurora?
Attachment #8556556 - Flags: approval-mozilla-aurora+
Depends on: 1131163
Looks like I had a bad merge and missed removing a critical preference:
https://hg.mozilla.org/releases/mozilla-beta/rev/ff6bff2f5694
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1132089
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: