990799 - Update search plugins to use rel="searchform"

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Drew Willcoxon :adw]

Bug 557665 added the ability to use <Url rel="searchform"> instead of <SearchForm> in search plugin XML.  We should update the search plugins to use it.

That bug also changed google.xml by adding an extra <Url> with rel="searchform", but instead we should add rel="searchform" to the text/html <Url> that was already there so that the right template and all the parameters are picked up for searchform.

Finally, PriorityUrlProvider.jsm (from bug 959576, which I'll mark as depending on this bug) has a related TODO: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PriorityUrlProvider.jsm#70  Adding rel="searchform" to <Url>s that have all the relevant parameters should either satisfy or help satisfy that to-do (I'm not sure which at the moment), so that should be updated in some appropriate way, too.

Comment 1

[Alex Bardas :alexbardas]

Attachment: Update search plugins to use rel="searchform"

Comment 2

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

Comment on attachment 8490203 [details] [diff] [review]
Update search plugins to use rel="searchform"

>diff --git a/browser/components/search/test/browser_google.js b/browser/components/search/test/browser_google.js

>-    searchForm: "https://www.google.com/",
>+    searchForm: "https://www.google.com/search?q=&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:unofficial&client=" + google_client,

The "rls" parameter depends on MOZ_OFFICIAL_BRANDING, so we need to adjust its expected value here to depend on that, to avoid this test failing on release/beta builds (similar to google_client). The parameter value is "org.mozilla:en-US:official" for those builds (see http://hg.mozilla.org/mozilla-central/annotate/543f57ab6a44/toolkit/components/search/nsSearchService.js#l153). 

>diff --git a/browser/components/search/test/browser_yahoo.js b/browser/components/search/test/browser_yahoo.js

>-    searchForm: "https://search.yahoo.com/",
>+    searchForm: "https://search.yahoo.com/search?p=&ei=UTF-8&fr=moz35",

Similarly the "fr" parameter here theoretically depends on the value of a pref. But apparently those are the same everywhere since bug 995390, so I guess no need to change anything here. I asked in bug 995390 why this is even still pref-based.

>diff --git a/browser/locales/en-US/searchplugins/answers.xml b/browser/locales/en-US/searchplugins/answers.xml
>diff --git a/browser/locales/en-US/searchplugins/creativecommons.xml b/browser/locales/en-US/searchplugins/creativecommons.xml

We don't ship these in en-US, and probably shouldn't be shipping them in any locales (though many apparently still do, e.g.: http://mxr.mozilla.org/l10n-mozilla-release/search?string=answers&find=list.txt). Can you not touch them here, and file a separate bug about removing them and CC me?

>diff --git a/browser/locales/en-US/searchplugins/bingmetrofx.xml b/browser/locales/en-US/searchplugins/bingmetrofx.xml
>diff --git a/browser/locales/en-US/searchplugins/googlemetrofx.xml b/browser/locales/en-US/searchplugins/googlemetrofx.xml
>diff --git a/browser/locales/en-US/searchplugins/wikipediametrofx.xml b/browser/locales/en-US/searchplugins/wikipediametrofx.xml
>diff --git a/browser/locales/en-US/searchplugins/yahoometrofx.xml b/browser/locales/en-US/searchplugins/yahoometrofx.xml

Similarly we don't use these anymore. Let's not touch them here.

>diff --git a/browser/locales/en-US/searchplugins/wikipedia.xml b/browser/locales/en-US/searchplugins/wikipedia.xml

>-<Url type="text/html" method="GET" template="http://en.wikipedia.org/wiki/Special:Search" resultdomain="wikipedia.org">
>+<Url type="text/html" method="GET" template="http://en.wikipedia.org/wiki/Special:Search"
>+     resultdomain="wikipedia.org" rel="searchfrom">

Typo: "searchfrom". Looks like this is the only place you made the mistake, but worth testing these all manually again to confirm :)

Looks good with those addressed, but will take a look at the next patch.

Comment 3

[Alex Bardas :alexbardas]

Attachment: Update search plugins to use rel="searchform" Patch 2

Comment 4

[Francesco Lodolo [:flod]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #2)
> We don't ship these in en-US, and probably shouldn't be shipping them in any
> locales (though many apparently still do, e.g.:
> http://mxr.mozilla.org/l10n-mozilla-release/search?string=answers&find=list.
> txt). Can you not touch them here, and file a separate bug about removing
> them and CC me?

Note: if we remove them from en-US builds will just break (for Creative Commons see also bug 939804).

And we have one more problem.
http://mxr.mozilla.org/l10n-mozilla-release/search?string=answers&find=list.txt

Most of these locales are inactive, not sure how we can move fixes to release (unless I simply land the change there, but I have no idea if the build system will simply ignore that).

Comment 5

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

(In reply to Francesco Lodolo [:flod] from comment #4)
> Note: if we remove them from en-US builds will just break (for Creative
> Commons see also bug 939804).

We should discuss this in the new bug.

Comment 6

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

Looks like this will need to be merged with bug 758857.

Comment 7

[Alex Bardas :alexbardas]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #5)
> (In reply to Francesco Lodolo [:flod] from comment #4)
> > Note: if we remove them from en-US builds will just break (for Creative
> > Commons see also bug 939804).
> 
> We should discuss this in the new bug.

I've filled bug 1068456 for this.

(In reply to :Gavin Sharp [email: [e-mail]] from comment #6)
> Looks like this will need to be merged with bug 758857.

It's a minor change, I can rebase my patch on top of that if this is ok.

Comment 8

[Alex Bardas :alexbardas]

Attachment: Update search plugins to use rel="searchform" Patch 3

The patch was rebased on top of https://hg.mozilla.org/integration/mozilla-inbound/rev/13c6ae2f5f3e, which is on mozilla-inbound.

Comment 9

[Alex Bardas :alexbardas]

Try run looks good:

https://tbpl.mozilla.org/?tree=Try&rev=c31cb5e40feb

If you land it, please do it on mozilla-inbound.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca3d3875c7d

Comment 11

[Robert Kaiser]

Unless this has actually visible consequences for users, I don't think we need manual testing by the QA team here. If you have concrete steps to test that are actually visible to users, please state those steps and flip the flag to + again.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/9ca3d3875c7d