883254 - Add DDG as a pre-installed search provider for Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Deb Richardson [:dria] (plz NEEDINFO)]

DDG (duckduckgo.com) is an increasingly popular search engine whose focus is on protecting & respecting user privacy. It seems sensible to give our users this choice an a pre-installed option.

Comment 1

[Karen Rudnitski [:kar]]

We will actively look into getting them included in our list. Stay tuned.

Comment 2

[Aaron Train [:aaronmt]]

Duping forward

Comment 3

[Karen Rudnitski [:kar]]

I'd like to include DDG in the 3rd search provider listing. In general, the search order would be:

1. Default (general) search engine
2. Second general search engine
3. DuckDuckGo
4+. others in alphabetical order

Locales will be in a following comment.

Comment 4

[Karen Rudnitski [:kar]]

Please include DDG in the third position for the following countries. 
* en-US
* Australia
* Austria
* Canada
* France
* Germany
* Mexico
* New Zealand
* Spain
* United Kingdom

[NB - an expanded list is requested where DDG is included in the 'Firefox Confidential' add-on to reach a larger number of markets where DDG has little current presence. An expanded set of countries may be included in the future in-code. We also have limited screen real estate and will likely need to revamp our UX to view the available search engines - or reduce the number - since we're already bumping some off-screen now.]

Comment 5

[Karen Rudnitski [:kar]]

Taking a thread from bug 1064863, I agree with the approach to promote the DDG search plug-in for certain locales instead of adding it to this add-on.

Comment 6

[Karen Rudnitski [:kar]]

ARG - wrong window = more spam. Sorry :/

SO: the 'Firefox Privacy Coach' add-on (formally known as firefox confidential) will not include an expanded list of locales in which DDG is supported, but rather be promoted to a set of locales we would like to target.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: ddg v0.1

Here is a basic patch, built from the duckduckgo.xml plugin found in several other locales already:
http://mxr.mozilla.org/l10n-central/find?text=&string=duckduckgo.xml

This patch does not change the ordering of the engines.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: fennec-ddg.png

Screenshot from a Galaxy Nexus

Comment 9

[:Margaret Leibovic]

Comment on attachment 8512330 [details] [diff] [review]
ddg v0.1

Review of attachment 8512330 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. Leaving the review of the specific parameters up to mconnor.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8512330 [details] [diff] [review]
ddg v0.1

I think you should use the same ddg.xml as attachment 8512367 [details] [diff] [review] (desktop version), with parameter tweaks if needed, and extra images removed.

Based on our discussion on IRC, it seems to me like this patch will only add it to en-US, which I don't think is what you want?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #10)
> Comment on attachment 8512330 [details] [diff] [review]
> ddg v0.1
> 
> I think you should use the same ddg.xml as attachment 8512367 [details] [diff] [review]
> [diff] [review] (desktop version), with parameter tweaks if needed, and
> extra images removed.

OK, but remember that this XML is already used in several locales shipped with Fennec. What are the benefits of using the other XML?

> Based on our discussion on IRC, it seems to me like this patch will only add
> it to en-US, which I don't think is what you want?

I have no idea how to add it to other locales without getting those locale to manually add it themselves. If someone else wants to pick this up and figure that out, feel free. Otherwise, we get en-US, the 8 other locales that already ship ddg and any others that want to manually add it.

Comment 12

[Karen Rudnitski [:kar]]

I would like to ensure the following locales have DDG included: 
* en-US
* fr
* de
* es-MX
* es-ES
* en-GB

As well as any l10n group that had specifically included it as per their search customization guidelines. we can revisit adding further locales as the program moves forward. 

We can also promote DDG as a search add-on for many other countries as well via our snippet rotation on fennec. Adding Arcadio so he can add it to his list of snippet asks.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> OK, but remember that this XML is already used in several locales shipped
> with Fennec. What are the benefits of using the other XML?

Consistency with desktop. Cross-product consistency seems more important to me than intra-product consistency between locales, but I don't feel that strongly.

(In reply to Mark Finkle (:mfinkle) from comment #11)
> I have no idea how to add it to other locales without getting those locale
> to manually add it themselves. If someone else wants to pick this up and
> figure that out, feel free. Otherwise, we get en-US, the 8 other locales
> that already ship ddg and any others that want to manually add it.

Not my call to make.

Given that, no need to block on my feedback-.

Comment 14

[Karen Rudnitski [:kar]]

FYI - discussed today and Jeff will take a look at hand-holding the inclusion of DDG in the other locales. NO specification for search order: the top 3 general providers will be in the top 3 positions, and the rest of the engines will be listed in alphabetical order.

Comment 15

[Francesco Lodolo [:flod]]

Note: if en-US is going to have a DDG xml file, we'll remove the l10n ones (build system should already pick up en-US file as soon as the file pops up in en-US).

Comment 16

[Jeff Beatty [:gueroJeff]]

Does DDG perform the locale redirect on their side?

Comment 17

[Francesco Lodolo [:flod]]

(In reply to Jeff Beatty [:gueroJeff] from comment #16)
> Does DDG perform the locale redirect on their side?

Yes. You can add parameters for country to tweak results, but we're using the same XML file on both desktop and mobile at the moment for all locales with DDG
https://duckduckgo.com/params

Comment 18

[Jeff Beatty [:gueroJeff]]

How public can we be about adding DDG to these locales? Can we use bugzilla to organize the work, or do we need to stick to private email?

Comment 19

[Karen Rudnitski [:kar]]

we cannot be public yet until mconnor gives the go-ahead.

Please stick to private email for the time being, until mconnor gives the thumbs' up.

Comment 20

[Jeff Beatty [:gueroJeff]]

What versions are we targeting for the locales and what is the timeline to have the changes landed in channel?

Comment 21

[Axel Hecht [:Pike]]

I find it hard to believe that our desktop trick doesn't work for android. That said, I don't understand what Joey's timestamp foo in the Makefile is supposed to do.

If we can't do a en-US-only fix adding locales, adding DDG to android should be at the very tail end of the things we're doing. We have two folks on PTO (me and Jeff) and we need our back-ups to do other decade stuff.

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Axel Hecht [:Pike] from comment #21)
> I find it hard to believe that our desktop trick doesn't work for android.
> That said, I don't understand what Joey's timestamp foo in the Makefile is
> supposed to do.

Yeah, I'll dig into this.  There's no reason this can't work.

Comment 23

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #22)
> (In reply to Axel Hecht [:Pike] from comment #21)
> > I find it hard to believe that our desktop trick doesn't work for android.
> > That said, I don't understand what Joey's timestamp foo in the Makefile is
> > supposed to do.
> 
> Yeah, I'll dig into this.  There's no reason this can't work.

A little investigation suggests there are multiple pieces here.  The bit that duplicates desktop appears to work fine.  I'll attach a patch; if you rebuild in just the right way (or clobber build), you'll find the JAR manifests are updated exactly as you'd expect.

However, I believe mfinkle's claim that it "didn't work" (https://bugzilla.mozilla.org/show_bug.cgi?id=1061736#c39) is also correct because we

* package list.txt into the omni.ja, and the written list.txt doesn't include the hacked in entry;
* and we write search engine properties into Android's raw/resources to use from Java (see Bug 1065306).

I'm going to try to understand how desktop handles the first, because I wrote the second and am confident we can bend it to our wills.

Comment 24

[Mike Connor [:mconnor]]

To address the n?: we shouldn't be public, lest we spoil messaging, but we can now engage with localizers if needed through email or cc on confidential bugs.

Comment 25

[Axel Hecht [:Pike]]

Thanks Nick, good catch. We're reading list.txt in http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java#132. It's packaged in jar.mn, we could either package a built/hacked version, or modify the java file to add createEngine('ddg') manually?

I guess packaging a patched list.txt would be cleaner.

Comment 26

[Karen Rudnitski [:kar]]

(In reply to Jeff Beatty [:gueroJeff] from comment #20)
> What versions are we targeting for the locales and what is the timeline to
> have the changes landed in channel?

This is going live in 33.1. I couldn't tell you when the actual changes need to be included in channel.

Jenn - can you help here (and also communicate out the locales we're targeting for DDG so that comms and marketing are aware and can promote accordingly)?

Comment 27

[Axel Hecht [:Pike]]

To clarify, the deadline for 33.1 l10n changes was October 3rd, same as for desktop. That's because it's the same release mechanics and risk profiles that went into that decision. For the addition of ddg to localized builds in 33.1 we'll need an en-US only patch.

Comment 28

[Jenn Chaulk (:jchaulk)]

Just to add, from lmandel's comment in the stand-up, they're ok with Android building later, as it's somewhat easier than the desktop build.  Tentative target for this is Friday, but some further how-to discussion is needed before confirming.

Comment 29

[Mike Connor [:mconnor]]

Comment on attachment 8512330 [details] [diff] [review]
ddg v0.1

Needs the following T values. (We can skip adding SA stuff for 33.1, but we'll likely want that breakout anyway.)

Tablet	Search Activity	ftsa
	Address Bar	ftas
		
Phone	Search Activity	fpsa
	Address Bar	fpas

Comment 30

[Nick Alexander :nalexander [he/him]]

(In reply to Jenn Chaulk from comment #28)
> Just to add, from lmandel's comment in the stand-up, they're ok with Android
> building later, as it's somewhat easier than the desktop build.  Tentative
> target for this is Friday, but some further how-to discussion is needed
> before confirming.

There seems to be a consensus that we want to do this at the build system level and not at the l10n wrangling level.  If we are doing this at the build system level, I need to know *yesterday* 'cuz these l10n repack things are very hard to get right the first time.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Nick - We can try to make something at the build system level work for the listed locales in comment 12.

I am updating the patch to add the codes.

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: ddg v0.2

Now with "t" values for Tablet and Phone. f? to mconnor.

Comment 33

[Aaron Train [:aaronmt]]

The desktop changeset has been linked and is public and is being talked about https://hg.mozilla.org/releases/mozilla-release/rev/0746f89a5aee, is there any reason to keep this bug confidential?

Comment 34

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Add ddg searchplugin to every locale. r=glandium

The purpose of this patch is to append the ddg searchplugin to every
locale in a multi-locale build.  This is a temporary hack for a single
release; for this release, we don't care about burning single-locale
repacks.

I don't think we support finding a localized JAR resource --
like (%searchplugins/list.txt) -- from the object directory directly, so:

* I made the list filename depend on the locale;
* generated the locale-dependent list at build time;
* had the JAR include the list from the object directory.

glandium: can you comment on this approach?  This all produces a
reasonable looking omni.ja locally (both en-US and multilocales).  I'm
about to test on device (my local build appears to be in a bad state
so I need to rebase).  If this is a reasonable way forward, I'll
follow-up to allow only appending the extra ddg entry to a subset of
locales.

Comment 35

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8514552 [details] [diff] [review]
Part 1: Add ddg searchplugin to every locale. r=glandium

>diff --git a/mobile/locales/en-US/searchplugins/ddg.xml b/mobile/locales/en-US/searchplugins/ddg.xml

Just know that you can't use this ddg.xml file. It's the desktop one.

Comment 36

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #35)
> Comment on attachment 8514552 [details] [diff] [review]
> Part 1: Add ddg searchplugin to every locale. r=glandium
> 
> >diff --git a/mobile/locales/en-US/searchplugins/ddg.xml b/mobile/locales/en-US/searchplugins/ddg.xml
> 
> Just know that you can't use this ddg.xml file. It's the desktop one.

Thanks, good to know.  I think we should call this duckduckgo.xml too, so that the localized version wins (in those locales where it is present).  Does anybody know what happens if list.txt contains a duplicate entry?  It would be really nice if it just took the first entry or something like that.  If not, we can find a way to filter list.txt so that each line is unique.

Comment 37

[Nick Alexander :nalexander [he/him]]

It took me a while to get a multi-locale build set up, but this hack produced ddg in the awesomebar suggetions and the settings, both in en-US and in de.

Comment 38

[Mike Hommey [:glandium]]

Comment on attachment 8514552 [details] [diff] [review]
Part 1: Add ddg searchplugin to every locale. r=glandium

Review of attachment 8514552 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/locales/Makefile.in
@@ +100,5 @@
>    $(NULL)
>  
> +$(plugin_file): $(plugin_source_file) $(call mkdir_deps,$(dir $(plugin_file)))
> +	cat $(plugin_source_file) > $@
> +	printf 'ddg\n' >> $@

That's an strange way to do "echo ddg >> $@"

::: mobile/locales/jar.mn
@@ +6,5 @@
>  
>  @AB_CD@.jar:
>  % locale browser @AB_CD@ %locale/@AB_CD@/browser/
>    locale/@AB_CD@/browser/region.properties        (%chrome/region.properties)
> +  locale/@AB_CD@/browser/searchplugins/list.txt   (overrides/list_@AB_CD@.txt)

If that works, it's thanks to luck. You don't have any dependency between the handling of jar.mn and the list file.
You should just add this line to the search-jar that is created in Makefile.in.

Comment 39

[Francesco Lodolo [:flod]]

(In reply to Nick Alexander :nalexander from comment #37)
> It took me a while to get a multi-locale build set up, but this hack
> produced ddg in the awesomebar suggetions and the settings, both in en-US
> and in de.

Can you test one of the locales that already have DDG? Locales: cy, es-MX, gd, ml, ta.

If there's a conflict, the en-US version should be picked, since it has more parameters than the localized one (the localized one is actually identical for all locales, locale detection is done on DDG's side).

Comment 40

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Add ddg searchplugin to every locale. r=glandium

The purpose of this patch is to append the ddg searchplugin to every
locale in a multi-locale build.  This is a temporary hack for a single
release; for this release, we don't care about burning single-locale
repacks.

I don't think we support finding a localized JAR resource --
like (%searchplugins/list.txt) -- from the object directory directly, so:

* I made the list filename depend on the locale;
* generated the locale-dependent list at build time;
* had the generated JAR include the list from the object directory.

glandium: I completely missed that the in-tree manifest and the
generated manifest were merged.  Here's a version that just adds the
generated list.txt to the generated manifest.  Better?

As for the comment about "weird way to echo" -- it's not just echoing,
it's copying the input file too.  In the next patch, we'll sort and
uniq the list.txt as well.  (Search engine ordering already comes from
elsewhere, namely region.properties.)

Comment 41

[Mike Hommey [:glandium]]

Comment on attachment 8515175 [details] [diff] [review]
Part 1: Add ddg searchplugin to every locale. r=glandium

Review of attachment 8515175 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/locales/Makefile.in
@@ +100,5 @@
>    $(NULL)
>  
> +$(plugin_file): $(plugin_source_file) $(call mkdir_deps,$(dir $(plugin_file)))
> +	cat $(plugin_source_file) > $@
> +	printf 'ddg\n' >> $@

still a weird way to do "echo ddg >> $@". That is, why use printf with a string ending with \n, when you can just use echo.

::: mobile/locales/en-US/searchplugins/ddg.xml
@@ +3,5 @@
> +  <ShortName>DuckDuckGo</ShortName>
> +  <Description>Search DuckDuckGo</Description>
> +  <InputEncoding>UTF-8</InputEncoding>
> +  <Image height="16" width="16">data:image/icon;base64,AAABAAIAEBAAAAEAIABoBAAAJgAAACAgAAABACAAqBAAAI4EAAAoAAAAEAAAACAAAAABACAAAAAAAAAEAAATCwAAEwsAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA11RgALs6oACbQ9wAj0v8AI9L/ACfQ9wAu0agANdUYAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAzzN4CNdL/oK/z//////////////////////+jsPv/BDXX/wAz0t4AAAAAAAAAAAAAAAAAAAAAAAAAAAAyzvNSduD//////8jK/v+P+Lf/IbQL/17RPP+J3Y//wOKX//////9YeuX/ADLO8wAAAAAAAAAAAAAAAAAw091piOX/8/X9/1Fx5P9xhu//WOWZ/0W9Lv9Lwjn/J8BB/xyDAP9bdfL/9fP//2mI5v8AMNPdAAAAAAc610YRQ9f//////0Zr4P8AGdD/sb32////////////wrv//wAh1/8MPab/ACPc/05r4///////EkPX/wc610YANtWkrr/y/6S48P8AJ9L/AB3R/+/w/v///////////3+D7f8AQeL/AYTw/wFr5/8AMNb/p7Tv/6698v8AM9WkADLW//////8yXt//AC3V/wAw1/////////////z///8A0P7/AKb1/wWI7P8AuPf/AJ3w/zZW3P//////ADHV/wAx2P//////AzrZ/wAu1/84ZOL////////////e////AND//wC1+f8Atff/AZbv/wY62f8ELNf//////wAw1/8AMtn//////wAw2f8ALNn/kKrz////+//cwbH////////////R////Rcb8/wDO/f8A/P//AHzo//////8AMNj/ADXa//////8vXuL/ACna/4yq9///79T/jUkg/9i+r///////r2Q0/7Cozv8BKdr/AirY/zdZ4P//////ADTa/wI72tOuv/T/prr0/wAl2v+JqPb//7yW/+bUxv/9+/n////u//W+n/+Op/L/ADPd/wAv2v+ru/T/r7/0/wI72tMLQd1DEEjg//////9Cbef/ADng///////////////////////R3///AC3g/wAy3v9SeOn//////xFI4P8LQd1DAAAAAAM64PNmiuz/9/j//2mN7f/m7P3///////////9Cb+n/ACXd/wAt3v9rju3//////2iL7P8DOuDzAAAAAAAAAAAAAAAAAT3g/0p16f//////3OT8/3OS7v8AKt3/ACPc/zhn5/+xw/b//////0956v8CPeD/AAAAAAAAAAAAAAAAAAAAAAAAAAAEPODzBUDh/5uz8//7/f7/////////////////prz0/wtF4v8FQeDzAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAtF5kYDQOOkADrj/wA44v8AOeP/ADzk/wVB46QPReZGAAAAAAAAAAAAAAAAAAAAAPAPAADgBwAAwAMAAIABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIABAADAAwAA4AcAAPAPAAAoAAAAIAAAAEAAAAABACAAAAAAAAAQAAATCwAAEwsAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAChIzyAnRNFwJ0TQryND0d8nRNH/J0TR/ydE0f8nRNH/I0PR3ydE0K8nRNFwKEjPIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAChE00AlRdK/J0XS/ydF0v8nRdL/XXPd/11z3f94i+P/k6Lp/5Oi6f9rf+D/NVDV/ydF0v8nRdL/JUXSvyhE00AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACBAzxAnRNOvJ0XT/ydF0/8lRdK/KEXSYOvu+6/+/v6//v7+v/39/c////////////7+/r/J0fOAKEXSYCVF0r8nRdP/J0XT/ydE068gQM8QAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAlRdUwJ0bT7ydG0/8nRtHPKETTQAAAAADHx8dA2vHhn5TYpN/o9+z/////////////////8PL83ydG0o8lRdUwAAAAAChE00AnRtHPJ0bT/ydG0+8lRdUwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAKEXVYCdG1P8nRtT/KEbTgAAAAAAmRtZQI0PU38jIyP/F6s//Rrtk/0a7ZP9/yIr/c796/4vLkv+JpNf/M3Kq/zyWh/8zeKTfJkbWUAAAAAAoRtOAJ0bU/ydG1P8oRdVgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACVF1TAnR9X/J0fV/yhF1WAgQM8QJ0fTrydH1f9CW8//2tra/6Pdsv9Gu2T/Rrtk/0WzWv9Gu2T/Rrtk/0a7ZP9Gu2T/Rrtk/z6egP8nR9X/J0fTryBAzxAoRdVgJ0fV/ydH1f8lRdUwAAAAAAAAAAAAAAAAAAAAAAAAAAAgQM8QJ0fV7ydH1f8oSNVgIEDPECdH1c8nR9X/J0fV/1xwyf/t7e3/o92y/0a7ZP9Gu2T/Ra5U/0a7ZP9Gu2T/Rrtk/0a7ZP9Gu2T/Pp6A/ydH1f8nR9X/J0fVzyBAzxAoSNVgJ0fV/ydH1e8gQM8QAAAAAAAAAAAAAAAAAAAAACdH1q8nR9b/KEjVgCBQzxAnR9bPJ0fW/ydH1v8nR9b/gIzB//r6+v+j3bL/Rrtk/13Ed/+i26//ruG7/z6egf8+noH/Rrtk/0a7ZP86kI//J0fW/ydH1v8nR9b/J0fWzyBQzxAoSNWAJ0fW/ydH1q8AAAAAAAAAAAAAAAAoSNdAJkjW/yZH1s8AAAAAJEfWryZI1v8mSNb/JkjW/yZI1v+jqsT//////+j37P/R7tj////////////W3ff/JkjW/yZI1v8uZbr/PJeI/zJzrP8mSNb/JkjW/yZI1v8mSNb/JEfWrwAAAAAmR9bPJkjW/yhI10AAAAAAAAAAACVI1r8mSNf/KEjXQCZJ1lAmSNf/JkjX/yZI1/8mSNf/JkjX/9HR0f///////////////////////////5Ok6/8mSNf/JkjX/yZI1/8mSNf/JkjX/yZI1/8mSNf/JkjX/yZI1/8mSNf/JknWUChI10AmSNf/JUjWvwAAAAAoSNcgJknY/yZH2M8AAAAAI0nY3yZJ2P8mSdj/JknY/yZJ2P9KZM//39/f////////////////////////////XHfi/yZJ2P8mSdj/JknY/yZJ2P8mSdj/JknY/yZJ2P8mSdj/JknY/yZJ2P8jSdjfAAAAACZH2M8mSdj/KEjXICdJ2HAmSdj/JUjXYCVK2jAmSdj/JknY/yZJ2P8mSdj/JknY/2V4yf/t7e3///////////////////////////9cd+L/HXTj/xSf7/8Nwfj/CdL8/wnS/P8J0vz/ELDz/xt85v8mSdj/JknY/yZJ2P8lStowJUjXYCZJ2P8nSdhwJErZryZK2f8oSNcgJUnajyZK2f8mStn/JkrZ/yZK2f8mStn/iJPA////////////////////////////0ff+/xjV/P8J0vz/Drn1/xiO6/8Yjuv/GI7r/xCw8/8Lyvr/CdL8/xmF6P8mStn/JkrZ/yVJ2o8oSNcgJkrZ/yRK2a8jStrfI0rZ3wAAAAAlSdq/Jkra/yZK2v8mStr/Jkra/yZK2v+xtsf///////////////////////////8o2Pz/CdL8/wvK+v8mStr/Jkra/yZK2v8mStr/Jkra/yZK2v8iW97/Jkra/yZK2v8mStr/JUnavwAAAAAjStnfI0ra3yZK2v8lSdq/AAAAACZH2O8mStr/Jkra/yZK2v8mStr/L1HY/9HR0f///////////////////////////yjY/P8J0vz/CdL8/xCw9P8QsPT/ELD0/xSf7/8ddeX/Jkra/yZK2v8mStr/Jkra/yZK2v8mR9jvAAAAACVJ2r8mStr/Jkvb/yVJ2r8AAAAAJkvb/yZL2/8mS9v/Jkvb/yZL2/9KZtL/4+Pj////////////////////////////4Pn//0fd/f8J0vz/CdL8/wnS/P8J0vz/CdL8/wnS/P8Lyvr/Fpfu/yJc3/8mS9v/Jkvb/yZL2/8AAAAAJUnavyZL2/8mS9z/JUncvwAAAAAmS9z/Jkvc/yZL3P8mS9z/Jkvc/26AyP/x8fH//////////////////////////////////////9H3/v/C9P7/o+7+/2fa+/8Oufb/CdL8/wnS/P8J0vz/CdL8/xiP7P8mS9z/Jkvc/wAAAAAlSdy/Jkvc/yZM3P8lTNy/AAAAACZJ2e8mTNz/Jkzc/yZM3P8mTNz/iJTB////////////qnth/5VaOf/x6eX///////////////////////Hp5f/x6eX/ydL2/yZM3P8kVN7/G37o/xKo8v8QsfT/HXbm/yZM3P8mSdnvAAAAACVM3L8mTNz/I0vc3yZJ2u8AAAAAJUzevyZM3f8mTN3/Jkzd/yZM3f+fqc3///////////+VWjn/v5yI/+re1///////////////////////jk8s/7iRe//J0vb/Jkzd/yZM3f8mTN3/Jkzd/yZM3f8mTN3/Jkzd/yVM3r8AAAAAI0vc3yNL3N8kTd2vJk3d/yhQ3yAlTd2PJk3d/yZN3f8mTd3/Jk3d/6St0v////////////Hp5f/q3tf///////////////////////////+xhm7/49PK/6Cx8P8mTd3/Jk3d/yZN3f8mTd3/Jk3d/yZN3f8mTd3/JU3djyhQ3yAmTd3/JE3drydN33AmTd7/J03fcCVK3zAmTd7/Jk3e/yZN3v8mTd7/pK7S///////Sp5r/////////////////////////////////////////////////T27k/yZN3v8mTd7/Jk3e/yZN3v8mTd7/Jk3e/yZN3v8lSt8wJ03fcCZN3v8nTd9wKFDfICZO3/8mTt3PAAAAACVN3r8mTt//Jk7f/yZO3/+EltX//////+fRyv/SqaD/59LO///////////////////////at63/vIBy/7Glxf8mTt//Jk7f/yZO3/8mTt//Jk7f/yZO3/8mTt//JU3evwAAAAAmTt3PJk7f/yhQ3yAAAAAAJE/dryZO3/8oUN9AKFDfQCZO3/8mTt//Jk7f/zhb2v/o6/T/////////////////////////////////////////////////XHrn/yZO3/8mTt//Jk7f/yZO3/8mTt//Jk7f/yZO3/8oUN9AKFDfQCZO3/8kT92vAAAAAAAAAAAoUN9AJk7g/yZO4M8AAAAAJk/hnyZO4P8mTuD/Jk7g/05v5v/k6fv//////////////////////////////////////3eR7P8mTuD/Jk7g/yZO4P8mTuD/Jk7g/yZO4P8mTuD/Jk/hnwAAAAAmTuDPJk7g/yhQ30AAAAAAAAAAAAAAAAAjT+GfJU/h/yVO4Y8gUN8QIk7gzyVP4f8lT+H/SWnW/0lp1v+bq+H/8fHx/////////////////6Cy8v9OcOb/JU/h/yVP4f8lT+H/JU/h/yVP4f8lT+H/JU/h/yJO4M8gUN8QJU7hjyVP4f8jT+GfAAAAAAAAAAAAAAAAAAAAACBQ3xAlTOHvJU/h/yVQ4mAgUN8QIk7hzyVP4f+ktOv///////////////////////H0/f9phur/JU/h/yVP4f8lT+H/JU/h/yVP4f8lT+H/JU/h/yVP4f8iTuHPIFDfECVQ4mAlT+H/JUzh7yBQ3xAAAAAAAAAAAAAAAAAAAAAAAAAAACVQ3zAlUOLvJVDi/yVQ4mAgUN8QI1Din4mb2//J0/j/ydP4/6299P93ku3/M1vk/yVQ4v8lUOL/JVDi/yVQ4v8lUOL/JVDi/yVQ4v8lUOL/I1DinyBQ3xAlUOJgJVDi/yVQ4u8lUN8wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACVQ5DAlUOLvJVDi/yVQ4o8AAAAAJFDjQCVQ4r8lUOL/JVDi/yVQ4v8lUOL/JVDi/yVQ4v8lUOL/JVDi/yVQ4v8lUOL/JVDivyRQ40AAAAAAJVDijyVQ4v8lUOLvJVDkMAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACVQ5DAjUeTfJVHj/yNR5N8kUONAAAAAACVQ5DAmUuOAJVHivyNR5N8lUeP/JVHj/yNR5N8lUeK/JlLjgCVQ5DAAAAAAJFDjQCNR5N8lUeP/I1Hk3yVQ5DAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACBQ3xAjUuSfJVHk/yVR5P8jUeTfJFLkcChQ5yAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAoUOcgJFLkcCNR5N8lUeT/JVHk/yNS5J8gUN8QAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAkUONAI1LknyVS5P8lUuT/JVLk/yVS5O8lUeS/JVHkvyVR5L8lUeS/JVLk7yVS5P8lUuT/JVLk/yRS468kUONAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIFDfECVS5GAjUuWfIlPlzyVS5f8lUuX/JVLl/yVS5f8iU+XPI1LlnyVS5GAgUN8QAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP/AA///AAD//AAAP/ggBB/wgAEP4AAAB8AAAAPAAAADiAAAEYAAAAEQAAAIAAAAAAAAAAAgAAAEIAAABCAAAAQgAAAEIAAABCAAAAQAAAAAAAAAABAAAAiAAAABiAAAEcAAAAPAAAAD4AAAB/CAAQ/4IAQf/AfgP/8AAP//wAP/</Image>
> +  <Image height="26" width="65">data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEEAAAAaCAYAAADovjFxAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAA2hpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAAAADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4OnhtcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuMy1jMDExIDY2LjE0NTY2MSwgMjAxMi8wMi8wNi0xNDo1NjoyNyAgICAgICAgIj4gPHJkZjpSREYgeG1sbnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjpEZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wTU09Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9tbS8iIHhtbG5zOnN0UmVmPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC8xLjAvc1R5cGUvUmVzb3VyY2VSZWYjIiB4bWxuczp4bXA9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC8iIHhtcE1NOk9yaWdpbmFsRG9jdW1lbnRJRD0ieG1wLmRpZDoxNTg5QTM3RjNCMjA2ODExODIyQUVEOUNBRDIxQzhDMyIgeG1wTU06RG9jdW1lbnRJRD0ieG1wLmRpZDoxRTYyNzYzMzFBQUUxMUU0ODc3NTg3NjMyNDFCNzExQSIgeG1wTU06SW5zdGFuY2VJRD0ieG1wLmlpZDoxRTYyNzYzMjFBQUUxMUU0ODc3NTg3NjMyNDFCNzExQSIgeG1wOkNyZWF0b3JUb29sPSJBZG9iZSBQaG90b3Nob3AgQ1M2IChNYWNpbnRvc2gpIj4gPHhtcE1NOkRlcml2ZWRGcm9tIHN0UmVmOmluc3RhbmNlSUQ9InhtcC5paWQ6MDE4MDExNzQwNzIwNjgxMTg3MUZCQUIxMEI4RjU1NzYiIHN0UmVmOmRvY3VtZW50SUQ9InhtcC5kaWQ6MTU4OUEzN0YzQjIwNjgxMTgyMkFFRDlDQUQyMUM4QzMiLz4gPC9yZGY6RGVzY3JpcHRpb24+IDwvcmRmOlJERj4gPC94OnhtcG1ldGE+IDw/eHBhY2tldCBlbmQ9InIiPz7hxyCFAAAF4UlEQVR42tSZa2wUVRTH/3dmp92+tnRpCiUgxFoKCDQplsRIfZSgCEHSapBGBCURQvCLWkkQg9GYGNDwTYlGUCMoCA2EtEpEq1KjRJCA9EEElba2FajbN213d+b6vzOzZZfi99mb/DJ3d/aczD33vO6skFLCq6O9YubYXChE4n1+LublaVJO5hLDvRUhjaSefMolno+XU0uOX7WG5BxLSBsXc448Qo6SFWSuywr3O3XvnPqtK3PbkWxGUL5whHzN7WyAZeWQRZBWFz+v4v6+w3sKztFFFpEc0mDLOLJinNIkCocMXppJLhdcCssMw4zu5fOX2XGi6RAa91Rot8aNMsB6kkJOk24yh6seSjZPEHzoZmlfZaaU1pqR/Dsv+XfVlQXf/QbZ67dBz5sKKzxK20TpGBbU5qrtJWXkEllDMqWrK94jPG0ESzpwPTUkl8yQFvZL09oaLlqIG1LDQFoA6Y9vxJQ9Dch+bjus4WEaIwxpmjSGdJKgw1ay39bh6KqJ6U8GTyjnc1aQheQ1UqV2Of3LPTBPfI40RkVsEdmVGzClphmZy9bAitAQlhnvEbBlHR1KV4VbVbydE66sLFI+e8mN5e1Qc7o6IqMIr3sVhVUb/le2e1c1Bk8cgvAxFWjj9rqQvA7HGIVeN4LqA1SJm0iOci/LQDc3aZrg4RZkpBgYHY4iK8OXIHetz4RPi6K/ao4b9PrtkuVKElL9hafDgfuzjrQQk5QxH9hxPphfgAy/gYHByDgDqJGXrSOYlYrAo1WQUTc32LJj+aHM1dlEnvK2EYClpIasiotrmL5URE2JiQEDuw+ewunGjgS5hrNtWLp5P/QHKhk9lh1BDuKmIYAnyWGyxNuJUWI2+ZncG5/djKF+DA45ZX7Hh9/j2HdNCWKzC/JRsXg+sgrmQ4aFjS/LQvqsYejpqiTYepTOn8g8n8c9QQ21zdPiDxD+620IhULIC2TiYl01/CmJy8jNNrCxMh/hjiMIVvYioziK1BlRQJcIHc1B6KscqpLTXN2pPo/nhNjwxR2jIFgdBtouAzPuSDSAZLPUXgX0HYFgLjRIcLnO3sFAb33AJnLdYFNp9w/6Lco97Qm57o7dbKC5wpHffgHuL09srjo22AYY/tOPoQsBiFRgtJ3zljQqo/HoCfbSnULR6eq2vJ4TWskCcnYsJ9hGYIz/1YQR65bTYOARe5FW2EDfqQD+rQ1iqDkN/qlhTHhwAJnFQ6oxiuUWpfMe0uzpPuH3ZUV7eZlFVpPWsRjh+WA4OAV579UiPzghQaajuhgZc1qRWTIKnQ4g0i27N77RlI5rXwQR/seI9cnTyWfkvNfDYR8v35K/iSoBd8eSY8r1doR6escZYfRqLvrPDEDsM+ALWNADJiLdBttoYXuJEPamK13t5D6y1evNUj3pJzvIJqfRYV5XyZGFf/CPlnEy6UXzoLEZYDqEHBCIdBpsmFSsODEgHb1K105Xd4PnD1B86M2kmjSSA/Yi6AlSaBhtPG3/5mTXcTT3nLPnwYcq4KP7X52cgvc3TcWvpQFVDu1UIoWt74CrS+nc7P3qIO00rkLiBfcQdReZCamVqAqRcuUiolzJDx11aO4+gxxjOnqtbmhvzITO+6Yh0DthhF4jYmeHs4Q1FJfd+T6ZLC9VpHOcnk7qyQJSqzzB39WKnr5h+HhS1DUfOvs64RNRVgATlnDawoghYoWl1pWtd3WVJ0WfEDf6pGqhnTdCTGqihCw3urt2h0LdecH0ydB1Aw8XLkfPjRGUTixB4JUtuDDXj0nXokMmsJbyddJJiOoYPZuO0ZdsRlDjsnTa5x/JCDTxEnPfpIH6Y28uXvHYttUFzyNb1cTY+4TyC8ezD37wltT1k5YmXmRkjbhhMI2BcTVpXrS2LJ011jmO/VfgPO7L/GKnfSQ0zU+EaR2SlnWGN53FaRpdw1cKTX+Cxlrr5oMt5G2713Kq7NhLxmQ1gpPvpXyWs2f4oSz+oHHznxrRwOvHnHwkYpK3McJ/AgwADmrfhvtTyFYAAAAASUVORK5CYII=</Image>

Do you actually need the non-icon images?

Comment 42

[:Gavin Sharp [email: [e-mail]]]

I don't think mobile wants the 26x65 and 52x130 images, those are just used by desktop for the new tab page. The 16x16 image includes a 32px version (assuming you're using the same one as bug 1061736).

Comment 43

[Francesco Lodolo [:flod]]

I think we're missing 2 different patches here. Mobile shouldn't have 26x65 and 52x130 images, and should have a flat 32px icon, not a 16+32 .ico

The patch with mconnor's f+ should be correct though.

Comment 44

[chris hofmann]

just one comment about landing the patches.  lets make sure there is time in the schedule to test all the locales that might be affected by any landings, and time to respond with back out or other response before the hard shipping deadline.

testing should be quick to bring up the locale, look at search options, verify all the icons and such look ok, check to see we get right search codes and such going to ddg, and that we get back some expected and resonable search results.

although this should be pretty quick it might need to be done 30 some odd times depending on the scope of the patch and the possible number of locales that are in the code path of the changes.

Comment 45

[Nick Alexander :nalexander [he/him]]

OK, I'm going to prepare an updated version of the build hacking approach, but with the alternate plugin referenced above.

Comment 46

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8513698 [details] [diff] [review]
ddg v0.2

Approval Request Comment
[Feature/regressing bug #]: Moziversary
[User impact if declined]: No DDG in Fx33.1
[Describe test coverage new/current, TBPL]: Did a local build and it worked
[Risks and why]: Some risk since this will be used in a multi-locale build, but it should only affect en-US
[String/UUID change made/needed]: None. Just adds the DDG engine to en-US

This is our worst-case, fallback. I want to land it so we can get some testing and be ready. If a better fix comes in, we can update it on mozilla-release.

Comment 47

[Nick Alexander :nalexander [he/him]]

Attachment: Add ddg searchplugin to every locale. r=glandium

The purpose of this patch is to union a set of extra search plugins into
the set of search plugins for a specific subset of locales in a
multi-locale build.  This is a temporary hack for a single release; for
this release, we don't care about burning single-locale repacks.

I don't think we support finding a localized JAR resource --
like (%searchplugins/list.txt) -- from the object directory directly, so:

* I made the list filename depend on the locale;
* generated the locale-dependent list at build time;
* had the generated JAR include the list from the object directory.

I wasn't sure how to pipe the original list and possible additions
through sort and uniq, so I wrote a temporary file.

This patch is against fx-team, although I expect it to apply to
mozilla-release.  This version includes the duckduckgo.xml from
mfinkle's patch against mozilla-release.

Comment 48

[Nick Alexander :nalexander [he/him]]

(In reply to Francesco Lodolo [:flod] from comment #39)
> (In reply to Nick Alexander :nalexander from comment #37)
> > It took me a while to get a multi-locale build set up, but this hack
> > produced ddg in the awesomebar suggetions and the settings, both in en-US
> > and in de.
> 
> Can you test one of the locales that already have DDG? Locales: cy, es-MX,
> gd, ml, ta.
> 
> If there's a conflict, the en-US version should be picked, since it has more
> parameters than the localized one (the localized one is actually identical
> for all locales, locale detection is done on DDG's side).

My new patch ignores most of those locales: it only tries to insert for kar's list of 6 in [1].  As for the conflict ordering, that ordering is determined by the existing jar packaging code, which I think favours the non-en-US locale without regard to the file contents.

If we really, really, really want to override the locale's duckduckgo.xml, then we rename en-US's duckduckgo.xml to ddg.xml; strip duckduckgo from locale's list.txt forcefully; and insert ddg.  That seems like bad behaviour.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=883254#c12

Comment 49

[Mike Hommey [:glandium]]

Comment on attachment 8516197 [details] [diff] [review]
Add ddg searchplugin to every locale. r=glandium

Review of attachment 8516197 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/locales/Makefile.in
@@ +106,5 @@
> +  en-US \
> +  es-ES \
> +  es-MX \
> +  fr \
> +  $(NULL)

Why do we need a hardcoded list of locales? It feels wrong to have one, but maybe there's a reason, but nothing in this patch tells me what it is.

If there's only a list of 6 locales that need ddg, why not simply land the necessary changes in their l10n repo? It also seems to be quicker to do than to get the right hacks in the already complicated l10n code here.

@@ +111,5 @@
> +
> +# These additional search plugins will be merged into the set of search plugins
> +# for each locale listed in extra_search_plugin_locales.
> +extra_search_plugins := \
> +  duckduckgo \

Seems to me this is an unnecessary extra layer of indirection. Do you really expect to have other additional search plugins that would apply to the exact same list of locales?

@@ +120,5 @@
> +	$(if $(filter $(AB_CD),$(extra_search_plugin_locales)),\
> +		$(foreach plugin,$(extra_search_plugins),echo $(plugin) >> $@;))
> +
> +$(plugin_file): $(plugin_file).tmp
> +	cat $< | sort | uniq > $@

sort $< | uniq > $@
Or even sort -u $< > $@ if that works everywhere.

That said, you don't need the temporary file:

$(plugin_file): ...
    (cat $(plugin_source_file) ; \
     $(if $(filter..... echo $(plugin);)) \
    ) | sort -u > $@

So, you're throwing away the order in that file. Is that important? Will that actually affect the engines order, or is the order handled some other way in fennec?

@@ +158,5 @@
>  	$(TOUCH) $@
>  
>  include $(topsrcdir)/config/rules.mk
>  
> +$(search-jar): $(GLOBAL_DEPS)

Add that to search-jar-preqs instead.

Comment 50

[Nick Alexander :nalexander [he/him]]

glandium: I haven't provided a lot of context here, but the bigger picture is that this is landing on mozilla-release for November to Remember stuff only.  Hence, no time to work with localizers; hence, duplicated lists of additional locales and additional engines.  Yes, this is not idea; no, I did not make this call.

Comment 51

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mike Hommey [:glandium] from comment #49)

Some answers, I hope...

> @@ +106,5 @@
> > +  en-US \
> > +  es-ES \
> > +  es-MX \
> > +  fr \
> > +  $(NULL)
> 
> Why do we need a hardcoded list of locales? It feels wrong to have one, but
> maybe there's a reason, but nothing in this patch tells me what it is.

See comment 12

> If there's only a list of 6 locales that need ddg, why not simply land the
> necessary changes in their l10n repo? It also seems to be quicker to do than
> to get the right hacks in the already complicated l10n code here.

The will be the approach used for the "ride the trains" patch, but for landing on mozilla-release, the general approach was to override. Desktop is doing that too, just to all the locales. Personally, Nick and I don't like the overriding approach, but it's the 11th hour.

> > +# These additional search plugins will be merged into the set of search plugins
> > +# for each locale listed in extra_search_plugin_locales.
> > +extra_search_plugins := \
> > +  duckduckgo \
> 
> Seems to me this is an unnecessary extra layer of indirection. Do you really
> expect to have other additional search plugins that would apply to the exact
> same list of locales?

Not exactly sure the context of the question, but we know that es-MX has a duckduckgo.xml file with different settings in it. 

> > +$(plugin_file): $(plugin_file).tmp
> > +	cat $< | sort | uniq > $@
> 
> sort $< | uniq > $@
> Or even sort -u $< > $@ if that works everywhere.
> 
> That said, you don't need the temporary file:
> 
> $(plugin_file): ...
>     (cat $(plugin_source_file) ; \
>      $(if $(filter..... echo $(plugin);)) \
>     ) | sort -u > $@
> 
> So, you're throwing away the order in that file. Is that important? Will
> that actually affect the engines order, or is the order handled some other
> way in fennec?

list.txt does not control the engine order. A region.properties file is used to select the order of the top 3 engines, the rest are displayed alphabetical, provided by nsSearchService.js

Comment 52

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #49)
> Comment on attachment 8516197 [details] [diff] [review]
> Add ddg searchplugin to every locale. r=glandium
> 
> Review of attachment 8516197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/locales/Makefile.in
> @@ +106,5 @@
> > +  en-US \
> > +  es-ES \
> > +  es-MX \
> > +  fr \
> > +  $(NULL)
> 
> Why do we need a hardcoded list of locales? It feels wrong to have one, but
> maybe there's a reason, but nothing in this patch tells me what it is.
> 
> If there's only a list of 6 locales that need ddg, why not simply land the
> necessary changes in their l10n repo? It also seems to be quicker to do than
> to get the right hacks in the already complicated l10n code here.

We've been told that it's not feasible to get l10n co-ordination in the time allotted :(
 
> @@ +111,5 @@
> > +
> > +# These additional search plugins will be merged into the set of search plugins
> > +# for each locale listed in extra_search_plugin_locales.
> > +extra_search_plugins := \
> > +  duckduckgo \
> 
> Seems to me this is an unnecessary extra layer of indirection. Do you really
> expect to have other additional search plugins that would apply to the exact
> same list of locales?

No, we don't expect others, but it's as easy to make the variables clear as not.

> @@ +120,5 @@
> > +	$(if $(filter $(AB_CD),$(extra_search_plugin_locales)),\
> > +		$(foreach plugin,$(extra_search_plugins),echo $(plugin) >> $@;))
> > +
> > +$(plugin_file): $(plugin_file).tmp
> > +	cat $< | sort | uniq > $@
> 
> sort $< | uniq > $@
> Or even sort -u $< > $@ if that works everywhere.
> 
> That said, you don't need the temporary file:
> 
> $(plugin_file): ...
>     (cat $(plugin_source_file) ; \
>      $(if $(filter..... echo $(plugin);)) \
>     ) | sort -u > $@

I will do this; it's the kind of thing I knew existed but wasn't familiar enough to bother to get it right.

> So, you're throwing away the order in that file. Is that important? Will
> that actually affect the engines order, or is the order handled some other
> way in fennec?

The order is handled by region.properties; list.txt is treated as a set and then alphabetically ordered in code.

> @@ +158,5 @@
> >  	$(TOUCH) $@
> >  
> >  include $(topsrcdir)/config/rules.mk
> >  
> > +$(search-jar): $(GLOBAL_DEPS)
> 
> Add that to search-jar-preqs instead.

Will do.

Comment 53

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #50)
> Hence, no time to work with localizers

All is in mercurial, right? why would that need to be "worked with localizers"? What about the D in DVCS?

Comment 54

[:Gavin Sharp [email: [e-mail]]]

Not the right time or place to argue about L10N infrastructure.

Comment 55

[Mike Hommey [:glandium]]

I'm not arguing, I'm trying to understand.

Comment 56

[:Gavin Sharp [email: [e-mail]]]

Not the best place/time for that either :)

Comment 57

[Nick Alexander :nalexander [he/him]]

In light of the circumstances, I owe glandium an apology: I have asked him for feedback and review, on an accelerated schedule, of a patch that is doing something that is not our standard policy, and I haven't provided much context into why I'm pursuing the approach I am.

I think the best way forward is to have glandium comment on the technical approach of the patch -- that is, does it achieve what it claims to, in a reasonable manner -- and to simultaneously continue the policy discussion.  Others (gavin, mfinkle, kar) are much better positioned to explain how we came to this position than I am.

Comment 58

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #57)
> I think the best way forward is to have glandium comment on the technical
> approach of the patch

... which I've already done.

Comment 59

[Mike Hommey [:glandium]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #56)
> Not the best place/time for that either :)

At least answer this: what would break if someone landed the necessary changes to http://hg.mozilla.org/releases/l10n/mozilla-release ?

Comment 60

[Francesco Lodolo [:flod]]

Shipping changesets are signed off on Beta one week before the end of the cycle, that happened a month ago. We don't accept updates on release channel, no clue of what would break in automation with stuff landing on release but not on the other branches.

Add to this that both l10n people in charge of sign-offs are on PTO this week, and we need a build by Thursday for QA (at least from last daily stand-up) to understand if this works.

Comment 61

[Nick Alexander :nalexander [he/him]]

Attachment: Add duckduckgo searchplugin to certain locales. f=glandium,r=mfinkle

The purpose of this patch is to union a set of extra search plugins into
the set of search plugins for a specific subset of locales in a
multi-locale build.  This is a temporary hack for a single release; for
this release, we don't care about burning single-locale repacks.

I don't think we support finding a localized JAR resource --
like (%searchplugins/list.txt) -- from the object directory directly, so:

* I made the list filename depend on the locale;
* generated the locale-dependent list at build time;
* had the generated JAR include the list from the object directory.

This version incorporates the last of glandium's suggestions.  This
patch applies cleanly to mozilla-release (over-top of mfinkle's
earlier patch).

Comment 62

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8516810 [details] [diff] [review]
Add duckduckgo searchplugin to certain locales. f=glandium,r=mfinkle

Approval Request Comment

[Feature/regressing bug #]: DDG for #fx10 ship date.

[User impact if declined]: won't hit the date for shipping DDG to listed locales.

[Describe test coverage new/current, TBPL]: tested locally.  Due to both the branch (release) and infrastructure limitations, I do not know of a way to produce a multilocale release build with these changes for testing.

[Risks and why]: I guess we could completely bust searchplugins across all locales, but that'll be pretty easy to test.  This is very unlikely.

[String/UUID change made/needed]: none.

Comment 63

[chris hofmann]

re:  > [Describe test coverage new/current, TBPL]:

see comment 44.  someone should do that before shipping the bits next week.

Comment 64

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8516810 [details] [diff] [review]
Add duckduckgo searchplugin to certain locales. f=glandium,r=mfinkle

Looks sane and local builds seem OK. We need to build this using the multi-locale infra in order to do real testing.

r+, let's get it started.

Comment 65

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to chris hofmann from comment #63)
> re:  > [Describe test coverage new/current, TBPL]:
> 
> see comment 44.  someone should do that before shipping the bits next week.

Defintely. Once the moz-release build candidate is ready, we can start testing.

Comment 66

[Nick Alexander :nalexander [he/him]]

OK, we have r and a.  Who lands?  Who kicks off the builds?

Comment 67

[Lawrence Mandel [:lmandel] (use needinfo)]

Ryan - Can you help with landing these changes on m-r?

Once the changes have landed and the ci builds on treeherder go green, Sylvestre, Lukas, or I will kick off the 33.1 build. (Depends on when the ci build completes.)

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/6233fa4970a4
https://hg.mozilla.org/releases/mozilla-release/rev/9b6e72bafd64

Comment 69

[Aaron Train [:aaronmt]]

Anyone want to toss up an m-r multi build in the interim?

Comment 70

[Nick Alexander :nalexander [he/him]]

Download the candidate build1 APK and unpacking, I see that the list.txt files look healthy but the omni.ja is missing the first locale-specific XML file.  I conclude that the first line of the generated JAR manifest is incorrect; I think that the initial printf is not printing a new line after it on the buildbots.  (Not clear why this prints a new line for me on Mac OS X, but my JAR manifests look fine.)

So we get manifests like:

en-US.jar:locale/en-US/browser/searchplugins/amazondotcom.xml (amazondotcom.xml)
 locale/en-US/browser/searchplugins/bing.xml (bing.xml)

and perhaps those parse correctly but drop the badly formed entry.

The relevant printf is at https://hg.mozilla.org/releases/mozilla-release/rev/9b6e72bafd64#l1.72 -- it's always been there, but I changed later printfs to be echos, changing the new lines a little.

I think we'll need to change that first printf to an echo and respin.  Sorry, folks.  I'll post a patch tomorrow.

Comment 71

[Nick Alexander :nalexander [he/him]]

Attachment: Follow-up to add extra new line in JAR manifest. r=trivial

Comment 72

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8517913 [details] [diff] [review]
Follow-up to add extra new line in JAR manifest. r=trivial

Thanks Nick

Comment 73

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8513698 [details] [diff] [review]
ddg v0.2

So we have them in the other branches.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8517913 [details] [diff] [review]
Follow-up to add extra new line in JAR manifest. r=trivial

Landed on both default and the MOBILE331_2014110511_RELBRANCH to be safe.

https://hg.mozilla.org/releases/mozilla-release/rev/61a0bf70b88a
https://hg.mozilla.org/releases/mozilla-release/rev/ff5068a4aa0c

Comment 75

[Nick Alexander :nalexander [he/him]]

I'll need to check a multi-locale build2 candidate; if somebody could flag me when it's ready, I'd appreciate it.  (I don't know the release candidate schedule.)

Comment 76

[Nick Alexander :nalexander [he/him]]

sledru, others: these patches should *not* land anywhere other than mozilla-release.  This is a hack to work around our schedule.

Comment 77

[Francesco Lodolo [:flod]]

(In reply to Nick Alexander :nalexander from comment #76)
> sledru, others: these patches should *not* land anywhere other than
> mozilla-release.  This is a hack to work around our schedule.

Note: we'll need to figure out what to do after Nov 10 for both mobile and desktop. 
I don't think we can add the searchplugin on Beta (less than one week before sign-off), which means people will lose DDG on Nov 25 if we don't do anything.

Comment 78

[Nick Alexander :nalexander [he/him]]

(In reply to Francesco Lodolo [:flod] from comment #77)
> (In reply to Nick Alexander :nalexander from comment #76)
> > sledru, others: these patches should *not* land anywhere other than
> > mozilla-release.  This is a hack to work around our schedule.
> 
> Note: we'll need to figure out what to do after Nov 10 for both mobile and
> desktop. 
> I don't think we can add the searchplugin on Beta (less than one week before
> sign-off), which means people will lose DDG on Nov 25 if we don't do
> anything.

flod: well spotted.  I guess we need to land on mozilla-beta?  All the way to mozilla-aurora?  At that point we might just want to ship this as a feature that we shouldn't use; at least then it will get tested.

Comment 79

[Francesco Lodolo [:flod]]

I don't think it's needed on mozilla-aurora (we have the entire next Beta cycle to fix it), but it makes sense to have on mozilla-beta.

Comment 80

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #75)
> I'll need to check a multi-locale build2 candidate; if somebody could flag
> me when it's ready, I'd appreciate it.  (I don't know the release candidate
> schedule.)

I have manually verified that the missing search plugin XML files are present in build2.  I can't say if that addresses AaronMT's tickets -- I think those are separate -- but at least I believe our APK is OK.

Comment 81

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #80)

> I have manually verified that the missing search plugin XML files are
> present in build2.  I can't say if that addresses AaronMT's tickets -- I
> think those are separate -- but at least I believe our APK is OK.

Thanks Nick. Great job!

Comment 82

[Aaron Train [:aaronmt]]

(In reply to Nick Alexander :nalexander from comment #80)
> (In reply to Nick Alexander :nalexander from comment #75)
> > I'll need to check a multi-locale build2 candidate; if somebody could flag
> > me when it's ready, I'd appreciate it.  (I don't know the release candidate
> > schedule.)
> 
> I have manually verified that the missing search plugin XML files are
> present in build2.  I can't say if that addresses AaronMT's tickets -- I
> think those are separate -- but at least I believe our APK is OK.

Yep, bug 1094644 resolved with https://bugzilla.mozilla.org/attachment.cgi?id=8517913 

We're testing and recording here: https://mozqa.etherpad.mozilla.org/testing-fennec-33-1

Comment 83

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Francesco Lodolo [:flod] from comment #77)
> (In reply to Nick Alexander :nalexander from comment #76)
> > sledru, others: these patches should *not* land anywhere other than
> > mozilla-release.  This is a hack to work around our schedule.
> 
> Note: we'll need to figure out what to do after Nov 10 for both mobile and
> desktop. 
> I don't think we can add the searchplugin on Beta (less than one week before
> sign-off), which means people will lose DDG on Nov 25 if we don't do
> anything.

Looks like Desktop landed their patches on Aurora and Beta. I assume we should do the same.

Comment 84

[Francesco Lodolo [:flod]]

Desktop is adding the searchplugin to all locales, Mobile only to a very small set of locales, not sure if the same should apply.

Comment 85

[Ryan VanderMeulen [:RyanVM]]

Per IRC discussion, I landed mfinkle's patch across all branches and nalexander's patch (with follow-up folded in) only on beta.

https://hg.mozilla.org/integration/mozilla-inbound/rev/368e58076230
https://hg.mozilla.org/releases/mozilla-aurora/rev/c6004c071eb8
https://hg.mozilla.org/releases/mozilla-beta/rev/f1c1658280cd
https://hg.mozilla.org/releases/mozilla-beta/rev/32c3529eb076

Comment 86

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/368e58076230

Comment 87

[Teodora Vermesan (:TeoVermesan)]

DuckDuckGo is present in the installed search engines list on Firefox for Android 34 Beta 10 on the following new locales: de, en-Gb, en-US, es-ES, fr.
So, i will mark status-firefox34 to verified.

Comment 88

[Teodora Vermesan (:TeoVermesan)]

DuckDuckGo is present in the installed search engines list for the following new locales:
34 Release: de, en-GB, en-US, es-ES, fr 
35 Beta 1 Build 2: de, en-US, es-ES, fr 
36.0a2 Aurora (2014-12-04): de, en-GB, en-US, es-ES, fr 
37.0a1 Nightly (2014-12-04): ): de, en-US, es-ES, fr

So:
-DuckDuckGo is missing for en-GB on 35 Beta 1 build1 and build2
-en-GB locale is missing from Menu -> Settings -> Language on latest Nightly.
Are these 2 issues expected?

Comment 89

[Francesco Lodolo [:flod]]

> -DuckDuckGo is missing for en-GB on 35 Beta 1 build1 and build2
Sort of expected, that's a sign-off problem. The change from bug 1090394 is there on the default branch
http://hg.mozilla.org/releases/l10n/mozilla-beta/en-GB/graph

In Firefox 34 we shipped DDG with a build hack, for fx35 we need a more recent sign-off, which didn't happen yet.

> -en-GB locale is missing from Menu -> Settings -> Language on latest Nightly.
That's expected, we don't have as many locales on nightly
http://hg.mozilla.org/mozilla-central/file/default/mobile/android/locales/maemo-locales

Comment 90

[Teodora Vermesan (:TeoVermesan)]

Based on Comment 12, DuckDuckGo is present in the installed search engines list on Firefox for Android 36 Beta 6 with the following locales: de, en-Gb, en-US, es-ES, fr, es-MX
So, I will mark status-firefox 36 to verified.

Comment 91

[Teodora Vermesan (:TeoVermesan)]

DuckDuckGo is present in the installed search engines list on Firefox for Android 35.0.1 with the following locales: de, en-Gb, en-US, es-ES, fr, es-MX