335443 - Support autodetection --> option of adding search engine definitions to browser search box

Status: RESOLVED FIXED

(Firefox :: Search, enhancement)

Description

[Joe Hughes]

If a page has an explicit link element to a search engine definition, we should notify the user somehow and give them the option of adding it to the browser search box options.

Comment 1

[Mike Connor [:mconnor]]

Joe, is there a spec somewhere around autodiscovery of searchplugins?  OpenSearch uses meta tags, iirc, but I'm not sure we want to do that.

Comment 2

[Phil Ringnalda (:philor)]

OpenSearch uses the same <link rel="" type="" href=""> that pretty much every other sort of autodiscovery uses (and that we get to discover nearly for free with DOMLinkAdded).

http://opensearch.a9.com/spec/1.1/description/#autodiscovery

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

See http://wiki.mozilla.org/Search_Service:Code_Design#Automatic_Detection for the auto-detection heuristic. 

Comment 4

[Joe Hughes]

(In reply to comment #3)

I copied this comment to the releveant bug 335448. 

Comment 5

[Pam Greene]

Attachment: Adds infrastructure and UI for adding <link rel> OpenSearch engines

Comment 6

[Pam Greene]

Attachment: Different icon when engines are available to be added

This image also needs to be added to 
/mozilla/browser/themes/{pinstripe|winstripe}/browser/

One page with a <link rel> tag, to use for testing, is
http://www.microsoft.com/windows/ie/searchguide/default_new.mspx#

Comment 7

[Brett Wilson]

Comment on attachment 221029 [details] [diff] [review]
Adds infrastructure and UI for adding <link rel> OpenSearch engines

Looks OK to me, but I am not in a state to do good reviews now...

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 221029 [details] [diff] [review]
Adds infrastructure and UI for adding <link rel> OpenSearch engines

>Index: browser/components/search/nsSearchService.js
>-                   [kOpenSearchNS_11, kOpenSearchNS_10])) {
>+                       [kOpenSearchNS_11, kOpenSearchNS_10]) ||
>+                   checkNameSpace(this._data, [kOpenSearchLocalName],
>+                        [kOpenSearchNS_11_alt, kOpenSearchNS_10_alt])) {

Your indentation is a little wacky here. tabs?

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

>+              var labelStr = this._stringBundle.getFormattedString("cmd_addFoundEngine", [engineInfo.title]);              

nit: Long line... wrap:

var labelStr = 
    this._stringBundle.getFormattedString("cmd_addFoundEngine",
                                          [engineInfo.title]);

>+          if (aTarget.getAttribute("class").indexOf("addengine-item") != -1) {
>+            var searchService = Components.classes["@mozilla.org/browser/search-service;1"]
>+                                    .getService(Components.interfaces.nsIBrowserSearchService);
>+            if (searchService) {
>+              // If the description file URI ends in "xml", assume an XML file; otherwise, assume 
>+              // text.  That's about the best we can do without loading the file itself.
>+              const engineURI = aTarget.getAttribute("uri");
>+              var type = Components.interfaces.nsISearchEngine.DATA_TEXT;              
>+              if (engineURI.search(/\.xml$/i))
>+                type = Components.interfaces.nsISearchEngine.DATA_XML;
>+              searchService.addEngine(engineURI, type, aTarget.getAttribute("src"));              
>+            }
>+          }
>+          else if (aTarget.engine) {

nit: long lines

>Index: browser/themes/pinstripe/browser/searchbar.css
>-  list-style-image: url("chrome://browser/skin/Search-bar.png");
>+	list-style-image: url("chrome://browser/skin/Search-bar.png");

nit: tab!

>Index: toolkit/content/widgets/browser.xml

>+      <field name="_engines">null</field>
>+      <property name="engines"
>+                onget="return this._engines;"
>+                onset="return this._engines = val;"/>
>+

You don't need to do this to set the property in browser.js... in fact, this is bad - it adds indication of a browser feature (search) to a toolkit level component. Just remove these lines. It will still work.

Comment 9

[Pam Greene]

Attachment: Patch addressing Ben's comments

Thanks for the comments.  Wrapped lines, replaced tab, removed declaration for .engines.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 221037 [details] [diff] [review]
Patch addressing Ben's comments

r+a=[e-mail]

I'm going to watch for Tp regressions before landing on the branch.

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

fixed-on-trunk, fixed-1.8-branch

Comment 12

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

The OpenSearch parts of this patch were already done as part of bug 335691, but were waiting for review. I would have appreciated being given a heads up about the changes to nsSearchService.js.

Comment 13

[Robert Kaiser]

The change
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/toolkit/components&command=DIFF_FRAMESET&file=Makefile.in&rev2=1.56&rev1=1.55
that is attributed to this bug but not in the patch looks strange to me...

Ben, why are you adding feeds to DIRS a second time here?