Closed
Bug 410613
Opened 15 years ago
Closed 13 years ago
SeaMonkey should support "OpenSearch"
Categories
(SeaMonkey :: Search, enhancement)
SeaMonkey
Search
Tracking
(seamonkey2.1 wanted)
RESOLVED
FIXED
seamonkey2.1b1
Tracking | Status | |
---|---|---|
seamonkey2.1 | --- | wanted |
People
(Reporter: amigomr, Assigned: kairo)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files, 6 obsolete files)
481.78 KB,
patch
|
kairo
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
2.03 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•15 years ago
|
Component: Sidebar → General
OS: Windows XP → All
Hardware: PC → All
Reporter | ||
Comment 1•15 years ago
|
||
well..Won't SeaMonkey have "OpenSearch" support?
Updated•15 years ago
|
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•15 years ago
|
||
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.
Component: General → Sidebar
Updated•15 years ago
|
Whiteboard: [wanted-seamonkey2+]
![]() |
||
Comment 5•15 years ago
|
||
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.
Component: Sidebar → Search
QA Contact: sidebar → search
![]() |
Assignee | |
Updated•15 years ago
|
Flags: wanted-seamonkey2+
Whiteboard: [wanted-seamonkey2+]
Comment 7•15 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081126 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #350166 -
Flags: superreview?(neil)
Attachment #350166 -
Flags: review?(neil)
Comment 9•15 years ago
|
||
(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.
Target Milestone: --- → seamonkey2.0a3
Version: unspecified → Trunk
Comment 10•15 years ago
|
||
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•15 years ago
|
||
> 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?
![]() |
Assignee | |
Comment 12•15 years ago
|
||
(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•15 years ago
|
||
My question was/is: who can tell me how we want to make use of it ?
![]() |
Assignee | |
Comment 14•14 years ago
|
||
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).
Keywords: helpwanted
Comment 15•14 years ago
|
||
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•14 years ago
|
||
we should probably remove the searching in the sidebar and find something better for to do searches with different search engines.
![]() |
Assignee | |
Comment 17•14 years ago
|
||
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•14 years ago
|
||
I disagree. Simply removing stuff doesn't get fixed, just ... deleted. This doesn't help anyone.
Updated•14 years ago
|
Assignee: sgautherie.bz → nobody
Status: ASSIGNED → NEW
Target Milestone: seamonkey2.0a3 → ---
Comment 19•14 years ago
|
||
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.
![]() |
Assignee | |
Comment 20•14 years ago
|
||
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.
Flags: wanted-seamonkey2+ → wanted-seamonkey2-
Reporter | ||
Comment 21•14 years ago
|
||
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...
![]() |
Assignee | |
Updated•14 years ago
|
Flags: wanted-seamonkey2.1+
![]() |
Assignee | |
Updated•13 years ago
|
status-seamonkey2.1:
--- → wanted
Flags: wanted-seamonkey2.1+
![]() |
Assignee | |
Comment 24•13 years ago
|
||
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.
![]() |
Assignee | |
Comment 25•13 years ago
|
||
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.
Assignee: nobody → kairo
Attachment #350166 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #472217 -
Flags: feedback?
Attachment #350166 -
Flags: superreview?(neil)
Attachment #350166 -
Flags: review?(neil)
![]() |
Assignee | |
Comment 26•13 years ago
|
||
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.
![]() |
Assignee | |
Comment 27•13 years ago
|
||
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.
Attachment #350169 -
Attachment is obsolete: true
Attachment #472217 -
Attachment is obsolete: true
Attachment #472315 -
Flags: feedback?(neil)
Attachment #472217 -
Flags: feedback?
Attachment #350169 -
Flags: review?(neil)
![]() |
Assignee | |
Comment 28•13 years ago
|
||
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.
Attachment #472315 -
Attachment is obsolete: true
Attachment #474126 -
Flags: superreview?(neil)
Attachment #474126 -
Flags: review?(mnyromyr)
Attachment #472315 -
Flags: feedback?(neil)
Comment 29•13 years ago
|
||
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.
Attachment #474126 -
Flags: review?(mnyromyr) → review-
![]() |
Assignee | |
Comment 30•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
Note also bug 518929.
![]() |
Assignee | |
Comment 33•13 years ago
|
||
(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).
![]() |
Assignee | |
Comment 34•13 years ago
|
||
(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.
![]() |
Assignee | |
Comment 35•13 years ago
|
||
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...
Attachment #474126 -
Attachment is obsolete: true
Attachment #478071 -
Flags: superreview?(neil)
Attachment #478071 -
Flags: review?(mnyromyr)
Attachment #474126 -
Flags: superreview?(neil)
Comment 36•13 years ago
|
||
Will the OpenSearch function be integrated in the Sidebar Search? Or merely add a firefox-like search-bar?
![]() |
||
Comment 37•13 years ago
|
||
> Will the OpenSearch function be integrated in the Sidebar Search?
Eventually.
Comment 38•13 years ago
|
||
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.
Attachment #478071 -
Flags: review?(mnyromyr) → review+
Comment 39•13 years ago
|
||
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•13 years ago
|
||
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...
![]() |
Assignee | |
Comment 41•13 years ago
|
||
(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.
![]() |
Assignee | |
Comment 42•13 years ago
|
||
(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•13 years ago
|
||
> 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•13 years ago
|
||
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•13 years ago
|
||
(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 ;-)
![]() |
Assignee | |
Comment 46•13 years ago
|
||
(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).
![]() |
Assignee | |
Updated•13 years ago
|
Keywords: helpwanted
Target Milestone: --- → seamonkey2.1b1
![]() |
Assignee | |
Comment 47•13 years ago
|
||
Filed bug 600243 for the postData followup work and bug 600244 for the backports of fixes to Firefox.
![]() |
Assignee | |
Comment 48•13 years ago
|
||
Here's a patch that should address all comments up to now, I hope that can pass our high quality bar. :)
Attachment #478071 -
Attachment is obsolete: true
Attachment #479086 -
Flags: superreview?(neil)
Attachment #479086 -
Flags: review+
Attachment #478071 -
Flags: superreview?(neil)
![]() |
Assignee | |
Comment 49•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
Sigh, no midair warning for those changes :-(
![]() |
||
Comment 52•13 years ago
|
||
(In reply to comment #51) > Sigh, no midair warning for those changes :-( It should!
![]() |
||
Comment 53•13 years ago
|
||
(In reply to comment #52) (and it does, at least upstream; didn't test on bmo)
Comment 54•13 years ago
|
||
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.
Attachment #479086 -
Flags: superreview?(neil) → superreview+
![]() |
Assignee | |
Comment 55•13 years ago
|
||
Landed with those changes as http://hg.mozilla.org/comm-central/rev/098388acf16b \o/
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 56•13 years ago
|
||
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.
Attachment #479767 -
Flags: review?(neil)
Updated•13 years ago
|
Attachment #479767 -
Flags: review?(neil) → review+
![]() |
Assignee | |
Comment 57•13 years ago
|
||
Landed the followup as http://hg.mozilla.org/comm-central/rev/e2b3ffe7a82e
Comment 58•13 years ago
|
||
Comment on attachment 479086 [details] [diff] [review] v2.1: address Neil's comments >- BrowserSearchInternet(); >+ BrowserSearch.webSearch(); Missed a call in navigatorOverlay.xul :-(
Comment 59•13 years ago
|
||
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•13 years ago
|
||
Firefox moved the null check into their isElementVisible(). We should too.
![]() |
Assignee | |
Comment 61•13 years ago
|
||
(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•13 years ago
|
||
> (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.
Depends on: 601435
Comment 63•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•