340857 - Support OpenSearch Suggestions extension (JSON format)

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Joe Hughes]

http://www.unto.net/wiki/OpenSearch_Suggestions_Extension

Right now, we're using a "SuggestionUrl" tag containing a prefix URL for the suggestion query.  We should replace this with support for the "application/x-suggestions+json" type of the "Url" tag.

Comment 1

[Joe Hughes]

Attachment: Adds support for JSON suggest URL type, and multiple URLs per engine in general

Comment 2

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

Comment on attachment 224935 [details] [diff] [review]
Adds support for JSON suggest URL type, and multiple URLs per engine in general

>Index: browser/components/search/nsIBrowserSearchService.idl

>+   * @param  responseType
>+   *         The MIME type of response to use.  If null, will default
>+   *         to "text/html"

"the MIME type of response to use" isn't very clear here, I think, but this is tricky to phrase. I'd prefer something like "the MIME type representing the response type of the obtained submission object". Hrm, maybe that's too long.

>Index: browser/components/search/nsSearchService.js

>+  addParam: function SRCH_ENG_addParam(aName, aValue, aResponseType) {

>+    var url = this._getUrlOfType(aResponseType);
>+
>+    ENSURE_WARN(url,
>+                "Engine object has no URL for response type " + aResponseType,
>+                Cr.NS_ERROR_UNEXPECTED);

Not having a responseType isn't really UNEXPECTED, and doesn't deserve a _WARN. I'd change it to be a ENSURE() with Cr.NS_ERROR_FAILURE.

>+  getSubmission: function SRCH_ENG_getSubmission(aData, aResponseType) {
>+    if (!aResponseType)
>+      aResponseType = URLTYPE_SEARCH_HTML;
>+
>+    var url = this._getUrlOfType(aResponseType);
>+
>+    ENSURE_WARN(url,
>+                "Engine object has no URL for response type " + aResponseType,
>                 Cr.NS_ERROR_UNEXPECTED);

Similarly, should this just return null if we don't have the response type? I think that'd be better than throwing. I don't think this should be a WARN, either.

>Index: browser/components/search/nsSearchSuggestions.js

>       if (status == 200 && responseText != "") {
>         var searchString, results, queryURLs;
>         var searchService = 
>           Components.classes["@mozilla.org/browser/search-service;1"]
>                     .getService(Components.interfaces.nsIBrowserSearchService);
>-        var sandboxHost = "http://" + searchService.currentEngine.suggestionURI.host;
>+        var sandboxHost = "http://" + this._suggestURI.host;

You changed this to prePath (which is correct) in your patch for bug 339441. You can also remove searchService here since it's now unused.

>+    this._suggestURI = engine.getSubmission(
>+        searchString,
>+        SEARCH_RESPONSE_SUGGESTION_JSON).uri;
>+    this._request.open("GET", this._suggestURI.spec, true);

Maybe we should document that we don't support POST suggestion URIs? I guess that belongs in the OpenSearch mini-spec.

>Index: browser/components/search/content/search.xml

>-          var submission = this.currentEngine.getSubmission(aData);
>+          var submission = this.currentEngine.getSubmission(aData,
>+                                                            null); // HTML

nit: put the comment above to avoid wrapping?

>Index: browser/locales/en-US/searchplugins/yahoo.xml

>+<Url type="application/x-suggestions+json"
>+     method="GET"
>+     template="http://ff.search.yahoo.com/gossip?output=fxjson&amp;command={searchTerms}" />

nit: keep the first two attributes on the same line?

Comment 3

[Joe Hughes]

Attachment: Revision #1 addressing Gavin's comments

Comment 4

[Mike Connor [:mconnor]]

ok, though supporting CSV is uesless to us, and I'd rather not have it be part of the spec.

Comment 5

[DeWitt Clinton]

Mike, I agree -- if you guys aren't using CSV then I'll drop it from the spec.

Comment 6

[DeWitt Clinton]

I updated the document to remove support for CSV.  Please review:

  http://www.unto.net/wiki/OpenSearch_suggestions_extension

Please let me anywhere else it can be improved.

Comment 7

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

Comment on attachment 224943 [details] [diff] [review]
Revision #1 addressing Gavin's comments

>Index: browser/base/content/browser.js

>-    var submission = engine.getSubmission(searchText);
>+    var submission = engine.getSubmission(searchText, null); // HTML response

Need to null check |submission| now, I guess. Maybe we should ensure that an engine has at least one HTML URL before adding it to the internal store (at http://lxr.mozilla.org/seamonkey/source/browser/components/search/nsSearchService.js#1249)?

>Index: browser/components/search/nsSearchService.js

>+const URLTYPE_SUGGEST_JSON = "application/x-suggestions+json";
>+const URLTYPE_SEARCH_HTML = "text/html";

uber-nit: align equal signs :)

>+  _getUrlOfType: function SRCH_ENG__getUrlOfType(type) {

nit: a-prefixed arguments in this file, and capitalize URL in _getURLOfType?

>+  addParam: function SRCH_ENG_addParam(aName, aValue, aResponseType) {

>+    ENSURE(url,
>+           "Engine object has no URL for response type " + aResponseType,
>+           Cr.NS_ERROR_FAILURE);

nit: keep the first two arguments on the same line

>+    var submission = engine.getSubmission(searchString,
>+                                          SEARCH_RESPONSE_SUGGESTION_JSON);
>+    this._suggestURI = submission.uri;

This is going to affect your backoff patch, since the URI you save to compare will no longer be unique (it will contain the query string). I guess you can deal with that then if this patch lands first :).

>Index: browser/components/search/content/search.xml

>       <method name="doSearch">
...
>           var postData = { value: null };
>-          var submission = this.currentEngine.getSubmission(aData);
>+
>+          // null parameter below specifies HTML response for search
>+          var submission = this.currentEngine.getSubmission(aData, null);
...
>             postData.value = submission.postData;
...
>             getBrowser().loadOneTab(url, null, null, postData.value, false, false);

I've just realized that postData.value can just be postData, since it's always passed directly. Do you mind making that change?

You forgot to remove the suggestionURI getter in nsSearchService, and the attribute from nsIBrowserSearchService.idl.

Comment 8

[Joe Hughes]

Attachment: Addresses comments from #7

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 225047 [details] [diff] [review]
Addresses comments from #7

For consistency's sake, can you put the suggestion before the text/html Url in the yahoo plugin as well?

Comment 10

[Joe Hughes]

Attachment: Updated patch to account for other landings

Comment 11

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

Comment on attachment 225898 [details] [diff] [review]
Updated patch to account for other landings

r=me on the merge changes

Comment 12

[Joe Hughes]

Landed on branch & trunk.