Closed Bug 1123980 Opened 5 years ago Closed 5 years ago

Write geo-specific search engine metadata from region.properties to res/raw at build time

Categories

(Firefox for Android Graveyard :: Search Activity, defect)

All
Android
defect
Not set

Tracking

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

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

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(2 files, 1 obsolete file)

This ticket tracks implementing a geo-specific version of Bug 1065306.  We need to extract geo-specific search settings from region.properties and write (to res/raw/browsersearch.json, unless we have a strong reason to write to a new file).

The new settings have been set by Firefox for Desktop.
Component: Search → Search Activity
Product: Firefox → Firefox for Android
/r/2777 - Bug 1123980 - Part 1: Handle common prefixes in .properties lists and dicts. r=gps
/r/2779 - Bug 1123980 - Part 2: DO NOT LAND - Write geo-specific search settings into res/raw/browsersearch.json. r=mfinkle

Pull down these commits:

hg pull review -r 42155d5571c121736d512c43afdde1cbbd41b6a0
Attachment #8552101 - Flags: review?(mark.finkle)
Attachment #8552101 - Flags: review?(gps)
(In reply to Nick Alexander :nalexander from comment #1)
> Created attachment 8552101 [details]
> MozReview Request: bz://1123980/nalexander
> 
> /r/2777 - Bug 1123980 - Part 1: Handle common prefixes in .properties lists
> and dicts. r=gps
> /r/2779 - Bug 1123980 - Part 2: DO NOT LAND - Write geo-specific search
> settings into res/raw/browsersearch.json. r=mfinkle
> 
> Pull down these commits:
> 
> hg pull review -r 42155d5571c121736d512c43afdde1cbbd41b6a0

mfinkle: with these patches, I find:

chocho:gecko nalexander$ ./mach build mobile/android/base/locales
 0:00.21 /usr/bin/make -C /Users/nalexander/Mozilla/gecko/objdir-droid -j8 -s backend.RecursiveMakeBackend
 0:00.70 /usr/bin/make -C mobile/android/base/locales -j8 -s
 0:00.76 Read 3 engines: [u'Google', u'Yahoo', u'Bing']
 0:00.76 Default engine is 'Yahoo'.
 0:00.76 Geo 'US': Read 3 engines: [u'Yahoo', u'Google', u'Bing']
 0:00.76 Geo 'US': Default engine is 'Yahoo'.
 0:00.76 /Users/nalexander/Mozilla/gecko/objdir-droid/mobile/android/base/res/raw/browsersearch.json already up-to-date
 0:00.84 Reading 4 suggested sites: [u'mozilla', u'fxmarketplace', u'fxaddons', u'fxsupport']
 0:00.84 Found 4 drawables in '/Users/nalexander/Mozilla/gecko/mobile/android/base/resources' for 'mozilla': ['drawable-hdpi/suggestedsites_mozilla.png', 'drawable-mdpi/suggestedsites_mozilla.png', 'drawable-xhdpi/suggestedsites_mozilla.png', 'drawable-xxhdpi/suggestedsites_mozilla.png']
 0:00.84 Found 4 drawables in '/Users/nalexander/Mozilla/gecko/mobile/android/base/resources' for 'fxmarketplace': ['drawable-hdpi/suggestedsites_fxmarketplace.png', 'drawable-mdpi/suggestedsites_fxmarketplace.png', 'drawable-xhdpi/suggestedsites_fxmarketplace.png', 'drawable-xxhdpi/suggestedsites_fxmarketplace.png']
 0:00.84 Found 4 drawables in '/Users/nalexander/Mozilla/gecko/mobile/android/base/resources' for 'fxaddons': ['drawable-hdpi/suggestedsites_fxaddons.png', 'drawable-mdpi/suggestedsites_fxaddons.png', 'drawable-xhdpi/suggestedsites_fxaddons.png', 'drawable-xxhdpi/suggestedsites_fxaddons.png']
 0:00.84 Found 4 drawables in '/Users/nalexander/Mozilla/gecko/mobile/android/base/resources' for 'fxsupport': ['drawable-hdpi/suggestedsites_fxsupport.png', 'drawable-mdpi/suggestedsites_fxsupport.png', 'drawable-xhdpi/suggestedsites_fxsupport.png', 'drawable-xxhdpi/suggestedsites_fxsupport.png']
 0:00.84 /Users/nalexander/Mozilla/gecko/objdir-droid/mobile/android/base/res/raw/suggestedsites.json already up-to-date
 0:00.93 Your build was successful!

You can clearly see the geo-specific settings are processed.  The resulting browsersearch.json looks like:

{"default": "Yahoo", "geos": {"US": {"default": "Yahoo", "engines": ["Yahoo", "Google", "Bing"]}}, "engines": ["Google", "Yahoo", "Bing"]}

This is 100% Android, so we can rework it; but we pretty much need to keep "default" and "engines" the same, since we're trying to uplift pretty far.
mfinkle: there's an additional question of whether these geo-specific-search settings should be tangled with the localization flow at all.  Right now, the script takes the union of the en-US (in tree) region.properties, overlays any localized (out of tree) region.properties, and then does its business.  That means localizers in en-GB can change the experience they see when in the geo code US.  That's either good (give me the best Spanish language search engine in Mexico!) or bad (never show a particular engine to a particular locale, no matter where they are).
Flags: needinfo?(mark.finkle)
https://reviewboard.mozilla.org/r/2779/#review2025

The Python changes look OK to me in general. Desktop is very US-centric right now, so adding support for regions in general will be more work everywhere.

One request: Could you rename 'geo' and 'geos' to 'region' and 'regions' in the JSON and Python?
Flags: needinfo?(mark.finkle)
Comment on attachment 8552101 [details]
MozReview Request: bz://1123980/nalexander

/r/2777 - Bug 1123980 - Part 1: Handle common prefixes in .properties lists and dicts. r=gps
/r/2779 - Bug 1123980 - Part 2: DO NOT LAND - Write region-specific search settings into res/raw/browsersearch.json. r=mfinkle

Pull down these commits:

hg pull review -r 30a52dcb3e96842a8ed17ce1976f33e5a2e1b413
Comment on attachment 8552101 [details]
MozReview Request: bz://1123980/nalexander

https://reviewboard.mozilla.org/r/2775/#review2079

This all looks good to me. You mentioned adding some tests for get_dict where no prefixed items were given (i.e. the case where no 'US' search overrides were supplied).
Attachment #8552101 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/bd22b0ce1c15
https://hg.mozilla.org/mozilla-central/rev/9747770f1ba0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8552101 - Flags: review?(gps)
Comment on attachment 8552101 [details]
MozReview Request: bz://1123980/nalexander

Approval Request Comment
[Feature/regressing bug #]: Needed for bug 1117186 and bug 1129576
[User impact if declined]: Tree bustage
[Describe test coverage new/current, TreeHerder]: Working fine on Nightly
[Risks and why]: Low. Only appends the JSON file used for the Search Activity
[String/UUID change made/needed]: None
Attachment #8552101 - Flags: approval-mozilla-beta?
Attachment #8552101 - Flags: approval-mozilla-aurora?
Attachment #8552101 - Flags: approval-mozilla-beta?
Attachment #8552101 - Flags: approval-mozilla-beta+
Attachment #8552101 - Flags: approval-mozilla-aurora?
Attachment #8552101 - Flags: approval-mozilla-aurora+
Attachment #8552101 - Attachment is obsolete: true
Attachment #8619184 - Flags: review+
Attachment #8619185 - Flags: review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.