1486820 - [meta] Convert builtin opensearch files to webextensions

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Shane Caraveo (:mixedpuppy)]

Either manually or write a script to create extensions from opensearch xml files.  Script may be useful later.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: wikipedia.xpi

This is a wikipedia extension based on builtin opensearch files, only 3 locales converted, but illustrates how extensions will be structured.

Comment 2

[Tian, Sheng]

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> Created attachment 9004590 [details]
> wikipedia.xpi
> 
> This is a wikipedia extension based on builtin opensearch files, only 3
> locales converted, but illustrates how extensions will be structured.

In the example, search_url contains `ml={moz:locale}&mdi={moz:distributionID}&mo={moz:official}` which is not supported on latest Nightly. Should these be removed or be supported (maybe only for built-in search engines) later?

The example configured is_default as true, is this intend behavior?

Comment 3

[Tian, Sheng]

The Malayalam locale includes wikipedia both in English and Malayalam.

Source: https://searchfox.org/mozilla-central/source/browser/components/search/searchplugins/list.json#572

If you convert wikipedia search in different languages into one extension. You have to add support for multiple search engine for web extension, or hack this by adding a special wikipedia-ml extension, or change the default search list currently shipped.

Comment 4

[Dale Harvey (:daleharvey)]

Going through that list, wikipedia is the only one we do that with and we do it twice
https://searchfox.org/mozilla-central/source/browser/components/search/searchplugins/list.json#110

So that alone it would seem fine to change the default search list, but it would probably be prudent to figure out if possible if having multiple copies of the same search engine for different locales is a thing users really like, if so we may want to consider doing a 1:1 extension per plugin instead of wrapping up multiple locales

I have started on a script to convert the opensearch files to webextensions @ https://github.com/daleharvey/ffx-opensearch-to-webext/, only works for the basics (and wikipedia) at the moment, going to go through the rest of the list and highlight any issues. 

First issue is that it seems web extensions only want http(s) favicons, seems like we would want data uris to be supported here right?

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Tian, Sheng from comment #3)
> The Malayalam locale includes wikipedia both in English and Malayalam.
> 
> Source:
> https://searchfox.org/mozilla-central/source/browser/components/search/
> searchplugins/list.json#572
> 
> If you convert wikipedia search in different languages into one extension.
> You have to add support for multiple search engine for web extension, or
> hack this by adding a special wikipedia-ml extension, or change the default
> search list currently shipped.

The extension is localized, so it would "include" both ml and en.  Does the user get access to both at the same time, or is one selected depending on what the locale of the browser is at that time?  If they have access to both in the UI at the same time, we'll have to figure something out to support that.  

search_provider does specify "alternate_urls" which can be used in place of the "search_url".  Would it be sufficient if the UI provided access to an alternate_url for en (which would result in an english search page), but the UI was sill in ml?

We'll have to look at the moz:* params, we should support those if they are indeed necessary.

"is_default" is handled different in firefox, and isn't how we'll set defaults for distributions, instead I think we should rely on the list.json file for now.

Icons should be included in the extension, so I would fetch those and include them.

Comment 6

[Tian, Sheng]

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> Does the user get access to both at the same time, or is one selected
> depending on what the locale of the browser is at that time?

Currently, user may get access to the both at the same time.
BTW, they are sharing the same icon in the one-off menu. (Name of the search engine was localized but not shown in the one-off menu.)

> Would it be sufficient if the UI provided access to an alternate_url for en
> (which would result in an english search page), but the UI was sill in ml?

I'm not a Malayalam user. I just try to point out this from source code. Personally, I would config an English version in my search engines list in additional of the one in my native language. That's because English Wikipedia has more and better articles.

And another question is which language should be the "default" one in the extension? For example, if a user using Japanese Firefox try to install Yandex, a webextension based search engine. Should it be configured as searching English or Russian? Both answer is reasonable to me.

Comment 7

[Shane Caraveo (:mixedpuppy)]

note: bug 1486819 has changes in the manifest format to better support params.

Comment 8

[Dale Harvey (:daleharvey)]

Attachment: dist.zip

Update the script, still got a few details to work through, invalid icons etc but this most of these should be good

Comment 9

[Shane Caraveo (:mixedpuppy)]

