1276740 - centralize all search plugins shipped in Firefox to mozilla-central

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Mike Connor [:mconnor]]

There are a few key problems with the current situation:

* Maintenance is high effort

Currently it takes days of manual work to apply changes to partner configs across repositories.  Yahoo, Ebay, and Amazon in particular have significant maintenance overhead.

* Duplication of plugins across repositories

This is especially noticed for European variants where many locales include UK/FR/ES variants for regional dialects. This compounds the problem above.

* Lack of clear oversight/visibility for core changes.

bug 1195683 has a few root causes, but one major issue was that the plugins lived in a different repo, so they didn't see the MozParam usage.

* No easy way to test international search plugins.

We have no automated tests covering international configurations, which creates a substantial revenue/financial risk.  The previously mentioned bug definitely cost Mozilla money. We shouldn't repeat that.

Comment 1

[Mike Kaply [:mkaply]]

This spreadsheet shows which engines are used where and which locales have engines named the same but with different content.

https://docs.google.com/spreadsheets/d/1VTdWNGW-UqO5U-8tsA11_r5l5fW_zB8Z6u3ciYFbi14/edit?usp=sharing

This document provides more detail on the cases where engines are named the same but have different content.

https://docs.google.com/document/d/1Y6695FMCBpp032kuGdjF_K0gL3FX4uKVYbeslfzLLJo/edit

It is Mozilla only right now due to some comments. I will cleanup and post in here.

Comment 2

[Mike Connor [:mconnor]]

Attachment: addLocalizedSearchplugins

patch against Aurora.  Just need to validate that all of the codes are current.

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Final engine list

This is what the final engine list looks like. I'm putting together the patch now.

Duplicates weren't as bad as I thought. See:

https://docs.google.com/spreadsheets/d/1VTdWNGW-UqO5U-8tsA11_r5l5fW_zB8Z6u3ciYFbi14/edit#gid=0

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Final final engine list

I went through and looked for engines that were not in the current list.json (based on your latest changes) and removed a few additional:

yandex-slovari.xml
wikipedia-sah.xml
wiktionary-oc.xml
wikipedia-oc.xml 
wikipedia-mn.xml 
wikipedia-ku.xml

I think all of the engines left here are used.

Comment 5

[Francesco Lodolo [:flod]]

Comment on attachment 8799946 [details]
Final final engine list

Almost there.

drae-es-AR.xml
We should use the es-ES version, the es-AR is just out of date (see comment in the doc)

enginelist.txt
Not relevant

These should be removed: they belong to csb, and removed from pl because not working or obsolete
fbc-pl.xml
merlin-pl.xml
wp-pl.xml

Missing searchplugins:
wikipedia-hi.xml
wikipedia-kab.xml
wikipedia-tl.xml
yahoo-tl.xml

I assume we should remove this, since we're ignoring Belarusian
yandex.by.xml

Comment 6

[Francesco Lodolo [:flod]]

Tried to check the diffs for duplicated searchplugins

leo-ende-de-rm
It should be updated following German, keeping only shortName and Description from the existing one

wikipedia-br
The same name is used for Breton (br) and Portuguese (pt-BR). pt-BR is basically identical to pt-PT (different description, out of date search URL), but at this point it would make sense to rename wikipedia-ptpt as wikipedia-pt

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Bug 1276740 - Centralize all search plugins into mozilla-central.

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

Comment 8

[Mike Kaply [:mkaply]]

Great finds. I've fixed everything and put the full review in.

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8800244 [details]
Bug 1276740 - Centralize all search plugins into mozilla-central.

https://reviewboard.mozilla.org/r/85222/#review83788

