Closed Bug 1108627 Opened 5 years ago Closed 5 years ago

Regression: market-specific search defaults (desktop) broke default engine behavior for Fennec

Categories

(Firefox for Android :: Locale switching and selection, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 37
Tracking Status
firefox34 + verified
firefox35 + verified
firefox36 + verified
firefox37 + verified
fennec 34+ ---

People

(Reporter: cos_flaviu, Assigned: Gavin)

References

Details

(Keywords: regression, reproducible)

Attachments

(1 file)

Environment: 
Device: Samsung Galaxy S4 (Android 4.4.2);
Build: Nightly 37.0a1 (2014-12-08);

Steps to reproduce:
1. Go to device 'Date and Time' settings.
2. Manually change the timezone to GMT-8:00 (Pacific Standard Time);
3. Launch Fennec with a clean profile;
4. Tap on the URL bar and search for a therm (e.g. "mozilla");
5. Observe that the search is made with Yahoo search engine;
6. Go to Settings -> Customize -> Search.

Expected result:
The default search engine is Yahoo.

Actual result:
The default search engine appears to be Amazon.com even if the search results from step 5 are  from Yahoo.
Changing the timezone has no relevance. I can reproduce this regardless.

It looks like amazon.com is claimed as default but is not actually the default.
tracking-fennec: --- → ?
Summary: Amazon.com appears as default search engine after changing the timezone to GMT -8:00 → en-US build Amazon.com appears as default search engine after a search
No search required.

[Tracking Requested - why for this release]: Listed search default not accurate; not actually default though
Summary: en-US build Amazon.com appears as default search engine after a search → Regression: en-US build specifies Amazon.com as default
Flags: in-testsuite?
Flags: in-moztrap?(fennec)
Well it does have something related to the timezone, because setting the timezone to GMT +2:00(my locale) will display the correct default search engine.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cef590a6f946&tochange=17de0f463944

Going with bug 1102416

gsharp@mozilla.com
Fri Nov 28 19:01:04 2014 +0000	17de0f463944	Gavin Sharp — Bug 1102416: make Yahoo the default search plugin for en-US in American time zones, r=dolske, a=me
Blocks: 1102416
Summary: Regression: en-US build specifies Amazon.com as default → Regression: en-US build specifies Amazon.com as default; on timezone change
[Tracking Requested - why for this release]:

My patch for bug 1102416 broke this in Fennec. We may want to fix this in Firefox 34 (Fennec-only).
Nightly (12/08) under new profiles e.g, 

EST (Toronto)

  Nightly specifies Amazon.com (default); alphabetical order - actual default when searching is Yahoo.
 
EEST (Cairo)

  Nightly specifies Yahoo (default), actual default when searching is Yahoo
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #7)
> Pushed a possible patch to Try:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1bb27213c727

With this build I see the correct specified default in our search settings
Attached patch patchSplinter Review
The core issue is that Fennec uses the search service in different ways than Firefox, and so bug 1102416 comment 5 doesn't apply.

This effectively makes bug 1102416 desktop-only by adding a pref check to the special getIsUS() function, and only setting that pref for desktop. This is fine because Fennec doesn't need market-specific defaults.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=122ef127a4af
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #8533361 - Flags: review?(dolske)
Comment on attachment 8533361 [details] [diff] [review]
patch

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

r+, but I really think it would be better to go with some code structure that doesn't assume "false" is the safe default for getIsUS(), which is a magic footgun.

As one idea, create a wrapper/helper as such:

 function getGeoSpecificPrefName(basepref) {
   ...
   if (!geoSpecificDefaults)
     return basepref;
   if (getIsUS())
     return basepref + "US.";
   return basepref; 
 }

This also might be useful if we're considering non-US geodefaults in the future.
Attachment #8533361 - Flags: review?(dolske) → review+
(In reply to Justin Dolske [:Dolske] from comment #10)
> r+, but I really think it would be better to go with some code structure
> that doesn't assume "false" is the safe default for getIsUS(), which is a
> magic footgun.

Yeah I agree. I am going to file a bug on a cleaner solution for this, but I want to avoid making too many changes for 34.*.
Summary: Regression: en-US build specifies Amazon.com as default; on timezone change → Regression: market-specific search defaults (desktop) broke default engine behavior for Fennec
Comment on attachment 8533361 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1102416
[User impact if declined]: comment 0
[Describe test coverage new/current, TBPL]: Fennec coverage lacking
[Risks and why]: low risk, since it's effectively a straight backout of bug 1102416, but for Fennec only
[String/UUID change made/needed]: none
Attachment #8533361 - Flags: approval-mozilla-release?
Attachment #8533361 - Flags: approval-mozilla-beta?
Attachment #8533361 - Flags: approval-mozilla-aurora?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #9)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=122ef127a4af

Aaron/Flaviu, can you test this build?
Flags: needinfo?(flaviu.cos)
Flags: needinfo?(aaron.train)
Hmm, I think that Android build didn't show up for some reason. Trying again:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5561771683e2
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #14)
> Hmm, I think that Android build didn't show up for some reason. Trying again:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5561771683e2

You need to change your trychooser syntax.

https://gbrownmozilla.wordpress.com/2014/12/07/new-android-job-names-on-treeherder-update-your-try-pushes/
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17)
> Pushing beta to try is broken. Let's try once again:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8f0a1c9ffb1b

Tested this build yesterday and again today, on new profile we get the correctly listed search default with the aforementioned timezone changes
Flags: needinfo?(aaron.train)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17)
> Pushing beta to try is broken. Let's try once again:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8f0a1c9ffb1b

Verified on multiple time zones and it works as expected.
Flags: needinfo?(flaviu.cos)
(In reply to Flaviu Cos, QA [:flaviu] from comment #19)
> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17)
> > Pushing beta to try is broken. Let's try once again:
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8f0a1c9ffb1b
> 
> Verified on multiple time zones and it works as expected.

\o/

let's try to get this landed and uplifted ASAP
tracking-fennec: ? → 34+
Comment on attachment 8533361 [details] [diff] [review]
patch

given the verifications, let's get this into today's beta - which is not how we usually do things but better to get the extra week of testing with the larger pop.
Attachment #8533361 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8533361 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/b94f28405aeb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Verified as fixed on Firefox 35 Beta 4 on Samsung Galaxy Nexus (Android 4.2.1)
Comment on attachment 8533361 [details] [diff] [review]
patch

Approved for Firefox for Android 34.0.1.
Attachment #8533361 - Flags: approval-mozilla-release? → approval-mozilla-release+
Verified as fixed on Firefox 34.0.1 and on latest Aurora, on Nexus 5(Android 5.0).
I filed bug 1117186 on fixing this properly for Fennec.
Verified as fixed in Firefox for Android 37.0a1 (2015-01-07)
Device: Samsung Galaxy S4 (Android 4.4.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.