You mentioned that they were not loading into prefs or search bar, but I see them (well, I only tried atlas).

Comment 10

[Dale Harvey (:daleharvey)]

If I load these on nightly, they show up in the url bar (with the default icon), when I had your patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1486819 applied I got the error I mentioned on irc to you when trying to install |extension is null, can't access property "id" of it extension-process-script.js:122|

I can test it out and debug it again, any chance you could rebase your patch though it doesnt look like it applies any more? Cheers

Comment 11

[Dale Harvey (:daleharvey)]

Attachment: dist.zip

Ok I went through and manually tested a bunch of these and it should all be working, the yandex icon for some reason doesnt show even though its just copied over from the tree

Would expect that for these to properly land we would do them one at a time and verify each manually. I will look into writing some tests to ensure we cant check broken search web extensions though

Comment 12

[Shane Caraveo (:mixedpuppy)]

Something I realized is that our opensearch files are named engine-region, not engine-language.  So, ebay.xml would be ebay for the US region.  ebay-de.xml would be ebay for the DE region, but may or may not be translated to de language.

The _locale directory is usually using a locale in the shape of language-region, or en-US, de-DE.  "uk" for example would normally be en-UK.

Our use of search engines tends to ignore language, but relies heavily on region.  I saw some xml files are in english rather than the typical language for the region, see ebay-fr for example, the description is in english.

The current conversion script is making _locale/en, however I believe that should be _locale/US.

I'm not certain yet whether the locale handling in webextensions can handle that, or if we must use en-US (and de-DE, en-UK, etc).

At a minimum we should:

- test if the extensions will load region-only codes properly
- make sure that second level directory is the region (and in caps) based on the opensearch filenames

Comment 13

[Shane Caraveo (:mixedpuppy)]

I think the second part of this bug (or another bug if preferred) is to make a patch that:

lands unziped extensions at browser/components/search/extensions
create a new list.json in browser/components/search/extensions
at build time zips them into XPIs and places them in the app build directory similar to our built-in system addons, but at:

obj-x86_64-apple-darwin17.7.0/dist/Nightly.app/Contents/Resources/browser/search/

Signing of those will be a releng topic.

Edit the new list.json
- normalize extension names
- fix regionOverrides as well as the use of it

Change nsSearchService to use the new list.json, and call into AddonManager to install the appropriate extension when necessary.

The remove browser/components/search/searchplugins

Comment 14

[Mike de Boer [:mikedeboer]]

I think it'd make sense for Mike Kaply to chime in here with a bit of background of using region vs. locale for search plugins (historically) and perhaps thoughts towards solving this issue. What do you think, Mike?

Comment 15

[Shane Caraveo (:mixedpuppy)]

I had a lengthy irc chat with kaply yesterday, which resulted in some of my prior comment.  I was going to outline it here, but it became a lengthy spec for region support, so I'm moving it to another bug.  I'll ni? mkaply on that for any corrections that are necessary.

Comment 16

[Shane Caraveo (:mixedpuppy)]

We do still need to change the conversion script to use proper locale-REGION directory names in the _locale directory.  e.g. "en" => "en-US", "de" => "de-DE".

Comment 17

[Mike de Boer [:mikedeboer]]

👍🏻

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> We do still need to change the conversion script to use proper locale-REGION
> directory names in the _locale directory.  e.g. "en" => "en-US", "de" =>
> "de-DE".

I misled here in a couple ways, sorry about that.

One is the locale directories, which should use underscore rather than dash.  en_US.  In code we use en-US, and that gets translated somewhere.

Second is, we don't need full lang-region locales after all, and in fact that makes it harder to handle list.json -> multiple engine loading.  Unfortunately, list.json is also slightly confusing in its use of lang and region.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Internationalization#Localized_string_selection


Special note on "google-2018".  In the google extension, that should be localized into locale/en-US/.  We should also have locale/en/ which contains the "google" default used by all other regions.  default_locale in the manifest should be "en".

Also, I think we should just use "_locale" for the files in m-c so we don't have to have a lengthy moz.build script to convert the directory names, unless I'm missing some reason to use "locale".

Comment 19

[Dale Harvey (:daleharvey)]

