centralize all search plugins shipped in Firefox to mozilla-central

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Search
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: mconnor, Assigned: mkaply)

Tracking

(Blocks: 3 bugs)

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [partner search] [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 1276741
(Reporter)

Updated

2 years ago
Blocks: 1276743
(Reporter)

Updated

2 years ago
Blocks: 1276745
(Assignee)

Comment 1

2 years ago
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.

Updated

2 years ago
Whiteboard: [partner search] [fxsearch]
(Reporter)

Comment 2

2 years ago
Created attachment 8762094 [details] [diff] [review]
addLocalizedSearchplugins

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

Comment 3

a year ago
Created attachment 8799823 [details]
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
Attachment #8799823 - Flags: review?(francesco.lodolo)
(Assignee)

Comment 4

a year ago
Created attachment 8799946 [details]
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.
Attachment #8799823 - Attachment is obsolete: true
Attachment #8799823 - Flags: review?(francesco.lodolo)
Attachment #8799946 - Flags: review?(francesco.lodolo)
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
Attachment #8799946 - Flags: review?(francesco.lodolo) → review-
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 hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
Great finds. I've fixed everything and put the full review in.
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 hidden (mozreview-request)
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
Assignee: mconnor → mozilla
(Assignee)

Comment 12

a year ago
> 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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8800244 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8800244 - Flags: review?(mconnor)
Attachment #8800244 - Flags: review?(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.
Flags: needinfo?(mozilla)
(Assignee)

Comment 15

a year ago
> 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....
Flags: needinfo?(mozilla)
(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?
(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.
(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 hidden (mozreview-request)

Comment 20

a year ago
mozreview-review
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.
Attachment #8800244 - Flags: review?(florian) → review+
@Mike
Since it hasn't landed yet, can you include changes to ru/mailru.xml and ne-NP/wikipedia-ne.xml in this bug?
(Assignee)

Comment 22

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

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4bef3f82e596
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Blocks: 1314235
(Assignee)

Comment 24

a year ago
Created attachment 8806421 [details] [diff] [review]
Change to fix artifact builds

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

Updated

a year ago
Attachment #8806421 - Flags: review?(standard8)
Attachment #8806421 - Flags: review?(standard8) → review+
(Assignee)

Comment 25

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

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0fab72ee7bbd
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.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.