::: browser/locales/search/list.json:546
(Diff revision 1)
>        }
>      },
>      "pt-PT": {
>        "default": {
>          "visibleDefaultEngines": [
>            "google", "amazon-en-GB", "ddg", "priberam", "sapo", "wikipedia-ptpt"

Need to update this one too

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 8800244 [details]
Bug 1276740 - Centralize all search plugins into mozilla-central.

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

Comment 11

[Francesco Lodolo [:flod]]

Should we use this to fix the missing MPL headers or that's not useful at this point?

amazondotcom-de.xml
diribg.xml
findbook-zh-TW.xml
meta-ua.xml
metamarket.xml
portalbgdict.xml
reta-vortaro.xml
webdunia.xml
yahoo-bid-zh-TW.xml
yandex-uk.xml

Comment 12

[Mike Kaply [:mkaply]]

> Should we use this to fix the missing MPL headers or that's not useful at this point?

Absolutely. I'll add them in.

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 8800244 [details]
Bug 1276740 - Centralize all search plugins into mozilla-central.

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

Comment 14

[Florian Quèze [:florian]]

I think I already asked this in another bug but I don't remember the answer, so asking again: why are we storing the search plugins for all locales in the browser/locales/en-US/searchplugins/ path which seems to be specifically about en-US? Could/should we instead have them in browser/searchplugins/ ?

I think flod's review is enough for the actual content of the search plugins, so I'm not sure what's left to review for me.

Comment 15

[Mike Kaply [:mkaply]]

> I think I already asked this in another bug but I don't remember the answer, so asking again: why are we storing the search plugins for all locales in the browser/locales/en-US/searchplugins/ path which seems to be specifically about en-US? Could/should we instead have them in browser/searchplugins/ ?

The reason we are putting them in en-US to start is because it utilizes the existing Makefile process that checks en-US first and then looks at the locales, so it was an easy switch.

Long term, the plan is to move the engines into toolkit if possible (combining them with mobile).

But you bring up a good point. Maybe we should just go searchplugins now....

Comment 16

[Florian Quèze [:florian]]

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

> the
> existing Makefile process that checks en-US first and then looks at the
> locales

We don't want the Makefile to go find files in the locales anymore, right?

Comment 17

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> I think I already asked this in another bug but I don't remember the answer,
> so asking again: why are we storing the search plugins for all locales in
> the browser/locales/en-US/searchplugins/ path which seems to be specifically
> about en-US? Could/should we instead have them in browser/searchplugins/ ?

Actually, browser/locales/searchplugins/ would probably make more sense, as the searchplugins we ship do depend on the locale. My comment was mostly because it felt wrong to put in a "en-US" folder files that aren't related to that locale.

Comment 18

[Axel Hecht [:Pike]]

(In reply to Florian Quèze [:florian] [:flo] from comment #17)
> (In reply to Florian Quèze [:florian] [:flo] from comment #14)
> > I think I already asked this in another bug but I don't remember the answer,
> > so asking again: why are we storing the search plugins for all locales in
> > the browser/locales/en-US/searchplugins/ path which seems to be specifically
> > about en-US? Could/should we instead have them in browser/searchplugins/ ?
> 
> Actually, browser/locales/searchplugins/ would probably make more sense, as
> the searchplugins we ship do depend on the locale. My comment was mostly
> because it felt wrong to put in a "en-US" folder files that aren't related
> to that locale.

Yeah, browser/locales/searchplugins is a great path for these.

Comment 19

[Mike Kaply [:mkaply]]

Comment on attachment 8800244 [details]
Bug 1276740 - Centralize all search plugins into mozilla-central.

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

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8800244 [details]
Bug 1276740 - Centralize all search plugins into mozilla-central.

https://reviewboard.mozilla.org/r/85222/#review87298

::: browser/locales/Makefile.in:67
(Diff revision 4)
>  endif
>  
>  SEARCHPLUGINS_FILENAMES := $(shell $(PYTHON) $(srcdir)/searchplugins.py $(srcdir)/search/list.json $(AB_CD))
>  SEARCHPLUGINS_PATH := .deps/generated_$(AB_CD)
>  SEARCHPLUGINS_TARGET := libs searchplugins
> -SEARCHPLUGINS := $(foreach plugin,$(addsuffix .xml,$(SEARCHPLUGINS_FILENAMES)),$(or $(wildcard $(call EN_US_OR_L10N_FILE,searchplugins/$(plugin))),$(warning Missing searchplugin: $(plugin))))
> +SEARCHPLUGINS := $(foreach plugin,$(addsuffix .xml,$(SEARCHPLUGINS_FILENAMES)),$(or $(wildcard $(srcdir)/searchplugins/$(plugin)),$(warning Missing searchplugin: $(plugin))))

You are removing the only use of the EN_US_OR_L10N_FILE macro, so we should probably remove it from http://searchfox.org/mozilla-central/source/config/config.mk#489

But it's used by mail/ and im/ until these changes are ported there, so maybe we can leave the cleanup for a future bug.

I'm also wondering if the .xml suffix should be added directly by searchplugins.py, but that too can be handled in another bug if we think it's a good idea.

Comment 21

[Francesco Lodolo [:flod]]

@Mike
Since it hasn't landed yet, can you include changes to ru/mailru.xml and ne-NP/wikipedia-ne.xml in this bug?

Comment 22

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bef3f82e596841daa19a9900c7f3e2912725dd4
Bug 1276740 - Centralize all search plugins into mozilla-central. r=florian

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4bef3f82e596

Comment 24

[Mike Kaply [:mkaply]]

Attachment: Change to fix artifact builds

When I moved the plugins to browser/searchplugins, I didn't update artifact builds in jar.mn.

Comment 25

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fab72ee7bbd16fdbcec17950c4ae0fe8fe13060
Bug 1276740 - Update jar.mn with new location for artifact builds. r=Standard8

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0fab72ee7bbd

Comment 27

[Simona Badau, Desktop QA]

Verified as fixed on the latest Nightly 52 - manually verified that the xml files of the search engines remain the same on the top 11 locales + a few more like: de, hsb, dsb, rm, es-AR, DA (for more details, please see phase 2 from: https://wiki.mozilla.org/QA/Partner_Search#Scope_of_Testing).

Verified as fixed on: Mac OS X 10.12, Ubuntu 16.04 x64 and Windows 10 x64.