Closed Bug 1276739 Opened 8 years ago Closed 8 years ago

replace list.txt with a region-aware JSON file format to allow different search configs within a single locale

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox52 --- verified

People

(Reporter: mconnor, Assigned: mkaply)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch][partner search])

Attachments

(2 files, 11 obsolete files)

36.57 KB, patch
glandium
: feedback+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
florian
: review+
Details
Language is a poor substitute for region, so many users get poor default sets.  en-US in particular is used in many territories around the world, but any provider that doesn't do server geo-detection gives the user US content.  This is less than useful for those users.

We should replace list.txt with a JSON-based file that handles the following:

* engines to ship for the locale
* default and visible engines (including deterministic ordering) for 1-N regions as appropriate, with a default region as a fallback.

Ideally this would be a signed/verifiable file that would be used to constrain loading of app directory files.

This would be used to select the engines to include for that locale at build time, and used by Firefox to apply the correct settings based on the location of the install (using the existing MLS lookup mechanism)

This would remove our hard dependency on absearch to set international defaults, and allow it to be an override/test service only.
Blocks: 1276743
Blocks: 1276745
Attached patch First pass (obsolete) — Splinter Review
This is my first pass, so it's a little rough. I've moved the engines to toolkit/components/search/searchplugins.

I've made one change from the original design; we generate the list of shippedEngines using the visible engine array that we already have. This removes the chance of adding an engine to one list and forgetting to put it in the other. This was based on a suggestion from florian.

The way the build works is that searchplugins.py parses the JSON file and generates a list.txt file that is only used for adding engines to the build. So list.txt exists, but only for build. (We might want to rename it so there is no confusion.)

Any tests that specifically relied on list.txt have been updated and all are passing except for test_defaultEngine.js and test_json_cache.js. I'm still debugging those.

nsBrowserSearch has both list.txt and list.json support. The way I did the XHR error handling is a little strange and I might revisit. But basically if I fail to get the JSON, I try for the txt. And if I fail to JSON.parse what was passed in, I try to use the old list.txt mechanism. This will allow everything non browser to continue to work while we migrate.

The way the concept of hidden engines works is that I create a list based on all the other locales with everything hidden, then read the default locale and make those visible. The tests are passing.

Things that don't work:

1. Artifact builds. These have a special path in jar.mn that I will probably update to include specific search engines by name (versus the current code which would end up including every search engine).

2. The tests that I already indicated.

Florian: I'd love your opinion on the test_json_cache.js failure. It's not making a lot of sense to me.
Attachment #8758391 - Flags: feedback?(florian)
Much of this is packaging and build, CCing :glandium as we'll need his input.
Apply this one line patch as well.
Attached patch Updated patch (obsolete) — Splinter Review
Updated patch that moves the engines back to en-US (per the first phase).

Also fixes the test_hidden error.

Test problems with:

toolkit/components/search/tests/xpcshell/test_json_cache.js
-----------------------------------------------------------
FAIL [Parent]
toolkit/components/search/tests/xpcshell/test_searchReset.js
------------------------------------------------------------
FAIL [Parent]
toolkit/components/search/tests/xpcshell/test_selectedEngine.js
---------------------------------------------------------------
FAIL [Parent]

Seems related to metadata in at least the first and last case.
Attachment #8758391 - Attachment is obsolete: true
Attachment #8758399 - Attachment is obsolete: true
Attachment #8758391 - Flags: feedback?(florian)
Attachment #8758419 - Flags: review?(florian)
Comment on attachment 8758419 [details] [diff] [review]
Updated patch

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

