341665 - Extend window.sidebar.addSearchEngine() to accept OpenSearch files

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P2)

Description

[Pam Greene]

Currently, addSearchEngine() requires a Sherlock file, but there's no technical reason it couldn't accept an XML OpenSearch description too.  The bulk of what's needed to allow that is to expand the regexp in the validateSearchEngine() function in nsSidebar.js (to be added in bug 337780).

Comment 1

[Mike Connor [:mconnor]]

Not a blocker, IMO, but a nice-to-have.

Comment 2

[Pam Greene]

Attachment: Fixes issue

Comment 3

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

Comment on attachment 226179 [details] [diff] [review]
Fixes issue

>Index: components/sidebar/src/nsSidebar.js

>+// File extensions for search plugin description files; must be the same length.
>+const XML_FILE_EXT      = "xml";
>+const SHERLOCK_FILE_EXT = "src";

I think validateSearchEngine should take an array of regexps instead of a string, and do the suffix and prefix checks separately. That way these constants can be "/\.xml$/" and "/\.src$/", and addSearchEngine can pass [XML_EXT, SRC_EXT] to validateSearchEngine. You can then use these regexp constants to find the type in addSearchEngine (if (XML_EXT.test(url)) { type = XML } etc).

Comment 4

[Pam Greene]

(In reply to comment #3)
> I think validateSearchEngine should take an array of regexps instead of a
> string, and do the suffix and prefix checks separately.

Hmm.  That'd be a fine way to do it, but I'm not convinced it's better.  It does avoid some string munging, but regexps aren't exactly lightweight, and passing an array of them strikes me as rather more obscure.  What advantages do you see?

Comment 5

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

(In reply to comment #4)
> Hmm.  That'd be a fine way to do it, but I'm not convinced it's better.  It
> does avoid some string munging, but regexps aren't exactly lightweight, and
> passing an array of them strikes me as rather more obscure.  What advantages
> do you see?

The advantage I see is simpler code, with no string munging. This is what regular expressions are meant to do :)

I don't understand what you mean when you say that they aren't lightweight. Passing an array of regexp objects seems cleaner to me than passing a concatenation of strings to be later converted into a regexp object.

Comment 6

[Pam Greene]

(In reply to comment #5)
> I don't understand what you mean when you say that they aren't lightweight.

I don't know the JavaScript implementation specifically, but in general, regular expressions are terribly processor-intensive, far more so than other types of string manipulation.  I couldn't say whether changing validateSearchEngine to do four small regexp tests rather than two large ones would be a net win, but using regexps instead of .substring in addSearchEngine() would almost certainly be slower.

From a code maintenance standpoint, it's debatable, but regexps are arguably harder to understand and maintain than concatenation: "heavier weight" in the sense of requiring more effort on the part of whoever comes along next to understand and modify what's going on.  That's obviously not a major consideration when regexps are a clear win, but in cases where other methods might well be just as good technically, I would argue that maintainability is important too.

Comment 7

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

(In reply to comment #6)
> I don't know the JavaScript implementation specifically, but in general,
> regular expressions are terribly processor-intensive, far more so than other
> types of string manipulation.  I couldn't say whether changing
> validateSearchEngine to do four small regexp tests rather than two large ones
> would be a net win, but using regexps instead of .substring in
> addSearchEngine() would almost certainly be slower.

Slower by how much? This is high level code - I very much doubt that there would be a perceptible performance difference between my suggested code and yours.

> From a code maintenance standpoint, it's debatable, but regexps are arguably
> harder to understand and maintain than concatenation: "heavier weight" in the
> sense of requiring more effort on the part of whoever comes along next to
> understand and modify what's going on.  That's obviously not a major
> consideration when regexps are a clear win, but in cases where other methods
> might well be just as good technically, I would argue that maintainability is
> important too.

I don't buy this argument. Are you saying that:
const SRC_EXT = /\.src$/;
SRC_EXT.test(url);
is harder to understand than:
const XML_EXT = ".xml"
var start = url.length - XML_EXT.length;
engineURL.substring(startLoc) == XML_EXT; // ?

Or that:
XML_EXT.test(url) || SRC_EXT.test(url);
is harder to understand than:
var e = new RegExp("(" + SRC_EXT + "|" + XML_EXT + ")");
e.test(url); // ?

And what if we wanted to, at some point in the future, support installing .plugin files, for example? Your code would need to be adjusted, since the extensions would then not all be of the same length. Not a huge deal, I admit, and not really that likely to happen, but if we can make the code easily extensible now, why not do it?

Comment 8

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

Bug 337780 comment 24 raises a good point: why are we even checking the end of the URL (file extension)? Is there any reason we can't just get rid of those checks?

Comment 9

[Pam Greene]

I can't think of any either.  And removing the check is the most efficient algorithm of all. ;)

Comment 10

[Pam Greene]

Attachment: Removing suffix check for description file

The suffix is still needed to determine what kind of file we're receiving -- autodetection based on the file contents would be nice, but is beyond the scope of this bug.  However, I expect that OpenSearch files will be far more common, and have less consistent suffixes, than Sherlock files.  So this patch assumes that ".src" is a Sherlock (text) file, and anything else is OpenSearch (XML).

Comment 11

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

Comment on attachment 226658 [details] [diff] [review]
Removing suffix check for description file

>+// File extension for Sherlock search plugin description files
>+const SHERLOCK_FILE_EXT_REGEXP = /\.src$/;

Probably want to make that /\.src$/i (ignore case).

>+    // make sure using HTTP, HTTPS, or FTP
>+    if (! /^(https?|ftp):\/\//i.test(engineURL))

Do you mind adding a "we're" or something to that comment, since you're changing it anyways?

Comment 12

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

Comment on attachment 226658 [details] [diff] [review]
Removing suffix check for description file

>+  if (SHERLOCK_FILE_EXT_REGEXP.test(engineURL))
>+    dataType = Components.interfaces.nsISearchEngine.DATA_TEXT;
>+  else
>+    dataType = Components.interfaces.nsISearchEngine.DATA_XML;

Oh, also, I think it'd be a good idea to add a comment here explaining what you explained in comment 10 (about assuming anything not .src is OpenSearch).

Comment 13

[Pam Greene]

(In reply to comment #11 and comment #12)

Sure, I'll add i and edit both of those comments before landing, although I won't bother making a new patch for second review.

Comment 14

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

Comment on attachment 226658 [details] [diff] [review]
Removing suffix check for description file

sr=[e-mail]

Comment 15

[Pam Greene]

Checked into trunk to bake before requesting 1.8 approval.

Comment 16

[Darin Fisher]

Comment on attachment 226658 [details] [diff] [review]
Removing suffix check for description file

a=darin on behalf of drivers (please land this on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to this bug)

Comment 17

[Pam Greene]

fixed-1.8-branch, fixed-on-trunk