Closed Bug 1437942 Opened 6 years ago Closed 6 years ago

Remove search engines from language packs

Categories

(Firefox :: Search, enhancement, P1)

Product:
Firefox
Component:
Search
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 + fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

First pass
8.78 KB, patch
Details | Diff | Splinter Review
Bug 1437942 - Bundle all search engines with Firefox.
59 bytes, text/x-review-board-request
Details
Bug 1437942 - Part 2a: Move browser/ searchplugins and search/list.json to browser/components/search/searchplugins.
59 bytes, text/x-review-board-request
mkaply
: review+
Details
Bug 1437942 - Part 2b: Make browser/ resource://search-plugins reference srcdir.
59 bytes, text/x-review-board-request
ted
: review+
mkaply
: review+
Details
Bug 1437942 - Pre: Drop unused variable.
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1437942 - Part 3a: Move mobile/android searchplugins and search/list.json to mobile/android/components/search/searchplugins.
59 bytes, text/x-review-board-request
mkaply
: review+
Details
Bug 1437942 - Part 3b: Make mobile/android resource://search-plugins reference srcdir.
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1437942 - Remove search engines from langpacks.
59 bytes, text/x-review-board-request
adw
: review+
Details 
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.
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.
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)
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?
Sorry, BZ interface to :ni sucks -- mkaply, above, please.
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(mozilla)
(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.
(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?
Status: NEW → ASSIGNED
Priority: -- → P1
Depends on: 1461345
Depends on: 1460419
No longer depends on: 1461345
Attached patch First pass — — Splinter Review 
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)
> 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? :)
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.
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.
> 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?
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.
> 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 :)
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.
Note I still need to do the android version of the build patch.
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.
Attachment #8980809 - Flags: review?(core-build-config-reviews) → review?(ted)
Attachment #8980808 - Flags: review?(core-build-config-reviews) → review?(ted)
Attachment #8980817 - Flags: review?(core-build-config-reviews) → review?(ted)
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 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
Attachment #8980816 - Flags: review?(mozilla) → review+
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
Attachment #8980807 - Flags: review?(mozilla) → review+
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?
Attachment #8980808 - Flags: review?(mozilla) → review+
Attachment #8980632 - Attachment is obsolete: true
(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 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 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.
Attachment #8982277 - Flags: review?(adw) → review+
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?
Attachment #8980808 - Flags: review?(ted) → review+
Comment on attachment 8980809 [details]
Bug 1437942 - Pre: Drop unused variable.

https://reviewboard.mozilla.org/r/246980/#review255196
Attachment #8980809 - Flags: review?(ted) → review+
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!
Attachment #8980817 - Flags: review?(ted) → review+
(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".
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 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 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 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.
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/f4a2094a00c5
Remove search engines from langpacks. r=adw.mielczarek
https://hg.mozilla.org/mozilla-central/rev/f4a2094a00c5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
This was backed out due to l10n bustages, e.g. bug 1467040.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1467040
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.
Side note: we have localized builds on try now (for 5 locales).
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.
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.
(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.
Depends on: 1467046
(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.
(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.
Attachment #8982277 - Flags: review?(ted) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/67f15f445506
Remove search engines from langpacks. r=adw
https://hg.mozilla.org/mozilla-central/rev/67f15f445506
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Comment on attachment 8980817 [details]
Bug 1437942 - Part 3b: Make mobile/android resource://search-plugins reference srcdir.

Dropping obsolete review request.
Attachment #8980817 - Flags: review?(mozilla)
Depends on: 1489820
Blocks: 1315953
You need to log in before you can comment on or make changes to this bug.