> Also, I think we should just use "_locale" for the files in m-c so we 
> don't have to have a lengthy moz.build script to convert the directory 
> names, unless I'm missing some reason to use "locale".

We need the moz.build files anyway (unless there is a way to just specify * to copy all directories into the FINAL_TARGET_FILES, but I couldnt see one). Either  way yeh switching to '_locales' everywhere is clearer than the build step changing that

Need another special case for "amazondotcom"

https://searchfox.org/mozilla-central/source/browser/components/search/searchplugins/amazondotcom.xml
https://searchfox.org/mozilla-central/source/browser/components/search/searchplugins/amazondotcom-de.xml

The 'de' locale doesnt have a suggest_url, so we have a 2 locales one of which has a suggest url and one doesnt, the extension code doesnt seem to like that so just added the english one as default for now

https://github.com/daleharvey/ffx-opensearch-to-webext/blob/master/index.js#L61

Comment 20

[Dale Harvey (:daleharvey)]

Just to update based on Mozlando discussions

We will land unzipped unsigned extensions for built in search engines (if someone can modify omni jar then it doesnt matter that the engine extension is signed + we will get the benefits of people working on securing omni jar)

The AddonManager will be given a list of all extensions and install them disabled on cold launch / update, nsSearchService will then keep trackof what extensions need enabled / disabled.

Comment 21

[Dale Harvey (:daleharvey)]

I took the meta off this because its not a meta bug, has specific implementation work that I will be doing, any reason to take the meta off it?

Comment 22

[Dale Harvey (:daleharvey)]

¯_(ツ)_/¯

Comment 23

[Dale Harvey (:daleharvey)]

Attachment: https://phabricator.services.mozilla.com/D16478

Cleaned up a little, and this is what I have so far

I have a repository for the script that does the conversions so we can keep it up to date if any engine changes land before this does, and any big changes can be done fairly quickly - https://github.com/daleharvey/ffx-opensearch-to-webext

Most of this should be good I believe, I didnt do any constructing urls so we should avoid introducing any bugs that way, each locale specifies their own full url, they all share an icon except yandex who gets a localised version