::: browser/locales/en-US.json
@@ +2,5 @@
> +  "settings": {
> +    "default": {
> +        "default": "google",
> +        "visibleDefaultEngines": [
> +              "google", "yahoo", "bing", "amazondotcom", "eBay", "twitter", "wikipedia"

Where is ddg?

@@ +3,5 @@
> +    "default": {
> +        "default": "google",
> +        "visibleDefaultEngines": [
> +              "google", "yahoo", "bing", "amazondotcom", "eBay", "twitter", "wikipedia"
> +          ]

nit: wrong indent.

@@ +6,5 @@
> +              "google", "yahoo", "bing", "amazondotcom", "eBay", "twitter", "wikipedia"
> +          ]
> +    },
> +    "US": {
> +        "default": "yahoo-en-US",

shouldn't we reject such a config file where the "default" value isn't in the set of "visibleDefaultEngines"?

@@ +12,5 @@
> +              "yahoo", "google-nocodes", "bing", "amazondotcom", "eBay", "twitter", "wikipedia"
> +        ]
> +    },
> +    "CA": {
> +        "default": "yahoo-en-CA",

Is this currently our default in Canada?

::: browser/locales/searchplugins.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +from __future__ import print_function

is this used?

::: toolkit/components/search/nsSearchService.js
@@ +3653,5 @@
>        throw new Task.Result(uris);
>      }.bind(this));
>    },
>  
> +  _parseList: function SRCH_SVC_parseListTxt(list, uris) {

I really dislike this function that's actually 2 different functions in one, with a big try/catch wrapping it, and eating all the exceptions/errors the JSON version may cause. Is there a reason why having separate _parseListTxt and _parseListJSON functions wouldn't work?

@@ +3658,5 @@
>      let engineNames;
> +    try {
> +      let searchSettings = JSON.parse(list);
> +
> +      let jarNames = new Map();

It looks like this could be a Set rather than a Map for the JSON case, as you never use the values.

@@ +3661,5 @@
> +
> +      let jarNames = new Map();
> +      for (let region in searchSettings["settings"]) {
> +        if (region == "default") {
> +          continue;

Looks to me like the "default" case can also be handled by this loop.

@@ +3663,5 @@
> +      for (let region in searchSettings["settings"]) {
> +        if (region == "default") {
> +          continue;
> +        }
> +        let engineList =  searchSettings["settings"][region]["visibleDefaultEngines"];

nit: double space

@@ +3664,5 @@
> +        if (region == "default") {
> +          continue;
> +        }
> +        let engineList =  searchSettings["settings"][region]["visibleDefaultEngines"];
> +        for (var i=0; i < engineList.length; i++) {

nit: spaces around '='... but actually:
for (let engine of engineList) {

@@ +3670,4 @@
>          }
>        }
> +      let engineList = searchSettings["settings"]["default"]["visibleDefaultEngines"];
> +      for (var i=0; i < engineList.length; i++) {

same here

@@ +3673,5 @@
> +      for (var i=0; i < engineList.length; i++) {
> +        jarNames.set(engineList[i], false);
> +      }
> +
> +      // Check if we have a useable country specific list of visible default engines.

This whole block shouldn't be duplicated. Can it become a helper method?

@@ +3685,5 @@
> +          // don't ship indicates the server is misconfigured to answer requests
> +          // from the specific Firefox version we are running, so ignoring the
> +          // value altogether is safer.
> +          if (!jarNames.has(engineName)) {
> +            LOG("_parseListTxt: ignoring visibleDefaultEngines value because " +

If you change the function name, this message needs to be updated.

@@ +3699,5 @@
> +        let region;
> +        if (Services.prefs.prefHasUserValue("browser.search.region")) {
> +          region = Services.prefs.getCharPref("browser.search.region");
> +        }
> +        if (region && region in searchSettings["settings"]) {

if (!region || !(region in ...))
  region = "default";
and then the duplicated next 2 lines can be merged.

::: toolkit/components/search/tests/xpcshell/data/list.json
@@ +1,4 @@
> +{
> +  "settings": {
> +    "default": {
> +        "visibleDefaultEngines": [

Does it make sense to have a list.json file that defines the list of visible engines but not the default engine?

@@ +2,5 @@
> +  "settings": {
> +    "default": {
> +        "visibleDefaultEngines": [
> +              "engine", "engine-pref", "engine-rel-searchform-purpose", "engine-system-purpose"
> +          ]

nit: indent

::: toolkit/components/search/tests/xpcshell/test_json_cache.js
@@ +70,4 @@
>        continue;
> +    }
> +    let engineList =  searchSettings["settings"][region]["visibleDefaultEngines"];
> +    for (var i=0; i < engineList.length; i++) {

for ... of
Attachment #8758419 - Flags: review?(florian) → feedback+
Thanks for the feedback.

I am planning on adding proper default support. Working on that now.
Whiteboard: [fxsearch]
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Where is ddg?

Fixed

> shouldn't we reject such a config file where the "default" value isn't in
> the set of "visibleDefaultEngines"?

I'll add that with default engine support.

> > +    "CA": {
> > +        "default": "yahoo-en-CA",
> 
> Is this currently our default in Canada?

As far as I know, yes.

> I really dislike this function that's actually 2 different functions in one,
> with a big try/catch wrapping it, and eating all the exceptions/errors the
> JSON version may cause. Is there a reason why having separate _parseListTxt
> and _parseListJSON functions wouldn't work?

Good idea. And would have made my debugging a lot easier :)

> It looks like this could be a Set rather than a Map for the JSON case, as
> you never use the values.

I do use the values to build the list of hidden engines.

> > +      for (let region in searchSettings["settings"]) {
> > +        if (region == "default") {
> > +          continue;
> 
> Looks to me like the "default" case can also be handled by this loop.

The problem is the default case has to be done last because it can clash with engines that were set as hidden. So the only way I could see to do that was to process default after the loop.



> > +{
> > +  "settings": {
> > +    "default": {
> > +        "visibleDefaultEngines": [
> 
> Does it make sense to have a list.json file that defines the list of visible
> engines but not the default engine?

No. That will end up being mandatory.
Can we make this even simpler and have the first engine in the list be the default engine?
(In reply to Mike Kaply [:mkaply] from comment #7)

> > It looks like this could be a Set rather than a Map for the JSON case, as
> > you never use the values.
> 
> I do use the values to build the list of hidden engines.

The only usage I see is "if (!jarNames.has(engineName)) {"

The values are used in the list.txt case. My comment was about the list.json branch of the code.
(In reply to Mike Kaply [:mkaply] from comment #8)
> Can we make this even simpler and have the first engine in the list be the
> default engine?

Maybe. Is the order in the visibleDefaultEngines array supposed to be the order in which the engines will be shown to users?
> The values are used in the list.txt case. My comment was about the list.json branch of the code.

It's used after the two if statements:

+        for (let [name, hidden] of jarNames) {
+          if (!hidden)
+            engineNames.push(name);
+        }

It will be more clear in the new patch.

> Maybe. Is the order in the visibleDefaultEngines array supposed to be the order in which the engines will be shown to users?

That's my plan. I'd love to get rid of browser.search.order and all of the alphabetization stuff in search service. Do we know why they are alphabetized in the list?
(In reply to Mike Kaply [:mkaply] from comment #11)
> Do we know why they are alphabetized in the list?

Do you mean in the UI / by the search service? I guess it's just the fallback that makes the most sense when there's no human-defined order.

If you mean inside the visibleDefaultEngines existing configurations, I think it was just for easier readability.
This patch addresses Florian's comments. Still working on tests.

You asked about an invalid default engine in the JSON file. The default right now is specified as a proper name (Google), whereas the list of engines is by name (alias?), so that's not easy to do right now.

I'd love to move to a model where we can use the non proper name to set the default, but I've run into other issues. Specifically that aliases do not map to filenames automatically.

Alternatively, the Firefox code is smart enough to use the first engine as the default engine, so if we remove the alphabetical stuff and use the order in the file, the first engine will be the default engine...

As far as tests go, most are working, but any tests that involve the metadata from the cache aren't working. I haven't been able to figure out why.
Attachment #8759310 - Flags: feedback?(florian)
Comment on attachment 8759310 [details] [diff] [review]
Patch with comments addressed and default engine

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

::: browser/locales/en-US.json
@@ +9,5 @@
> +    "US": {
> +      "searchDefault": "Yahoo",
> +        "visibleDefaultEngines": [
> +          "yahoo", "google-nocodes", "bing", "amazondotcom", "ddg", "eBay", "twitter", "wikipedia"
> +        ]

indent still wrong.

::: toolkit/components/search/nsSearchService.js
@@ +3603,5 @@
>      let sis = Cc["@mozilla.org/scriptableinputstream;1"].
>                  createInstance(Ci.nsIScriptableInputStream);
> +    try {
> +      sis.init(chan.open2());
> +      this._parseListJSON(sis.read(sis.available()), uris);

If _parseListJSON causes an exception, it'll be eaten by the big try/catch it contains, and we will end up returning an empty uris array, and having a completely broken search service initialization.

Also, if the list.json file exists and we fail to parse it, I don't think falling back to list.txt is useful.

@@ +3659,5 @@
>      }.bind(this));
>    },
>  
> +  _parseListJSON: function SRCH_SVC_parseListJSON(list, uris) {
> +    try {

Why do we need such a big try/catch? Which part of the function is expected to sometimes fail?

@@ +3662,5 @@
> +  _parseListJSON: function SRCH_SVC_parseListJSON(list, uris) {
> +    try {
> +      let searchSettings = JSON.parse(list);
> +
> +      let jarNames = new Map();

this could be a Set, the true/false values this map contains are still not used, despite comment 11.

::: toolkit/components/search/tests/xpcshell/data/list.json
@@ +2,5 @@
> +  "settings": {
> +    "default": {
> +        "visibleDefaultEngines": [
> +              "engine", "engine-pref", "engine-rel-searchform-purpose", "engine-system-purpose"
> +          ]

broken indent.
Attachment #8759310 - Flags: feedback?(florian)
Attached patch Updated patch (obsolete) — Splinter Review
Latest patch addressing review comments and getting most tests passing.

The two tests left that fail (test_json_cache.js, test_geodefaults.js) need to be evaluated because they rely on behavior that isn't necessarily true anymore.

Some open issues:

1. If browser.search.geoSpecificDefaults is set to false, should we always use the "default" in the JSON file?

2. Should we remove the separate default engine concept and just use the first engine in the list?

3. Should we get rid of the sorted engine concept completely in the search service and use the order in the JSON file? There's a lot of code related to sorted engines that could be removed.
Attachment #8761311 - Flags: review?(florian)
I figured out test_json_cache and fixed it.

Only remaining item is test_geodefaults.js which I think I'll need florian's help to understand/rewrite.
Comment on attachment 8761311 [details] [diff] [review]
Updated patch

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

The changes to test_location_funnelcake.js and test_location_partner.js are unfinished, right?

(In reply to Mike Kaply [:mkaply] from comment #15)

> Some open issues:
> 
> 1. If browser.search.geoSpecificDefaults is set to false, should we always
> use the "default" in the JSON file?

Given the name of the pref, the answer seems to be 'yes'. But I suspect that pref was more meant to be about fetching geo info from a server, so the answer may still be 'no'.

> 2. Should we remove the separate default engine concept and just use the
> first engine in the list?

Would be nice; if this doesn't cause complications that I don't see now.

> 3. Should we get rid of the sorted engine concept completely in the search
> service and use the order in the JSON file? There's a lot of code related to
> sorted engines that could be removed.

Completely, no as we want to let users keep reordering engines from the preferences UI.

But getting rid of the sorting code that touches preferences would be nice.

::: browser/locales/jar.mn
@@ +99,2 @@
>      locale/browser/searchplugins/               (.deps/generated_@AB_CD@/*.xml)
> +    locale/browser/searchplugins/list.json      (@AB_CD@.json)

This requires us having such a .json file in the tree for each locale, right? If so, I think they need to be in a subfolder, as I don't see us adding dozens of files directly in browser/locales/

::: browser/themes/windows/searchbar.css
@@ +25,5 @@
>  
>  .searchbar-engine-image {
>    height: 16px;
>    width: 16px;
> +  list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");

This change is from the other bug.

::: toolkit/components/search/nsSearchService.js
@@ -2985,5 @@
>                         otherDirs.some(d => !cacheOtherPaths.has(d.path)) ||
>                         cache.visibleDefaultEngines.length != this._visibleDefaultEngines.length ||
>                         this._visibleDefaultEngines.some(notInCacheVisibleEngines) ||
>                         otherDirs.some(modifiedDir);
> -

Please revert.

@@ -3112,5 @@
>                           otherDirs.some(d => !cacheOtherPaths.has(d.path)) ||
>                           cache.visibleDefaultEngines.length != this._visibleDefaultEngines.length ||
>                           this._visibleDefaultEngines.some(notInCacheVisibleEngines) ||
>                           (yield checkForSyncCompletion(hasModifiedDir(otherDirs)));
> -

Here too.

@@ +3665,5 @@
> +    try {
> +      searchSettings = JSON.parse(list);
> +    } catch(e) {
> +      Cu.reportError(e);
> +      // What should we do here? We have nothing to fall back to...

I guess the only thing we can do is return early to avoid further errors :-/.
Add a LOG("failing to parse list.json: " + e);

@@ +3669,5 @@
> +      // What should we do here? We have nothing to fall back to...
> +    }
> +
> +    let jarNames = new Set();
> +    if (!isPartnerBuild()) {

Why are partner builds ignoring region specific default settings, rather than later overriding them? This may need a comment.

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +289,1 @@
>    if (isUS) {

let region = isUS ? "US" : "default"; and then merged the 2 following duplicated lines into one.

::: toolkit/components/search/tests/xpcshell/test_json_cache.js
@@ +65,5 @@
>    let list = sis.read(sis.available());
> +  let searchSettings = JSON.parse(list);
> +
> +  for (let region in searchSettings["settings"]) {
> +    if (region == "default") {

could you remove "default" from the array and push it at the end?

@@ +77,2 @@
>    }
> +  for (let engine of searchSettings["settings"]["default"]["visibleDefaultEngines"]) {

Then this duplicated code would disappear.
Attachment #8761311 - Flags: review?(florian) → feedback+
Attached patch Final patch (obsolete) — Splinter Review
After working on this a little more, I realized that setting the default engine via JSON should not be a part of this patch. It'a making it more complicated (and breaking tests).

There are three separate things that need to be done:

1. Convert to JSON
2. Get rid of search order in service (and browser.order prefs).
3. Move default engine to JSON.

This patch is for 1.

Right now en-US is hardcoded for using this code. I've built the old way with locales and they work fine.

Once I create JSON files for all the locales, I will update the Makefile.in, jar.mn and moz.build.

I'm investigating checking for the presence of the AB_CD.json file as part of the build so that we won't need the if statements.
Attachment #8758419 - Attachment is obsolete: true
Attachment #8759310 - Attachment is obsolete: true
Attachment #8761311 - Attachment is obsolete: true
Attachment #8762522 - Flags: review?(florian)
Comment on attachment 8762522 [details] [diff] [review]
Final patch

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

Sorry for the delay...

::: browser/locales/jar.mn
@@ +97,4 @@
>      locale/browser/searchplugins/               (%searchplugins/*.xml)
>  #else
> +    locale/browser/searchplugins/               (.deps/generated_@AB_CD@/*.xml)
> +#if AB_CD == en_US

Are you sure this test works? I wonder if the preprocessor supports having a string on the right hand side; all the examples I've seen are with numeric constants. And if it does, I wonder if quotes are needed.

::: browser/locales/search/en-US.json
@@ +1,1 @@
> +{

I'm still a little bit unsure about where this file should go in the tree. I assume browser/locale/en-US/ is specifically what you want to avoid as it's localized. I was going to suggest browser/components/search, but given that the files we end up with are actually different for each locale, browser/locale may indeed be the right place.

This patch will need a build peer to have a quick look at it (for the moz.build addition, and the searchplugins.py file), so if we got this wrong they can comment on it too.

::: toolkit/components/search/nsSearchService.js
@@ +3677,5 @@
> +    let searchSettings;
> +    try {
> +      searchSettings = JSON.parse(list);
> +    } catch(e) {
> +      LOG("failing to parse list.json: " + e);

I think we should return early here, or we'll just throw an exception later with a confusing error.

@@ +3693,5 @@
> +    let visibleDefaultEngines = this.getVerifiedGlobalAttr("visibleDefaultEngines");
> +    if (visibleDefaultEngines) {
> +      engineNames = visibleDefaultEngines.split(",");
> +      for (let engineName of engineNames) {
> +        // If all engineName values are part of shippedEngines,

shippedEngines -> jarNames

@@ +3714,5 @@
> +      let region;
> +      if (Services.prefs.prefHasUserValue("browser.search.region")) {
> +        region = Services.prefs.getCharPref("browser.search.region");
> +      }
> +      if (!region || !(region in searchSettings["settings"]) || isPartnerBuild()) {

Why are we ignoring the region for partner builds? (not saying this is wrong, but this question from comment 17 hasn't been answered afaik)

@@ +3717,5 @@
> +      }
> +      if (!region || !(region in searchSettings["settings"]) || isPartnerBuild()) {
> +        region = "default";
> +      }
> +      if (!engineNames) {

This test will always pass (it's a duplicate of the test 8 lines above), you can just remove it.

::: toolkit/components/search/tests/xpcshell/test_json_cache.js
@@ +65,5 @@
>    let list = sis.read(sis.available());
> +  let searchSettings = JSON.parse(list);
> +
> +  for (let engine of searchSettings["settings"]["default"]["visibleDefaultEngines"]) {
> +    if (!visibleDefaultEngines.includes(engine)) {

Why do we need this test? Do you expect duplicates?
Attachment #8762522 - Flags: review?(florian) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> Are you sure this test works? I wonder if the preprocessor supports having a
> string on the right hand side; all the examples I've seen are with numeric
> constants. And if it does, I wonder if quotes are needed.

It seemed to in my test. My hope is to have all the locale JSON files done so we won't need this.

> I'm still a little bit unsure about where this file should go in the tree. I
> assume browser/locale/en-US/ is specifically what you want to avoid as it's
> localized. I was going to suggest browser/components/search, but given that
> the files we end up with are actually different for each locale,
> browser/locale may indeed be the right place.

Yeah, I was not sure either. Once we centralize mobile and browser, I think it will end up in components.

> This patch will need a build peer to have a quick look at it (for the
> moz.build addition, and the searchplugins.py file), so if we got this wrong
> they can comment on it too.

Will do.

> @@ +3714,5 @@
> > +      let region;
> > +      if (Services.prefs.prefHasUserValue("browser.search.region")) {
> > +        region = Services.prefs.getCharPref("browser.search.region");
> > +      }
> > +      if (!region || !(region in searchSettings["settings"]) || isPartnerBuild()) {
> 
> Why are we ignoring the region for partner builds? (not saying this is
> wrong, but this question from comment 17 hasn't been answered afaik)

I think it's the same principal as the absearchdata stuff. Basically if it's a partner build, they get the default set of engines and then they override that.

Otherwise the partner builds would have to have knowledge of the various locales in a given region...


> Why do we need this test? Do you expect duplicates?

Holdover from old test. I'll fix.
(In reply to Mike Kaply [:mkaply] from comment #20)

> > @@ +3714,5 @@
> > > +      let region;
> > > +      if (Services.prefs.prefHasUserValue("browser.search.region")) {
> > > +        region = Services.prefs.getCharPref("browser.search.region");
> > > +      }
> > > +      if (!region || !(region in searchSettings["settings"]) || isPartnerBuild()) {
> > 
> > Why are we ignoring the region for partner builds? (not saying this is
> > wrong, but this question from comment 17 hasn't been answered afaik)
> 
> I think it's the same principal as the absearchdata stuff. Basically if it's
> a partner build, they get the default set of engines and then they override
> that.
> 
> Otherwise the partner builds would have to have knowledge of the various
> locales in a given region...

Can't the partner builds just add and remove/replace specific engines without needing to provide/replace the whole list?
Depends on: 1281630
> Are you sure this test works? I wonder if the preprocessor supports having a string on the right hand side; all the examples I've seen are with numeric constants. And if it does, I wonder if quotes are needed.

I copied this from:

https://dxr.mozilla.org/mozilla-central/source/browser/extensions/loop/chrome/locale/jar.mn#35

but you are correct, it's not working. I'll see what's up.

> Can't the partner builds just add and remove/replace specific engines without needing to provide/replace the whole list?

Thinking about this more, the primary concern with partner builds is that they will change the default search engine and override what the distribution is doing. So I'm going to remove it from this patch and it will come up again when we figure out how to do default search engines.
Attached patch Final final patch :) (obsolete) — Splinter Review
After discussion in bug 1281630, this adds the full JSON file with all search lists.

As part of the build, the correct section of that JSON file is used in the build (generated as a second JSON file).

The format is slightly different (simpler).

This should address all other issues as well.
Attachment #8762522 - Attachment is obsolete: true
Attachment #8765065 - Flags: review?(florian)
Forgot to remove some unneeded build stuff.

Glandium:

Would like some build feedback here.

The basic concept is that we have a JSON file (searc/list.json) with all locale information in it and in the build we generate a list of the XML files that correspond to that build (list.txt) and the JSON snippet that will be used for that locale (list.json).

list.txt is read in Makefile.in and used to copy the files.
list.json is placed in the .deps directory and packaged with the build.

Unfortunately the build part is pretty convoluted, but then again so was the original build stuff for the XML files.

Not sure how to make it simpler.
Attachment #8765173 - Flags: review?(florian)
Attachment #8765173 - Flags: feedback?(mh+mozilla)
Comment on attachment 8765173 [details] [diff] [review]
Removing some unnecessary buld stuff

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

::: browser/locales/searchjson.py
@@ +13,5 @@
> +  locale = ntpath.basename(locale)
> +  with open(locale_json) as f:
> +    searchinfo = json.load(f)
> +
> +  if searchinfo["locales"].has_key(locale):

I confess I don't know much about the build system and I'm still trying to learn Python decently, but is there a specific reason to go for .has_key instead of in? E.g.

if locale in searchinfo["locales"]:

Also, why npath vs os.path?
> if locale in searchinfo["locales"]:

I tried that on another project and it didn't work. In my research, has_key was the only thing that did, but i'll double check.

> Also, why npath vs os.path?

It was in the eample. You're right that os.path should work here as well. I'll switch it.
'foo' in dict should work.

Please don't use ntpath, but mozpack.path. That doesn't do \, which hurts us badly often in life.

As for the list.txt stuff, the mobile/locales/Makefile generates a temporary jar.mn, which might be an easier way to do it if we can make searchplugin.py do that.

Generally, I think the python files should be in toolkit, and it might be beneficial to just have one instead of two of them.

But that's really stuff for glandium to make a call on.
Attachment #8765065 - Attachment is obsolete: true
Attachment #8765065 - Flags: review?(florian)
Comment on attachment 8765173 [details] [diff] [review]
Removing some unnecessary buld stuff

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

Removing the review flag for now because I'm not sure if the list.txt stuff still in the patch is leftover from previous versions of the patch, or if I'm missing something.

::: browser/locales/Makefile.in
@@ +65,1 @@
>  SEARCHPLUGINS_FILENAMES = $(subst :hidden,,$(SEARCHPLUGINS_NAMES))

So, I'm confused by what list.txt is still used for.

If it's just to decide which plugin files will be packaged you don't need this :hidden processing anymore. And then... you probably don't need list.txt at all, and it would be better to make a temporary jar.mn file, as suggested in comment 27.

@@ +84,5 @@
>  
>  include $(topsrcdir)/toolkit/locales/l10n.mk
>  
> +$(list-json): $(call mkdir_deps,$(SEARCHPLUGINS_PATH)) $(if $(IS_LANGUAGE_REPACK),FORCE)
> +	$(RM) $(list-json)

You don't need this line anymore, the rm was needed before doing |echo ... >> list.txt|

::: browser/locales/search/en-US.json
@@ +1,1 @@
> +{

Where/when is this file used?

::: browser/locales/search/list.json
@@ +1,5 @@
> +{
> +  "default": {
> +    "visibleDefaultEngines": [
> +      "google", "yahoo", "amazondotcom", "bing", "ddg", "eBay", "twitter", "wikipedia"
> +    ]    

nit: trailing whitespace
Attachment #8765173 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> ::: browser/locales/Makefile.in
> @@ +65,1 @@
> >  SEARCHPLUGINS_FILENAMES = $(subst :hidden,,$(SEARCHPLUGINS_NAMES))
> 
> So, I'm confused by what list.txt is still used for.
> 
> If it's just to decide which plugin files will be packaged you don't need
> this :hidden processing anymore. And then... you probably don't need
> list.txt at all, and it would be better to make a temporary jar.mn file, as
> suggested in comment 27.

A temporary jar.mn is not exactly "better".
Comment on attachment 8765173 [details] [diff] [review]
Removing some unnecessary buld stuff

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

Please tell me if I'm wrong, but this patch only really implements the real change suggested in comment 0 for en-US, where there are two EN and CA regions with different search plugins.

How is it supposed to work at the larger scale? What happens when someone needs to add region X with plugins a,b,c and d to language Y? They add the plugins to the locale Y and add the definitions for the region X in mozilla-central? Doesn't that negate why locales currently have their own list.txt, and makes it more complex to land those changes?

What about plugins that are currently spread across locales? I don't have an explicit example in mind, but aren't we going to have regions with plugins that currently live in different locales? Are they going to be duplicated across the locales that have them for different regions?

Additionally, I'm not thrilled about keeping this depending on rather complex interactions with the build system. As a matter of fact, if we could leave the build system completely out of the equation, it would be much better.
Attachment #8765173 - Flags: feedback?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #30)
> Comment on attachment 8765173 [details] [diff] [review]
> Removing some unnecessary buld stuff
> 
> Review of attachment 8765173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please tell me if I'm wrong, but this patch only really implements the real
> change suggested in comment 0 for en-US, where there are two EN and CA
> regions with different search plugins.

That is correct, but it sets up this ability for other locales which will be happening once this has landed.

> How is it supposed to work at the larger scale? What happens when someone
> needs to add region X with plugins a,b,c and d to language Y? They add the
> plugins to the locale Y and add the definitions for the region X in
> mozilla-central? Doesn't that negate why locales currently have their own
> list.txt, and makes it more complex to land those changes?

All search plugins will be moving into mozilla-central. This is step one in that process. You can see bug 1276745 for an overview.

So the process will be "If the search plugin exists, just add it to your locale in the search.json file. If not, check the engine into mozilla-central and add it to your locale."

The end goal here is central management of all search plugins in mozilla-central with no search plugins in locales at all.

> What about plugins that are currently spread across locales? I don't have an
> explicit example in mind, but aren't we going to have regions with plugins
> that currently live in different locales? Are they going to be duplicated
> across the locales that have them for different regions?

Our goal is to unify search engines as much as possible (bug 1276740). There will be some locale unique plugins, and they will be named as such. This will actually simplify things greatly because right now we have the same engines duplicated across locales.

> Additionally, I'm not thrilled about keeping this depending on rather
> complex interactions with the build system. As a matter of fact, if we could
> leave the build system completely out of the equation, it would be much
> better.

Unfortunately I don't think that's possible, but I'd like to simplify as much as possible.

Originally I was going to have one JSON file for each locale, but I don't think that's as manageable as one JSON file that is parsed at build time.

The build will be primarily responible for parsing the JSON for a given locale and generating the JSON to be bundled with the build and for determining which search engines should be packaged with a given locale

I would love some better suggestions on doing this.

I think anything is better than what we had before...
Attached patch Greatly simplified build process (obsolete) — Splinter Review
Primarily asking for build review from glandium based on my recent comments and the new patch.

After talking to mshal, I got some great ideas on how to cleanup/simplify everything.

A new python file outputs the correct engine names replacing generating list.txt.

I generate the JSON file directly into the .deps directory avoiding all the extra copy work.

Just to reiterate, all search engines will be going into the main tree and this is just step one. All engines and list.txt files will be removed from the locale trees (For browser to start) and this master list.json will be used for every locale.
Attachment #8769361 - Flags: review?(mh+mozilla)
Attachment #8769361 - Flags: feedback?(florian)
Comment on attachment 8769361 [details] [diff] [review]
Greatly simplified build process

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

::: browser/locales/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +JAR_MANIFESTS += ['jar.mn']
> +
> +searchjson = '.deps/generated_' + CONFIG['MOZ_UI_LOCALE'] + '/list.json'

This most likely will massively fail on l10n repack builds because configure is only run once, meaning only the rule for .deps/generated_en_US/list.json (or whatever locale configure is run for). will be generated. It's something that I've been meaning to fix, but for now, you can't rely on this working. You'll have to convert this to some Makefile rules.

::: browser/locales/searchjson.py
@@ +8,5 @@
> +
> +engines = []
> +
> +def main(output, locale_json):
> +  locale = buildconfig.substs['MOZ_UI_LOCALE']

Likewise.
Attachment #8769361 - Flags: review?(mh+mozilla)
Convert to AB_CD being passed in from the Makefile.in, right?
(In reply to Axel Hecht [:Pike] from comment #34)
> Convert to AB_CD being passed in from the Makefile.in, right?

I can do that for searchjson, but it will require some changes to make the first one work from a makefile instead of GENERATED_FILES.

I didn't realize GENERATED_FILES only happens at configure. That makes my whole process not work correctly for l10n.
Comment on attachment 8769361 [details] [diff] [review]
Greatly simplified build process

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

::: browser/locales/jar.mn
@@ +96,2 @@
>      locale/browser/searchplugins/               (%searchplugins/*.xml)
> +    locale/browser/searchplugins/list.json      (search/list.json)

Is this file in the same format as the single-locale file generated by searchjson.py? Or is the default section at the top before the locales a hack to handle this case?
Attachment #8769361 - Flags: feedback?(florian) → feedback+
> Is this file in the same format as the single-locale file generated by searchjson.py? Or is the default section at the top before the locales a hack to handle this case?

Argh. No. That was a hack to handle that case, but it used the early JSON format. The BUILD_FASTER is a real pain as it results to search plugins. This whole thing is just hacky.
Actually, it has the default case, so it will work fine for go faster builds.
(In reply to Mike Kaply [:mkaply] from comment #38)
> Actually, it has the default case, so it will work fine for go faster builds.

I think it'll work fine, but it's non-obvious so I'm afraid we may break it in the future. It's one of these cases where it's annoying that we can't add comments in JSON files...
Attached patch 1276739-10.diffSplinter Review
Moved build process to Makefile.in.
Made sure artifact builds work.
Attachment #8765173 - Attachment is obsolete: true
Attachment #8769361 - Attachment is obsolete: true
Comment on attachment 8770250 [details] [diff] [review]
1276739-10.diff

This moves things to makefiles. I left the internal name in the Makefile as list-txt because it seemed to be gratuitous change.
Attachment #8770250 - Flags: review?(mh+mozilla)
Comment on attachment 8770250 [details] [diff] [review]
1276739-10.diff

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

::: browser/locales/Makefile.in
@@ +69,5 @@
>  # default en-US ones do not.
>  SEARCHPLUGINS_FLAGS := --silence-missing-directive-warnings
>  PP_TARGETS += SEARCHPLUGINS
>  
> +list-txt = $(SEARCHPLUGINS_PATH)/list.json

This variable should really be named list-json. You are already touching most occurrences of list-txt anyway.

::: toolkit/components/search/nsSearchService.js
@@ +3683,5 @@
> +    let jarNames = new Set();
> +    for (let region in searchSettings) {
> +      // Artifact builds use the full list.json which parses
> +      // slightly differently
> +      if (!("visibleDefaultEngines" in searchSettings[region])) {

So this is meant to be equivalent to: if (region == "locales") right?
(In reply to Florian Quèze [:florian] [:flo] from comment #42)
> This variable should really be named list-json. You are already touching
> most occurrences of list-txt anyway.

You're right. I'll change it.

> So this is meant to be equivalent to: if (region == "locales") right?

Yes. Basically it's kind of a hack for artifact builds. We have the default that works fine for the artifact, but when we enumerate the locales in the main JSON file, we hit "locales" which doesn't have visibleDefaultEngines.

Do you think I should be more explicit and say "if region == "locales""?
(In reply to Mike Kaply [:mkaply] from comment #43)
> (In reply to Florian Quèze [:florian] [:flo] from comment #42)

> > So this is meant to be equivalent to: if (region == "locales") right?
> 
> Yes. Basically it's kind of a hack for artifact builds. We have the default
> that works fine for the artifact, but when we enumerate the locales in the
> main JSON file, we hit "locales" which doesn't have visibleDefaultEngines.
> 
> Do you think I should be more explicit and say "if region == "locales""?

I would prefer this, yes. To avoid hiding the JS error if somehow we ever typo on 'visibleDefaultEngines' in the JSON file for one of the regions.
Comment on attachment 8770735 [details]
Bug 1276739 - Switch search to use a JSON based format - NOT READY FOR CHECKIN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64110/diff/1-2/
Comment on attachment 8770735 [details]
Bug 1276739 - Switch search to use a JSON based format - NOT READY FOR CHECKIN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64110/diff/2-3/
Attachment #8770735 - Attachment description: Bug 1276739 - Switch search to use a JSON based format → Bug 1276739 - Switch search to use a JSON based format - NOT READY FOR CHECKIN
Attachment #8770735 - Flags: review?(florian)
https://reviewboard.mozilla.org/r/64110/#review61900

Why is browser/locales/en-US/searchplugins/list.txt moved to toolkit/components/search/searchplugins/list.txt rather than removed?

If Thunderbird is the reason why we keep the list.txt handling inside the search service, can we ensure there are bugs on file to transition TB to list.json, and a bug to cleanup the search service, blocked by the former?
> Why is browser/locales/en-US/searchplugins/list.txt moved to toolkit/components/search/searchplugins/list.txt rather than removed?

Mercurial did that somehow. If you look at the most recent patch in reviewboard, that should be gone.

> If Thunderbird is the reason why we keep the list.txt handling inside the search service, can we ensure there are bugs on file to transition TB to list.json, and a bug to cleanup the search service, blocked by the former?

It's not just Thunderbird, it's SeaMonkey and Chatzilla. And Mobile until I get it switched. But I'll get bugs opened.
(In reply to Mike Kaply [:mkaply] from comment #49)
> > Why is browser/locales/en-US/searchplugins/list.txt moved to toolkit/components/search/searchplugins/list.txt rather than removed?
> 
> Mercurial did that somehow. If you look at the most recent patch in
> reviewboard, that should be gone.

The most recent patch is https://reviewboard-hg.mozilla.org/gecko/raw-rev/4b5dfe844846f568bb962f2bd5ac94d160be6954 and contains:

diff --git a/browser/locales/en-US/searchplugins/list.txt b/toolkit/components/search/searchplugins/list.txt
rename from browser/locales/en-US/searchplugins/list.txt
rename to toolkit/components/search/searchplugins/list.txt
Argh.

I fixed that in revision 2:

https://reviewboard.mozilla.org/r/64110/diff/2/

Somehow it came back.
Comment on attachment 8770735 [details]
Bug 1276739 - Switch search to use a JSON based format - NOT READY FOR CHECKIN

Removing the review flag for now as comment 48 and comment 50 need to be addressed.
Attachment #8770735 - Flags: review?(florian)
Attachment #8770735 - Attachment is obsolete: true
Attachment #8770250 - Flags: review?(mh+mozilla) → feedback+
Whiteboard: [fxsearch] → [fxsearch][partner search]
Blocks: 1300198
Blocks: 1300199
Blocks: 1300201
ARGH.

toolkit/components/search/searchplugins/list.txt
Was browser/locales/en-US/searchplugins/list.txt

This looks like a bug in mozreview. This absolutely was not in my local patch.

I give up. I'm not going to use mozreview.
Attachment #8787782 - Attachment is obsolete: true
Attachment #8787782 - Flags: review?(florian)
Just a comment

>> Language is a poor substitute for region, so many users get poor default sets. 

Region is a poor subsititute for language too. Google does this too now. My browser is en-US but I am located in Germany. I now regularly get useless junk in German presented when I search for non German content. Good for DuckDuckgo and bad for Google and any other service which uses region or ip checking. This should be a visible user pref not some pre choosen default based on region or ip.
(In reply to Frank-Rainer Grahl from comment #56)
> Just a comment
> 
> >> Language is a poor substitute for region, so many users get poor default sets. 
> 
> Region is a poor subsititute for language too. Google does this too now. My
> browser is en-US but I am located in Germany. I now regularly get useless
> junk in German presented when I search for non German content. Good for
> DuckDuckgo and bad for Google and any other service which uses region or ip
> checking. This should be a visible user pref not some pre choosen default
> based on region or ip.

You can go to http://www.google.com/ncr and it will turn off country redirect. I also have that feature in my keyword search addon.

What you're talking about is not the browser's fault, it's google. We're presenting you with google.com and Google is choosing to give country specific results.
>> What you're talking about is not the browser's fault,

In a fresh profile the language of the browser is ignored and search is set to the OS region:

>> browser.search.countryCode;DE
>> browser.search.region;DE

This is not related to the bug but what I want to express is that region is as bad as language or worse when it comes to picking search providers or doing searches. I usually only want local results when I do shopping.

FRG
Comment on attachment 8787808 [details]
Bug 1276739 - Switch search to use a JSON based format.

https://reviewboard.mozilla.org/r/76490/#review76210

Almost there :-)

::: browser/locales/Makefile.in:73
(Diff revision 1)
>  # Some locale-specific search plugins may have preprocessor directives, but the
>  # default en-US ones do not.
>  SEARCHPLUGINS_FLAGS := --silence-missing-directive-warnings
>  PP_TARGETS += SEARCHPLUGINS
>  
> -list-txt = $(SEARCHPLUGINS_PATH)/list.txt
> +list-txt = $(SEARCHPLUGINS_PATH)/list.json

As already noted in comment 42 (and agreed in comment 43), this variable should be named list-json.

::: toolkit/components/search/nsSearchService.js:3713
(Diff revision 1)
> +        }
> +      }
> +    }
> +
> +    // Fallback to building a list based on the regions in the JSON
> +    if (!engineNames) {

I don't see any code preventing us from saving an empty visibleDefaultEngines attribute (if the absearch server ever ends up returning an empty visibleDefaultEngines list due to some misconfiguration), so I think it would be safer to change this test to:

if (!engineNames || !engineNames.length) {

::: toolkit/components/search/tests/xpcshell/xpcshell.ini:27
(Diff revision 1)
>    data/engine-chromeicon.xml
>    data/engine-resourceicon.xml
>    data/ico-size-16x16-png.ico
>    data/invalid-engine.xml
>    data/install.rdf
>    data/list.txt

Is this data/list.txt file still used by tests?
Attachment #8787808 - Flags: review?(florian) → review-
Comment on attachment 8787808 [details]
Bug 1276739 - Switch search to use a JSON based format.

https://reviewboard.mozilla.org/r/76490/#review76328

Looks good. r=me assuming the tests still pass (I assume you verified that they pass locally after removing the data/list.txt file; could be a good idea to push to try with at xpcshell tests and browser-chrome mochitests, or more generally any test that's likely to depend on custom search service initialization).
Attachment #8787808 - Flags: review?(florian) → review+
(In reply to Mike Kaply [:mkaply] from comment #62)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=db0950fd96c3

The toolkit/modules/tests/browser/browser_Troubleshoot.js failures here were fixed by this backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f19715529c24 The other failures seem intermittent to me.
Per RyanVM, I'm putting a needinfo to figure out QA on this.

I've put together the beginning of a basic test plan here.

https://docs.google.com/document/d/1XZcNGcGXxlwFBQ82DFzYBcnCfD431-e2z3UZt_urOKg/edit?usp=sharing

If this goes as planned, it should have zero impact on the product (since it's just switching how the engines are read).
Flags: needinfo?(rares.bologa)
Note to self: the only locale added in the meantime is 'kab', but we have 'tl' in the work, plus removal of at least 5. That's material for a follow-up bug, shouldn't block this one.
Added Brindusa as QA contact.
Flags: needinfo?(rares.bologa)
QA Contact: brindusa.tot
https://hg.mozilla.org/mozilla-central/rev/3cd0102f89e9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Blocks: 1309204
Depends on: 1309273
Depends on: 1310712
Blocks: 1311365
Verified on the top 11 locales that the TXT list was successfully replaced with a region-aware JSON list (the json list contains the same search engines as the txt list).

Verified as fixed on the latest Nightly 52.0a1 on: Mac OS X 10.11, Mac OS X 10.12, Ubuntu 15.04 x64, Ubuntu 16.04 x64, Windows 10 x64, Windows 8.1 x64, Windows 7 x86 and Windows XP x86. 

For more details about the verification of this bug, please consult Phase 1 from:
https://wiki.mozilla.org/QA/Partner_Search
Status: RESOLVED → VERIFIED
Blocks: 1105092
You need to log in before you can comment on or make changes to this bug.