Closed Bug 963132 Opened 10 years ago Closed 10 years ago

Implement add to searchbar (search-engine autodiscovery)

Categories

(SeaMonkey :: Search, defect)

Product:
SeaMonkey
Component:
Search
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.26 fixed, seamonkey2.27 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.27
Tracking Flags:
Tracking Status
seamonkey2.26 --- fixed
seamonkey2.27 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(2 files, 1 obsolete file)

Part 1: Refactor the DOMLinkAdded event handler code in tabbrowser
9.25 KB, patch
neil
: review+
Details | Diff | Splinter Review
Part 2 Implement add to searchbar (search-engine autodiscovery)
5.66 KB, patch
neil
: review+
Details | Diff | Splinter Review 
When we ported OpenSearch from Firefox, we missed out on the autodiscovery feature. Probably because Firefox had refactored their code in the mean time and it ended somewhere else entirely.

I noticed this when I came across this SeaMonkey extension:

> https://addons.mozilla.org/en-US/seamonkey/addon/smsengineadd/
> Add Search Engines for SeaMonkey  0.1.0
> by Herrminator
> Add missing search-engine detection to SeaMonkey
Attached patch Patch v1.0 Proposed fix (obsolete) — — Splinter Review 
I refactored the DOMLinkAdded handler and moved most of the icon handling logic to a separated method [getLinkIconURI] (like in Firefox). Mostly because I found the current code to be rather opaque not to mention hard to understand.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #8364437 - Flags: review?(neil)
Comment on attachment 8364437 [details] [diff] [review]
Patch v1.0 Proposed fix

The way the BrowserSearch object is written it expect to receive an "engine" and a document, and performs all the necessary checks, so you don't have to. It does however mean that the regular progress listener is a bad place to add the search engine notification, since its intent is to only listen to the current tab. One option would be for the tabbrowser to add the engine itself. (Not sure why the BrowserSearch object only checks the favicon.ico icon though.) Another option would be to make the BrowserSearch object a DOMLinkAdded handler.

>-            if (!isIcon && !isFeed)
This got lost.

>+            var isSearch = /(?:^|\s)search(?:\s|$)/i.test(rel) && link.title &&
>+                           /^(https?|ftp):/i.test(href) &&
>+                           /(?:^|\s)application\/opensearchdescription\+xml(?:;?).*$/i.test(type);
This would have been a good place to add a notification, if there had been a good way of adding a notification.

>-              // The following code is only executed once, see
>-              // 'continue' above, and the following return(s)
This would have been a good place to add a per-tab notification, if that had been the requirement.
Attachment #8364437 - Flags: review?(neil) → review-
> +            if (isIcon) {
> +             var iconUri = this.getLinkIconURI(link);
Around here Firefox checks for isFailedFavicon(). We never did. However nobody has ever complained so I didn't add this check.

> +             if (iconUri)
> +               this.setIcon(this.tabs[index], iconUri);
> +            }

