410613 - SeaMonkey should support "OpenSearch"

Status: RESOLVED FIXED

(SeaMonkey :: Search, enhancement)

Description

[Ryo Matsui (Amigomr)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.8.1.11) Gecko/20071128 SeaMonkey/1.1.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.8.1.11) Gecko/20071128 SeaMonkey/1.1.7

Firefox2 supports "OpenSearch". But SeaMonkey doesn't support it.
Therefore, while I made "OpenSearch" files for Firefox2, those doesn't work on SeaMonkey.

Also, "window.external.AddSearchProvider(engineURL);" (http://developer.mozilla.org/en/docs/Adding_search_engines_from_web_pages) isn't supported on SeaMonkey..

I think SeaMonkey should support "OpenSearch" file and install as well as Firefox2.

Reproducible: Always

Comment 1

[Ryo Matsui (Amigomr)]

well..Won't SeaMonkey have "OpenSearch" support?

Comment 3

[Mycroft Project (CC)]

This seems like a reasonably large omission.
At the Mycroft Project I am moving to a position where Sherlock is going to be no longer offered - already Sherlock duplicates of OpenSearch plugins are being removed to reduce confusion for the much larger volume of Firefox2+/IE7+ visitors.

Comment 5

[Philip Chee]

Bug 444735 moved the backend of the Firefox search service to /toolkit/ (mostly
because fennec wanted this). We should take leverage this to support openSearch
plugins in the suite search service.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Build the component

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081126 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Package the component

Untested, but looks trivial.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> Bug 444735 moved the backend of the Firefox search service to /toolkit/

The (only) part of that patch which I didn't port is
http://mxr.mozilla.org/comm-central/search?string=search&find=%2FnsSidebar%5C.js%24

Moreover, I would need some guidance about what to do for the UI part,
especially as I'm not familiar with this (Open)Search feature.

Comment 10

[[e-mail]]

Unfortunately these attachments aren't much use as we don't have any code that would use it (sidebar search requires sherlock plugins).

Comment 11

[Philip Chee]

> Unfortunately these attachments aren't much use as we don't have any code that
> would use it (sidebar search requires sherlock plugins).

But sidebar search could be rewritten to use the new API couldn't it?

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> But sidebar search could be rewritten to use the new API couldn't it?

In some way, probably yes. Still, the patches here aren't of much use by themselves as they don't actually make us use OpenSearch, only build it.

Comment 13

[Serge Gautherie (:sgautherie)]

My question was/is: who can tell me how we want to make use of it ?

Comment 14

[Robert Kaiser]

Well, we want to make use of it in any places where we are using Sherlock plugins right now, so that we can remove the sherlock-specific support (the OpenSearch code still supports the Sherlock plugins in some way, AFAIK).

Comment 15

[Mycroft Project (CC)]

One (?The) issue (as neil alludes to above) is that OpenSearch contains none of the parsing info that Sherlock does (the <INTERPRET> section) so any parsing of results into the sidebar would have to be done differently.

Comment 16

[Matthias Versen [:Matti]]

we should probably remove the searching in the sidebar and find something better for to do searches with different search engines.

Comment 17

[Robert Kaiser]

I agree with Matti here, let's find something else to for doing searches with different search engines. And then, Firefox add-ons seems to exist to do very similar things to our current search sidebar, so a solution for doing something like that should be possible.

Comment 18

[Karsten Düsterloh]

I disagree.
Simply removing stuff doesn't get fixed, just ... deleted.
This doesn't help anyone.

Comment 19

[Jonathan Pritchard]

I don't want to spam the bug, but I just wanted to say that I installed SeaMonkey 2 Beta 1 today and was shocked that I couldn't get hardly any search engines for it at Mycroft. I think this feature is needed a lot more urgently than you think. It really interferes with my browsing habits. Thanks for all the hard work.

Comment 20

[Robert Kaiser]

Actually, with this getting no traction for quite some time and the feature freeze nearing, I don't think we should look at this in any way for 2.0 any more and need to push it off to the next release.
It's unfortunate, but we have higher priority items we need to concentrate on, and it's better to get 2.0 out sooner rather than later. We really do want to switch to OpenSearch, but not for this release yet, sorry.

Comment 21

[Ryo Matsui (Amigomr)]

Sorry for not commenting long time...

Umm... I installed 2.0 Beta1 and thought the same thing as well as Jonathan. In our internet life, "search" becomes very important, and OpenSearch is the best search feature... so If seamonkey 2.0 hadn't OpenSeach feature, many users would be disappointed (especially, from fx users...)

I understand there is many higher priority items, but think OpenSearch should be marked as high priority...

Comment 24

[Robert Kaiser]

We have decided in SeaMonkey status meetings that when we're taking a first shot at this, we'll ignore extended sidebar functionality and concentrate on getting the basic support done first.
I'm currently taking a look, but it digs into areas I don't really understand, and I can't guarantee I'll get to know them well enough or find enough time before 2.1 goes into a feature freeze.
Also, if I do this, I'll probably remove the search sidebar completely for the moment and leave it up to followups or add-ons to implement this for now.

Comment 25

[Robert Kaiser]

Attachment: WIP1: make URLbar autocomplete, search button and context menu work

I'd like feedback from whoever has code knowledge and is looking here. :)

This first WIP patch makes the URLbar autocomplete search item, the search button and the context menu in browser and mailnews work with the toolkit version, but there might still be some more cleanup needed, the sidebar is mostly ignored, and some work for a toolkit search button is there but it's not implemented, not even as a default-hidden item.

Comment 26

[Robert Kaiser]

Oh, note that this doesn't switch us to package any OpenSearch plugins right now, as the old Mycroft plugins actually do work with the toolkit search implementation as well. Also, the pref panel is not touched by this patch.

Comment 27

[Robert Kaiser]

Attachment: WIP2: everything except sidebar and pref panel

OK, here's a second WIP, in which I think everything except the sidebar and the pref panel work - at least everything I tested.
It also adds new OpenSearch searchplugins instead of the old Mycroft ones, I guess we should go and add the old ones to removed-files, but we need to take a look to remove the ones of the locales as well, which will need some working out to do.

A selection of search engines can't be done easily with this state of the patch, but I have a very rough WIP for bug 401417 where the optional search bar then shows the installed search engines correctly.

The patch here is not ready for review yet, as the sidebar and pref panel are broken, but if you have time for it, I'd like you to take a look and tell me if the way it's going is alright.

Comment 28

[Robert Kaiser]

Attachment: v1: switch us to OpenSearch

Here is a v1 patch ready for review. The sidebar is being reduced to a very small and limited thing, but it works and is a good starting point for making it more full-featured again in the future. Switching the default engine works fine from the pref panel, and we're even using the default engine for keyword searches with this patch. I hope I have covered all I need to cover in a first round, would be good if we can still make beta with that, esp. as localizers will need to react to the default searchplugins changes.
The list of search plugins is a completely arbitary extract from the Firefox list.

Comment 29

[Karsten Düsterloh]

Comment on attachment 474126 [details] [diff] [review]
v1: switch us to OpenSearch

In toto, a good start - but with issues. :-)