The main call that I am not sure is right is where to split up the locale / lang-region / engine (and what the default locale is). As Shane said this can be fairly arbitrary and we can change the line to wherever Mike draws it, but for right now I treat enginename-* as a single engine for everything except yahoo-jp-auctions and google-2018 which get set manually as distinct engines (https://github.com/daleharvey/ffx-opensearch-to-webext/blob/master/index.js#L367)

And for every engine that doesnt specify a locale in its filename, gets defaulted to 'en' as its locale. I started picking what seemed like sensible defaults (gd_GB for bbc-alba etc) but I am not sure we need that at all and we may want to just take the localisation aspect out of these single language engines completely

Comment 24

[Dale Harvey (:daleharvey)]

Btw realised I converted these of an old build that didnt include 'google-b-1-d' etc, I have an updated version where these are just listed as seperate engines, will update soon as my other patch is up but can still discuss the locale issues

Comment 25

[Dale Harvey (:daleharvey)]

Attachment: https://phabricator.services.mozilla.com/D17213

More up to date

Comment 26

[Dale Harvey (:daleharvey)]

So this started as a question, but is slowly become a request for confirmation now, one of the last remaining open questions is how we make the distinction between locale / engine.

Currently we have multiple opensearch definitions for the same 'engine' in different 'locales', we have wikipedia for 30+ languages but we also have 6 opensearch definitions for google, all targeting english (some for the US region, others targeting different firefox versions)

Shane added support for us to be able to use multiple locales within a single engine so we could have a single wikipedia engine that contained all of its locales which is much nicer, but leads to how we handle the cases where the different engines dont really map to locales well, we have 3 choices in terms of directory structure

#1. We can do as we currently do, one web extension per locale

search/

• google
• google-2018
• google-b-1-d
• wikipedia
• wikipedia-af

#2. We can do one engine with everything as a seperate locale:

search/

• google/
• 2018
• b-1-d
• default?
• wikipedia/
• af

#3. We can use different locales when things map to locales, and use different engines when they dont

search/

• google
• google-2018
• google-b-1-d
• wikipedia/
• af

#1 and #2 are both fairly straightforward, they dont require any changes to absearch or list.json

#3 makes the most sense but to implement we would need to reinterpret how we define engines because we currently use - as a seperator for 'locales' (google-2018, wikipedia-af) either we could change list.json + absearch data to refer to locales differently (wikipedia:af) or have a translation table in nsSearchService that just hardcodes the fact that some engines understand locales

I think it would be reasonable to rule out changes to absearch, its fairly complicated to handle changes across firefox versions, I think doing translations inside nsSearchService is also quite complicated (they are already translated @ https://hg.mozilla.org/projects/cedar/file/tip/toolkit/components/search/nsSearchService.js#l595)

I think we should stick with what we currently do (#1), it isnt elegant in its locale handling but requires no changes to the way we handle engines in nsSearchService, list.json or absearch and doesnt have any risk of us getting the engine codes wrong.

If we want to take advantage of the ability to organise the locales differently I think it would be best to do seperately in a follow up.

Comment 27

[Dale Harvey (:daleharvey)]

Trying that again, my bugzilla seems to have markdown enabled so didnt notice that it didnt format for everyone else

(#1) We can do as we currently do, one web extension per locale

• search/
  • google
  • google-2018
  • google-b-1-d
  • wikipedia
  • wikipedia-af

(#2) We can do one engine with everything as a seperate locale:

• search/
  • google/
    • 2018
    • b-1-d
    • default?
  • wikipedia/
    • af

(#3) We can use different locales when things map to locales, and use different engines when they dont

• search/
  • google
  • google-2018
  • google-b-1-d
  • wikipedia/
    • af

Comment 28

[Shane Caraveo (:mixedpuppy)]

I don't understand the reason to consider #3.

With google, everyone on desktop outside the US is configured for google-b-d. This is still an english opensearch file (strings in the file). That should just be google-en.

There is the mobile version, which should just rely on the mobile url (see D8012). This would remove the need to translate to google-b-m or google-b-1-m. Those would just be contained in google-en and google-en-US. If we need something further for mobile we can do further updates to what we handle in the manifest for this, or make this the exception for where we have an extra extension, google-android.xpi. The search service can use ${name}-${AppConstants.platform} as the base name for the extension, requiring no support in list.json to differentiate between the two.

For the US region the override is google-b-1-d. That should be google-en-US.

The items I don't understand are google-b-1-e and google-b-e. These are in the experimental-hidden section, I'm unsure how that section is used. While I haven't tested it in the webext code, locales do support variants. We could use en-name-US variants when needed. That is typically used for dialects IIUC, but for our use case it could map well to experiments (assuming that's what that section is used for).

Assuming that list.json is the only place these new google names exist, and that convertGoogleEngines exists to fixup data from absearch, we can drop convertGoogleEngines entirely and only translate google-2018 to google.

The google extension would contain:

google/_locale/en
google/_locale/en-US

The manifest.json would set the default_locale to en.

list.json would have regionOverrides.US.google-2018: google-en-US

Comment 29

[Dale Harvey (:daleharvey)]

Happy to rule out #3, I was worried that overloading locales for things that arent locales (-2018, -b-1-d) was confusing, but yeh it would cause a bunch of problems.

As for the proposal of changing b-1-d to en-US, I think what you are proposing sounds a lot cleaner and certainly makes sense to use the extra capability that web extensions have, but not sure it makes sense to do it at the same time (at the very least in the same patch) as the initial switch

I think we can be pretty much ready to get 1 extension per open search engine into review today, then look at using the new functionality given by web extensions on follow up patch(es)

Comment 30

[Mike Connor [:mconnor]]

Please don't rename the plugins. #3 is probably closer to the right end state, but we can handle that in a followup.

Comment 31

[Dale Harvey (:daleharvey)]

Attachment: Bug 1486820 - Convert Opensearch files to WebExtensions. r?mikedeboer,mixedpuppy

Comment 32

[Dale Harvey (:daleharvey)]

Attachment: Bug 1486820 - Part 2: Add tests to ensure valid manifest. r?rpl

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/111b88dd28d6
Convert Opensearch files to WebExtensions. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/041d3ca2a427
Part 2: Add tests to ensure valid manifest. r=rpl

Comment 34

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/111b88dd28d6
https://hg.mozilla.org/mozilla-central/rev/041d3ca2a427