1352539 - Move defaultenginename pref setting into list.json

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Mike Kaply [:mkaply]]

The goal of moving everything to list.json is to allow complete control of search settings for every locale from list.json. This includes the default engine as well as the search order.

This bug is the next step in that process, moving to the default search engine to  list.json.

The preferences will still work if they are there (for distributions), but going forward, list.json will control search defaults.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: First pass

This is my first pass.

Right now it doesn't add any new tests, but it passes all of the old tests.

Next step is to add tests specific to the new format (but keep all the old defaultenginename tests)

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8853520 [details] [diff] [review]
First pass

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

::: python/mozbuild/mozbuild/action/generate_searchjson.py
@@ +30,5 @@
>  if "regionOverrides" in searchinfo:
>    regionOverrides = searchinfo["regionOverrides"]
>  
>    for region in regionOverrides:
> +    # Only add the region if it has engines that need to be overridden

It seems this comment needs updating. And I'm not sure I actually fully understand this change, but I haven't tried very hard as it's not a final review.

::: toolkit/components/search/nsSearchService.js
@@ +2833,4 @@
>        let defaultPrefB = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
>        let nsIPLS = Ci.nsIPrefLocalizedString;
>  
>        let defPref = getGeoSpecificPrefName("defaultenginename");

Should we look at that pref only if a distribution id is set?

