337780 - Support JS for adding search engines that matches IE's API (provide a way to install search engines from web script)

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Pam Greene]

According to http://www.microsoft.com/windows/ie/searchguide/default_new.mspx# , IE7 will support 

"window.external.AddSearchProvider(‘URL’) where ‘URL’ points to an OpenSearch description document" 

to add an engine to the browser.  Although there doesn't seem to be a standard API for such things, it'd be nice if we could make use of the same mechanism, in addition to our window.sidebar.addSearchEngine().

Comment 1

[Pam Greene]

Would it be appropriate to register nsSidebar as "external" as well as "sidebar" to allow this syntax, or would it be better to create a new class with the sole function (for now) of passing window.external.AddSearchProvider() to the search service?

Comment 2

[Pam Greene]

Attachment: Adds feature by adding new interface to nsSidebar.js

Adding a new interface to the existing sidebar object as suggested by beng -- hope this is what he meant.  I'll get him to review the patch after you've had a look at it.

This is a branch patch.  The trunk patch needs to be slightly different due to differences in surrounding portions of nsSidebar.js.

The confirmSearchEngine and validateSearchEngine methods are not new, they're refactored out of addSearchEngine.  The dialog that confirmSearchEngine shows is pretty horrible, but fixing that is a matter for bug 336452.

Comment 3

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

This patch needs to be updated for the changes made in bug 338989.

I'm also unsure about the interface name... nsIExternal isn't very descriptive, but I don't really know what else I'd call it. At the very least it should be put into it's own file and not in nsISidebar.idl. I wish there was a cleaner way to add content-accessible DOM methods without using nsSidebar :(

Comment 4

[Pam Greene]

Attachment: Merged with latest files

(In reply to comment #3)

> I'm also unsure about the interface name... nsIExternal isn't very descriptive,
> but I don't really know what else I'd call it. At the very least it should be
> put into it's own file and not in nsISidebar.idl.

I talked with Ben a bit and we decided on nsISidebarExternal, to make the connection clearer, and to leave it in the same file.  Moving the search-engine and microsummary functions and other assorted hodgepodge out of sidebar and into some other file would be nice someday.

Comment 5

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

Comment on attachment 225067 [details] [diff] [review]
Merged with latest files

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

>+  var iconURL = "";
>+  if (browser.shouldLoadFavIcon(selectedBrowser.currentURI))
>+    iconURL = selectedBrowser.currentURI.prePath + "/favicon.ico";
>+  
>+  if (!this.validateSearchEngine(aDescriptionURL, "xml", iconURL))
>+    return;

Won't validateSearchEngine always fail here, since favicon.ico won't match it's gif|jpg|jpeg|png regexp? Probably want to just add .ico to the regexp.

>+  if (!this.confirmSearchEngine(aDescriptionURL, ""))
>+    return;

Showing a blank space for the "name" needs to be fixed somehow... maybe just use a generic "Search Engine" or something like that, for now? I think you had mentioned changing things around so that we can load the engine file and then prompt, is there a bug on that already?

>+  const typeText = Components.interfaces.nsISearchEngine.DATA_XML;

this isn't typeText, it's typeXML :)

>+    catman.addCategoryEntry(JAVASCRIPT_GLOBAL_PROPERTY_CATEGORY,
>+                            "external",
>+                            SIDEBAR_CONTRACTID,
>+                            true,
>+                            true);

Does this mean that window.external.addSearchEngine (and the other implemented nsISidebar methods) will now be on both window.external and window.sidebar? I don't see how it associates the interface with the global property, but I don't know very much about how this works.

I think that exposing such methods to content (and modifying nsISidebar.idl) should be approved by a DOM peer. The patch as a whole also needs review from a browser peer too, of course :).

>+[scriptable, uuid(EDD70275-C131-4558-8F6C-9C4716B554A6)]

nit: keep this lowercase

Comment 6

[Pam Greene]

Attachment: Patch addressing Gavin's comments

(In reply to comment #5)

> Probably want to just add .ico to the regexp.

Er, right.  Good catch, thanks.

> >+  if (!this.confirmSearchEngine(aDescriptionURL, ""))
> >+    return;
> 
> Showing a blank space for the "name" needs to be fixed somehow... maybe just
> use a generic "Search Engine" or something like that, for now?

The dialog as it shows up here is very much a temporary solution.  One step better, without the ugly empty Name:, is in bug 336452.  Two further improvements, including loading the engine file so we have a good title right away, are listed in bug 336452 comment #6 but may be split into separate bugs for tracking and timing.

> >+  const typeText = Components.interfaces.nsISearchEngine.DATA_XML;
> 
> this isn't typeText, it's typeXML :)

Renamed.

> Does this mean that window.external.addSearchEngine (and the other implemented
> nsISidebar methods) will now be on both window.external and window.sidebar?

Yes, I tried it out and indeed window.sidebar and window.external will be aliases of each other.  That's perhaps a bit unattractive from a clutter point of view, but since we don't have any window.external methods yet (as far as I know), I don't think it's harmful.  I'll get other reviews too.

> >+[scriptable, uuid(EDD70275-C131-4558-8F6C-9C4716B554A6)]
> 
> nit: keep this lowercase

