1437942 - Remove search engines from language packs

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Mike Kaply [:mkaply]]

The end goal for the search JSON work was to move to a place where we shipped all searcn engines in Firefox and did the right thing regardless of your language (Region focus).

It's time to finally do that.

We should package all engines in the base product. Language packs should be just language.

This will mean moving to a model where the entire list.json file is packaged in Firefox and parsed at runtime (including regionOverrides).

This was my original vision, but we just weren't there yet with multilocale Firefox. We're getting there now, so it's time to change.

Comment 1

[(not currently active) Ted Mielczarek]

From a build system perspective this sounds great. :) We've been working on moving things out of Makefile.in files into moz.build files, but I punted on the searchplugins stuff in bug 1407427 because we didn't have any moz.build construct that would work for it. If you want to fix it to not be localized instead that would also solve our problem! I'd be happy to advise or help out with any build system issues here.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

For the build system I think we need a metal concept of "different list of locales to package" per data piece.

For example, we want to build Firefox with the following language resources: ["en-US"] and the following searchplugins: [...].

In the future we may want to package, say, 4 locales for a Swiss Firefox and 100 searchplugins and 100 crashreporter/installer languages (because other languages can be installed via langpacks but won't cover installer/crashreporter).

Haveing ability to decide what locales to package for what dataset separately would help us get to a multilocale builds.

(In this case, instead of thinking of searchplugins as "package them all", I'd like us to think about it as the first example of a dataset which will get it's own list of locales to package)

Comment 3

[Nick Alexander :nalexander [he/him]]

mkaply: can you help me understand what the differences between list.json (from https://bugzilla.mozilla.org/show_bug.cgi?id=1300201) and browsersearch.json (see https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/action/generate_browsersearch.py) are?

The former is clearly for nsSearchService and the latter is for Android resources.  But there seems to be a decent amount of duplication, and I'd like to see some unification here.  What's the desired end state?

Comment 4

[Nick Alexander :nalexander [he/him]]

Sorry, BZ interface to :ni sucks -- mkaply, above, please.

Comment 5

[Mike Kaply [:mkaply]]

I think at one point I tried to figure out the browsersearch.json stuff and how it all worked and ended up punting.

The proper end state here would be one file (list.json) that would be the source of truth for all of our products (even Focus).

It would be used as the basis for generating everything.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Kaply [:mkaply] from comment #5)
> I think at one point I tried to figure out the browsersearch.json stuff and
> how it all worked and ended up punting.

It's rather complicated only because it needs to be localized.  In essence it walks some per-locale lists with a fallthrough mechanism to use defaults in the case that locales don't specify things, collecting tidbits of data.

My guess is that locales are not really using the per-locale features now, although we could determine that fairly easily.

> The proper end state here would be one file (list.json) that would be the
> source of truth for all of our products (even Focus).
> 
> It would be used as the basis for generating everything.

OK, that's helpful.  Is list.json constant across locales?  (I think this is a goal of the search team, although I don't have a ticket tracking this.  Link appreciated.)

One wrinkle of browsersearch.json is that the some of the entries identify Android resources that are *very* Fennec specific -- they are literally names of files in the Fennec source tree.  Presumably that doesn't play well with a global "basis for generating everything", and will need some consideration to unify.

The build system could do things here if needed, like extract base64 encoded image data or SVG data from the JSON and produce the required files for the Android APK.  Although I would prefer to significantly simplify the Fennec feature rather than build warts into list.json that do not have a future.

Comment 7

[Mike Kaply [:mkaply]]

(In reply to Nick Alexander :nalexander from comment #6)
> (In reply to Mike Kaply [:mkaply] from comment #5)
> > The proper end state here would be one file (list.json) that would be the
> > source of truth for all of our products (even Focus).
> > 
> > It would be used as the basis for generating everything.
> 
> OK, that's helpful.  Is list.json constant across locales?  (I think this is
> a goal of the search team, although I don't have a ticket tracking this. 
> Link appreciated.)

Yes. There are two list.json files for browser and mobile that contain the information for each locale.
They don't contain searchDefault and searchOrder yet, but that's in the works:

https://searchfox.org/mozilla-central/source/browser/locales/search/list.json
https://searchfox.org/mozilla-central/source/mobile/locales/search/list.json

The list.json files that are a part of the build are generated based on those files, but there's no reason we couldn't just use that file as is.

> One wrinkle of browsersearch.json is that the some of the entries identify
> Android resources that are *very* Fennec specific -- they are literally
> names of files in the Fennec source tree.  Presumably that doesn't play well
> with a global "basis for generating everything", and will need some
> consideration to unify.

I just took a quick glance at browsersearch.json and saw this:

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

Is there sometimes something different? Note this is using names instead of engines.

> 
> The build system could do things here if needed, like extract base64 encoded
> image data or SVG data from the JSON and produce the required files for the
> Android APK.  Although I would prefer to significantly simplify the Fennec
> feature rather than build warts into list.json that do not have a future.

So it seems to be browsersearch.json is really only used to make SearchEngineManager.java work? 

Thats another thing I've never completely understood. SearchEngineManager.java versus the nsSearchService.js being used by Gecko.

Are we not using the opensearch files in Fennec in some cases?

Comment 9

[Mike Kaply [:mkaply]]

Attachment: First pass

So here's the basic patch.

Some things we need to think about.

1. What happens when you switch languages dynamically? Do we swap out all the default engine and leave user added engines alone? Do we have any way to do that?

2. What if your default engine is not in the new language?

(Note we probably have this problem today, it just happens on restart)

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

> 1. What happens when you switch languages dynamically? Do we swap out all the default engine and leave user added engines alone? Do we have any way to do that?

That's a great question for product. Should we? Is search engine selection based on language really, or region or a combination of those two (locale)?

What if I use French Firefox in Germany? Should I get French or German search engines? :)

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Also, I think you can remove `REQ_LOCALES_CHANGED_TOPIC` - there's no reason to rescan when user changes their requested locales if we can't update the app-locales in result. And if we do, we'll fire the app-locales-changed event.

Comment 12

[Mike Kaply [:mkaply]]

To answer my own questions.

When you switch locales, on restart, you get all your default engines replaced with the ones from the new locale.

All custom added engines stay behind.

You default is maintained unless it's not there anymore and then it gets switched to the default for that language.

So it looks like we work the way we should, we just need to make it dynamic.

Comment 13

[Mike Kaply [:mkaply]]

> Also, I think you can remove `REQ_LOCALES_CHANGED_TOPIC` - there's no reason to rescan when user changes their requested locales if we can't update the app-locales in result. And if we do, we'll fire the app-locales-changed event.

This was only done for Android it looks like. So if a user changed their locale on their phone, we updated properly.

Is that not still the case?

Comment 14

[Mike Kaply [:mkaply]]

So intl:app-locales-changed is working perfectly.

The awesome bar engines don't update when it is changed, but if I open a new window, I see the changes.

So we're going to need a separate bug to have the one off search engines update when the locale changes.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Is that not still the case?

We should broadcast `intl:app-locales-changed` on Android as well *if* the change of the OS locales impacted Firefox locale selection. :)