@@ +3650,5 @@
>      }
>  
> +    let region;
> +    if (Services.prefs.prefHasUserValue("browser.search.region")) {
> +      region = Services.prefs.getCharPref("browser.search.region");

This can be simplified to:
let region = Services.prefs.getCharPref("browser.search.region", "");

@@ +3655,5 @@
> +    }
> +    if (!region && getIsUS()) {
> +      region = "US";
> +    }
> +    if (!region || !(region in searchSettings)) {

Given that 'region' is a string that the user can mess with in about:config (or hijackers can mess with by writing to the user's pref.js file in the profile), I wonder if we should make this test a bit more robust.
Example of what could go wrong: "watch" in {}   evaluates to true.

A more robust test could be Object.keys(searchSettings).includes(region)


(and now I see this is code you just moved, oh well...)

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: Patch with xpcshell tests

Comment 4

[Punam Dahiya [:pdahiya]]

Attaching part 2 patch implemented on top of first pass with below fixes:

- Added new tests test_list_json_searchdefault.js
- Removed browser.search.defaultenginename string from browser/locales/en-US/chrome/browser-region/region.properties
- eslint fix in head_search.js
- incorporated review feedback to simplify "browser.search.region" get pref

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e91fad9f3c4afb42c6bb327fe2e7f9299a9be53

Mike, I believe with removal of browser.search.defaultenginename pref, en-US locale region.properties can safely remove respective string. Setting feedback flag before squashing commits for review. Thanks!

Comment 5

[Mike Kaply [:mkaply]]

Attachment: Bug 1352539 - Move default search engine to list.json.

Review commit: https://reviewboard.mozilla.org/r/203560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203560/

Comment 6

[Mike Kaply [:mkaply]]

Comment on attachment 8866934 [details] [diff] [review]
Patch with xpcshell tests

This was great. Sorry it's taken me so  long to get to this. I have everything merged and unbitrotted now. Ready to finally get this in.

Comment 7

[Mike Kaply [:mkaply]]

I realized I hadn't fully addressed florian's comments. I'll do that as well.

Comment 8

[Mike Kaply [:mkaply]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> It seems this comment needs updating. And I'm not sure I actually fully
> understand this change, but I haven't tried very hard as it's not a final
> review.

I made the comment more clear.

> ::: toolkit/components/search/nsSearchService.js
> @@ +2833,4 @@
> >        let defaultPrefB = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
> >        let nsIPLS = Ci.nsIPrefLocalizedString;
> >  
> >        let defPref = getGeoSpecificPrefName("defaultenginename");
> 
> Should we look at that pref only if a distribution id is set?

This makes the code more complex and I think it's unnecessary.

If we're thinking because of hijacking, a hijacker could just set a distribution.id.

> 
> @@ +3650,5 @@
> >      }
> >  
> > +    let region;
> > +    if (Services.prefs.prefHasUserValue("browser.search.region")) {
> > +      region = Services.prefs.getCharPref("browser.search.region");
> 
> This can be simplified to:
> let region = Services.prefs.getCharPref("browser.search.region", "");

Done

> 
> @@ +3655,5 @@
> > +    }
> > +    if (!region && getIsUS()) {
> > +      region = "US";
> > +    }
> > +    if (!region || !(region in searchSettings)) {
> 
> Given that 'region' is a string that the user can mess with in about:config
> (or hijackers can mess with by writing to the user's pref.js file in the
> profile), I wonder if we should make this test a bit more robust.
> Example of what could go wrong: "watch" in {}   evaluates to true.
> 
> A more robust test could be Object.keys(searchSettings).includes(region)

I can make it more robust, but a hacker can't mess with this because they can't mess with list.json at all. So adding a new region value is useless (we'd switch to default).

Comment 9

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/1-2/

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/2-3/

Comment 11

[Mike Kaply [:mkaply]]

> This makes the code more complex and I think it's unnecessary.

I changed my mind and made the originaldefaultenginename code only happen for distributions. Also fixed a dumb typo.

Comment 12

[Mike Kaply [:mkaply]]

One more comment on this. Because of the new l10n build features, I was able to test this with other languages. I tested Russian and German and verified everything works as expected.

Comment 13

[Francesco Lodolo [:flod]]

I see zh-CN has Baidu set as "searchDefault", but that doesn't happen for Yandex locales. Is that expected?

Comment 14

[Mike Kaply [:mkaply]]

> I see zh-CN has Baidu set as "searchDefault", but that doesn't happen for Yandex locales. Is that expected?

Can you be more specific what you mean? The idea is that for zh-CN we use Baidu everywhere. So even for Yandex locales.

Comment 15

[Francesco Lodolo [:flod]]

zh-CN has a     

"zh-CN": {
   "default": {
      "searchDefault": "百度",
...

Look at Turkish, for example: there's no 'searchdefault' in 'default', only a searchdefault by region (TR, BY, KZ, RU). My assumption is that Turkish used in Germany would get Google by default, while Simplified Chinese in Germany would get Baidu.
Not sure if that's expected or not.

Comment 16

[Mike Kaply [:mkaply]]

Ah, I see what you are saying. That's actually a really good question. Given that we will now be able to do region specific search engine defaults easier within the product, does it make sense to only use Baidu in CN and use Google everywhere else?

Comment 17

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/3-4/

Comment 18

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #16)
> Ah, I see what you are saying. That's actually a really good question. Given
> that we will now be able to do region specific search engine defaults easier
> within the product, does it make sense to only use Baidu in CN and use
> Google everywhere else?

Since we have region support, it seems more logical to tie the default to locale+region, unless there are contractual obligations to do otherwise. 

One potential concern against that is that Google will remain the default if, for any reason, region doesn't get correctly updated.

Comment 19

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/4-5/

Comment 20

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #18)
> One potential concern against that is that Google will remain the default
> if, for any reason, region doesn't get correctly updated.

To clarify, also per IRC discussion, it might be safer to have an explicit searchDefault in both locale and region. If you're using zh-CN in China or tr in Turkey, and absearch fails for Internet blocks, you're still setting the right search engine.

    "zh-CN": {
      "default": {
        "searchDefault": "百度",
        "visibleDefaultEngines": [
          "baidu", "google", "bing", "ddg", "wikipedia-zh-CN", "amazondotcn"
        ]
      },
      "CN": {
        "searchDefault": "百度"
      }

Comment 21

[Mike Kaply [:mkaply]]

In that situation, you wouldn't need the CN part at all because it would always get the global searchDefault.

If the region default (CN) is the same as the global default, you don't need the separate entry.

The only reason to have separate is if you want different engines for global versus local.

Comment 22

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #21)
> In that situation, you wouldn't need the CN part at all because it would
> always get the global searchDefault.

Right, at that point you don't need the region. Current patch is correct then.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review211814

Removing the review flag for now because I don't have time for a real review before Austin, and it'll bitrot with bug 1417678 that we want to land sooner.

I wonder if it may be time to remove a lot of the US specific behaviors we have here. Do we still need the browser.search.*.US prefs if the same information is available in a list.json file that we ship?

Comment 24

[Mike Kaply [:mkaply]]

> I wonder if it may be time to remove a lot of the US specific behaviors we have here. Do we still need the browser.search.*.US prefs if the same information is available in a list.json file that we ship?

My next patch after this will be to move browser.search.order to JSON and then we can remove them. I didn't want to do browser.search.order as part of this patch because there are still some things to work out.

Comment 25

[Mike Kaply [:mkaply]]

Attachment: Search Order patch

Search order patch wasn't as bad as I thought. Posting it here in case I decide to combine the two patches (this patch is dependent on the other).

Comment 26

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/5-6/

Comment 27

[Mike Kaply [:mkaply]]

I decided to combine default engine and search order into the same patch.

For search order, I decided to always add the default engine as first in the search order. This saved unnecessary changes to list.json marking the default engine as the first engine in the list. This did make me wonder if I should call "searchOrder" "additionalSearchOrder" to make it clear what it is.

FYI, I get quite a few eslint errors in toolkit/components/search, so it made it difficult to see which ones I should fix.

Comment 28

[Francesco Lodolo [:flod]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review215920

::: mobile/locales/en-US/chrome/region.properties:8
(Diff revisions 5 - 6)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # Default search engine
>  browser.search.defaultenginename=Google
>  
>  # Search engine order (order displayed in the search bar dropdown).

Do you still need search.order in this file (both US and non)?

Comment 29

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/6-7/

Comment 30

[Mike Kaply [:mkaply]]

> Do you still need search.order in this file (both US and non)?

Misssed that. It's removed now.

Comment 31

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/7-8/

Comment 32

[Francesco Lodolo [:flod]]

Quick question about searchOrder: take for example be, ca or zh-CN. They don't have Yahoo, and searchorder is not overridden in the locale. Should it have an override to exclude Yahoo?

There are a few more differences, but first I need to figure out this point to reduce the diff.

Comment 33

[Mike Kaply [:mkaply]]

> Quick question about searchOrder: take for example be, ca or zh-CN. They don't have Yahoo, and searchorder is not overridden in the locale. Should it have an override to exclude Yahoo?

If an engine is not present in the locale, it is simply ignored in the order.

The allows us to have a default order (Google, Yahoo, Bing) and still work for locales that don't have either Yahoo or Bing.

This seemed liek the right solution to me versus having unnecessary searchOrder information in all the locales. My goal is to have very few locales actually require search order.

Comment 34

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #33)
> This seemed liek the right solution to me versus having unnecessary
> searchOrder information in all the locales. My goal is to have very few
> locales actually require search order.

That makes sense. Here's a few differences, unless I'm misunderstanding the logic.

be, kk, ru, tr, uk
Currently have Google as 2 (or 1 for uk), now they would only have Яндекс. This might be OK, just pointing it out.

zh-CN
Would lose Google as 2.

These other locales have weird settings, I think it's fine to switch to default.

lt
"1": "Google", 
"2": "Wikipedia (lt)", 
"3": "Bing"

ml
"1": "Google", 
"2": "Webdunia Search"

sk
"1": "Google", 
"2": "Azet"

Comment 35

[Francesco Lodolo [:flod]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review216220

::: browser/locales/search/list.json:262
(Diff revision 8)
>            "google", "yahoo", "bing", "amazondotcom", "ddg", "reta-vortaro", "wikipedia-eo"
>          ]
>        }
>      },
>      "es-AR": {
> +      "searchOrder": ["Yahoo Argentina"],

This should be inside "default"

Comment 36

[Mike Kaply [:mkaply]]

(In reply to Francesco Lodolo [:flod] from comment #34)
> That makes sense. Here's a few differences, unless I'm misunderstanding the
> logic.
> 
> be, kk, ru, tr, uk
> Currently have Google as 2 (or 1 for uk), now they would only have Яндекс.
> This might be OK, just pointing it out.

Actually, I think they'll get the default order with Yandex at the front of the order (as the originalDefaultEngine). I'll test that.

> 
> zh-CN
> Would lose Google as 2.

I think that's the same as above. I'll check.

> 
> These other locales have weird settings, I think it's fine to switch to
> default.
> 
> lt
> "1": "Google", 
> "2": "Wikipedia (lt)", 
> "3": "Bing"

That's really weird.

> 
> ml
> "1": "Google", 
> "2": "Webdunia Search"

I'll fix.

> 
> sk
> "1": "Google", 
> "2": "Azet"

I'll fix.

Comment 37

[Francesco Lodolo [:flod]]

Per IRC discussion, let's leave lt, ml, sk as they currently are in the patch (no override). 

Let's remove the one for 'et' (neti-ee). It seems also broken, need to verify that in a follow up bug.

The one for 'cs' makes sense, it's a general search engine and they ship plenty of services from Seznam (mailto, maps, etc).

Comment 38

[Florian Quèze [:florian]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review217566

- This patch contains debug logging that doesn't seem to be meant to stay.
- New test files shouldn't use generators/yield, please switch to async/await.

Comment 39

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/8-9/

Comment 40

[Florian Quèze [:florian]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review217656

::: toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js:21
(Diff revisions 8 - 9)
>  
>    run_next_test();
>  }
>  
>  // Check that current engine matches with US searchDefault from list.json
> -add_task(function* test_searchDefaultEngineUS() {
> +add_task(async function* test_searchDefaultEngineUS() {

function* -> function

Comment 41

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/9-10/

Comment 42

[Florian Quèze [:florian]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review217764

note: I haven't reviewed the new test.

::: python/mozbuild/mozbuild/action/generate_searchjson.py:34
(Diff revision 10)
> +  localeSearchInfo["default"]["searchDefault"] = searchinfo["default"]["searchDefault"]
> +
> +# If the selected locale doesn't have a searchOrder,
> +# use the global one.
> +if not "searchOrder" in localeSearchInfo["default"]:
> +  localeSearchInfo["default"]["searchOrder"] = searchinfo["default"]["searchOrder"]

This implies a requirement that the in-tree list.json always has a default searchOrder defined.

I wonder if this needs to be documented, or if we need to put automated tests in place to detect errors when editing list.json.

Also, what happens if a locale customizes the list of search plugins that are shipped, and we take the default 'searchOrder' value that includes engines that we don't ship for this locale? Should the python script filter them out?

::: python/mozbuild/mozbuild/action/generate_searchjson.py:43
(Diff revision 10)
>    regionOverrides = searchinfo["regionOverrides"]
>  
>    for region in regionOverrides:
> -    if not region in localeSearchInfo:
> +    # Only add a new engine list if there is an engine that is overridden
> -      # Only add the region if it has engines that need to be overridden
> -      if set(localeSearchInfo["default"]["visibleDefaultEngines"]) & set(regionOverrides[region].keys()):
> +    if set(localeSearchInfo["default"]["visibleDefaultEngines"]) & set(regionOverrides[region].keys()):

What happens if localeSearchInfo[region]["visibleDefaultEngines"] is set, and contains an engine that we need to override?

I think you need to replace this test with something like this:
  enginesToOverride = set(regionOverrides[region].keys())
  if region in localeSearchInfo and
     "visibleDefaultEngines" in localeSearchInfo[region]:
     visibleDefaultEngines = set(localeSearchInfo[region]["visibleDefaultEngines"])
  else:
     visibleDefaultEngines = set(localeSearchInfo["default"]["visibleDefaultEngines"])
  if visibleDefaultEngines & enginesToOverride:

::: python/mozbuild/mozbuild/action/generate_searchjson.py:48
(Diff revision 10)
> -      if set(localeSearchInfo["default"]["visibleDefaultEngines"]) & set(regionOverrides[region].keys()):
> +    if set(localeSearchInfo["default"]["visibleDefaultEngines"]) & set(regionOverrides[region].keys()):
> -        localeSearchInfo[region] = copy.deepcopy(localeSearchInfo["default"])
> -      else:
> -        continue
> +      if region not in localeSearchInfo:
> +        localeSearchInfo[region] = {}
> +      if "visibleDefaultEngines" not in localeSearchInfo[region]:
> +        localeSearchInfo[region]["visibleDefaultEngines"] = copy.deepcopy(localeSearchInfo["default"]["visibleDefaultEngines"])
> -    for i, engine in enumerate(localeSearchInfo[region]["visibleDefaultEngines"]):
> +        for i, engine in enumerate(localeSearchInfo[region]["visibleDefaultEngines"]):

and move this back to the previous indent level.

::: python/mozbuild/mozbuild/action/output_searchplugins_list.py:20
(Diff revision 10)
>  # Get a list of the engines from the locale or the default
>  engines = set()
>  if locale in searchinfo["locales"]:
>    for region, table in searchinfo["locales"][locale].iteritems():
> +    if "visibleDefaultEngines" in table:
> -    engines.update(table["visibleDefaultEngines"])
> +      engines.update(table["visibleDefaultEngines"])

If a locale doesn't have a 'default' section with a 'visibleDefaultEngines' field, we may end up with an empty 'engines' set here.

If this happens, I wonder if we should fail loudly, or instead just ship the default engines. Doing the later seems as simple as replacing the 'else:' of the next line with a test checking if the 'engines' set is empty.

::: toolkit/components/search/nsSearchService.js:2800
(Diff revision 10)
>    },
>  
>    _engines: { },
>    __sortedEngines: null,
>    _visibleDefaultEngines: [],
> +  _defaultSearchEngine: null,

I think you are using this as a way to store the default engine as indicated from the list.json file. If so, this is a terrible name as it's actually different from both what the defaultEngine getter returns, and from what the originalDefaultEngine getter returns.

::: toolkit/components/search/nsSearchService.js:2801
(Diff revision 10)
>  
>    _engines: { },
>    __sortedEngines: null,
>    _visibleDefaultEngines: [],
> +  _defaultSearchEngine: null,
> +  _searchOrder: [],

You'll need to clear these 2 new properties in _asyncReinit.

::: toolkit/components/search/nsSearchService.js:2814
(Diff revision 10)
>    // ignoring changes the user may have subsequently made.
>    get originalDefaultEngine() {
>      let defaultEngine = this.getVerifiedGlobalAttr("searchDefault");
>      if (!defaultEngine) {
> +      // We only allow the old defaultenginename pref for distributions
> +      // We can't use isPartnerBuild because we need to allow reading

If we inline isPartnerBuild, you can simplify this comment to // Only allow the old defaultenginename pref for partner distributions and funnelcakes.

fwiw, I'm sad that we are not completely getting rid of this pref :-(.

::: toolkit/components/search/nsSearchService.js:2821
(Diff revision 10)
> -      let defaultPrefB = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
> +        let defaultPrefB = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
> -      let nsIPLS = Ci.nsIPrefLocalizedString;
> +        let nsIPLS = Ci.nsIPrefLocalizedString;
>  
> -      let defPref = getGeoSpecificPrefName("defaultenginename");
> -      try {
> +        try {
> -        defaultEngine = defaultPrefB.getComplexValue(defPref, nsIPLS).data;
> +          defaultEngine = defaultPrefB.getComplexValue("defaultenginename", nsIPLS).data;

Is this still a localized pref if it's only set by distribution/funnel cakes? If not, can we simplify to getStringPref?

::: toolkit/components/search/nsSearchService.js:3484
(Diff revision 10)
>        LOG("failing to parse list.json: " + e);
>        return;
>      }
>  
> +    let region;
> +    region = Services.prefs.getCharPref("browser.search.region", "");

merge these 2 lines

::: toolkit/components/search/nsSearchService.js:3486
(Diff revision 10)
>      }
>  
> +    let region;
> +    region = Services.prefs.getCharPref("browser.search.region", "");
> +
> +    if (!region && getIsUS()) {

Can we finally remove the getIsUS function? I think this timezone check has far outlived its usefulness (and is as likely to return true for Canada as for the US). And the check for the en-US locale is also useless now that we ship a different list.json file with a specific all-regions-default for each locale.

::: toolkit/components/search/nsSearchService.js:3489
(Diff revision 10)
> +    region = Services.prefs.getCharPref("browser.search.region", "");
> +
> +    if (!region && getIsUS()) {
> +      region = "US";
> +    }
> +    if (!region || !(region in searchSettings)) {

else if

::: toolkit/components/search/nsSearchService.js:3500
(Diff revision 10)
>      // it will not be set for distributions.
>      let engineNames;
>      let visibleDefaultEngines = this.getVerifiedGlobalAttr("visibleDefaultEngines");
>      if (visibleDefaultEngines) {
>        let jarNames = new Set();
>        for (let region in searchSettings) {

This region variable is now shadowing your previous region variable.

::: toolkit/components/search/nsSearchService.js:3530
(Diff revision 10)
> -        region = Services.prefs.getCharPref("browser.search.region");
> -      }
> -      if (!region || !(region in searchSettings)) {
> -        region = "default";
> -      }
>        engineNames = searchSettings[region].visibleDefaultEngines;

I'm afraid this can be undefined. I think you need the same checks as you are later doing for searchDefault and searchOrder.

::: toolkit/components/search/nsSearchService.js:3536
(Diff revision 10)
>      }
>  
>      // Remove any engine names that are supposed to be ignored.
>      // This pref is only allows in a partner distribution.
>      let branch = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
>      if (isPartnerBuild() &&

This is the last remaining use of isPartnerBuild, could we please just inline it, and take this as an opportunity to clean it up (ie. get rid of the try/catch)?

::: toolkit/components/search/nsSearchService.js:3554
(Diff revision 10)
>      }
>  
>      // Store this so that it can be used while writing the cache file.
>      this._visibleDefaultEngines = engineNames;
> +
> +    if ("searchDefault" in searchSettings[region]) {

The smell of code duplication here means r- ;-).

function getRegionValue(property) {
  if (property in searchSettings[region])
    return searchSettings[region][property];
  return searchSettings.default[property];
}

this._defaultSearchEngine = getRegionValue("searchDefault");
...

(use this helper when getting visibleDefaultEngines too)

::: toolkit/components/search/nsSearchService.js:3688
(Diff revision 10)
> +        this.__sortedEngines.push(this.originalDefaultEngine);
> +        addedEngines[this.originalDefaultEngine.name] = this.originalDefaultEngine;
> +      }
>        try {
>          var extras =
>            Services.prefs.getChildList(BROWSER_SEARCH_PREF + "order.extra.");

Can we entirely remove all the code reading order.extra and order prefs?

::: toolkit/components/search/nsSearchService.js:3851
(Diff revision 10)
>      } catch (e) {
>        LOG("Getting extra order prefs failed: " + e);
>      }
>  
>      // Now look through the "browser.search.order" branch.
> -    let prefNameBase = getGeoSpecificPrefName(BROWSER_SEARCH_PREF + "order");
> +    // This should only be used on legacy distributions

Can you think of a real case where a distribution would customize the order of search plugins, but not set its plugin as the default (which causes that plugin to be at the top of the list)?

::: toolkit/components/search/tests/xpcshell/head_search.js:249
(Diff revision 10)
> +   });
> +   let sis = Cc["@mozilla.org/scriptableinputstream;1"].
> +               createInstance(Ci.nsIScriptableInputStream);
> +   sis.init(chan.open2());
> +   let list = sis.read(sis.available());
> +   let searchSettings = JSON.parse(list);

let searchSettings = parseJsonFromStream(chan.open2());

Comment 43

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review217764

> Is this still a localized pref if it's only set by distribution/funnel cakes? If not, can we simplify to getStringPref?

Yes, it's still localized pref in funnelcakes/distros. So we can't simplify here.

> Can we finally remove the getIsUS function? I think this timezone check has far outlived its usefulness (and is as likely to return true for Canada as for the US). And the check for the en-US locale is also useless now that we ship a different list.json file with a specific all-regions-default for each locale.

Yes, I will try to do that.

> Can we entirely remove all the code reading order.extra and order prefs?

We could probably get away with removing extra if we always put the default at the top of the list (only Qwant uses two extra values). But we still need order prefs for other distros, including MozillaOnline and Yahoo.

> Can you think of a real case where a distribution would customize the order of search plugins, but not set its plugin as the default (which causes that plugin to be at the top of the list)?

No, but as indicated above, some distributions set an order that contains more than one plugin.

Comment 44

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review217764

> The smell of code duplication here means r- ;-).
> 
> function getRegionValue(property) {
>   if (property in searchSettings[region])
>     return searchSettings[region][property];
>   return searchSettings.default[property];
> }
> 
> this._defaultSearchEngine = getRegionValue("searchDefault");
> ...
> 
> (use this helper when getting visibleDefaultEngines too)

Do you mean for this function to be nested inside of parseListJSON?

Or separate function that takes a searchSettings param?

Comment 45

[Florian Quèze [:florian]]

(In reply to Mike Kaply [:mkaply] from comment #43)
> Comment on attachment 8932519 [details]
> Bug 1352539 - Move default search engine and search order to list.json.
> 
> https://reviewboard.mozilla.org/r/203560/#review217764
> 
> > Is this still a localized pref if it's only set by distribution/funnel cakes? If not, can we simplify to getStringPref?
> 
> Yes, it's still localized pref in funnelcakes/distros. So we can't simplify
> here.

Do we have plans to change this?

> > Can we entirely remove all the code reading order.extra and order prefs?
> 
> We could probably get away with removing extra if we always put the default
> at the top of the list (only Qwant uses two extra values). But we still need
> order prefs for other distros, including MozillaOnline and Yahoo.

That's annoying. Do we have to keep backward compatibility here, or could distributions be changed to store the default and order they want in a single pref (that could even be JSON matching the list.json format) ? (of course that would have to be a follow-up even if we can do it)

> > Can you think of a real case where a distribution would customize the order of search plugins, but not set its plugin as the default (which causes that plugin to be at the top of the list)?
> 
> No, but as indicated above, some distributions set an order that contains
> more than one plugin.

Can we restrict this code checking all these prefs to distributions, and return early if if we have no distribution id?

I'm also wondering if we should cache the distribution.id pref value. See also bug 1426135.

> Do you mean for this function to be nested inside of parseListJSON?

Yes, just a simple helper.

Comment 46

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/10-11/

Comment 47

[Florian Quèze [:florian]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review220854

It's not clear if comment 42 has been fully addressed (looks like it hasn't), and this patch would benefit from a self-review before being pushed for review (obvious indent error).

Comment 48

[Mike Kaply [:mkaply]]

(In reply to Florian Quèze [:florian] from comment #45)
> (In reply to Mike Kaply [:mkaply] from comment #43)
> > Comment on attachment 8932519 [details]
> > Bug 1352539 - Move default search engine and search order to list.json.
> > 
> > https://reviewboard.mozilla.org/r/203560/#review217764
> > 
> > > Is this still a localized pref if it's only set by distribution/funnel cakes? If not, can we simplify to getStringPref?
> > 
> > Yes, it's still localized pref in funnelcakes/distros. So we can't simplify
> > here.
> 
> Do we have plans to change this?
> 
> > > Can we entirely remove all the code reading order.extra and order prefs?
> > 
> > We could probably get away with removing extra if we always put the default
> > at the top of the list (only Qwant uses two extra values). But we still need
> > order prefs for other distros, including MozillaOnline and Yahoo.
> 
> That's annoying. Do we have to keep backward compatibility here, or could
> distributions be changed to store the default and order they want in a
> single pref (that could even be JSON matching the list.json format) ? (of
> course that would have to be a follow-up even if we can do it)

We need to keep backwards compatibility. We don't update distribution.ini files that are alread in the field. Note that the order can't match the JSON format anyway because JSON uses engine internal names and search order uses the actual name.

> > > Can you think of a real case where a distribution would customize the order of search plugins, but not set its plugin as the default (which causes that plugin to be at the top of the list)?
> > 
> > No, but as indicated above, some distributions set an order that contains
> > more than one plugin.
> 
> Can we restrict this code checking all these prefs to distributions, and
> return early if if we have no distribution id?

There's really no good way to return early in these various functions because they do post processing once they have the order from the various locations. I could make checking them distribution only. Are you thinking this due to hijacking?

> I'm also wondering if we should cache the distribution.id pref value. See
> also bug 1426135.

It boils down to how often we get the originalDefaultEngine. 

> 
> > Do you mean for this function to be nested inside of parseListJSON?
> 
> Yes, just a simple helper.

Comment 49

[Florian Quèze [:florian]]

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

> There's really no good way to return early in these various functions
> because they do post processing once they have the order from the various
> locations. I could make checking them distribution only. Are you thinking
> this due to hijacking?

I was thinking both about hijacking and performance (as apparently repeated .get*Pref calls are not completely cheap, due to crossing xpconnect).

Comment 50

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/11-12/

Comment 51

[Mike Kaply [:mkaply]]

This is ready for review. I undestand your concern about the preferences, but I think it's out of scope for this.

We're not making things any worse than they are and long term, they will be better since we will only read the preferences for distributions.

Comment 52

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review217764

> This implies a requirement that the in-tree list.json always has a default searchOrder defined.
> 
> I wonder if this needs to be documented, or if we need to put automated tests in place to detect errors when editing list.json.
> 
> Also, what happens if a locale customizes the list of search plugins that are shipped, and we take the default 'searchOrder' value that includes engines that we don't ship for this locale? Should the python script filter them out?

> Also, what happens if a locale customizes the list of search plugins that are shipped, and we take the default 'searchOrder' value that includes engines that we don't ship for this locale? Should the python script filter them out?

We ignore them in nsSearchService (It's always been this way).. We can't filter them out at build time the search order contains the actual proper name of the engine whereas what we have is the filename of the engine.

Filtering them out at runtime is a better way to do it because it allows us to specify a default order that applies even when one or more engines isn't present in that particular region (even within the particular locale). So the order could be A,B,C, but one region could have A,B and the other region could have A,C.

As far as default search order goes, you're right. I'll see about making this fail in that case.

> What happens if localeSearchInfo[region]["visibleDefaultEngines"] is set, and contains an engine that we need to override?
> 
> I think you need to replace this test with something like this:
>   enginesToOverride = set(regionOverrides[region].keys())
>   if region in localeSearchInfo and
>      "visibleDefaultEngines" in localeSearchInfo[region]:
>      visibleDefaultEngines = set(localeSearchInfo[region]["visibleDefaultEngines"])
>   else:
>      visibleDefaultEngines = set(localeSearchInfo["default"]["visibleDefaultEngines"])
>   if visibleDefaultEngines & enginesToOverride:

When I desigined regionOverrides, it was for cases where the locale explicitly wasn't in the list, but I see your point. Fixing.

> If a locale doesn't have a 'default' section with a 'visibleDefaultEngines' field, we may end up with an empty 'engines' set here.
> 
> If this happens, I wonder if we should fail loudly, or instead just ship the default engines. Doing the later seems as simple as replacing the 'else:' of the next line with a test checking if the 'engines' set is empty.

We should ship default. We had dicussed in the past that in theory we could not have locales in the list.json that didn't need to be there and just use the default. This would facilitate that (Assuming we can solve the "Too many wikipedias" problem. I have an idea for that).

Comment 53

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/12-13/

Comment 54

[Mike Kaply [:mkaply]]

So I've gone back to the original patch here. This got too complicated too fast.

This patch is only to move searchDefault into list.json. Nothing else.

Comment 55

[Florian Quèze [:florian]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review243422

Almost there :-)

::: browser/app/profile/firefox.js:398
(Diff revision 13)
>  
>  // Market-specific search defaults
>  pref("browser.search.geoSpecificDefaults", true);
>  pref("browser.search.geoSpecificDefaults.url", "https://search.services.mozilla.com/1/%APP%/%VERSION%/%CHANNEL%/%LOCALE%/%REGION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%");
>  
>  // US specific default (used as a fallback if the geoSpecificDefaults request fails).

I think "(used as a fallback if the geoSpecificDefaults request fails)" in this comment refers to the defaultenginename pref, not to the order prefs, so should be removed.

::: mobile/locales/search/list.json
(Diff revision 13)
> +        "searchDefault": "Yandex"
>        }
>      },
>      "trs": {
>        "default": {
> -        "searchDefault": "Google",

Ugh. Given the importance of this file for our business, I think we need to figure out a way to detect when there's unexpected stuff in it, and verify that the configuration in the file is valid for all locales. That's probably more something for a linter that works on our tree than for tests running on the compiled build. Anyway, that's for another bug.

::: toolkit/components/search/nsSearchService.js:3543
(Diff revision 13)
> +    } else {
> +      this._searchDefault = searchSettings.default.searchDefault;
> +    }
>    },
>  
>    _parseListTxt: function SRCH_SVC_parseListTxt(list, uris) {

Is it time to file a bug to remove this legacy list.txt code?

::: toolkit/components/search/tests/xpcshell/head_search.js:239
(Diff revision 13)
>  const kTestEngineName = "Test search engine";
>  const REQ_LOCALES_CHANGED_TOPIC = "intl:requested-locales-changed";
>  
>  function getDefaultEngineName(isUS) {
> -  const nsIPLS = Ci.nsIPrefLocalizedString;
> -  // Copy the logic from nsSearchService
> +   // The list of visibleDefaultEngines needs to match or the cache will be ignored.
> +   let chan = NetUtil.newChannel({

Using 3 spaces of indent looks broken here.

::: toolkit/components/search/tests/xpcshell/head_search.js:249
(Diff revision 13)
> +   let defaultEngineName = searchSettings.default.searchDefault;
> +
> -  if (isUS === undefined)
> +   if (isUS === undefined)
> -    isUS = Services.locale.getRequestedLocale() == "en-US" && isUSTimezone();
> +     isUS = Services.locale.getRequestedLocale() == "en-US" && isUSTimezone();
> -  if (isUS) {
> -    pref += ".US";
> +
> +   if (isUS && ("searchDefault" in searchSettings.US)) {

Is this dead code from a previous patch? I don't see how we could have a 'US' field in the searchSettings objects (that would only contain "default" or locale names, not country codes).

::: toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js:9
(Diff revision 13)
> +/* Check default search engine is picked from list.json searchDefault */
> +
> +"use strict";
> +
> +function run_test() {
> +  removeCacheFile();

xpcshell tests start with a clean profile, so this seems useless.

::: toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js:13
(Diff revision 13)
> +function run_test() {
> +  removeCacheFile();
> +
> +  Assert.ok(!Services.search.isInitialized, "search isn't initialized yet");
> +
> +  registerCleanupFunction(() => {

and this too.

::: toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js:22
(Diff revision 13)
> +  run_next_test();
> +}
> +
> +// Check that current engine matches with US searchDefault from list.json
> +add_task(async function test_searchDefaultEngineUS() {
> +  Services.prefs.setCharPref("browser.search.region", "US");

What's the expected impact of setting "browser.search.region" to "US" here and throughout this test file? I thought this patch was removing all the US-specific behaviors as far as the default engine is concerned.

::: toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js:81
(Diff revision 13)
> +add_task(async function test_defaultEngineNameDefaultPrefUS() {
> +  Services.prefs.setCharPref("distribution.id", "partner-test");
> +  Services.prefs.setCharPref("browser.search.region", "US");
> +
> +  // Set the browser.search.defaultenginename pref.
> +  Services.prefs.getDefaultBranch(null).setCharPref(kDefaultenginenamePref, "data:text/plain,browser.search.defaultenginename=Bing");

nit: wrap this long line.

Comment 56

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review243422

> Ugh. Given the importance of this file for our business, I think we need to figure out a way to detect when there's unexpected stuff in it, and verify that the configuration in the file is valid for all locales. That's probably more something for a linter that works on our tree than for tests running on the compiled build. Anyway, that's for another bug.

We have an bug for that. https://bugzilla.mozilla.org/show_bug.cgi?id=1445628

> Is it time to file a bug to remove this legacy list.txt code?

https://bugzilla.mozilla.org/show_bug.cgi?id=1300198

I still show SeaMonkey using it. Need to check.

> Is this dead code from a previous patch? I don't see how we could have a 'US' field in the searchSettings objects (that would only contain "default" or locale names, not country codes).

the searchSettings object is the actual list.json in product. So it looks like this:


	"default": {
		"searchDefault": "Google",
		"visibleDefaultEngines": ["google", "amazondotcom", "bing", "ddg", "ebay", "twitter", "wikipedia"]
	},
	"CA": {
		"visibleDefaultEngines": ["google", "amazondotcom", "bing", "ddg", "ebay-ca", "twitter", "wikipedia"]
	},
	"US": {
		"visibleDefaultEngines": ["google-2018", "amazondotcom", "bing", "ddg", "ebay", "twitter", "wikipedia"]
	}

And there could theoretically be an engine for US (and there was when I wrote the patch).

You can see it by going to resource://search-plugin/list.json.

> What's the expected impact of setting "browser.search.region" to "US" here and throughout this test file? I thought this patch was removing all the US-specific behaviors as far as the default engine is concerned.

There is still US specific behavior based on the country value in list.json. It just so happens that since US doesn't have a separate engine value it uses default, but we should test the common case (region = US)

> nit: wrap this long line.

That's a hard line to wrap and have it look good :)

Comment 57

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/13-14/

Comment 58

[Florian Quèze [:florian]]

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

> > Is it time to file a bug to remove this legacy list.txt code?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1300198
> 
> I still show SeaMonkey using it. Need to check.

This was filed 2 years ago, giving SeaMonkey a heads up that change is coming is nice, but we shouldn't indefinitely block on it.

Comment 59

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/14-15/

Comment 60

[Florian Quèze [:florian]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

https://reviewboard.mozilla.org/r/203560/#review243848

::: toolkit/components/search/tests/xpcshell/test_list_json_searchdefault.js:76
(Diff revision 15)
> +  Services.prefs.setCharPref("distribution.id", "partner-test");
> +  Services.prefs.setCharPref("browser.search.region", "US");
> +
> +  // Set the browser.search.defaultenginename pref.
> +  Services.prefs.getDefaultBranch(null).setCharPref(kDefaultenginenamePref,
> +                                                    "data:text/plain,browser.search.defaultenginename=Bing");

This is still pretty long. Here's another possible wrapping:
let defaultBranch = Services.prefs.getDefaultBranch(null);
defaultBranch.setCharPref(kDefaultenginenamePref,
                          "data...

Comment 61

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/15-16/

Comment 62

[Mike Kaply [:mkaply]]

Comment on attachment 8932519 [details]
Bug 1352539 - Move default search engine to list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203560/diff/16-17/

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8a023df39e2
Move default search engine to list.json. r=florian

Comment 64

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f8a023df39e2