Closed
Bug 963132
Opened 11 years ago
Closed 11 years ago
Implement add to searchbar (search-engine autodiscovery)
Categories
(SeaMonkey :: Search, defect)
SeaMonkey
Search
Tracking
(seamonkey2.26 fixed, seamonkey2.27 fixed)
RESOLVED
FIXED
seamonkey2.27
People
(Reporter: philip.chee, Assigned: philip.chee)
References
Details
Attachments
(2 files, 1 obsolete file)
|
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
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
|
Assignee | |
Comment 1•11 years ago
|
||
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.
|
||
Comment 2•11 years ago
|
||
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-
|
|
Assignee | |
Comment 3•11 years ago
|
||
> + 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)
|
|
Assignee | |
Comment 4•11 years ago
|
||
> + /(?:^|\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 5•11 years ago
|
||
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+
|
|
||
Comment 6•11 years ago
|
||
(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) {
|
|
||
Comment 7•11 years ago
|
||
(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 8•11 years ago
|
||
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+
|
|
Assignee | |
Comment 9•11 years ago
|
||
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: 11 years ago
status-seamonkey2.27:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.27
|
||
Comment 10•10 years ago
|
||
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.
Description
•