> So intl:app-locales-changed is working perfectly.

Yay for unified locale management :)

Comment 16

[Mike Kaply [:mkaply]]

Attachment: Bug 1437942 - Bundle all search engines with Firefox.

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

Comment 17

[Mike Kaply [:mkaply]]

This new patch makes the one off searches change dynamically. It's pretty cool.

I'm going to wait until Florian comes back to get this reviewed.

As far as Android goes, I'm leaving that code alone for now until I can test it more.

Comment 18

[Mike Kaply [:mkaply]]

Note I still need to do the android version of the build patch.

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8980632 [details]
Bug 1437942 - Bundle all search engines with Firefox.

https://reviewboard.mozilla.org/r/246786/#review253086

This would save me pushing the try builds for doing this l10n+search stuff better at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b146fc37fde75bfa36167afbcbe4aecdf4ba355 across the line, so I'd love to see it get priority.

::: browser/locales/Makefile.in:64
(Diff revision 1)
>  
>  include $(topsrcdir)/config/rules.mk
>  
>  include $(topsrcdir)/toolkit/locales/l10n.mk
>  
> -$(list-json): $(call mkdir_deps,$(SEARCHPLUGINS_PATH)) $(if $(IS_LANGUAGE_REPACK),FORCE)
> +searchplugins:: $(srcdir)/search/list.json