> +            if (isFeed) {
> +              try {
> +                var feedUri = this.mIOService.newURI(href, targetDoc.characterSet, null);
> +                this.mSecMan.checkLoadURIWithPrincipal(targetDoc.nodePrincipal, feedUri,
> +                                                       this.mSecMan.DISALLOW_INHERIT_PRINCIPAL);
The equivalent code path in firefox calls isValidFeed() which for some obscure reason we never do.
isValidFeed() calls urlSecurityCheck() which calls checkLoadURIWithPrincipal.
Attachment #8364437 - Attachment is obsolete: true
Attachment #8369104 - Flags: review?(neil)
> +                   /(?:^|\s)application\/opensearchdescription\+xml(?:;?.*)$/i.test(link.type);
I found some useful sites to check my regexp:
  http://regexpal.com/
  http://regex101.com/#javascript

Also see: http://hg.mozilla.org/mozilla-central/annotate/25c361f6661f/browser/base/content/test/general/browser_discovery.js#l84

> +      <method name="onError">
> +        <parameter name="aErrorCode"/>
> +        <body><![CDATA[
> +        ]]></body>
> +      </method>
Error: Error invoking addEngine install callback: TypeError: aCallback.onError is not a function
Source file: resource://gre/components/nsSearchService.js
Line: 3859

Discovered when testing with:
http://www.opensearch.org/Community/OpenSearch_search_engine_directories
(Irony)
SeaMonkey could not download the search plugin from:
http://www.opensearch.org/opensearch_desc.php

Annoyingly even if you handle the error condition in your code, nsSearchService.js still pops up an alert().
http://hg.mozilla.org/mozilla-central/annotate/25c361f6661f/toolkit/components/search/nsSearchService.js#l1416

> -          var allbrowsers = getBrowser().mPanelContainer.childNodes;
> -          for (var tab = 0; tab < allbrowsers.length; tab++) {
> -            var browser = getBrowser().getBrowserAtIndex(tab);
> +          for (var browser of getBrowser().browsers) {
Error: browser is undefined
Source file: chrome://communicator/content/search/search.xml
Line: 267

Somehow I managed to get six browsers but three tabs. The three browsers without corresponding tabs were all about:blank. Probably a misbehaving extension.
Anyway this prompted me to take a look at the code which looked rather iffy, so I fixed it (x2).
Attachment #8369105 - Flags: review?(neil)
Comment on attachment 8369104 [details] [diff] [review]
Part 1: Refactor the DOMLinkAdded event handler code in tabbrowser

>+            var rel = link.rel;
>+            var type = link.type;
>             var isIcon = /(?:^|\s)icon(?:\s|$)/i.test(rel) &&
>                          this.mPrefs.getBoolPref("browser.chrome.site_icons");
> 
>             var isFeed = /(?:^|\s)feed(?:\s|$)/i.test(rel) ||
>                          (/(?:^|\s)alternate(?:\s|$)/i.test(rel) &&
>                           !/(?:^|\s)stylesheet(?:\s|$)/i.test(rel) &&
>                           /^\s*application\/(?:atom|rss)\+xml\s*$/i.test(type));
> 
>             if (!isIcon && !isFeed)
>               return;
> 
>+            var targetDoc = link.ownerDocument;
>+            var index = this.getBrowserIndexForDocument(targetDoc);
>+            if (index < 0)
>+              return;
>+
>+            if (isIcon) {
>+             var iconUri = this.getLinkIconURI(link);
>+             if (iconUri)
>+               this.setIcon(this.tabs[index], iconUri);
>+            }
[So, what you could do here is something like this:

var targetDoc = link.ownerDocument;
var index = this.getBrowserIndexForDocument(targetDoc);
if (index < 0)
  return;

var rel = link.rel;
var type = link.type;

if (/(?:^|\s)icon(?:\s|$)/i.test(rel) &&
    this.mPrefs.getBoolPref("browser.chrome.site_icons")) {
  var iconUri = this.getLinkIconURI(link);
  if (iconUri)
    this.setIcon(this.tabs[index], iconUri);
}

etc.]

>+          if (contentPolicy.shouldLoad(nsIContentPolicy.TYPE_IMAGE,
>+                                       uri, targetDoc.documentURIObject,
>+                                       aLink, aLink.type, null)
>+                                       == nsIContentPolicy.ACCEPT) {
>+            return uri;
>+          }
>+          return null;
Since we're changing this anyway, it probably makes sense to reverse this so that we "return early" on all failures. Also, I prefer the previous version where the last parameter to shouldLoad was deliberately wrapped on to the next line so that the comparison operator didn't look out of place. r=me with those fixed.
Attachment #8369104 - Flags: review?(neil) → review+
(In reply to comment #5)
> if (/(?:^|\s)icon(?:\s|$)/i.test(rel) &&
>     this.mPrefs.getBoolPref("browser.chrome.site_icons")) {
>   var iconUri = this.getLinkIconURI(link);
>   if (iconUri)
>     this.setIcon(this.tabs[index], iconUri);
> }
Well, I guess you could leave this as
var isIcon = /(?:^|\s)icon(?:\s|$)/i.test(rel) &&
             this.mPrefs.getBoolPref("browser.chrome.site_icons");
if (isIcon) {
(In reply to Philip Chee from comment #4)
> Somehow I managed to get six browsers but three tabs. The three browsers
> without corresponding tabs were all about:blank. Probably a misbehaving
> extension.
Regular undo close tab, but thanks for spotting this.
Comment on attachment 8369105 [details] [diff] [review]
Part 2 Implement add to searchbar (search-engine autodiscovery)

>+  handleEvent: function (event) {
>+    if (event.type != "DOMLinkAdded")
>+      return;
>+
>+    var link = event.originalTarget;
>+    if (!link || !link.ownerDocument || !link.rel || !link.href)
>+      return;
[None of these checks are actually necessary. You only add a listener for the given event type, events always have a target, elements always have an owner document, and the following test fails for empty rel or href (and they can never be null either). However if you keep this check, then for consistency I would check the title and type too (and remove the title from the following check).]

>+    if (isSearch) {
>+      var engine = {title: link.title, href: link.href};
>+      this.addEngine(engine, link.ownerDocument);
Just pass the link itself as the engine, it has the correct title and href properties already. r=me with that fixed.
Attachment #8369105 - Flags: review?(neil) → review+
Pushed:
Part 1: http://hg.mozilla.org/comm-central/rev/a95d8a70b714
Part 2: http://hg.mozilla.org/comm-central/rev/483a91df5ddb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-seamonkey2.27: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.27
Blocks: 1018792
landing on comm-release for SeaMonkey 2.26.1

$ hg log -r tip:-2
changeset:   20181:6ceb04d3717d
branch:      SEA_2_26_1_RELBRANCH
tag:         tip
user:        Philip Chee <philip.chee@gmail.com>
date:        Thu Feb 20 01:40:59 2014 +0800
summary:     Bug 963132 Part 2 Implement add to searchbar (search-engine autodiscovery) r=Neil

changeset:   20180:38065a20158c
branch:      SEA_2_26_1_RELBRANCH
user:        Philip Chee <philip.chee@gmail.com>
date:        Thu Feb 20 01:40:05 2014 +0800
summary:     Bug 963132 Part 1 - Refactor the DOMLinkAdded event handler code in tabbrowser. r=Neil
status-seamonkey2.26: --- → fixed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: