If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Support OpenSearch Suggestions extension (JSON format)

RESOLVED FIXED in Firefox 2 beta1

Status

()

Firefox
Search
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Joe Hughes, Assigned: Joe Hughes)

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 beta1
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Flags: blocking-firefox2?
(Assignee)

Comment 1

12 years ago
Created attachment 224935 [details] [diff] [review]
Adds support for JSON suggest URL type, and multiple URLs per engine in general
Attachment #224935 - Flags: superreview?(mconnor)
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?
(Assignee)

Comment 3

12 years ago
Created attachment 224943 [details] [diff] [review]
Revision #1 addressing Gavin's comments
Attachment #224935 - Attachment is obsolete: true
Attachment #224943 - Flags: review?(gavin.sharp)
Attachment #224935 - Flags: superreview?(mconnor)
ok, though supporting CSV is uesless to us, and I'd rather not have it be part of the spec.
Flags: blocking-firefox2? → blocking-firefox2+

Comment 5

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

Comment 6

11 years ago
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 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.
Attachment #224943 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 8

11 years ago
Created attachment 225047 [details] [diff] [review]
Addresses comments from #7
Attachment #224943 - Attachment is obsolete: true
Attachment #225047 - Flags: superreview?(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?
Attachment #225047 - Flags: superreview?(mconnor)
Attachment #225047 - Flags: superreview+
Attachment #225047 - Flags: approval-branch-1.8.1+
(Assignee)

Comment 10

11 years ago
Created attachment 225898 [details] [diff] [review]
Updated patch to account for other landings
Attachment #225047 - Attachment is obsolete: true
Attachment #225898 - Flags: review?(gavin.sharp)
Comment on attachment 225898 [details] [diff] [review]
Updated patch to account for other landings

r=me on the merge changes
Attachment #225898 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 12

11 years ago
Landed on branch & trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.