You can get rid of quite a bit more here -- basically, everything searchplugin-y.

Comment 20

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1437942 - Bundle all search engines with Firefox.

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

Comment 21

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1437942 - Part 2a: Move browser/ searchplugins and search/list.json to browser/components/search/searchplugins.

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

Comment 22

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1437942 - Part 2b: Make browser/ resource://search-plugins reference srcdir.

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

Comment 23

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1437942 - Pre: Drop unused variable.

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

Comment 24

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1437942 - Part 3a: Move mobile/android searchplugins and search/list.json to mobile/android/components/search/searchplugins.

This is just the `hg mv` parts.

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

Comment 25

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1437942 - Part 3b: Make mobile/android resource://search-plugins reference srcdir.

So nice to purge some of the worst Make code in the entire tree.

It's unfortunate to expand the duplicate whitelist, but
{ddg,duckduckgo}.xml is a business requirement.  We expect to clean
bolcom*.xml in follow-up.

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

Comment 26

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8980809 [details]
Bug 1437942 - Pre: Drop unused variable.

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

Comment 27

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8980807 [details]
Bug 1437942 - Part 2a: Move browser/ searchplugins and search/list.json to browser/components/search/searchplugins.

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

Comment 28

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8980808 [details]
Bug 1437942 - Part 2b: Make browser/ resource://search-plugins reference srcdir.

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

Comment 29

[Nick Alexander :nalexander [he/him]]

Try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a9dcf45174289e641a10706e089547c056e4a22 looks pretty healthy, but we'll need to update the "all files referenced" test at https://searchfox.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js#38.

Comment 30

[Mike Kaply [:mkaply]]

Comment on attachment 8980817 [details]
Bug 1437942 - Part 3b: Make mobile/android resource://search-plugins reference srcdir.

https://reviewboard.mozilla.org/r/246998/#review253762

::: mobile/android/components/search/moz.build:10
(Diff revision 1)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +JAR_MANIFESTS += ['jar.mn']
> +
> +with Files('**'):
> +    BUG_COMPONENT = ('Firefox', 'Awesomescreen')

Is this BUG_COMPONENT right?

Comment 31

[Mike Kaply [:mkaply]]

Comment on attachment 8980816 [details]
Bug 1437942 - Part 3a: Move mobile/android searchplugins and search/list.json to mobile/android/components/search/searchplugins.

https://reviewboard.mozilla.org/r/246996/#review253764

Comment 32

[Mike Kaply [:mkaply]]

Comment on attachment 8980807 [details]
Bug 1437942 - Part 2a: Move browser/ searchplugins and search/list.json to browser/components/search/searchplugins.

https://reviewboard.mozilla.org/r/246976/#review253766

Comment 33

[Mike Kaply [:mkaply]]

Comment on attachment 8980808 [details]
Bug 1437942 - Part 2b: Make browser/ resource://search-plugins reference srcdir.

https://reviewboard.mozilla.org/r/246978/#review253770