(On startup, I get "Error: formatURL: Couldn't find value for key: SIDEBAR_VERSION" even without your patch, so I filed Bug 595394.)


The first search via the urlbar (enter search term, hit up arrow, hit enter) results in

Warning: assignment to undeclared variable engine
Source File: chrome://navigator/content/urlbarBindings.xml
Line: 377


Changing the default search engine in the "Internet Search" pref pane causes 

Chrome file doesn't exist: /XXX/obj/sr/mozilla/dist/bin/chrome/comm/content/navigator/[xpconnect wrapped nsIURI @ 0x7f9e6c14dcf0 (native @ 0x7f9e632832c0)]


(I also got chrome JS assignment warnings when adding OpenSearch engines from Mycroft, but those weren't reproducable.)


Finally, some code related notes:
>-pref("browser.search.opensidebarsearchpanel", true);

There's no need to destroy this functionality, it doesn't really depend upon the rest of the patch and looks as if it can easily be retained.

>+const BrowserSearch = {

Much of the code of this object is meaningless because of

>+  get searchBar() {
>+    return document.getElementById("searchbar");
>+  },

We don't have that searchbar widget, but I think we could redirect the functionality to the sidebar?
Your basic implementation is rather equivalent to the searchbar widget, so maybe this doesn't need much tweaking to make it work.

r- for the time being.

Comment 30

[Robert Kaiser]

(In reply to comment #29)
> >-pref("browser.search.opensidebarsearchpanel", true);
> 
> There's no need to destroy this functionality, it doesn't really depend upon
> the rest of the patch and looks as if it can easily be retained.

But is is completely meaningless when the sidebar doesn't actually show the search results, and we shouldn't keep it just for the possibility that someone could re-implement something like that some time in the future. For now, and pretty surely for 2.1, having this pref makes not a single bit of sense - why should we (want to) open an empty sidebar when the users issues a search?

> >+const BrowserSearch = {
> 
> Much of the code of this object is meaningless because of
> 
> >+  get searchBar() {
> >+    return document.getElementById("searchbar");
> >+  },

Well, I was hoping I won't need to defer all that to bug 401417 but could keep code that belongs together in that state even now. I can rip it all out, of course.

> We don't have that searchbar widget, but I think we could redirect the
> functionality to the sidebar?

If anything, we need to make it work with both, and if I attempt that for the first patch, we'll not make the 2.1 beta/feature cutoff set for end of September (not even if it slips 2-3 weeks), as my time will be tightening up until the end of the month and go to almost zero between Oct 1 and the Vienna meeting.


I'll take a look at the errors you have been seeing, I didn't get them, so maybe I missed something in this patch (having this together with a rough sketch of bug 401417 and some small part of bug 595235 in the same tree on top of a few other patches like the whole doorhanger series might hide some things on my machine).

Comment 31

[Karsten Düsterloh]

(In reply to comment #30)
> why should we (want to) open an empty sidebar when the users issues a search?

What empty sidebar?
The sidebar search panel opens, so that the user can issue another search there, without using a context menu or changing the urlbar content (or the need for another searchbar widget).
This is totally independent from listing search results there!

I know you never liked this feature, but there's no need to remove it here. ;-)


> Well, I was hoping I won't need to defer all that to bug 401417

Ah, okay, so there's at least a rational behind it. ;-)
It looked pretty much like an oversight, so you should at least add repective comments pointing to that bug.

> but could keep code that belongs together in that state even now. 
> I can rip it all out, of course.

If 401417 is going to happen soonishly, there's be no need to rip it out.

> > We don't have that searchbar widget, but I think we could redirect the
> > functionality to the sidebar?
> 
> If anything, we need to make it work with both

Well, actually, 401417 hasn't happened yet, so: no, just with the sidebar. ;-)
But once 401417 happens: yes, of course.

I see you've already put up a WIP patch to 401417, which - at a fast glance - won't fix the sidebar case. But keeping the sidebar search panel intact is more important than adding a random widget, IMO (wearing my Sidebar owner hat). 

Anyway, punting that out into a new bug should be okay.

Comment 32

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

Note also bug 518929.

Comment 33

[Robert Kaiser]

(In reply to comment #31)
> (In reply to comment #30)
> > why should we (want to) open an empty sidebar when the users issues a search?
> 
> What empty sidebar?

The one that will not contain anything related to the just started search.

> The sidebar search panel opens, so that the user can issue another search
> there, without using a context menu or changing the urlbar content (or the need
> for another searchbar widget).

Any search site I know actually has controls right on the web page to do that, which are much better fit to it than the sidebar after this patch.

> This is totally independent from listing search results there!

From all I know, the only reason we ever has this pref and function was to be able to follow multiple search results from there as we did list them there. With that functionality, this pref and functionality is completely useless.

> I know you never liked this feature, but there's no need to remove it here. ;-)

I found it reasonable as long as as scraping search results into the sidebar worked somewhat reasonably - even if I didn't use it myself.

> > Well, I was hoping I won't need to defer all that to bug 401417
> 
> Ah, okay, so there's at least a rational behind it. ;-)
> It looked pretty much like an oversight, so you should at least add respective
> comments pointing to that bug.

OK, will look into that. I'm planning to work on that followup bug as soon as this lands and hope to still get it into the 2.1 release.

> Anyway, punting that out into a new bug should be okay.

If I can do all the integration of functionality with the sidebar into a followup, I'd be happy, yes. Getting the basic functionality in should be the focus of this bug (I had pondered even removing/disabling the sidebar completely in the first step, but it looked easy enough to get at least something there and leave the file existing), improving it, integrating its parts better, building upon it, all that should go into followups, ideally (and don't need me to do every piece of work, hopefully, as I'm somewhat overworked).

Comment 34

[Robert Kaiser]

(In reply to comment #29)
> Changing the default search engine in the "Internet Search" pref pane causes 
> 
> Chrome file doesn't exist:
> /XXX/obj/sr/mozilla/dist/bin/chrome/comm/content/navigator/[xpconnect wrapped
> nsIURI @ 0x7f9e6c14dcf0 (native @ 0x7f9e632832c0)]

Can't reproduce that, but this looks strange to me. Maybe something strange with the icons?

> (I also got chrome JS assignment warnings when adding OpenSearch engines from
> Mycroft, but those weren't reproducable.)

I actually found an error when trying around there with Sherlock plugins and fixed that in the upcoming new patch.

Also, it seems that we get a JS strict warning for either window.sidebar or window.external not existing, but only on first access of either and it still seems to succeed getting its type, so I'd call that bogus.


Regarding the BrowserSearch object, the most important parts of it, webSearch() and loadSearch() even work fine without the search bar object or any equivalent.
The addEngine function refers to search engine discovery functionality that is not in our current sidebar code but in my search bar patch, and the updateSearchButton code directly refers to internals of search bar, so I'll move both into that patch for the moment and just stub them out so possible callers see them and won't break.

Comment 35

[Robert Kaiser]

Attachment: v2: address Mnyromyr's comments

OK, here's an updated patch.
This addresses the other comments and brings back the "open sidebar" pref, but disables it by default. Still, even when enabled, it doesn't work well - which is the same as in pre-patch trunk code. I added a comment as to what's the problem - when sidebarObj.never_built, the sidebarObj actually doesn't have any other properties than this one, so retrieving its datasource doesn't work.
We should put that in a followup, as fixing sidebar APIs is really not my job here...

Comment 36

[Yhn]

Will the OpenSearch function be integrated in the Sidebar Search? Or merely add a firefox-like search-bar?

Comment 37

[Philip Chee]

> Will the OpenSearch function be integrated in the Sidebar Search?
Eventually.

Comment 38

[Karsten Düsterloh]

Comment on attachment 478071 [details] [diff] [review]
v2: address Mnyromyr's comments

Starting up sidebar with a Sherlock search resulted in bug 599751 in nsSearchService.js.
Let's walk on from here.

Comment 39

[[e-mail]]

Comment on attachment 478071 [details] [diff] [review]
v2: address Mnyromyr's comments

>+// Check whether we need to perform engine updates every 6 hours
Does this mean the same as "Check for engine updates every 6 hours"?

>+      if (window.location.href != getBrowserURL()) {
Ironically you test against getBrowserURL() here...

>+          win.focus()
Nit: missing ;

>+          win = window.openDialog("chrome://navigator/content/", "_blank",
>+                                  "chrome,all,dialog=no", "about:blank");
... but then hardcode the URL here.

>+      openUILinkIn(Services.search.defaultEngine.searchForm, "current");
Why not just call loadURI directly?

>+      gBrowser.loadOneTab(submission.uri.spec, {
>+                          postData: submission.postData,
>+                          relatedToCurrent: true});
Don't we always want to focus the new tab?
The {} style looks wrong, perhaps one of these would look better:
foo(arg, {
      bar: bar,
      baz: baz
    });
foo(arg,
    { bar: bar,
      baz: baz });

>+   * Returns the search bar element if it is present in the toolbar, null otherwise.
Once we have a search bar element it will always be present in the document.

>+  loadAddEngines: function BrowserSearch_loadAddEngines() {
>+    var newWindowPref = gPrefService.getIntPref("browser.link.open_newwindow");
>+    var where = newWindowPref == 3 ? "tab" : "window";
[Why just tab or window? And should we use tabfocused?]

> function readRDFString(aDS,aRes,aProp)
No longer used?

[Didn't get much further because commenting on large patches is too slow.]

Comment 40

[[e-mail]]

Comment on attachment 478071 [details] [diff] [review]
v2: address Mnyromyr's comments

>diff --git a/suite/browser/src/Makefile.in b/suite/browser/src/Makefile.in
(Not sure which part to quote here)

If this only has source code for Windows, what happens on other platforms?

>+            // XXXsearch: the submission object may have postData
Is there any way we can predict that and not show the search engine?


>-    this.setItemAttr("context-searchselect", "label", searchSelectText);
>-    this.setItemAttr("context-searchselect", "accesskey",
>-                     bundle.getString("searchText.accesskey"));
>+    document.getElementById("context-searchselect").label = menuLabel;
>+    document.getElementById("context-searchselect").accessKey =
>+             bundle.getString("contextMenuSearchText.accesskey");
Why these changes? (Apart from the searchSelectText/menuLabel change.)

>+  // Let load finish first, so selections work.
???

>+function LoadEngineList() {
>+  var engines = Services.search.getVisibleEngines();
>+  for (let i = 0; i < engines.length; i++) {

>+    let menuitem = document.createElementNS(kXULNS, "menuitem");
>+    let name = engines[i].name;
>+    menuitem.setAttribute("label", name);
>+    menuitem.setAttribute("value", name);
...
>+    menulist.menupopup.appendChild(menuitem);
menulist.appendItem(name, name); [I assume the name is a unique ID here?]

>+      menuitem.setAttribute("src", engines[i].iconURI.spec);
"image" (automatically copied to the menulist, unlike "src").

>+      menulist.selectedIndex = i;
Nit: prefer menulist.selectedItem = menuitem;

To be continued...

Comment 41

[Robert Kaiser]

(In reply to comment #39)
> >+// Check whether we need to perform engine updates every 6 hours
> Does this mean the same as "Check for engine updates every 6 hours"?

No, it means "Check every 6 hours for every engine if its update interval has passed since it has been last checked for an update and if so, check for an update". We save an update interval and last time of an update (check) per-engine, this pref is just for running the code that runs to go through those.

> Ironically you test against getBrowserURL() here...
> ... but then hardcode the URL here.

Good catch, will also backport to FF! :)

> >+      openUILinkIn(Services.search.defaultEngine.searchForm, "current");
> Why not just call loadURI directly?

I see no reason, will backport this as well.

> Don't we always want to focus the new tab?

I'd think so. Strange that FF people didn't do that, as coming from search bar I guess loading in the background must be confusing.

> >+   * Returns the search bar element if it is present in the toolbar, null otherwise.
> Once we have a search bar element it will always be present in the document.

True, but I'm still wondering if we should make that method still return null if the search bar is not displayed, as it's e.g. somewhat useless to update it with search engines when it's not even shown. Still, right now, it is null because it's never present. I'd rather leave changing the comment to the search bar patch in bug 401417.

> >+  loadAddEngines: function BrowserSearch_loadAddEngines() {
> >+    var newWindowPref = gPrefService.getIntPref("browser.link.open_newwindow");
> >+    var where = newWindowPref == 3 ? "tab" : "window";
> [Why just tab or window? And should we use tabfocused?]

Because this is called from the engine manager and I guess it would be strange to replace a currently loaded page with clicking a link from there. But then, I guess it's reasonable to move this whole function to the patch in bug 401417 that will add the only caller.

> > function readRDFString(aDS,aRes,aProp)
> No longer used?

Good catch, I agree after double-checking. :)

(In reply to comment #40)
> >diff --git a/suite/browser/src/Makefile.in b/suite/browser/src/Makefile.in
> (Not sure which part to quote here)
> 
> If this only has source code for Windows, what happens on other platforms?

The other platforms just don't have that module. But thanks for reminding me, my packaging changes are actually wrong for Windows, as we still build suitebrowser.xpt there so must still package/include it. ;-)

> >+            // XXXsearch: the submission object may have postData
> Is there any way we can predict that and not show the search engine?

Hmm, not sure, and the solution is also suboptimal, IMHO, as ideally we'd find a way to call this with the postData somehow. Is it acceptable to put handling that XXX into a followup?

> >-    this.setItemAttr("context-searchselect", "label", searchSelectText);
> >-    this.setItemAttr("context-searchselect", "accesskey",
> >-                     bundle.getString("searchText.accesskey"));
> >+    document.getElementById("context-searchselect").label = menuLabel;
> >+    document.getElementById("context-searchselect").accessKey =
> >+             bundle.getString("contextMenuSearchText.accesskey");
> Why these changes? (Apart from the searchSelectText/menuLabel change.)

No, other than the fact that I think the way through .setItemAttr is overly complicated, given that it does exactly the same thing. Still, I'll revert this for now.

> >+  // Let load finish first, so selections work.
> ???

For whatever reason, I couldn't get this to work during onload directly, as it wouldn't allow setting a selected element to stick. The only way I could get this working was by doing it lazily.

> menulist.appendItem(name, name); [I assume the name is a unique ID here?]

Yes, it's unique ("for the moment", at least the name is the unique identifier for the engine throughout the search service - the is a bug filed to make it the URL but nobody ever seemed to actually care).

> >+      menuitem.setAttribute("src", engines[i].iconURI.spec);
> "image" (automatically copied to the menulist, unlike "src").

I remember having problems with getting it to actually work that way, but I'll try again.

Comment 42

[Robert Kaiser]

(In reply to comment #41)
> > >+      menuitem.setAttribute("src", engines[i].iconURI.spec);
> > "image" (automatically copied to the menulist, unlike "src").
> 
> I remember having problems with getting it to actually work that way, but I'll
> try again.

Hmm, seems to work now. Will apply the same to the sidebar, then. :)

Comment 43

[Philip Chee]

> True, but I'm still wondering if we should make that method still return null
> if the search bar is not displayed, as it's e.g. somewhat useless to update it
> with search engines when it's not even shown. Still, right now, it is null

You can use isElementVisible() in utilityOverlay.js

Comment 44

[[e-mail]]

Comment on attachment 478071 [details] [diff] [review]
v2: address Mnyromyr's comments

>+  while (menulist.menupopup.lastChild)
>+    menulist.menupopup.removeChild(menulist.menupopup.lastChild);
If you use appendItem then you can use removeAllItems (which happens to clear the selected item too, which is always a good idea when emptying a menulist)

>+    <menulist id="sidebar-search-engines"
>+              oncommand="SelectEngine(this);">
>+      <menupopup/>
>+    </menulist>
Actually you don't need the menupopup; appendItem will create one for you.

>+               onkeypress="if (event.keyCode == 13) doSearch();"
event.DOM_VK_RETURN

>+    const SEARCHSERVICE_CONTRACTID = "@mozilla.org/browser/search-service;1";
>+    const nsIBrowserSearchService = Components.interfaces.nsIBrowserSearchService;
>+    this.searchService =
>+      Components.classes[SEARCHSERVICE_CONTRACTID].getService(nsIBrowserSearchService);
Worth importing Services.jsm instead?

>+  // Get the favicon URL for the current page
I suppose the favicon service can't help us here?

>+  var browser = win.document.getElementById("content");
[win.getBrowser()]

>+  // Use documentURIObject in the check for shouldLoadFavIcon so that we
>+  // do the right thing with about:-style error pages.  Bug 453442
>+  if (browser.shouldLoadFavIcon(browser.selectedBrowser
>+                                       .contentDocument
>+                                       .documentURIObject))
>+    iconURL = win.gProxyFavIcon.getAttribute("src");
But why are error pages adding search engines?

>+# Search engine order (order displayed in the search bar dropdown)s
Stray trailing s?

>     topWindow = window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no");
[I wonder whether this still works; probably needs an extra about:blank]

Comment 45

[[e-mail]]

(In reply to comment #41)
> (In reply to comment #40)
> > >diff --git a/suite/browser/src/Makefile.in b/suite/browser/src/Makefile.in
> > (Not sure which part to quote here)
> > If this only has source code for Windows, what happens on other platforms?
> The other platforms just don't have that module. But thanks for reminding me,
> my packaging changes are actually wrong for Windows, as we still build
> suitebrowser.xpt there so must still package/include it. ;-)
So can we avoid calling this Makefile on other platforms completely?

> > >+            // XXXsearch: the submission object may have postData
> > Is there any way we can predict that and not show the search engine?
> Hmm, not sure, and the solution is also suboptimal, IMHO, as ideally we'd find
> a way to call this with the postData somehow. Is it acceptable to put handling
> that XXX into a followup?
Probably. Do many search engines need postData?

> > Why these changes? (Apart from the searchSelectText/menuLabel change.)
> No, other than the fact that I think the way through .setItemAttr is overly
> complicated, given that it does exactly the same thing.
Well then you should fix .setItemAttr and speed up the whole file in one go ;-)

Comment 46

[Robert Kaiser]

(In reply to comment #44)
> Worth importing Services.jsm instead?

Did that and went through the whole file for places to use it, I hope I caught all.

> >+  // Get the favicon URL for the current page
> I suppose the favicon service can't help us here?

Hmm, not sure, I guess it could, but then we'd need to fetch the URI of the current page and then fetch the icon for it from the favicon service. I like the change better they meanwhile made on the FF side (after they removed the proxy icon) to just do a getIcon() on the browser. :)

> But why are error pages adding search engines?

Hmm, good question that I can't explain, but I wonder if those working on bug 453442 had good reasons for adding this. Maybe worth raising this with :johnath.

> >     topWindow = window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no");
> [I wonder whether this still works; probably needs an extra about:blank]

(In reply to comment #45)
> So can we avoid calling this Makefile on other platforms completely?

Both this and its corresponding public/ dir, yes.

> Probably. Do many search engines need postData?

Honestly, I'm not sure, I haven't researched all available hundreds of OpenSearch plugins, but I suspect it's quite rare. Our default ones don't, from what I saw. I think the problem is small enough to put it in a followup and even ship a beta with it not fixed.

> Well then you should fix .setItemAttr and speed up the whole file in one go ;-)

Might be a good idea, but not in this bug (and probably not in the cards for me in the next few weeks).

Comment 47

[Robert Kaiser]

Filed bug 600243 for the postData followup work and bug 600244 for the backports of fixes to Firefox.

Comment 48

[Robert Kaiser]

Attachment: v2.1: address Neil's comments

Here's a patch that should address all comments up to now, I hope that can pass our high quality bar. :)

Comment 49

[Robert Kaiser]

(In reply to comment #46)
> > >     topWindow = window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no");
> > [I wonder whether this still works; probably needs an extra about:blank]

Forgot to add my actual comment to that in the comment above - I tested selecting text in mailnews with no browser window open and doing a search from the context menu, and that works, so I think this command still works after all. :)

Comment 50

[[e-mail]]

(In reply to comment #49)
> (In reply to comment #46)
> > > >     topWindow = window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no");
> > > [I wonder whether this still works; probably needs an extra about:blank]
> Forgot to add my actual comment to that in the comment above - I tested
> selecting text in mailnews with no browser window open and doing a search from
> the context menu, and that works, so I think this command still works after
> all. :)
I was actually worried that it would try to load the home page group (if prefs set), or to undo close window, or something, although I hadn't checked.

Comment 51

[[e-mail]]

Sigh, no midair warning for those changes :-(

Comment 52

[Frédéric Buclin]

(In reply to comment #51)
> Sigh, no midair warning for those changes :-(

It should!

Comment 53

[Frédéric Buclin]

(In reply to comment #52)

(and it does, at least upstream; didn't test on bmo)

Comment 54

[[e-mail]]

Comment on attachment 479086 [details] [diff] [review]
v2.1: address Neil's comments

> ifeq ($(OS_ARCH),WINNT)
[Don't really need this in child Makefiles any more?]

>+  // Let load finish first, so selections work.
>+  setTimeout(LoadEngineList, 0);
I don't think those timeouts are necessary. I removed the one in search-panel.js and it just worked. For the search pref pane, the binding hasn't loaded yet, so you would need to switch to menuitem.setAttribute("selected", "true") when building the list, but I'm not quite so worried about that one.

>+    if (engines[i] == Services.search.defaultEngine)
>+      menulist.selectedItem = menuitem;
An alternative approach is to write
menulist.value = Services.search.defaultEngine.name;
This would be particularly useful in the sidebar if there's a way of knowing that the engine name has changed without adding engines.

Comment 55

[Robert Kaiser]

Landed with those changes as http://hg.mozilla.org/comm-central/rev/098388acf16b

\o/

Comment 56

[Robert Kaiser]

Attachment: followup bustage fix

I forgot to address the nits of removing the ifdef from the src/ and public/ Makefiles, and also realized we now break on Linux and Mac clobbers as libsuitebrowser_s.a is not present for including into libsuite (the Mac nightly is burning with this).

The patch here should fix both those things.

Comment 57

[Robert Kaiser]

Landed the followup as http://hg.mozilla.org/comm-central/rev/e2b3ffe7a82e

Comment 58

[[e-mail]]

Comment on attachment 479086 [details] [diff] [review]
v2.1: address Neil's comments

>-      BrowserSearchInternet();
>+      BrowserSearch.webSearch();
Missed a call in navigatorOverlay.xul :-(

Comment 59

[[e-mail]]

Comment on attachment 479086 [details] [diff] [review]
v2.1: address Neil's comments

>+    if (isElementVisible(this.searchBar)) {
>+      searchBar.select();
>+      searchBar.focus();
>+    } else if (isElementVisible(this.searchSidebar)) {
Need to null-check calls to isElementVisible. (Other calls do null-check.)

Comment 60

[Philip Chee]

Firefox moved the null check into their isElementVisible(). We should too.

Comment 61

[Robert Kaiser]

(In reply to comment #60)
> Firefox moved the null check into their isElementVisible(). We should too.

Good idea, could you file a followup for that?

(In reply to comment #58)
> Comment on attachment 479086 [details] [diff] [review]
> v2.1: address Neil's comments
> 
> >-      BrowserSearchInternet();
> >+      BrowserSearch.webSearch();
> Missed a call in navigatorOverlay.xul :-(

Hrm, I did a search and thought I looked through all entries. But then, everything's possible. Not sure when I have time to look into anything like that, though. If you want a patch fast, please file it in a followup.

Comment 62

[Philip Chee]

> (In reply to comment #60)
>> Firefox moved the null check into their isElementVisible(). We should too.

> Good idea, could you file a followup for that?

Filed Bug 601435.

Comment 63

[Ian Neal]

(In reply to comment #58)
> Comment on attachment 479086 [details] [diff] [review]
> v2.1: address Neil's comments
> 
> >-      BrowserSearchInternet();
> >+      BrowserSearch.webSearch();
> Missed a call in navigatorOverlay.xul :-(

Filed bug 601466

Comment 64

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

(In reply to comment #40)
> >+            // XXXsearch: the submission object may have postData
> Is there any way we can predict that and not show the search engine?

You can call getSubmission and check for a non-null .postData - that's what keyword search does:

http://hg.mozilla.org/mozilla-central/annotate/1d7cd2d8b830/docshell/base/nsDefaultURIFixup.cpp#l430

Comment 65

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

(In reply to comment #44)
> >+  // Use documentURIObject in the check for shouldLoadFavIcon so that we
> >+  // do the right thing with about:-style error pages.  Bug 453442
> >+  if (browser.shouldLoadFavIcon(browser.selectedBrowser
> >+                                       .contentDocument
> >+                                       .documentURIObject))
> >+    iconURL = win.gProxyFavIcon.getAttribute("src");
> But why are error pages adding search engines?

They're not - the fix for bug 453442 is unrelated to search engines.