Closed
Bug 1108627
Opened 9 years ago
Closed 9 years ago
Regression: market-specific search defaults (desktop) broke default engine behavior for Fennec
Categories
(Firefox for Android Graveyard :: Locale switching and selection, defect)
Firefox for Android Graveyard
Locale switching and selection
ARM
Android
Tracking
(firefox34+ verified, firefox35+ verified, firefox36+ verified, firefox37+ verified, fennec34+)
People
(Reporter: cos_flaviu, Assigned: Gavin)
References
Details
(Keywords: regression, reproducible)
Attachments
(1 file)
2.72 KB,
patch
|
Dolske
:
review+
Sylvestre
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
lmandel
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
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: --- → ?
status-firefox35:
--- → ?
status-firefox36:
--- → ?
status-firefox37:
--- → affected
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
Updated•9 years ago
|
Reporter | ||
Updated•9 years ago
|
Comment 2•9 years ago
|
||
No search required. [Tracking Requested - why for this release]: Listed search default not accurate; not actually default though
tracking-firefox35:
--- → ?
Summary: en-US build Amazon.com appears as default search engine after a search → Regression: en-US build specifies Amazon.com as default
Updated•9 years ago
|
Flags: in-testsuite?
Flags: in-moztrap?(fennec)
Reporter | ||
Comment 3•9 years ago
|
||
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.
Comment 4•9 years ago
|
||
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
Keywords: regressionwindow-wanted
Summary: Regression: en-US build specifies Amazon.com as default → Regression: en-US build specifies Amazon.com as default; on timezone change
Assignee | ||
Comment 5•9 years ago
|
||
[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).
tracking-firefox34:
--- → ?
Comment 6•9 years ago
|
||
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
Assignee | ||
Comment 7•9 years ago
|
||
Pushed a possible patch to Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1bb27213c727
Comment 8•9 years ago
|
||
(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
Updated•9 years ago
|
tracking-firefox36:
--- → +
tracking-firefox37:
--- → +
Assignee | ||
Comment 9•9 years ago
|
||
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
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
(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.*.
Updated•9 years ago
|
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
Assignee | ||
Comment 12•9 years ago
|
||
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?
Assignee | ||
Comment 13•9 years ago
|
||
(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)
Assignee | ||
Comment 14•9 years ago
|
||
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
Comment 15•9 years ago
|
||
(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/
Assignee | ||
Comment 16•9 years ago
|
||
New push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0dbaf5e11391
Assignee | ||
Comment 17•9 years ago
|
||
Pushing beta to try is broken. Let's try once again: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8f0a1c9ffb1b
Comment 18•9 years ago
|
||
(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)
Reporter | ||
Comment 19•9 years ago
|
||
(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)
Comment 20•9 years ago
|
||
(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 21•9 years ago
|
||
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+
Assignee | ||
Comment 22•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/46b84b0fb7a9
Updated•9 years ago
|
Attachment #8533361 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b94f28405aeb https://hg.mozilla.org/releases/mozilla-aurora/rev/7c581a020aa6
Comment 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b94f28405aeb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment 25•9 years ago
|
||
Verified as fixed on Firefox 35 Beta 4 on Samsung Galaxy Nexus (Android 4.2.1)
Comment 26•9 years ago
|
||
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+
Comment 28•9 years ago
|
||
Verified as fixed on Firefox 34.0.1 and on latest Aurora, on Nexus 5(Android 5.0).
Assignee | ||
Comment 29•9 years ago
|
||
I filed bug 1117186 on fixing this properly for Fennec.
Comment 30•9 years ago
|
||
Verified as fixed in Firefox for Android 37.0a1 (2015-01-07) Device: Samsung Galaxy S4 (Android 4.4.2)
Status: RESOLVED → VERIFIED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•