::: browser/components/search/jar.mn:10
(Diff revision 2)
>  browser.jar:
>          content/browser/search/search.xml                           (content/search.xml)
>          content/browser/search/searchReset.xhtml                    (content/searchReset.xhtml)
>          content/browser/search/searchReset.js                       (content/searchReset.js)
> +
> +        searchplugins/                                              (searchplugins/**)

What do two stars mean in this context?

Comment 34

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Kaply [:mkaply] from comment #33)
> Comment on attachment 8980808 [details]
> Bug 1437942 - Part 2b: Make browser/ resource://search-plugins reference
> srcdir.
> 
> https://reviewboard.mozilla.org/r/246978/#review253770
> 
> ::: browser/components/search/jar.mn:10
> (Diff revision 2)
> >  browser.jar:
> >          content/browser/search/search.xml                           (content/search.xml)
> >          content/browser/search/searchReset.xhtml                    (content/searchReset.xhtml)
> >          content/browser/search/searchReset.js                       (content/searchReset.js)
> > +
> > +        searchplugins/                                              (searchplugins/**)
> 
> What do two stars mean in this context?

It's a deep recursion through paths.  So searchplugins/*, searchplugins/foo/*, searchplugins/foo/bar/*, etc.

Comment 35

[Mike Kaply [:mkaply]]

Attachment: Bug 1437942 - Remove search engines from langpacks.

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

Comment 36

[Code Review Bot [:reviewbot]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

https://reviewboard.mozilla.org/r/248220/#review254472


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/search/nsSearchService.js:3414
(Diff revision 1)
>        return;
>      }
>  
> +    let searchSettings;
> +    let locale = Services.locale.getAppLocaleAsBCP47();
> +    Components.utils.reportError("*****************" + locale);

Error: Use cu rather than components.utils [eslint: mozilla/use-cc-etc]

Comment 37

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 38

[Drew Willcoxon :adw]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

https://reviewboard.mozilla.org/r/248220/#review254564

::: toolkit/components/search/nsSearchService.js:3419
(Diff revision 2)
> +    if ("locales" in json &&
> +        locale in json.locales) {
> +      searchSettings = json.locales[locale];
> +    } else {
> +      // json.default should always be there.
> +      // Should we assert?

What does this first comment mean?  "default" should have been in json, so it's weird that we ended up in the else-branch; or, it's OK we're in the else-branch because "default" will (or should) be in json?

If the json doesn't look like what you expect, I would think we should LOG() and return like we do in the code right above this.

Comment 39

[(not currently active) Ted Mielczarek]

Comment on attachment 8980808 [details]
Bug 1437942 - Part 2b: Make browser/ resource://search-plugins reference srcdir.

https://reviewboard.mozilla.org/r/246978/#review255194

Nothing makes me happier than patches that remove a bunch of Makefile rules!

::: browser/locales/moz.build
(Diff revision 2)
>  
> -with Files("search/**"):
> -    BUG_COMPONENT = ("Firefox", "Search")
> -
> -with Files("searchplugins/**"):
> -    BUG_COMPONENT = ("Firefox", "Search")

Do these files wind up under some other bug component if you remove this?

Comment 40

[(not currently active) Ted Mielczarek]

Comment on attachment 8980809 [details]
Bug 1437942 - Pre: Drop unused variable.

https://reviewboard.mozilla.org/r/246980/#review255196

Comment 41

[(not currently active) Ted Mielczarek]

Comment on attachment 8980817 [details]
Bug 1437942 - Part 3b: Make mobile/android resource://search-plugins reference srcdir.

https://reviewboard.mozilla.org/r/246998/#review255198

This patch is even better than the other one!

Comment 43

[Nick Alexander :nalexander [he/him]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #39)
> Comment on attachment 8980808 [details]
> Bug 1437942 - Part 2b: Make browser/ resource://search-plugins reference
> srcdir.
> 
> https://reviewboard.mozilla.org/r/246978/#review255194
> 
> Nothing makes me happier than patches that remove a bunch of Makefile rules!
> 
> ::: browser/locales/moz.build
> (Diff revision 2)
> >  
> > -with Files("search/**"):
> > -    BUG_COMPONENT = ("Firefox", "Search")
> > -
> > -with Files("searchplugins/**"):
> > -    BUG_COMPONENT = ("Firefox", "Search")
> 
> Do these files wind up under some other bug component if you remove this?

Yes -- browser/components/search is already listed under "Firefox", "Search".

Comment 44

[Nick Alexander :nalexander [he/him]]

mkaply has a few issues to address in his patch, and then will squash these all down to land as a single atomic commit (with r=adw,ted.mielczarek).  We can't bisect through this series so this is necessary.

Comment 45

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 46

[Mike Kaply [:mkaply]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54b52472b1653b354d7dd5e603deb56824b9f971

Comment 47

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 48

[Code Review Bot [:reviewbot]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

https://reviewboard.mozilla.org/r/248220/#review255272


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/search/nsSearchService.js:3421
(Diff revision 3)
> +      searchSettings = json.locales[locale];
> +    } else {
> +      // No locales were found, so use the JSON as is.
> +      // It should have a default section.
> +      if (!("default" in json)) {
> +        LOG("missing default in list.json: " + e);

Error: 'e' is not defined. [eslint: no-undef]

Comment 49

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 50

[Florian Quèze [:florian]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

https://reviewboard.mozilla.org/r/248220/#review255408

::: browser/modules/ContentSearch.jsm:199
(Diff revision 5)
>        });
>        this._processEventQueue();
>        break;
> +    case "browser-search-service":
> +      if (data == "init-complete") {
> +        this._eventQueue.push({

nit: You can avoid duplicating this by moving the browser-search-service case before browser-search-engine-modified, and using break only when data != "init-complete".

::: toolkit/components/search/nsSearchService.js:3421
(Diff revision 5)
> +      searchSettings = json.locales[locale];
> +    } else {
> +      // No locales were found, so use the JSON as is.
> +      // It should have a default section.
> +      if (!("default" in json)) {
> +        LOG("missing default in list.json");

When do we expect this to happen?

::: toolkit/components/search/nsSearchService.js:3513
(Diff revision 5)
>      if (searchRegion && searchRegion in searchSettings &&
>          "searchDefault" in searchSettings[searchRegion]) {
>        this._searchDefault = searchSettings[searchRegion].searchDefault;
> -    } else {
> +    } else if ("searchDefault" in searchSettings.default) {
>        this._searchDefault = searchSettings.default.searchDefault;
> +    } else if ("searchDefault" in json.default) {

Why is this "if ("searchDefault" in json.default)" test needed? In which case would this be false? If there are realistic chances of it happening, should we fail loudly in that case where we end up without any search default?

Comment 51

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

https://reviewboard.mozilla.org/r/248220/#review255408

> nit: You can avoid duplicating this by moving the browser-search-service case before browser-search-engine-modified, and using break only when data != "init-complete".

I tried to do that, but I thought the eslinter didn't like it. I'll check again.

> When do we expect this to happen?

This should really never happen. What is the right thing to do in our Javascript code in situations like this? This is really an "assert"

> Why is this "if ("searchDefault" in json.default)" test needed? In which case would this be false? If there are realistic chances of it happening, should we fail loudly in that case where we end up without any search default?

It really can never happen so we should fail loudly.

Comment 52

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 53

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 54

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4a2094a00c5
Remove search engines from langpacks. r=adw.mielczarek

Comment 56

[Arthur Iakab [arthur_iakab]]

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

Comment 57

[Cosmin Sabou [:CosminS]]

Hello! :flod brought this to the sheriffs attention:
looks like mozilla-central and mozilla-unified are out of sync? 
https://hg.mozilla.org/mozilla-central/file/default/browser/locales vs https://hg.mozilla.org/mozilla-unified/file/central/browser/locales
Is this ok to be like this? And if not what's to be done in this case? Thank you.

Comment 58

[Francesco Lodolo [:flod]]

Filed bug 1467025

Comment 59

[Narcis Beleuzu [:NarcisB]]

Backed out as requested by pascalc.

Backout link: https://hg.mozilla.org/mozilla-central/rev/78c1a9ad32c674dc272490993c7ec5a259ac84f5

Comment 60

[Mark Banner (:standard8)]

This was backed out due to l10n bustages, e.g. bug 1467040.

Comment 61

[Mark Banner (:standard8)]

I did some initial investigation into this. It looks like the *.xml files aren't being included in the localised builds.

My suspicion would be this line in jar.mn (and the android jar.mn):

https://hg.mozilla.org/mozilla-central/rev/f4a2094a00c5#l4.13

        searchplugins/                                              (searchplugins/**)

From looking on searchfox, we only seem to use a double star when doing something like (%browser/**/*.ftl), in all other cases of including directory contents we use a single star.

