Closed Bug 990799 Opened 6 years ago Closed 5 years ago

Update search plugins to use rel="searchform"

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 35
Iteration:
35.2

People

(Reporter: adw, Assigned: alexbardas)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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.
Flags: firefox-backlog+
Blocks: 958188
Points: --- → 2
Flags: qe-verify+
Blocks: 1065997
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Attachment #8490203 - Flags: review?(gavin.sharp)
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.
Attachment #8490203 - Flags: review?(gavin.sharp) → review-
Attachment #8490203 - Attachment is obsolete: true
Attachment #8490520 - Flags: review?(gavin.sharp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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).
(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.
Looks like this will need to be merged with bug 758857.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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: gavin@gavinsharp.com] 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.
Attachment #8490520 - Flags: review?(gavin.sharp) → review+
The patch was rebased on top of https://hg.mozilla.org/integration/mozilla-inbound/rev/13c6ae2f5f3e, which is on mozilla-inbound.
Attachment #8490520 - Attachment is obsolete: true
Attachment #8491215 - Flags: review+
Try run looks good:

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

If you land it, please do it on mozilla-inbound.
Keywords: checkin-needed
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.
Flags: qe-verify+ → qe-verify-
https://hg.mozilla.org/mozilla-central/rev/9ca3d3875c7d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Blocks: 1100670
Blocks: 1100672
You need to log in before you can comment on or make changes to this bug.