Sure, I certainly don't mind either way.  But there are both formats all over the codebase -- the very file I'm adding to already has one uuid in upper-case and one in lower.  (I've changed them all to lower in this patch.) If there's a guideline, people are really lax about following it.

Comment 7

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

Comment on attachment 225634 [details] [diff] [review]
Patch addressing Gavin's comments

>+  // Build the favicon URL for the current page, or our best guess at
>+  // the current page since we don't have easy access to the active document.
>+  var WINMEDSVC = Components.classes['@mozilla.org/appshell/window-mediator;1']
>+                            .getService(Components.interfaces.nsIWindowMediator);
>+  var win = WINMEDSVC.getMostRecentWindow("navigator:browser");
>+  
>+  var browser = win.document.getElementById("content");
>+  var selectedBrowser = browser.selectedBrowser;
>+  var iconURL = "";
>+  if (browser.shouldLoadFavIcon(selectedBrowser.currentURI))
>+    iconURL = selectedBrowser.currentURI.prePath + "/favicon.ico";

I think you can replace this with just |iconURL = win.gProxyFavIcon.getAttribute("src")| (getting the src of the icon in chrome directly). This will also allow it to work with pages that use <link rel="icon"> to specify a favicon.

Comment 8

[Pam Greene]

Attachment: Patch addressing Gavin's comments

Thanks, Gavin, that's better.

Comment 9

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

Comment on attachment 225770 [details] [diff] [review]
Patch addressing Gavin's comments

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

>+nsSidebar.prototype.AddSearchProvider =
>+function (aDescriptionURL)
>+{
...
>+  iconURL = win.gProxyFavIcon.getAttribute("src");

Looks like this is undeclared now.

Comment 10

[Pam Greene]

Thanks, I'll fix it along with whatever other comments the next reviewer has.

Comment 11

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

Comment on attachment 225770 [details] [diff] [review]
Patch addressing Gavin's comments

r=[e-mail]

Comment 12

[Pam Greene]

Attachment: Patch as checked in on 1.8 branch

Comment 13

[Pam Greene]

Attachment: Patch as checked in on trunk

Comment 14

[Pam Greene]

fixed-1.8-branch, fixed-on-trunk

Comment 15

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

This didn't get approval from a DOM peer, from what I can tell, which is generally required before adding/changing interfaces in dom/public. I think it would be a good idea to do that, even if it's an ex-post-facto review...

Comment 16

[Pam Greene]

Sorry, I was under the impression that a review from a super-reviewer would cover issues related to integration, interfaces, and the like.  But perhaps that's not the kind of review Ben did.  Anyway, I've no objection to more eyes on the code.

Comment 17

[Pam Greene]

Comment on attachment 225937 [details] [diff] [review]
Patch as checked in on 1.8 branch

Ex-post-facto review request, with apologies.

Comment 18

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

I didn't mean to imply that the patch wasn't sufficiently reviewed for code correctness, I'm just saying that non-trivial changes should normally be approved by a peer or owner from the affected module, and neither Ben nor I are DOM peers :). So more of an "approval" than "review".

Comment 19

[Asaf Romano (gone)]

Er, why did you rev the uuid of nsISidebar (esp. on 1.8)?

Comment 20

[Pam Greene]

(In reply to comment #19)
> Er, why did you rev the uuid of nsISidebar (esp. on 1.8)?

It wasn't changed, just lower-cased for consistency, following comment #5.

Comment 21

[Asaf Romano (gone)]

Apology for the false alarm.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 225937 [details] [diff] [review]
Patch as checked in on 1.8 branch

I'm not really the right person to review this, especially if you want a real peer review

Comment 23

[Pam Greene]

(In reply to comment #22)
> I'm not really the right person to review this

Sorry for the mixup; it's had browser review, and you're listed as a DOM peer on http://www.mozilla.org/owners.html .  Who would you recommend for DOM instead?

Comment 24

[Paul Millar]

I've found two problems with this code:

1. When you click on a window.external.AddSearchProvider() link and it brings up the 'Add Search Engine' dialogue the 'Name:' is not filled in.

2. Using window.external.AddSearchProvider() only works with a file extension of .xml and not dynamically generated XML files (i.e. file extension .aspx) - shouldn't it be detecting the file format rather than just the extension? Using the auto detect from the search bar directly works with both sets of URLs.

Go to http://www.edazzle.net/test/test4.aspx to try this out.


Mozilla/5.0 (Windows; N; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060621 BonEcho/2.0a3

Comment 25

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

(In reply to comment #24)
> 1. When you click on a window.external.AddSearchProvider() link and it brings
> up the 'Add Search Engine' dialogue the 'Name:' is not filled in.

That's known, see the first part of comment 6.

> 2. Using window.external.AddSearchProvider() only works with a file extension
> of .xml and not dynamically generated XML files (i.e. file extension .aspx) -
> shouldn't it be detecting the file format rather than just the extension?

I suspect these checks were added here because the original addSearchEngine method checked the extension. I suppose it did that because it could not deal with invalid plugins - they'd be downloaded to disk and then would fail to load and stick around forever - so checking the extension was one way to protect against that. The new search service doesn't have this problem (engines that can't be parsed aren't saved to disk, and proper user notification is given when the load fails) so I can't think of any reason for this URL suffix check to continue to exist, now that you mention it. I think we can get rid of the suffix checking in validateSearchEngine.