I'm not 100% sure if a single star would include the images/ directory though (I hope it would), maybe look through some of the existing usages or ask a build config person.

Comment 62

[Francesco Lodolo [:flod]]

Side note: we have localized builds on try now (for 5 locales).

Comment 63

[Axel Hecht [:Pike]]

Pretty sure that the hard-coded "let's repack searchplugins" in https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/l10n-repack.py#18 needs to go to fix this.

Mark, thanks for the investigation, knowing what's missing made it easier to find why.

Comment 64

[Pascal Chevrel:pascalc]

FYI, the respun builds work correctly and missing search engines was not the only bug reported, the "open in a new tab" context menu item was no longer working for localized builds and right-clicking on a link was proposing weird context menu items such as "add to dictionary". The awesome bar had also lost all of its history. All these bugs disappeared when the patch was backed out, mentioning in case it matters for the next landing of the patch.

Comment 65

[Francesco Lodolo [:flod]]

(In reply to Pascal Chevrel:pascalc from comment #64)
> FYI, the respun builds work correctly and missing search engines was not the
> only bug reported, the "open in a new tab" context menu item was no longer
> working for localized builds and right-clicking on a link was proposing
> weird context menu items such as "add to dictionary". The awesome bar had
> also lost all of its history. All these bugs disappeared when the patch was
> backed out, mentioning in case it matters for the next landing of the patch.

That happens when we break search (e.g. for Italian in ESR 52), and that's a bit surprising. 
I think the browser should still be functional, even if search plugins are missing for some reasons.

Comment 66

[Mike Kaply [:mkaply]]

(In reply to Axel Hecht [:Pike] from comment #63)
> Pretty sure that the hard-coded "let's repack searchplugins" in
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/
> l10n-repack.py#18 needs to go to fix this.
> 
> Mark, thanks for the investigation, knowing what's missing made it easier to
> find why.

That first searchplugins reference there is from when we used to have a top level searchplugins directory (and should probably be removed).

I would think the chrome/**/searchplugins/*.xml line would grab the rest of the search engines.

What's odd is that we are grabbing the list.json and the images directory, so it's almost like we're excluding the files.

Comment 67

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Banner (:standard8) from comment #61)
> I did some initial investigation into this. It looks like the *.xml files
> aren't being included in the localised builds.
> 
> My suspicion would be this line in jar.mn (and the android jar.mn):
> 
> https://hg.mozilla.org/mozilla-central/rev/f4a2094a00c5#l4.13
> 
>         searchplugins/                                             
> (searchplugins/**)
> 
> From looking on searchfox, we only seem to use a double star when doing
> something like (%browser/**/*.ftl), in all other cases of including
> directory contents we use a single star.
> 
> I'm not 100% sure if a single star would include the images/ directory
> though (I hope it would), maybe look through some of the existing usages or
> ask a build config person.

I'm pretty confident that ** is deep pattern matching, so that **/* is equivalent, but I can definitely verify.  (In my local builds, I'm quite confident it is.)

I think it's much more likely that there's so l10n-repacking stuff that has to be removed.  mkaply and I will iterate on it.

Comment 68

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 69

[Mike Kaply [:mkaply]]

Comment on attachment 8982277 [details]
Bug 1437942 - Remove search engines from langpacks.

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

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67f15f445506
Remove search engines from langpacks. r=adw

Comment 71

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/67f15f445506

Comment 72

[Mark Banner (:standard8)]

Comment on attachment 8980817 [details]
Bug 1437942 - Part 3b: Make mobile/android resource://search-plugins reference srcdir.

Dropping obsolete review request.