317107 - New search service

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[O. Atsushi (Torisugari)]

It seems nobody is working on search engine backend extensively,
so I'd like to suggest new interfaces for search component.

Thoughts in my mind are
1. Do not lose any features working in the current version of Firefox.
2. Make it possilbe to uninstall search engine (if ui is ready).
3. Make it possible to implement POST method search.
4. Make it easy to apply a new search engine format.
5. Clean up the relationship between the search component and search.xml.
   Some should be implemeted at backend and others at frontend.
6. Make it easy to hack / to use in a extension.
7. And move the component from xpfe/components to browser/search.

Comment 1

[O. Atsushi (Torisugari)]

Attachment: Interface proposal (idl file) v1

Comment 2

[Benjamin Smedberg]

Torisugari, please discuss search refinements with mconnor before proceeding, we may want to consider replacing the .src format.

Comment 3

[O. Atsushi (Torisugari)]

(In reply to comment #2)
> Torisugari, please discuss search refinements with mconnor before proceeding,
> we may want to consider replacing the .src format.

I didn't mean to maintain mycroft engine either. Anyway, I'll ask him to share
the plan. Thanks.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

- why do some uris use AString/AUTF8String while others use nsIURI? Why don't all of them use nsIURI?
- moz is an unusual prefix, ns is much more common
- this interfaces needs docs.

Comment 5

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

Why have both createSearchQuery and createSearchParams? Perhaps it would be useful to have an mozISearchEngineResult or something to hold post data, URI, referrer, search terms, etc?

I also don't think I agree with your XXX comment about storing the engine by baseURI, since those tend to change while the name remains constant.

Comment 6

[O. Atsushi (Torisugari)]

(In reply to comment #4)
> - why do some uris use AString/AUTF8String while others use nsIURI?
>   Why don't all of them use nsIURI?

To make it easy to write js code. For xul UI, all info are stored in
RDF data source and its resource. Sometimes they're id properties of
template child nodes. So string is better than nsIURI at least as a
setter method. 

- e.g. ------------------------------------------------------------

var searchEngine =
  searchEngineManagerObject.parseEngine(myRDFResourceObject.Value);

or

searchEngineManagerObject.RemoveSearchEngine(myXULElementObject.id);

etc.
-------------------------------------------------------------------
But I agree it's not difficult to convert it to nIURI via IO service...

> - moz is an unusual prefix, ns is much more common

Hmm. In my tree, both this component and old nsInternetSearch
co-exists, without any dependency. Then I don't mind other name,
say, nsISearchService2 or something.


(In reply to comment #5)
> Why have both createSearchQuery and createSearchParams?
Um, there's no reason.

> Perhaps it would be useful to have an mozISearchEngineResult
> or something to hold post data, URI, referrer, search terms, etc?
Maybe. If you are talking about js, such a code should work.

---------- Instead of LoadSearchURL() in browser.js ------------ 
function LoadSearchResult(aEngine, aSearchTerms, aInNewTab) {
  if (!aEngine)
    return;

  var url = new Object();
  var referrer = new Object();
  var postData = new Object();

  if (aSearchTerms) {
    aEngine.createSearchParams(aSearchTerms, url, referrer, postData);
  }
  else {
    url.value = aEngine.referrer? aEngine.referrer:aEngine.baseURI;
    referrer.value = null;
    postData.value = null;
  }

  if (gBrowser.localName == "tabbrowser" && aInNewTab) {
    content.focus();
    // open link in new tab
    var t = gBrowser.addTab(url.value, 0, referrer, postData);
    gBrowser.selectedTab = t;
    if (gURLBar)
      gURLBar.value = url.value;
  }
  else
    loadURI(url.value, referrer.value, postData.value);
  content.focus();
}
--------------------------------------------------------------
I confirmed POST method search works with above function.
For the test, I used the "post" plugin at
http://www.iseli.org/mozilla/searchplugins.mv


> I also don't think I agree with your XXX comment about storing the engine by
> baseURI, since those tend to change while the name remains constant.

You are right. Name remains constant. Please ignore XXX comment.

> the referer field is an addition you are making, correct?

Yes and NO. mycroft (and probably sherlock) has |searchForm| field. Now
I'm not too sure whether we should attach referrer to the HTTP request,
but at least, I'd like to use it as "default URL" for blank input. As a
host might run several search services.

http://mycroft.mozdev.org/deepdocs/searchtag.html#searchForm

Comment 7

[Mycroft Project (CC)]

(In reply to comment #3)
> (In reply to comment #2)
> > Torisugari, please discuss search refinements with mconnor before proceeding,
> > we may want to consider replacing the .src format.
> 
> I didn't mean to maintain mycroft engine either. Anyway, I'll ask him to share
> the plan. Thanks.
> 
If someone is talking about adopting mycroft.mozdev.org... then I should like to know please (as the project owner of m.m.o)

Comment 8

[Jesse Ruderman]

See also bug 308674, "Monitor and implement the OpenSearch 1.1 standard".

Comment 9

[O. Atsushi (Torisugari)]

Attachment: Interface proposal (idl file) v2

with some more description for discussion. Maybe we don't need such
a lot of attributes...?

I've not yet got contact to mconnor since then, so I'm not too sure which
direction to go.

Comment 10

[O. Atsushi (Torisugari)]

Attachment: Interface proposal (idl file) v2.1

Oops v2 was another file. Sorry for bug spam.

Comment 11

[O. Atsushi (Torisugari)]

Attachment: Patch version (equivalent to v2.1)

PoC Patch. Though browser.js and search.xml are still rough.

- nsMycroftUtils.cpp is totally a blackbox to me.

BTW, I don't have cvs write access, so cvs diff -N did not work for me.
I manually wrote diff headers...

Comment 12

[Benjamin Smedberg]

You can use cvsutils to "cvsdo add" a file even if you don't have cvs write access.

Comment 13

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

Comment on attachment 204040 [details]
Interface proposal (idl file) v2.1

>interface nsISearchEngineResult : nsISupports {
>  // These params are used by nsIWebNavigation::LoadURI(...).
>  readonly attribute AString uri;
>  readonly attribute nsIURI referrer;
>  readonly attribute nsIInputStream postData;
>};

Maybe it's a good idea to expose the terms used to produce the result? And the engine used to produce it? What do you think? It would go unused with the current search system, but I could imagine UI to have "saved searches" which could essentially just deal with nsISearchEngineResult objects.

>interface nsISearchEngine : nsISupports {

>  /*
>   * Label for UI. This param is used as a kind of identifier.
>   * See also nsISearchEngineManager::findEngineByName(...)
>   */
>  readonly attribute AString name;

I kind of worry about name being used as the identifier. What happens when someone tries to install two different engines with the same name? Maybe engines should have a GUID? I think Benjamin was planning to package engines in an extension-like bundle, I don't know if anything like that was fleshed out.

>  /*
>   * This param is equivalent to "searchForm" property in sherlock.
>   * When user's input were empty, client should load this URI.
>   */
>  readonly attribute AUTF8String referrer;

Ah, I understand this attribute now. Referrer is a misleading name, I confused it with HTTP's referer. What's wrong with just using "searchForm"?

>interface nsISearchEngineManager : nsISupports {

>  nsIURI findEngineByName(in AString aName);
>  void removeSearchEngine(in AUTF8String aEngineFileSpec);

I think findEngine should return an nsISearchEngine, and removeEngine should take either a name (or ID) or an nsISearchEngine, maybe have methods for both.

>  void addSearchEngine(in AUTF8String aEngineURL, in AUTF8String aIconURL,
>                       in AString aTitle, in AString aCategory);

This method seems pretty specific to the Sherlock format... I don't know that nsISideBar's addSearchEngine should be moved to this interface. Can't it nsISideBar map the current addSearchEngine to a more generic method on nsISearchEngineManager?

It might be a good idea to hold back on implmenting this until the interface is finalized.

Comment 14

[O. Atsushi (Torisugari)]

(In reply to comment #13)
> I kind of worry about name being used as the identifier. What happens when
> someone tries to install two different engines with the same name? Maybe
> engines should have a GUID? I think Benjamin was planning to package
> engines in an extension-like bundle, I don't know if anything like
> that was fleshed out.

The real identifier is the file URI, so you can install as many search
plugins as you have, if some of names are same. But sometimes we use
name-based system now, then they can be a problem, and indeed some bugs
are already filed (google.com v.s. google.co.uk etc.)

The solution would be,
1. last selected item could use simply file URI
2. default search plugin could use resource:// scheme.

e.g.
pref("browser.search.selected",
     "file:///c:/mozilla/firefox/dist/bin/searchplugins/answers.src");
pref("browser.search.default",
     "resource://searchplugins/google.src");l

But if we want to install same *file name* plugins, we do need GUID, and
probably it'd be time to land the new plugin format.

Comment 15

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

We are planning significant changes to the way search works for Fx2.0. I wouldn't develop this patch much further until we publish requirements in the next few weeks on the dev wiki. Search is a delicate area and there are many considerations that need to be made. 

Comment 16

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

I'm working on a new search service based on Ben's extension, implemented in JavaScript with an API very similar to the one attached here. See http://wiki.mozilla.org/Search_Service for details.

Comment 17

[Peter Lairo]

Depends on bug 232272 ?

Comment 18

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

(In reply to comment #17)
> Depends on bug 232272 ?

No, it doesn't...

Comment 19

[Mycroft Project (CC)]

(Speaking as the Mycroft Project Owner @ Mozdev...)

Is it possible to get details of what is planned / in process beyond what is available on the wiki? (Or at least beyond what I understand from the wiki)

Specifically, is my reading correct that the search plugins (or whatever you want to call them) will still be stored as a source file and an icon file - say google.xml and google.png? If so, how does
void addEngine(in AString engineURI);
manage to use just one parameter - is the icon daisychained from the xml (I've read the opensearch stuff as well but it seems that you could do one thing or the other and I don't quite get how you choose if you see what I mean)

And could anyone explain the seemless migration mentioned (in passing I have no idea how this will cope with the plugins that aren't exactly written to sherlock but that's not my concern...) is the plan that addEngine and addSearchEngine both work to start with and then addSearchEngine is withdrawn? What is addEngine going to do if you pass it a .src plugin. Will updates function in a similar way or entirely differently...

Is MoCo/MoFo planning to roll its own repository for plugins?

After being caught napping somewhat with the 1.5 release I'd like to be a bit better informed in future...

And briefly on the defensive... 700 000 plugins were downloaded from Mycroft in January (raw numbers, some users may have clicked twice etc...) and there were 1.4m page views.

And basically, assuming you're generally supportive of Mycroft, I'd like to know what's going to happen!

And finally, do I assume correctly that nsInternetSearchService.cpp is now essentially frozen?

Comment 20

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

(In reply to comment #19)
> Specifically, is my reading correct that the search plugins (or whatever you
> want to call them) will still be stored as a source file and an icon file - say
> google.xml and google.png?

Yes. OpenSearch also allows the XML files to specify the icon to use, and this will be supported as well. Icons specified in the XML will be downloaded and stored next to the plugin file as before.

> If so, how does void addEngine(in AString engineURI);
> manage to use just one parameter - is the icon daisychained from the xml (I've
> read the opensearch stuff as well but it seems that you could do one thing or
> the other and I don't quite get how you choose if you see what I mean)

I've yet to determine exactly what add* methods should be exposed. The existing window.sidebar.addSearchEngine() API will be preserved and will remain unchanged, although the third and fourth arguments will likely be unused. It might be extended to support the addition of XML-source engines, but I think I'm leaning towards a new method for those (especially given IE's introduction of a new method for adding engines). Again, the proposed API is not final. I'd very much appreciate any input on this matter. I'll be updating the API wiki page in the next few days to reflect my thoughts on the matter.

> And could anyone explain the seemless migration mentioned (in passing I have no
> idea how this will cope with the plugins that aren't exactly written to
> sherlock but that's not my concern...) is the plan that addEngine and
> addSearchEngine both work to start with and then addSearchEngine is withdrawn?

Again, there are no plans to remove window.sidebar.addSearchEngine. It will remain implemented on nsSidebar and will forward to the new search service. The "seamless migration" I mentioned on the wiki is profile migration - changing existing plugins to the new XML format when upgrading.

> What is addEngine going to do if you pass it a .src plugin. Will updates
> function in a similar way or entirely differently...

nsISearchService will likely have a addEngine(file, iconuri, type) method where type is one of Sherlock/Firefox XML Format/OpenSearch. Updates will likely work just as they did before, though this is also an area in which I'd appreciate some feedback. What's wrong with the current update system? How could it be improved? Is it even needed?

> After being caught napping somewhat with the 1.5 release I'd like to be a bit
> better informed in future...
> 
> And briefly on the defensive... 700 000 plugins were downloaded from Mycroft in
> January (raw numbers, some users may have clicked twice etc...) and there were
> 1.4m page views.

Sherlock files will still be completely supported - there are no plans to drop that support.

> And finally, do I assume correctly that nsInternetSearchService.cpp is now
> essentially frozen?

The plan is to remove nsInternetSearchService from the Firefox build, so I'm not sure what you mean by this. It will be replaced with the new nsISearchService.

Comment 21

[Mycroft Project (CC)]

I'll reply properly later when I've digested what you've written (looks very useful, thanks) just on the last point... sorry, it wasn't clear - I just meant that I assume there aren't likely to be any further fixes to nsInternetSearchService beyond anything trivial (and I don't think there is anything in that category)?

Comment 22

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

(In reply to comment #21)
> I'll reply properly later when I've digested what you've written (looks very
> useful, thanks) just on the last point... sorry, it wasn't clear - I just meant
> that I assume there aren't likely to be any further fixes to
> nsInternetSearchService beyond anything trivial (and I don't think there is
> anything in that category)?

Well, it's still currently used for SeaMonkey (much more extensively than it is in Firefox, as I understand it), so there will likely still be interest from the SeaMonkey folk in mainting it and fixing bugs.

Comment 23

[Axel Hecht [:Pike]]

Regarding the icons, I am hoping to get rid of separate icon files,
they tend to be broken in l10n, in favor of data: urls in the XML.

Gavin, I read the opensearch stuff to be able to do that, would you want to 
support that? That may put additional stress on the converter, but not too
much I'd think.

Comment 24

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

(In reply to comment #23)
> Gavin, I read the opensearch stuff to be able to do that, would you want to 
> support that? That may put additional stress on the converter, but not too
> much I'd think.

I thought of that - it seemed like a cleaner solution, but I couldn't find an easy way to do it from a JS component, since there is no btoa() there, and didn't look into it further. You're right though, it would make things easier, so I've filed bug 326854 on exposing btoa() to JS components. If that bug isn't fixed for 1.8, I'll just fall back to using my own version of btoa. I've got the rest of the functionality working OK so it shouldn't be an issue.

Comment 25

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

Attachment: search backend (rv1)

This is the initial implementation of the new search service. This does not include any UI changes. Will post a little bit of documentation about the patch shortly.

Comment 26

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

Attachment: some information about the patch

Comment 27

[Mycroft Project (CC)]

I haven't caught up with this in terms of the attachments you've recently added but following on from comment #19 - #22...

I'm very much relieved that come the release of Firefox 2.0, the 6000 plugins at Mycroft won't become obsolete, I do have a few things to add that may be of some value.

Firstly on the icons, including them as 'data:xxx' would seem like a good idea but how well does this suit cross-browser use - Internet Explorer doesn't seem to cope with them in my experience (I realise this might not be a primary concern and that IE doesn't use icons at the moment but would search engines then need to host one version for IE and one for Fx? Anyway, poss. worth considering) Also on icons, if the icon is just referenced from the actual plugin, how is it then downloaded... would this mean an even greater lag between adding the engine and it being completely added?

On migration, I'll be interested to see how that's handled in the code you've added and whether it imports all of the possible data in a src file (in particular, I'm thinking about character set handling and whether the interpret tags will be imported to allow parsing by an extension...)

On updating, I think that it depends... either it should take care of itself which would seem to fit with the ethos of greater automation in Fx itself in which case there needs to be some provision for auto update... I think the current system is ok though it does have some issues withing changing icon type and I've seen some issues with case (in)sensitivity. The other alternative is to leave it up to the user and in this case it needs to be possible to addengine where there is already an extension with that filename - this is currently impossible - there's a bug somewhere, something about stopping malicious installs.
This is based on the attitude that search scripts and inputs WILL change - Google and so on may be able to keep a stable set of parameters but we get a fair few updates at Mycroft because the location of the search engine changed or some parameter name was changed or capability was added etc...
(Something that strikes me... any update should IMO allow update from a sherlock or opensearch link... and the check to see if it really is an update should be better than the current system which can install an update with an older date sometimes)

Finally for now... I was asking about nsInternetSearchService because I hadn't really realised (can't have been awake, the product is clearly Fx above) that this was a Fx change only... I presume this means that Seamonkey won't accept a Firefox search engine... from my point of view at Mycroft, this means that it would either be necessary to run two streams of plugins or just stick with Sherlock - would this disadvantage users at all or is the main advantage of moving to opensearch that the search engines only need the one file?

Actually one last question which I guess is the one really on my mind.
Should I convert from sherlock and why? What (dis)advantages are there for me and for users?

If there's anything that you'd particularly appreciate feedback on rather than rambling thoughts from the top of my head then do please let me know. And I'll take a look at the attachments and post later.

Comment 28

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

Attachment: search backend (rv1.1)

Added some documentation, tweaked the observer topics to be more searchable, and fixes a bug with the icon setting.

Comment 29

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

(In reply to comment #27)
> I'm very much relieved that come the release of Firefox 2.0, the 6000 plugins
> at Mycroft won't become obsolete

I'm glad you're relieved :). Is there any way I could get hold of the entire set of plugins? I'd love to be able to test my sherlock parsing code against as large a set as possible (as mentioned in my previous attachement, I've tested a fairly large set already, but more can't hurt).
 
> Firstly on the icons, including them as 'data:xxx' would seem like a good idea
> but how well does this suit cross-browser use - Internet Explorer doesn't seem
> to cope with them in my experience (I realise this might not be a primary
> concern and that IE doesn't use icons at the moment but would search engines
> then need to host one version for IE and one for Fx? Anyway, poss. worth
> considering)

The way icons are stored really is just a Firefox specific implementation detail - Firefox will support references to icons in the search plugin description file itself, as described in the OpenSearch specification (as an aside, thanks for making me look into this: I found a bug with my initial patch :). The initial implementation I've attached downloads the icons and saves them in the file as data: URIs.

> Also on icons, if the icon is just referenced from the actual
> plugin, how is it then downloaded... would this mean an even greater lag
> between adding the engine and it being completely added?

There would be a slight delay, in that case, since the request for the icon needs to happen once the request for the plugin description file is complete, but I don't think it's really an issue. The old way of specifying an icon to use (as a parameter to the addSearchEngine call) will still be supported, for all supported plugin types.

> On migration, I'll be interested to see how that's handled in the code you've
> added and whether it imports all of the possible data in a src file (in
> particular, I'm thinking about character set handling

I'd love feedback on the way the attached implementation handles the sherlock file character set information. It currently first looks for the queryCharset parameter on the search element, falling back to the numeric queryEncoding parameter if queryCharset is not specified (see getCharSetFromCode and it's caller in the above patch). That parameter is used to determine which encoding to use to send the user entered search terms. This matches the previous nsInternetSearchService behavior, as far as I can tell. My goal was backwards compatibility, but if you see room for improvement then I'm all ears.

> and whether the interpret tags will be imported to allow parsing by an
> extension...)

I've not included any support for parsing and gathering data from the interpret tags. Firefox itself did not make use of this data, thus it didn't seem worth implementing. I think the attached patch provides a clean and stable API to extensions that want to provide search enhancements, but I'm very open to suggestions on how it could be better in that regard. Maybe this could be filed as an enhancement request?

> On updating, I think that it depends... either it should take care of itself
> which would seem to fit with the ethos of greater automation in Fx itself in
> which case there needs to be some provision for auto update... I think the
> current system is ok though it does have some issues withing changing icon type
> and I've seen some issues with case (in)sensitivity. The other alternative is
> to leave it up to the user and in this case it needs to be possible to
> addengine where there is already an extension with that filename - this is
> currently impossible - there's a bug somewhere, something about stopping
> malicious installs.

I don't think search plugin updating is something that should be exposed to the user in any significant way - the current update system has it's flaws, as you mention, but I don't think "lack of user interaction" is one of them. Engine updates will likely be used mainly in the case where sites change form parameters or URLs, so as long as their searches continue to work as they should, I'm pretty sure most users will not want to to know about it.

One issue that came up while planning out the search service's behavior was how to handle duplicate engines (engines with the same name). The current patch just ignores new engines if they already exist - I think the error handling in this case could be improved, though I'm not entirely sure how yet. That is something I will be giving more thought to in the next little while. I've filed bug 327932 about this.

> This is based on the attitude that search scripts and inputs WILL change -
> Google and so on may be able to keep a stable set of parameters but we get a
> fair few updates at Mycroft because the location of the search engine changed
> or some parameter name was changed or capability was added etc...
> (Something that strikes me... any update should IMO allow update from a
> sherlock or opensearch link... and the check to see if it really is an update
> should be better than the current system which can install an update with an
> older date sometimes)

This was an issue that I planned on dealing with specifically - bug 269658 is one case, where the plugin updating system "updated" the engine to a "network authentication" page. The attached patch makes it easy to ensure that the new plugin is valid before overwriting the existing one, so I plan on addressing that bug when I implement the plugin updating system.

> Finally for now... I was asking about nsInternetSearchService because I hadn't
> really realised (can't have been awake, the product is clearly Fx above) that
> this was a Fx change only... I presume this means that Seamonkey won't accept a
> Firefox search engine... from my point of view at Mycroft, this means that it
> would either be necessary to run two streams of plugins or just stick with
> Sherlock - would this disadvantage users at all or is the main advantage of
> moving to opensearch that the search engines only need the one file?

The main advantages of using an XML format are that it's easy to parse. It also means that Firefox can support a larger set of search plugins. I don't see how continuing to provide search plugins in the Sherlock format would disadvantage users - at least for now, Sherlock plugins can, with a few minor exceptions (being able to specify an alias, or keyword, for the engine is the only one I can think of), provide the same amount of functionality to Firefox users as OpenSearch plugins do.

> Actually one last question which I guess is the one really on my mind.
> Should I convert from sherlock and why? What (dis)advantages are there for me
> and for users?

The only thing that's really changing is how Firefox stores the engines internally, and the addition of formats which can be used to add search engines to the Firefox search bar and other search features. There is no "converting" as far as end users are concerned, nor do I think this means Mycroft should be "converting" to anything.

> If there's anything that you'd particularly appreciate feedback on rather than
> rambling thoughts from the top of my head then do please let me know. And I'll
> take a look at the attachments and post later.

I appreciate the feedback Charles. If you have a chance to take a look at the patch or maybe even test it out that'd be great. The more eyes the better!

Comment 30

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

Attachment: search backend (rv1.2)

-Fixed bug with extension-shipped sherlock plugins (the icons wouldn't display)
-Fixed bug to ensure that non-profile plugins aren't modified (_readOnly property)
-Fixed bug with addEngineWithDetails which caused engines created that way to have two files

Comment 31

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

Attachment: extension used to test extension-shipped plugins

tests sherlock files with/without icons, and opensearch files with data: and http: icon URLs

Comment 32

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

Comment on attachment 212829 [details] [diff] [review]
search backend (rv1.2)

>Index: browser/base/content/browser.js

>+function OpenSearch(aSearchText, aNewTab) {
>+  var currentEngine =
>+                 Components.classes["@mozilla.org/browser/search-service;1"]
>+                           .getService(Components.interfaces.nsISearchService)
>+                           .currentEngine;

We really should start using Cc/Ci/Cr for all new code - to save our wrists if nothing else!

>Index: browser/components/search/jar.mn
>===================================================================
>RCS file: browser/components/search/jar.mn
>diff -N browser/components/search/jar.mn
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ browser/components/search/jar.mn	23 Feb 2006 01:21:13 -0000
>@@ -0,0 +1,6 @@
>+browser.jar:
>+*       content/browser/search/search.xml                           (content/search.xml)
>+        content/browser/search/searchbarBindings.css                (content/searchbarBindings.css)
>+*       content/browser/search/searchDialog.js                      (content/searchDialog.js)
>+*       content/browser/search/searchDialog.xul                     (content/searchDialog.xul)
>+*       content/browser/search/searchDialog.css                     (content/searchDialog.css)

So, let's not add this search dialog back. It adds a whole lot of additional complexity most users will never encounter. In addition, the dialog is buggy. To cover the condition where the user invokes the search command and there is no search field in the UI, there are two choices that I can think of off the top of my head:

- disable the command
- find the URL of the default search engine and go to it in the current tab.

>Index: browser/components/search/nsISearchService.idl

>+  /**
>+   * Adds a parameter to the search engine's submission data.
>+   * @param name
>+   *        The parameter's name.
>+   * @param value
>+   *        The value to pass. If value is "{searchTerms}", it will be
>+   *        substituted with the user-entered data when retrieving the
>+   *        submission. XXX
>+   *
>+   * @throws NS_ERROR_FAILURE if the search engine is read-only.
>+   */
>+  void addParam(in AString name, in AString value);

Does this mean that an engine object (a singleton object held by the singleton search service) has a temporary buffer that accumulates search params used when generating a submission with getSubmission? This seems awkward. Can we do either:

- make getSubmission take an ary parameter
- make the nsISearchSubmission object have this method instead?

>+  /**
>+   * Adds a new search engine from the file at the supplied URI.
>+   *  @param engineURL the URL to the search engine's description file.

nit: I like the formatting better like this:

@param  engineURL
        the URL to the search engine's

... it's more deterministic. Sorry if I set a bad example in my original prototype, I may have learned this from Darin since then ;-) 

>Index: browser/components/search/nsSearchService.js
>+function LOG(aText) {
>+  var prefB = Cc["@mozilla.org/preferences-service;1"].
>+              getService(Ci.nsIPrefBranch);
>+  var shouldLog = false;
>+  try {
>+    shouldLog = prefB.getBoolPref(BROWSER_SEARCH_PREF + "log");
>+  } catch (ex) {}
>+
>+  if (shouldLog) {
>+    var consoleService = Cc['@mozilla.org/consoleservice;1'].
>+                         getService(Ci.nsIConsoleService);
>+    consoleService.logStringMessage(aText);
>+  }
>+}

nit: ' vs " quote style. 

If search logging is enabled you should also log to stdout. 

>+
>+//XXX Bug 326854: no btoa for components, use our own
>+function b64(aBytes) {

nit: no "a" prefixes for arguments anymore. That goes for all instances across this file so I won't mention them anymore. If you have to disambiguate, use a more descriptive variable name. It's better practice. 

>+function checkNameSpace(aElement, aLocalNameArray, aNameSpaceArray) {

What does this function do? What is it used for? What are the input conditions? (it looks like it won't fail if aElement is null - is that OK, or will that lead to problems elsewhere) - document please!

>+function getDir(aKey) {
>+  if (!aKey)
>+    throw Cr.NS_ERROR_INVALID_ARG;

let's start using ASSERT... grab the code from places. It'll move into a common location soon. Replace with:

ASSERT(key, "getDir requires a special directory key");
if (!key)
  throw Cr.NS_ERROR_INVALID_ARG;

or some such. This allows us to consolidate our error handling and reporting. 

It might be nice to also have an ENSURE()[1] function similar to NS_ENSURE_* macros:

ENSURE_ARG(key, "getDir requires a special directory key");

>+// XXX This isn't a full list - this is just copied over from
>+// nsInternetSearchService to maintain backwards compat with Firefox 1.0.x
>+var kCharsetCodes = [];

nit: k-prefix but not a const?

General comments: your functions all need more documentation!

>+/**
>+ * Removes all characters not in the "chars" string from aName.
>+ * @returns a sanitized name to be used as a filename, or a random name if a
>+ * sanitized name cannot be obtained (aName contains no valid characters).

nit: Formattting!

>+/**
>+ * Sets the .iconURI property of the given engine.
>+ *
>+ *  @param aIconURI a nsIURI pointing to the engine's icon. Must have a http[s]
>+ *         or data scheme. Icons with HTTP schemes will be downloaded and
>+ *         converted to data URIs for storage in the engine XML files.
>+ *  @param aEngine the engine to set the icon for.
>+ */
>+function setIcon(aIconURI, aEngine) {
>+  if (!(aIconURI instanceof Ci.nsIURI))
>+    return;

this looks like an assertion. 

Why is this function that is relevant to a particular engine global? Shouldn't this be a member of the engine... or at least the service?

>+function iconLoadListener(aChannel, aEngine) {
>+  this.mCountRead = 0;

nit: use _ to prefix members, not m.

>+        // The engine might not have a file yet, if it's being downloaded,
>+        // because onLoad may not have been called yet. If it hasn't, this
>+        // change will be written to file when it is.

What do you mean by "onLoad may not have been called yet" - where would "load" be called in this type of operation - there is no document, document viewer, etc. 

>+function QueryParameter(aName, aValue) {
>+  if (!aName || !aValue)
>+    throw Cr.NS_ERROR_INVALID_ARG;

ENSURE_ARG(name && value, "no name or value for QueryParameter?!");

>+function EngineURL(aType, aMethod, aTemplate) {
>+  if (!aType || !aMethod || !aTemplate)
>+    throw Cr.NS_ERROR_INVALID_ARG;

ENSURE_ARG(type && method && template, "no type, method or template for EngineURL?!");

Also, document all the conditions, parameter ranges etc. 

>+  _serializeToElement: function (aDoc, aElement) {

Documentation. 

>+function Engine(aLocation, aSourceDataType, aIsReadOnly) {

Documentation. 

...
>+  } else if (aLocation instanceof Ci.nsIURI) {
>+    // we have a URI, and no file
>+    this._getDataFromURI(aLocation);
>+  } else
>+    throw Cr.NS_ERROR_INVALID_ARG;

Time for an ERROR() function. This is like ASSERT(0, "crap!");

ERROR_ARG("Engine location is neither a File nor a URI object", Cr.NS_ERROR_INVALID_ARG);

>+Engine.prototype = {
>+  _alias: null,
>+  _dataType: "",
>+  _readOnly: true,
>+  _description: "",
>+  _file: null,
>+  _hidden: null,
>+  _req: null,
>+  _name: null,
>+  _type: null,
>+  _queryCharset: null,

Document!

>+  _getData: function () {

Document! I'm going to stop harping on this now. Make sure every function of any kind of complexity has better documentation. 

>+      default:
>+        throw Cr.NS_ERROR_INVALID_ARG;

isn't this more ERROR("Unexpected data type", Cr.NS_ERROR_UNEXPECTED) ?

>+//          if (this._file)
>+//            LOG("Initing MozSearch plugin: " + this._file.leafName);
>+//          else
>+//            LOG("Initing MozSearch plugin from a URL");

Why commented out - if you've enabled search service logging, is this of no interest?

>+    if (!this.name || !(this._urls.length > 0)) {
>+      LOG("_parseAsOpenSearch: No name, or missing URL. Failing!");
>+      throw Cr.NS_ERROR_FAILURE;
>+    }

ENSURE(this.name && (this._urls.length > 0), 
       "_parseAsOpenSearch: No name or urls!", 
       Cr.NS_ERROR_FAILURE);

>+    /**
>+     * Extracts one Sherlock "section" from aSource. A section is essentially
>+     * an HTML element with attributes, but each attribute must be on a new
>+     * line, by definition.

Did you define all the wacky cases nsInternetSearchService allows? Do you care? Say so. 

Is it possible to factor this code in such a way that it can be easily tested? e.g. for debug or testing builds, here is a directory full of .src files, check that they parse... 

>+  getSubmission: function (aData) {
>+    if (!this._urls[0])
>+      throw Cr.NS_ERROR_FAILURE;

ENSURE..?

>+  _loadEngines: function (aDir) {
...
>+          case SHERLOCK_FILE_EXT:

Is this the entry point to the migration process? 

>+  // XXX not complete - need to figure out how to add search parameters
>+  // too
>+  addEngineWithDetails: function(aName, aIconURI, aAlias, aDescription,
I'm not sure what I meant here - who calls addEngineWithDetails? I think it was autodetection, so leaving this as-is is probably fine for now.

>+
>+  //XXX need to document and fill out API
>+  addEngine: function (aEngineURL, aType, aIconURL) {

Document me please.

>+  removeEngine: function (aEngine) {
>+    if (!aEngine)
>+      throw Cr.NS_ERROR_NULL_POINTER;

ENSURE_ARG();

>Index: browser/components/search/content/searchDialog.xul

See comments at the start of this review about these files. 

>Index: browser/locales/en-US/searchplugins/amazondotcom.xml
>===================================================================
>RCS file: browser/locales/en-US/searchplugins/amazondotcom.xml
>diff -N browser/locales/en-US/searchplugins/amazondotcom.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ browser/locales/en-US/searchplugins/amazondotcom.xml	23 Feb 2006 01:21:21 -0000
>@@ -0,0 +1,13 @@
>+<SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/" xmlns:os="http://a9.com/-/spec/opensearchdescription/1.1/">

I appreciate Axel's point about there being confusion and likely errors on the part of localizers when creating these files. The best way to avoid that is to:

- use strict namespace checking in the search service and pro-actively throw exceptions when there are failures
- use a namespace prefix on the root, e.g.

<moz:SearchPlugin xmlns:moz="http://www.mozilla.org/2006/browser/search/" xmlns="http://a9.com/-/spec/opensearchdescription/1.1/">
  <ShortName>Amazon.com</ShortName>
  ...
  <moz:Alias>am</moz:Alias>
</moz:SearchPlugin>

since this is what people copy from, and it makes it clear what our extensions are. 


[1] Some useful sounding ENSURE_ functions:

/**
 * Ensures an assertion is met before continuing. 
 * @param  assertion
 *         An assertion that must be met
 * @param  message
 *         A message to display if the assertion is not met
 * @param  resultCode
 *         The NS_ERROR_* value to throw if the assertion is not met
 * @throws resultCode
 */
function ENSURE(assertion, message, resultCode) {
  ASSERT(assertion, message);
  if (!assertion)
    throw resultCode;
}

/**
 * Ensures an argument assertion is met before continuing. 
 * @param  assertion
 *         An argument assertion that must be met
 * @param  message
 *         A message to display if the assertion is not met
 * @throws NS_ERROR_INVALID_ARG for invalid arguments
 */
function ENSURE_ARG(assertion, message) {
  ENSURE(assertion, message, Cr.NS_ERROR_INVALID_ARG);
}

Note that my comments about using these functions should only apply to circumstances where an error is a "catastrophic" condition and should not happen in reasonable circumstances, because we may end up showing some kind of failure UI, at least in debug/alpha releases that people can use to help report problems. 

If there is legitimate handling of thrown exceptions, it's best not to use these since they all end up showing UI.

Comment 33

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

Attachment: patch (rv1.3)

Fixes:
-Sherlock parsing fixes turned up by my automated tester
-Fixed handling of readonly search plugins
-Fixed installer packaging manifests
-Fixed error reporting for the "install from the web" case
-Fixed error that occurred if the sorted engines didn't exist
-Added a "value" property on the search bar binding
-Addressed all of Ben's comments, will post summary

Comment 34

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

(In reply to comment #32)
> (From update of attachment 212829 [details] [diff] [review] [edit])
>
> >Index: browser/base/content/browser.js
> We really should start using Cc/Ci/Cr for all new code - to save our wrists if
> nothing else!

Done

> >Index: browser/components/search/jar.mn
> So, let's not add this search dialog back.

I added a "searchForm" property on nsISearchEngine and made it load the searchForm for the current engine.

> >Index: browser/components/search/nsISearchService.idl
> >+  void addParam(in AString name, in AString value);
> 
> Does this mean that an engine object (a singleton object held by the singleton
> search service) has a temporary buffer that accumulates search params used when
> generating a submission with getSubmission? This seems awkward. Can we do
> either:

No, engines get their parameters when they are initialized, and then never again. This method was meant to be used in conjunction with addEngineWithDetails. Passing arrays to XPCOM methods is a hassle, so I figured

engine = addEngineWithDetails(...);
for (all name/value pairs)
  engine.addParam(name, value)

was cleaner. Added documentation to this effect.

> nit: I like the formatting better like this:
> 
> @param  engineURL
>         the URL to the search engine's

Fixed everywhere.

> >Index: browser/components/search/nsSearchService.js
> nit: ' vs " quote style.
> If search logging is enabled you should also log to stdout.

Fixed.

> nit: no "a" prefixes for arguments anymore. That goes for all instances across
> this file so I won't mention them anymore. If you have to disambiguate, use a
> more descriptive variable name. It's better practice.

Why is it better practice? I think being able to easily distinguish arguments from other local variables is valuable, and was kind of disappointed to see that Places code doesn't honor this convention. I'd rather not go and change all the arguments in this file, unless you're strongly opposed to the idea.

> >+function checkNameSpace(aElement, aLocalNameArray, aNameSpaceArray) {
> 
> What does this function do? What is it used for? What are the input conditions?
> (it looks like it won't fail if aElement is null - is that OK, or will that
> lead to problems elsewhere) - document please!

First thing it did is check whether aElement is null, so I don't think that is an issue. It's meant to be used to determine which type of XML engine we're dealing with. Added some documentation for it.

> >+function getDir(aKey) {
> >+  if (!aKey)
> >+    throw Cr.NS_ERROR_INVALID_ARG;
> 
> let's start using ASSERT... grab the code from places. It'll move into a common
> location soon. Replace with:
> 
> ASSERT(key, "getDir requires a special directory key");
> if (!key)
>   throw Cr.NS_ERROR_INVALID_ARG;

I've added ENSURE_ARG, ENSURE, and ENSURE_WARN and used them appropriately.

> or some such. This allows us to consolidate our error handling and reporting. 

> >+var kCharsetCodes = [];
> nit: k-prefix but not a const?

Well, it's a constant in that it's never supposed to be changed beyond it's initialization. const arrays aren't really const-like, though.

> General comments: your functions all need more documentation!

Added documentation for all the methods.

> >+/**
> >+ * Removes all characters not in the "chars" string from aName.
> >+ * @returns a sanitized name to be used as a filename, or a random name if a
> >+ * sanitized name cannot be obtained (aName contains no valid characters).
> nit: Formattting!

Fixed.

> >+function setIcon(aIconURI, aEngine) {
> >+  if (!(aIconURI instanceof Ci.nsIURI))
> >+    return;
> this looks like an assertion.

I just changed this to accept the URL and create the nsIURI itself, since all callers had URL strings anyways. It ignores invalid URLs because that shouldn't be a fatal error.

> Why is this function that is relevant to a particular engine global? Shouldn't
> this be a member of the engine... or at least the service?

Yes, I made it a member of the engine.


> >+function iconLoadListener(aChannel, aEngine) {
> >+  this.mCountRead = 0;
> nit: use _ to prefix members, not m.

Fixed.

> >+        // The engine might not have a file yet, if it's being downloaded,
> >+        // because onLoad may not have been called yet. If it hasn't, this
> >+        // change will be written to file when it is.
> 
> What do you mean by "onLoad may not have been called yet" - where would "load"
> be called in this type of operation - there is no document, document viewer,
> etc.

I meant _onLoad, for engine's loaded using the XMLHTTPRequest. Clarified the comment.

> >+function QueryParameter(aName, aValue) {
> >+  if (!aName || !aValue)
> >+    throw Cr.NS_ERROR_INVALID_ARG;
> 
> ENSURE_ARG(name && value, "no name or value for QueryParameter?!");

Done.

> >+function EngineURL(aType, aMethod, aTemplate) {
> ENSURE_ARG(type && method && template, "no type, method or template for
> EngineURL?!");
Done

> Also, document all the conditions, parameter ranges etc.
Done


> >+  _serializeToElement: function (aDoc, aElement) {
> >+function Engine(aLocation, aSourceDataType, aIsReadOnly) {
> Documentation.

Added.

> >+  } else if (aLocation instanceof Ci.nsIURI) {
> >+    // we have a URI, and no file
> >+    this._getDataFromURI(aLocation);
> >+  } else
> >+    throw Cr.NS_ERROR_INVALID_ARG;
> 
> Time for an ERROR() function. This is like ASSERT(0, "crap!");

Added that too.

> >+Engine.prototype = {
> >+  _alias: null,
> >+  _dataType: "",
> >+  _readOnly: true,
> >+  _description: "",
> >+  _file: null,
> >+  _hidden: null,
> >+  _req: null,
> >+  _name: null,
> >+  _type: null,
> >+  _queryCharset: null,
> Document!

Done.

> >+  _getData: function () {
> Document! I'm going to stop harping on this now. Make sure every function of
> any kind of complexity has better documentation.

Added documentation to all methods. Let me know if you think it could be improved.

> >+      default:
> >+        throw Cr.NS_ERROR_INVALID_ARG;
> 
> isn't this more ERROR("Unexpected data type", Cr.NS_ERROR_UNEXPECTED) ?

Yep. Fixed.

> >+//          if (this._file)
> >+//            LOG("Initing MozSearch plugin: " + this._file.leafName);
> >+//          else
> >+//            LOG("Initing MozSearch plugin from a URL");
> Why commented out - if you've enabled search service logging, is this of no
> interest?

Made this logging better by keeping track of the URL from which we're loading the engine (_location).

> >+    if (!this.name || !(this._urls.length > 0)) {
> >+      LOG("_parseAsOpenSearch: No name, or missing URL. Failing!");
> >+      throw Cr.NS_ERROR_FAILURE;
> >+    }
> 
> ENSURE(this.name && (this._urls.length > 0), 
>        "_parseAsOpenSearch: No name or urls!", 
>        Cr.NS_ERROR_FAILURE);

Done.

> >+    /**
> >+     * Extracts one Sherlock "section" from aSource. A section is essentially
> >+     * an HTML element with attributes, but each attribute must be on a new
> >+     * line, by definition.
> 
> Did you define all the wacky cases nsInternetSearchService allows? Do you care?
> Say so.

I didn't take the time to go through all the cases it used to allow, but I've followed essentially the same approach in extracting this data, and have tested it on a large sample of plugins to ensure backwards compatibility (the only failures were with plugins that also failed with the current impl). I don't think listing all of the "wacky" cases the old impl allowed is of any use. I'm not sure what kind of comment you want me to add here: "I don't care about the wacky cases the old impl allowed"?

> Is it possible to factor this code in such a way that it can be easily tested?
> e.g. for debug or testing builds, here is a directory full of .src files, check
> that they parse...

I still think it might be valuable to have something like nsISherlockParser, but it's not something I want to spend a lot of time on. I have written a little XULRunner app that I've used to verify sherlock parsing, and the latest patch has correctly parsed and verified the 2069 Sherlock plugins I've been able to obtain from Mycroft.

> >+  getSubmission: function (aData) {
> >+    if (!this._urls[0])
> >+      throw Cr.NS_ERROR_FAILURE;
> ENSURE..?

Done.

> >+  _loadEngines: function (aDir) {
> ...
> >+          case SHERLOCK_FILE_EXT:
> Is this the entry point to the migration process? 

Yes. Added a comment to make it a bit clearer.

> >+  // XXX not complete - need to figure out how to add search parameters
> >+  // too
> >+  addEngineWithDetails: function(aName, aIconURI, aAlias, aDescription,
> I'm not sure what I meant here - who calls addEngineWithDetails? I think it was
> autodetection, so leaving this as-is is probably fine for now.

Yes, it was autoDetection. The "need to figure out how to add parameters" part was what addParam solved. This is functional but not used, and will be focused on more in the next step (adding autodetection and management UI).

> >+  //XXX need to document and fill out API
> >+  addEngine: function (aEngineURL, aType, aIconURL) {
> Document me please.

Documented in the IDL since it's a public nsISearchService method.

> >+  removeEngine: function (aEngine) {
> >+    if (!aEngine)
> >+      throw Cr.NS_ERROR_NULL_POINTER;
> ENSURE_ARG();

Done.

> >Index: browser/locales/en-US/searchplugins/amazondotcom.xml
> 
> I appreciate Axel's point about there being confusion and likely errors on the
> part of localizers when creating these files. The best way to avoid that is to:
> 
> - use strict namespace checking in the search service and pro-actively throw
> exceptions when there are failures
> - use a namespace prefix on the root, e.g.
> 
> <moz:SearchPlugin xmlns:moz="http://www.mozilla.org/2006/browser/search/"
> xmlns="http://a9.com/-/spec/opensearchdescription/1.1/">
>   <ShortName>Amazon.com</ShortName>
>   ...
>   <moz:Alias>am</moz:Alias>
> </moz:SearchPlugin>
> 
> since this is what people copy from, and it makes it clear what our extensions
> are.

If you don't have any objection I'd rather leave the format as is for now - making ours be the default namespace simplifies checking which type of plugin we're dealing with. This can be revisited for a2.

Comment 35

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

Attachment: sherlock parser app

this is the app I used to test sherlock parsing, in case anyone wants to run it

Comment 36

[Pavel Franc - Mozilla.cz]

> this is the app I used to test sherlock parsing, in case anyone wants to run it

Tested on Czech search plugins [http://www.czilla.cz/doplnky/vyhledavani/] 
It reported no errors but the character encoding was wrong :(

Comment 37

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

(In reply to comment #36)
> Tested on Czech search plugins [http://www.czilla.cz/doplnky/vyhledavani/] 
> It reported no errors but the character encoding was wrong :(

Can you email me the entire set of plugins? What do you mean by "the encoding was wrong"?

Comment 38

[Mycroft Project (CC)]

Okay, was trying to post an attachment but seems to be failing - is there a size limit? I'll put it here instead: http://downloads.mozdev.org/mycroft/ (should be available as soon as the mirror system picks it up - I'll try again if it doesn't work within 24 hours)

****

This should give you a few to run through the tester...
I think you've already answered most of my rpevious queries... a new question (I apologise for not bothering to read the code to work out the answer myself):

Sherlock specification defaults to x-mac-roman for the name and description unless srcTextEncoding is specified and then there is no option for iso-8859 or utf-8 so the only way to get, for example an n tilde is to substitute an en dash (see in this case googleES.src) Are you considering this? (I seem to remember that you were storing them in utf-8 now?)

One thing from reading your last reply...
The case of duplicate engines also applies to an attempted forced update. At the moment, the only way to force an update is to delete the old files and reinstall the search engine. This was even clumsier prior to extensions that handled deleting plugins but remains... if I know I've just updated the source for a plugin I'd like to be able to download the update without needing to delete the original. (I should think a "You already have a plugin called x installed are you sure you want to overwrite it" type message would work?) (I realise that most users don't install and delete and update as many plugins as I do and that the interface should be designed for the majority but I don't see much disadvantage to this)

(And I would run the parser over these myself but I'm not quite sure how.
Also worth pointing out that at least some of these are known to not work - but are not necessarily invalid - and I'm sure there are plenty more that don't work but we don't know about - the results would be interesting to see from my point of view - might save some manual testing. See http://mycroft.mozdev.org/list_broken2.html)

Comment 39

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

Attachment: patch (rv1.5)

changes since rv1.3:
-add logging pref to firefox.js
-change nsISearchService to nsIBrowserSearchService so that it doesn't conflict with the existing nsISearchService
-don't forget to remove the old searchbarbindings.css
-various sherlock fixes: ~6500 engines have been tested to not only load correctly, but also match the previous parser's output
-use nsILineInputStream instead of nsIScriptableInputStream
-fix converter so that it backs up old engines
-ignore readonly, empty, and hidden search engine files
-make removeEngine more robust
-other minor fixes

Comment 40

[Mike Connor [:mconnor]]

Comment on attachment 214513 [details] [diff] [review]
patch (rv1.5)


>-                oncommand="OpenSearch('internet', gContextMenu.searchSelected(), true);"/>
>+                oncommand="OpenSearch(gContextMenu.searchSelected(), true);"/>

hmm, should we call the function something besides opensearch, and provide a fallback for extensions that might be calling opensearch?  I'm pretty sure we should, with the whole backwards compat push.

>+function OpenSearch(aSearchText, aNewTab) {
>+  var currentEngine = Cc["@mozilla.org/browser/search-service;1"].
>+                      getService(Ci.nsIBrowserSearchService).
>+                      currentEngine;
>+
>+  var submission = currentEngine.getSubmission(aSearchText);
>+
>+  if (aNewTab)
>+    getBrowser().loadOneTab(submission.uri.spec, null, null,
>+                            submission.postData, false);
>+  else
>+    loadURI(submission.uri.spec, null, submission.postData);
> }

discussed this on IRC with gavin, and I think that using the selected engine when the searchbar isn't displayed will be problematic, since if you customize to ebay or something non-general and hide the toolbar, this gets kinda strange.  Using the default engine in the hidden-searchbar case seems to solve a lot of this, and this is pref-controlled, so users can change to something else if they want.

We should also change the contextmenu label to use the search engine's title "Search X for Y"

Probably want to apply the same thinking to the Ctrl-K behaviour.


>diff -N browser/components/search/nsIBrowserSearchService.idl
>--- /dev/null  1 Jan 1970 00:00:00 -0000
>+++ browser/components/search/nsIBrowserSearchService.idl      9 Mar 2006 03:03:22 -0000
>@@ -0,0 +1,276 @@

missing a line, methinks, in the comment around the license header?

>+[scriptable, uuid(b64edb5b-4d05-44df-b9bf-2098d9069f88)]
>+interface nsISearchEngine : nsISupports
>+{
>+  /**
>+   * Gets a nsISearchSubmission object that contains information about what to
>+   * send to the search engine, including the URI and postData, if applicable. 
>+   * @param   data
>+   *          Data to add to the submission object.
>+   *          i.e. the search terms.
>+   * @returns A nsISearchSubmission object that contains information about what
>+   *          to send to the search engine.
>+   */
>+  nsISearchSubmission getSubmission(in AString data);

can you add a space before the @param in all of these to make them more readable?

>+  /**
>+   * A nsIURI corresponding to the engine's icon, stored locally. May be null.
>+   */
>+  readonly attribute nsIURI iconURI;
>+
>+  /**
>+   * A string corresponding to the engine icon's URL. This will return
>+   * iconURI.spec, or an empty string if the engine has no iconURI.
>+   */
>+  readonly attribute AString iconURL;

why do we need a separate attr for URL vs. just getting iconURI.spec?

>+[scriptable, uuid(eaa8c60f-a0d7-4afe-b629-ab5ac9434298)]
>+interface nsIBrowserSearchService : nsISupports
>+{
>+  /**
>+   * Adds a new search engine from the file at the supplied URI.
>+   *  @param engineURL the URL to the search engine's description file.
>+   *  @param type an integer representing the plugin file format. Must be one
>+   *         of the supported search engine data types defined above.
>+   *  @param iconURL the URL to an icon file to be used as the search engine's
>+   *         icon.
>+   *
>+   *  @throws NS_ERROR_FAILURE if the type is invalid, or if the description
>+   *          file cannot be successfully loaded.
>+   */
>+  void addEngine(in AString engineURL, in long type, in AString iconURL);

you borked the indentation here ;)

>+  /**
>+   * Adds a new search engine.
>+   * @param name
>+   *        The search engine's name. Must be unique. Must not be null.
>+   * @param iconURI
>+   *        Optional: A URL string pointing to the icon to be used to represent
>+   *        the engine.

iconURL, not iconURI, be consistent!

>+   * @param alias
>+   *        Optional: A unique shortcut that can be used to retrieve the
>+   *        search engine.
>+   * @param description
>+   *        Optional: a description of the search engine.
>+   * @param method
>+   *        The HTTP request method used when submitting a search query.
>+   *        Must be a case insensitive value of either "get" or "post".
>+   * @param url
>+   *        The URL to which search queries should be sent.
>+   *        Must not be null.
>+   */
>+  void addEngineWithDetails(in AString name,

>+  /**
>+   * Returns an array of all installed search engines.
>+   * @returns an array of nsISearchEngine objects.
>+   */
>+  void getEngines(
>+            out unsigned long engineCount, 
>+            [retval, array, size_is(engineCount)] out nsISearchEngine engines);
>+
>+  /**
>+   * Returns an array of all installed search engines whose hidden attribute is
>+   * false.
>+   *
>+   * @returns an array of nsISearchEngine objects.
>+   */
>+  void getVisibleEngines(
>+            out unsigned long engineCount, 
>+            [retval, array, size_is(engineCount)] out nsISearchEngine engines);

I'm not an IDL guru, but how is engines an nsISearchEngine in these two? pretty sure you need to make these return an nsISimpleEnumerator consisting of nsISearchEngines

Why not make these readonly attrs (engines/visibleEngines) instead?

>+  /**
>+   * Removes the search engine. If the search engine is installed in a global
>+   * location, this will just hide the engine. If the engine is in the user's
>+   * profile directory, it will be removed from disk.
>+   *
>+   * @param   engine
>+   *          The engine to remove.
>+   */
>+  void removeEngine(in nsISearchEngine engine);

still working my way through, but how do we handle the user installing the engine again?

more to come

Comment 41

[[e-mail]]

(In reply to comment #40)
>>+  void getVisibleEngines(
>>+            out unsigned long engineCount, 
>>+            [retval, array, size_is(engineCount)] out nsISearchEngine engines);
>I'm not an IDL guru, but how is engines an nsISearchEngine in these two?
engines is a JS array of nsISearchEngine objects. Compare e.g. http://lxr.mozilla.org/mozilla/source/xpcom/base/nsIConsoleService.idl#58

Comment 42

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

Attachment: patch (rv1.6)

(In reply to comment #40)
> (From update of attachment 214513 [details] [diff] [review] [edit])
> 
> >-                oncommand="OpenSearch('internet', gContextMenu.searchSelected(), true);"/>
> >+                oncommand="OpenSearch(gContextMenu.searchSelected(), true);"/>
> 
> hmm, should we call the function something besides opensearch, and provide a
> fallback for extensions that might be calling opensearch?  I'm pretty sure we
> should, with the whole backwards compat push.

Fixed.

> discussed this on IRC with gavin, and I think that using the selected engine
> when the searchbar isn't displayed will be problematic, since if you customize
> to ebay or something non-general and hide the toolbar, this gets kinda strange.
>  Using the default engine in the hidden-searchbar case seems to solve a lot of
> this, and this is pref-controlled, so users can change to something else if
> they want.
> 
> We should also change the contextmenu label to use the search engine's title
> "Search X for Y"
> 
> Probably want to apply the same thinking to the Ctrl-K behaviour.

Fixed. Made Ctrl+K with no search bar go to the default engine. Context menu uses the default engine if the search bar is not displayed, or the currently selected engine if it is.

> >diff -N browser/components/search/nsIBrowserSearchService.idl
> >--- /dev/null  1 Jan 1970 00:00:00 -0000
> >+++ browser/components/search/nsIBrowserSearchService.idl      9 Mar 2006 03:03:22 -0000
> >@@ -0,0 +1,276 @@
> 
> missing a line, methinks, in the comment around the license header?

I just removed the trailing "*/", was a leftover from copy/pasting the other license header.

> >+[scriptable, uuid(b64edb5b-4d05-44df-b9bf-2098d9069f88)]
> >+interface nsISearchEngine : nsISupports
> >+{
> >+  /**
> >+   * Gets a nsISearchSubmission object that contains information about what to
> >+   * send to the search engine, including the URI and postData, if applicable. 
> >+   * @param   data
> >+   *          Data to add to the submission object.
> >+   *          i.e. the search terms.
> >+   * @returns A nsISearchSubmission object that contains information about what
> >+   *          to send to the search engine.
> >+   */
> >+  nsISearchSubmission getSubmission(in AString data);
> 
> can you add a space before the @param in all of these to make them more
> readable?

Fixed. Spaced these out a bit better, fixed alignment.

> >+  /**
> >+   * A nsIURI corresponding to the engine's icon, stored locally. May be null.
> >+   */
> >+  readonly attribute nsIURI iconURI;
> >+
> >+  /**
> >+   * A string corresponding to the engine icon's URL. This will return
> >+   * iconURI.spec, or an empty string if the engine has no iconURI.
> >+   */
> >+  readonly attribute AString iconURL;
> 
> why do we need a separate attr for URL vs. just getting iconURI.spec?

I added this because URI iconURI can be null, and allowed me to use just |setAttribute("src", engine.iconURL)|, removing the need to null check in search.xml. I can change it if you want, but I just thought it would be convenient.

> iconURL, not iconURI, be consistent!

Fixed.

> >+  void getVisibleEngines(
> >+            out unsigned long engineCount, 
> >+            [retval, array, size_is(engineCount)] out nsISearchEngine engines);
> 
> I'm not an IDL guru, but how is engines an nsISearchEngine in these two? pretty
> sure you need to make these return an nsISimpleEnumerator consisting of
> nsISearchEngines

I'm no IDL guru either, but this lets me do |var engines = getVisibleEngines({});| and have engines be a regular JS array of nsISearchEngines, which I found much nicer than any of the XPCOM alternatives (nsISimpleEnumerator etc). I'd be nicer as an attribute but I don't think that can be done without it being an enumerator.

> >+  /**
> >+   * Removes the search engine. If the search engine is installed in a global
> >+   * location, this will just hide the engine. If the engine is in the user's
> >+   * profile directory, it will be removed from disk.
> >+   *
> >+   * @param   engine
> >+   *          The engine to remove.
> >+   */
> >+  void removeEngine(in nsISearchEngine engine);
> 
> still working my way through, but how do we handle the user installing the
> engine again?

Right now, the second install silently fails (which from a user perspective is equivalent to what happens in 1.5). Could make this prompt for now, I guess, but I still need to determine what should happen in this case.

Comment 43

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

Comment on attachment 215317 [details] [diff] [review]
patch (rv1.6)

Index: browser/base/content/browser-menubar.inc
-                  datasources="rdf:bookmarks rdf:files rdf:localsearch" 
+                  datasources="rdf:bookmarks rdf:files" 

How is this related? Won't this break bookmarks/history search in non-places builds?

Index: browser/base/content/browser.js
-    }
-    else {
+    } else {

Why change the style? This is not consistent with that used elsewhere in the code... why not make the rest of this function consistent?

+// Backwards compat with 1.5
+function OpenSearch(tabName, searchStr, newTabFlag) {
+  LaunchSearch(searchStr, newTabFlag);
+}

What do you mean by "backwards compat with 1.5"?

I don't really want to promise to preserve function names to this level. It's not likely that anyone (or that many at leats) people use the function OpenSearch. Just remove it. That's what extension versioning is for. I've already torn up browser.js quite a bit for places. Search is changing, it's reasonable to expect some of the API to change. 

>+/**
>+ * Returns the search bar element if it is present in the toolbar and not
>+ * hidden, null otherwise.
>+ */
>+function getSearchBar() {
>+  var searchBar = document.getElementById("searchbar");
>+  if (searchBar && !searchBar.parentNode.parentNode.collapsed &&
>+      !(window.getComputedStyle(searchBar.parentNode, null).display == "none"))
>+    return searchBar;
>+  return null;
>+}

So, in the interests of incrementally improving the quality and layout of code within browser.js, can we do some rejiggery here:

Create a new object:

var BrowserSearch = {

  search: function BS_search(...) {
    // LaunchSearch implementation
  },
  
  get searchBar() {
    // contents of getSearchBar
  }
};

This lumps all the code together, partitions it from the "global" variable namespace, etc. 
  
>Index: browser/components/search/content/search.xml
>+#   Pierre Chanial V 2.0 <[e-mail]>
>+#   Gavin Sharp <[e-mail]>
>+#   Ben Goodger <[e-mail]>

Go on, give yourself v3.0 ;-) 

>+      <field name="mStringBundle">document.getAnonymousNodes(this)[0];</field>

nit: Rename these to use _ as a private prefix, not m. In general, _ is a better prefix 
since it's useful to fields, properties _AND_ methods. When was the last time you saw a 
method being invoked like this.mFoo(bar)? this._foo(bar) looks nicer. 

OK. So one more brief round. Fix my nits, reorg the code in browser.js into an object, and I will stamp r=.

Comment 44

[Mike Connor [:mconnor]]

Comment on attachment 215317 [details] [diff] [review]
patch (rv1.6)

Will look at this tonight after Ben's comments are addressed!

Comment 45

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

Attachment: patch (rv1.7)

(In reply to comment #43)
> (From update of attachment 215317 [details] [diff] [review] [edit])
> Index: browser/base/content/browser-menubar.inc
> -                  datasources="rdf:bookmarks rdf:files rdf:localsearch" 
> +                  datasources="rdf:bookmarks rdf:files" 
> 
> How is this related? Won't this break bookmarks/history search in non-places
> builds?

The localsearch datasource isn't used for anything, as far as I can tell.

> Index: browser/base/content/browser.js
> -    }
> -    else {
> +    } else {
> 
> Why change the style? This is not consistent with that used elsewhere in the
> code... why not make the rest of this function consistent?

All the code that I'm familiar with uses this style, and this function already used it just below.
http://landfill.mozilla.org/mxr-test/seamonkey/search?string=else+%7B&regexp=on&find=&filter=%2Fbrowser%2F%5Bbc%5D
looks about 50/50 to me, and I prefer same-line brackets (as do the mozilla style guidelines, but I realize that doesn't really mean much ;)

> +// Backwards compat with 1.5

> I don't really want to promise to preserve function names to this level. It's
> not likely that anyone (or that many at leats) people use the function
> OpenSearch. Just remove it. That's what extension versioning is for. I've
> already torn up browser.js quite a bit for places. Search is changing, it's
> reasonable to expect some of the API to change.

You're right, I removed this.

> This lumps all the code together, partitions it from the "global" variable
> namespace, etc.

Did this for OpenSearch, WebSearch, and getSearchBar. Also documented those functions. Removed SearchLoadURL, it is now unused.

> Go on, give yourself v3.0 ;-)

Fixed :)

> >+      <field name="mStringBundle">document.getAnonymousNodes(this)[0];</field>
> 
> nit: Rename these to use _ as a private prefix, not m.

Fixed.

Comment 46

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

Comment on attachment 215341 [details] [diff] [review]
patch (rv1.7)

(In reply to comment #45)
> > Index: browser/base/content/browser.js
> > -    }
> > -    else {
> > +    } else {
> > 
> > Why change the style? This is not consistent with that used elsewhere in the
> > code... why not make the rest of this function consistent?
> 
> All the code that I'm familiar with uses this style, and this function already
> used it just below.

I was suggesting changing it in all places in the function. 

I've been trying to move all of my code over to this style: 

var Container = 
  _bar: null,

  foo: function C_foo(param1, param2) {
    if (blah) {
    
    }
    else {
    }
    switch (param1) {
    case ONE:
    case TWO:
    }
  },
  ...
};

(see places code, nsExtensionManager.js.in, nsUpdateService.js etc)

... and have been slowly converting browser.js as I redo various bits (feeds, bookmarks)

Comment 47

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

Comment on attachment 215341 [details] [diff] [review]
patch (rv1.7)

sr=[e-mail]

Comment 48

[Mike Connor [:mconnor]]

Comment on attachment 215341 [details] [diff] [review]
patch (rv1.7)

couple notes, talked about this with gavin tonight, there's only one consumer of iconURL, so we're going to drop this from the interface and tighten up the API a little, also fixed some minor stuff in the stringbundle bits

>+        <xul:button class="searchbar-dropmarker"
>+                    type="menu"
>+                    id="searchbar-dropmarker"
>+                    popup="_child"
>+                    xbl:inherits="src">

shouldn't this be an anonid?

>+        var os = Components.classes["@mozilla.org/observer-service;1"]
>+                   .getService(Components.interfaces.nsIObserverService);

isn't typical style to do this instead, where we don't have Cc/Ci available?  You did this elsewhere

        var os = 
          Components.classes["@mozilla.org/observer-service;1"]
                    .getService(Components.interfaces.nsIObserverService);

>+      <property name="currentEngine">
>+        <setter><![CDATA[
>+          this.searchService.currentEngine = val;
>+        ]]></setter>
>+        <getter><![CDATA[
>+          return this.searchService.currentEngine;
>+        ]]></getter>
>+      </property>

why not use onget/onset here since its one line only? also return val from the setter?

>+      <property name="searchService">
>+        <getter><![CDATA[
>+          if (!this._ss) {
>+            const nsIBSS = Components.interfaces.nsIBrowserSearchService;
>+            this._ss =
>+                 Components.classes["@mozilla.org/browser/search-service;1"]
>+                           .getService(nsIBSS);
>+          }
>+          return this._ss;
>+        ]]></getter>
>+      </property>

should be readonly

r+a181=me, let's get this in before freeze, you have 80 minutes remaining!

Comment 49

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

Attachment: patch (rv1.8) for checkin

comments addressed

Comment 50

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

Attachment: additional fix

Comment 51

[Steffen Wilberg]

Comment on attachment 215374 [details] [diff] [review]
patch (rv1.8) for checkin

>Index: browser/base/content/browser.js
>+const BrowserSearch = {
...
>+  webSearch: function BrowserSearch_webSearch() {
...
>+      } else {
>+        // If there are no open browser windows, open a new one
>+        win = window.openDialog("chrome://browser/content/", "_blank",
>+                                "chrome,all,dialog=no", "about:blank");
>+        win.addEventListener("load", BrowserSearch.WebSearch, false);
I guess this should be BrowserSearch.webSearch.

Comment 52

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

(In reply to comment #51)
> I guess this should be BrowserSearch.webSearch.

You're right, thanks for catching that! I checked in that fix.

Comment 53

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

Attachment: additional fix #2

Comment 54

[Mike Connor [:mconnor]]

Comment on attachment 215385 [details] [diff] [review]
additional fix #2

r+a=me for checkin to slushy-frozen tree, this will save us a lot of annoying dataloss in the next set of nightlies, can't wait till morning.

Comment 55

[Axel Hecht [:Pike]]

Can we add an additional fix to wildcard for .src or .xml in the build logic
until we settled the namespace and format questions?
Then we don't have to migrate all 200 or so search plugins right now.

Comment 56

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

Attachment: potential tp fix

The old xpfe build copied a Sherlock file over, and sherlock parsing is slower than before, which is likely the cause of the tp hit. This makes sure the bogus file doesn't get added.

Comment 57

[Mike Connor [:mconnor]]

Comment on attachment 215427 [details] [diff] [review]
potential tp fix

ok, let's get this in on trunk and see if it helps

Comment 58

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

Attachment: copy .src files too, if they exist

There is almost certainly a better to do this than to hardcode in list.txt to cover the case where there are no .SRC files, but I don't know what it is.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Has this been reviewed for security issues like bug 290038?

Comment 60

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

(In reply to comment #59)
> Has this been reviewed for security issues like bug 290038?

There is no updating system yet, and installing an engine with the same name as an existing engine fails. The install prompt shows the full path to the engine, as before.

Comment 61

[Axel Hecht [:Pike]]

Comment on attachment 215433 [details] [diff] [review]
copy .src files too, if they exist

won't this add list.txt to the final build? I'm not sure we want that. It may make nsinstall happy when there's not src file, but an "if" sounds more like it.

Comment 62

[Ronny Perinke]

Error: Cc is not defined
Source: chrome://browser/content/browser.js
Line: 4512

var ss = Cc["@mozilla.org/browser/search-service;1"].
               getService(Ci.nsIBrowserSearchService);

Ci will be not defined, too.
I think this may only occur, if places is not enabled (like here on my build).

The easiest way would be to add

const Ci               = Components.interfaces;
const Cc               = Components.classes;

to browser.js.

Comment 63

[Mike Connor [:mconnor]]

Feel free to file a bug on that, though disable-places won't be supported for long, so you might as well hack it into your own tree in an #ifndef MOZ_PLACES block.

Marking this fixed, since its getting to the really long and unusable state, please file followups.

Comment 64

[Ronny Perinke]

(In reply to comment #63)
> Feel free to file a bug on that, though disable-places won't be supported for
> long, so you might as well hack it into your own tree in an #ifndef MOZ_PLACES
> block.
> 
> Marking this fixed, since its getting to the really long and unusable state,
> please file followups.
> 
Ok.
I only use disable-places because places not localized yet ...

Comment 65

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

(In reply to comment #61)
> (From update of attachment 215433 [details] [diff] [review] [edit])
> won't this add list.txt to the final build? I'm not sure we want that. It may
> make nsinstall happy when there's not src file, but an "if" sounds more like
> it.

Yes, I have a better patch, I thought I posted it. I filed bug 331118 for this.

Comment 66

[Axel Hecht [:Pike]]

I filed bug 331143 on the namespace sanitization and parsing (see comment 34).

Comment 67

[Paul Millar]

I'm working on the Advanced Searchsidebar extension for Firefox 2 and I have some questions concerning the new search backend.

1. Is there a method to retrieve the file path of a search plugin, for both .xml and .src files?

2. Are there any plans to support RSS/Atom links which are specified in the OpenSearch format?

3. Are there any plans to support the OpenSearch Description format specified by the OpenSearch website which is the format IE7 uses?

4. Is it possible to test adding search plugins via a web page using the addEngine() method, and if so, how?

Thanks for any help you can provide.

Comment 68

[K.yukichi]

Is addEngineWithDetails metod (nsIBrowserSearchService) designed to support Encoding parameter ? 

Comment 69

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

(In reply to comment #68)
> Is addEngineWithDetails metod (nsIBrowserSearchService) designed to support
> Encoding parameter ?

You mean the encoding used to submit the data to the search engine? Good catch. Please file a new bug for this issue.

Comment 70

[(not reading, please use [e-mail] instead)]

from comment #43:

>Index: browser/base/content/browser-menubar.inc
>-                  datasources="rdf:bookmarks rdf:files rdf:localsearch" 
>+                  datasources="rdf:bookmarks rdf:files" 
>
>How is this related? Won't this break bookmarks/history search in non-places
>builds?

ben was correct, this breaks searching in bookmarks in non-places builds in the bookmarks window, and ff 2.0 is now non-places build.

this regression is bug #336488 and the fix is to add back rdf:localsearch to the three places where it was removed.  

I'll ask gavin to review that patch.

Comment 71

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Fixed a long time ago. Marking it verified.