335460 - re-implement special search plugin parameters

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

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

Need to be able to pass current locale (from general.useragent.locale), distribution ID (MOZ_DISTRIBUTION_ID), "official" or "unofficial" depending on  OFFICIAL_BRANDING (MOZ_BRANDING_DIRECTORY). Also need configurable parameters for communicating the default engine order.

I think that the indication for which parameters should be sent should be in the plugin file itself instead of in prefs sourced from searchconfig. That avoids the problems with engines names that can't be used as keys in properties files (engines are still uniquely identified by display name, which I plan to change in bug 335102).

Comment 1

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

Does anyone see any problems with putting, for example, <input name="foo" value="%LOCALE%"> in the search plugin files themselves? This means that the files themselves need to be tightly controlled, and perhaps makes managing them harder since the information is spread out instead of centralized, but it has the advantage of making it easier to customize parameters for different plugins individually, while eliminating the problem of engines that have names that can't be used as pref keys in .properties files.

Thoughts?

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Other thoughts: we could post-process the search plugins to add these common elements at build time.  We could have an all-plugins.xml that we just brute-force merged with all of our default plugins when we load them, which would contain just these sorts of bits.

Do different search engines need to have the locale in different parameter names?

Comment 3

[Benjamin Smedberg]

Doing this at build time instead of at runtime makes it more difficult to ship extensions to change search tracking codes for co-branded builds.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

How so?  Extensions can package new search plugins that replace the ones we ship, just as they would have to do with any other mechanism that puts the params in the  search plugins.

Comment 5

[Benjamin Smedberg]

They can? They have not been able to do that before (they can add new searchplugins, but not replace the default ones).

Comment 6

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

(In reply to comment #5)
> They can? They have not been able to do that before (they can add new
> searchplugins, but not replace the default ones).

The current behavior for engines with the same name is "first one loaded wins". Are there any garantees about the order the entries in the NS_APP_SEARCH_DIR_LIST enumerator are returned?

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I don't see how "insert the <input name="LOCALE" value="%LOCALE%"> tag into the search plugin at build-time" affects what we can do with extensions.  I'm not proposing that we substitute %LOCALE% at build time, and neither is Gavin to my knowledge.  We're talking about how to deal with the fact that we want a bunch of attributes in all of them, and that we don't want to have to manually duplicate (and maintain in duplicate) those parameters in every search plugin.

Comment 8

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

Attachment: patch

Comment 9

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

I'll provide more details on the patch tonight, I have to leave now so I'm dumping it here so mconnor can review.

Comment 10

[Axel Hecht [:Pike]]

Regarding the locale, is general.useragent.locale enough or do we need a build time locale string? Somehwat related to my post in .platform.
Who do we need to ask about the distribution ID stuff in our other builds?

Comment 11

[Mike Connor [:mconnor]]

I think in this case, UA locale should suffice.  Build-time is hard since we always build en-US and repackage, no?

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 228069 [details] [diff] [review]
patch

}
else if

is the new standard, bah :P

>+      } else if (param.localName == "MozParam" &&
>+                 // We only support MozParams for appdir-shipped search engines
>+                 this._isInAppDir) {
>+        var value;
>+        switch (param.getAttribute("condition")) {
>+          case "defaultEngine":
>+            const defPref = BROWSER_SEARCH_PREF + "defaultenginename";
>+            var defaultPrefB = Cc["@mozilla.org/preferences-service;1"].
>+                               getService(Ci.nsIPrefService).
>+                               getDefaultBranch(null);
>+            const nsIPLS = Ci.nsIPrefLocalizedString;
>+            var defaultName;
>+            try {
>+              defaultName = defaultPrefB.getComplexValue(defPref, nsIPLS).data;
>+            } catch (ex) {}
>+
>+            if (this.name == defaultName)
>+              value = param.getAttribute("trueValue");
>+            else
>+              value = param.getAttribute("falseValue");
>+            url.addParam(param.getAttribute("name"), value);
>+            break;
>+        }

this shouldn't be a switch statement, unless there will be many more of these in the near future.

I'm not sure this is quite right for the google thing, but we have time to tweak this.

Comment 13

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

(In reply to comment #10)
> Regarding the locale, is general.useragent.locale enough or do we need a build
> time locale string? Somehwat related to my post in .platform.
> Who do we need to ask about the distribution ID stuff in our other builds?

The old code (nsInternetSearchService.cpp) used the pref, so I did too. These parameters and their meaning really needs to be sorted out - I'm not sure how much either of the engines really rely on them beyond looking for identifiable strings, but that's a whole other issue. For now, I just want parity with 1.5.

Comment 14

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

mozilla/browser/components/search/Makefile.in 	1.7
mozilla/browser/locales/en-US/searchplugins/google.xml 	1.10
mozilla/browser/components/search/nsSearchService.js 	1.46

Comment 15

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

Attachment: as checked in

Comment 16

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

Comment on attachment 228148 [details] [diff] [review]
as checked in

This has been fairly well tested, and is not likely to cause any regressions, since it only adds to the parameters that are already being replaced.

Comment 17

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

mozilla/browser/components/search/Makefile.in 	1.3.2.4
mozilla/browser/locales/en-US/searchplugins/google.xml 	1.1.2.9
mozilla/browser/components/search/nsSearchService.js 	1.1.2.41

Comment 18

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

The patch from this bug added the following parameters:
{moz:distributionID}: equivalent to the MOZ_DISTRIBUTION_ID build-time variable (e.g. the "org.mozilla" in "rls=org.mozilla:en-US:unofficial")
{moz:locale}: the ab-CD locale code of the current build (the value of the general.useragent.locale pref)
{moz:official}: whether or not the build is "official" (has official branding)

These parameters are only available to engines in the app search plugin directory.

The other thing this patch adds is support for the following type of parameter:
<MozParam name="client" condition="defaultEngine" trueValue="firefox-a" falseValue="firefox"/>

The only currently supported "condition" is defaultEngine, but other conditions could be added in the future. The parameter has the value of the trueValue attribute if the engine is the default engine for that build as configured when it was built, and the value of falseValue otherwise. This is also only available to appdir engines.

This means that the parameters must be added to the plugin description file, instead of added in through some other means, like the old system. It sounds like we're going to be using a single Google plugin for all locales, so I think that having the parameter in the description file itself shouldn't be too much of a burden.

Comment 19

[Takanori MATSUURA]

(In reply to comment #18)
> {moz:locale}: the ab-CD locale code of the current build (the value of the
> general.useragent.locale pref)

When using google search plugin, search result returns in English.

Example 1:
In Japanese langpack from Mozilla Japan, "general.useragent.locale" is set as "ja" (for non-MacOS X) or "ja-JP-mac" (for MacOS X).
http://ftp.mozilla-japan.org/pub/mozilla-japan/firefox/development/2.0a3/

Google accepts only the value "ja" for the parameter "hl" for Japanese language. When hl parameter is set as "ja-JP-mac", google returns pages in English.


Another examples:
Firefox has es-AR and es-ES langpack for Spanish but google returns English page for those hl values.

Languages list in Google is http://www.google.com/language_tools .

Comment 20

[Axel Hecht [:Pike]]

I filed bug 343849 on that issue, reopening this bug would be overdoing it.