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

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: 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

Comment 2

[Nick Alexander :nalexander [he/him]]

(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.

Comment 3

[Nick Alexander :nalexander [he/him]]

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).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 5

[Nick Alexander :nalexander [he/him]]

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 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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).

Comment 7

[Michael Shal [:mshal]]

https://reviewboard.mozilla.org/r/2777/#review2109

Comment 8

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/bd22b0ce1c15
https://hg.mozilla.org/integration/fx-team/rev/9747770f1ba0

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd22b0ce1c15
https://hg.mozilla.org/mozilla-central/rev/9747770f1ba0

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/cece31e1d159
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/beb045b96bc9

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/923404bb30ac
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/05befa69d765

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1123980 - Part 1: Handle common prefixes in .properties lists and dicts. r=gps

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1123980 - Part 2: DO NOT LAND - Write region-specific search settings into res/raw/browsersearch.json. r=mfinkle