This bug is for tracking the inclusion of the OpenSearch add-on[1] into Thunderbird core. [1] https://addons.mozilla.org/en-US/thunderbird/addon/opensearch/

Here's a work-in-progress patch, which as far as I can tell works in Linux, and maybe even works on other platforms too!

Comment on attachment 554302 [details] [diff] [review] WIP Fly-by comments: - You're missing strings from the patch (they still seem to be in the extension) - I wonder if it would be more appropriate to make contentTabType and searchTabType have a common prototype and save some of the code duplication (and subsequent forgetful copying when we change things in one and not the other).

Some more progress: everything should work now, assuming you've previously installed the OpenSearch add-on (I haven't added the engines to Thunderbird yet, since I think I need to restructure that a bit). > - You're missing strings from the patch (they still seem to be in the > extension) I added them in now. They were missing as a temporary measure just to get everything up and running as quickly as possible. > - I wonder if it would be more appropriate to make contentTabType and > searchTabType have a common prototype and save some of the code duplication > (and subsequent forgetful copying when we change things in one and not the > other). This is probably a good idea. Once I get the engine stuff working, I'll probably do this.

I think everything basically works here. Standard8, do you mind looking at the build system changes I made? The important files are: mail/installer/package-manifest.in mail/locales/Makefile.in mail/locales/en-US/searchplugins/* mail/locales/filter.py mail/base/content/opensearch.js (just the getFiles method, L103) It seems to work on my system, but I have no idea if it will work elsewhere or if I'm doing even remotely the right thing. I really just copied and pasted from Firefox's build system and tried to reverse engineer the bits I needed to change.

This version fixes a piece of the build system that I forgot to change and renames the opensearch.* files to webSearch.*.

(In reply to Mark Banner (:standard8) from comment #2) > - I wonder if it would be more appropriate to make contentTabType and > searchTabType have a common prototype and save some of the code duplication > (and subsequent forgetful copying when we change things in one and not the > other). Looking at this again, does anyone have a good implementation for this? It's obviously not difficult, but I'm wondering if there's an idiomatic Mozilla way to do this.

(In reply to Jim Porter (:squib) from comment #7) > (In reply to Mark Banner (:standard8) from comment #2) > > - I wonder if it would be more appropriate to make contentTabType and > > searchTabType have a common prototype and save some of the code duplication > > (and subsequent forgetful copying when we change things in one and not the > > other). > > Looking at this again, does anyone have a good implementation for this? It's > obviously not difficult, but I'm wondering if there's an idiomatic Mozilla > way to do this. I think the way we've generally done things like this in the past is to use a base object, with derived classes using __proto__. The folder & message panes do this.

Comment on attachment 561646 [details] [diff] [review] WIP v4 Review of attachment 561646 [details] [diff] [review]: ----------------------------------------------------------------- Good start, but needs some more work to get it going right and fix some issues. General comment: I eventually found how to set the default search client, but that is really hidden. Maybe we could add an additional right-click context menu somewhere. The heart on mac needs to be lifted away from the status bar a bit at least, because it is currently touching it. ::: mail/app/profile/all-thunderbird.js @@ +702,5 @@ > pref("mail.taskbar.lists.enabled", true); > pref("mail.taskbar.lists.tasks.enabled", true); > #endif > + > +pref("mail.websearch.open_externally", false); Unless I'm missing something, we don't have the prefs here equivalent to browser.search.order.* so we can't set the default search order. I believe we'll want that. ::: mail/base/content/webSearch.js @@ +36,5 @@ > + * > + * ***** END LICENSE BLOCK ***** */ > + > +// From mozilla/toolkit/components/search/nsSearchService.js > +const NS_APP_SEARCH_DIR_LIST = "SrchPluginsDL"; This constant isn't used when you remove the unused getFiles function @@ +91,5 @@ > + }, false); > +} > + > +WebSearch.prototype = { > + log: function os_log(whereFrom, engine) { This function needs dropping, it was just something for getting numbers for the add-on. @@ +100,5 @@ > + req.channel.loadFlags |= Components.interfaces.nsIRequest.LOAD_BYPASS_CACHE; > + req.send(null); > + }, > + > + // Make ourselves an nsIDirectoryServiceProvider2. This won't work if extensions want to do things with searchplugins or if nsSearchService.js started requiring to do something if we didn't have the main window open. We need to move the nsIDirectoryServiceProvider2 stuff out to a separate module - I suggest a javascript one that is registered under the XPCOM_DIRECTORY_PROVIDER_CATEGORY. As part of this we need to make the getFiles function equivalent to that in DirectoryProvider::GetFiles so that we can use searchplugins from extensions, distributions directory, user profile and the app folder. @@ +119,5 @@ > + > + onLoad: function(event) { > + try { > + let tabmail = document.getElementById("tabmail"); > + if (!tabmail) I don't know why we need this check, we shouldn't be overlaying this on any non-tabmail window. @@ +127,5 @@ > + this.glodaCompleter = Components.classes["@mozilla.org/autocomplete/search;1?name=gloda"] > + .getService().wrappedJSObject; > + > + // Add us as the second completer. > + this.glodaCompleter.completers.splice(1, 0, new WebSearchCompleter()); This needs moving to a module or something. If you open up two three-pane windows, you currently then get two entries in the gloda autocompleter. We'll also end up leaking functions and code in the current state as well. @@ +133,5 @@ > + this.engine = this.engine; // load from prefs > + tabmail.registerTabType(webSearchTabType); > + > + Services.dirsvc.registerProvider(this); > + Services.search; // Force the search service to get instantiated now Not sure we'll need this force if we have a proper dir service provider. @@ +147,5 @@ > + > + onUnload: function (event) { > + if (!document.getElementById("tabmail")) > + return; > + Services.obs.removeObserver(this, "autocomplete-did-enter-text", false); nit: the false isn't reqired. @@ +225,5 @@ > + }; > + > + let tabmail = document.getElementById("tabmail"); > + if (!tabmail) { > + // Try opening new tabs in an existing 3pane window I think we have functions for this now don't we? (utilityOverlay.js iirc) ::: mail/base/content/webSearchTab.js @@ +150,5 @@ > + this._setDefaultButtonState(aTab, aTab.engine == gWebSearch.engine); > + > + // Set up onclick/oncommand listeners. > + let self = this; > + clone.getElementsByClassName("back")[0].addEventListener("click", This doesn't work for the back button - it always remains looking disabled even though it isn't (strangely you can click it). I would suggest that you hook the buttons up to cmd_* and use the supportsCommand/isCommandEnabled/doCommand. You should also be able to use aTab.browser.canGoBack() and aTab.browser.canGoForward() to properly enable/disable the buttons. ::: mail/base/content/webSearchTab.xul @@ +37,5 @@ > + - > + - ***** END LICENSE BLOCK ***** --> > + > +<?xml-stylesheet href="chrome://messenger/skin/webSearch.css" type="text/css"?> > +<?xml-stylesheet href="chrome://messenger/content/websearch-binding.css" type="text/css"?> This file isn't present in the patch. ::: mail/locales/Makefile.in @@ +99,4 @@ > $(DIST)/branding: > $(NSINSTALL) -D $@ > > +SEARCH_PLUGINS = $(shell cat $(LOCALE_SRCDIR)/searchplugins/list.txt) We need to use MERGE_FILE here, which will mean copying it into our config/config.mk if we need to. @@ +150,4 @@ > $(RM) $(STAGEDIST)/chrome/$(AB_CD).jar \ > $(STAGEDIST)/chrome/$(AB_CD).manifest \ > $(STAGEDIST)/defaults/pref/all-l10n.js > + $(RM) -r $(STAGEDIST)/searchplugins \ nit: whilst we're here please change this to -rf. ::: mail/locales/en-US/chrome/messenger/glodaComplete.properties @@ +51,5 @@ > # We use the same words in en-US, but maybe that's not always true. > glodaComplete.messagesMentioningMany.label=Messages mentioning: #1 > > +glodaComplete.webSearch.label=Search the web for: #1 > +glodaComplete.webSearch.label.truncated=Search the web for: #1… Although these are both fairly obvious, we still need to include localization notes for these because of the #1 replacement. ::: mail/locales/filter.py @@ +18,5 @@ > # ignore dictionaries > + if mod == "extensions/spellcheck": > + return False > + # ignore search plugins > + return not (re.match(r"searchplugins\/.+\.xml", path) I'd like this done in the same manner as Firefox's filter.py, however I'm currently doing a bulk update of filter.py in bug 697429, so maybe wait until that's done, or at least add in the if not entity check.

(In reply to Mark Banner (:standard8) from comment #9) > General comment: I eventually found how to set the default search client, > but that is really hidden. Maybe we could add an additional right-click > context menu somewhere. I guess you mean the heart here? We could add a context menu to each of the engine buttons. > Unless I'm missing something, we don't have the prefs here equivalent to > browser.search.order.* so we can't set the default search order. I believe > we'll want that. I can add those back in. I think I removed them because we didn't appear to use them (but I'm guessing core code does). > > +WebSearch.prototype = { > > + log: function os_log(whereFrom, engine) { > > This function needs dropping, it was just something for getting numbers for > the add-on. Removed. > @@ +119,5 @@ > > + > > + onLoad: function(event) { > > + try { > > + let tabmail = document.getElementById("tabmail"); > > + if (!tabmail) > > I don't know why we need this check, we shouldn't be overlaying this on any > non-tabmail window. We also overlay on the standalone message window. > @@ +225,5 @@ > > + }; > > + > > + let tabmail = document.getElementById("tabmail"); > > + if (!tabmail) { > > + // Try opening new tabs in an existing 3pane window > > I think we have functions for this now don't we? (utilityOverlay.js iirc) Only for opening a content tab, I think. > ::: mail/base/content/webSearchTab.js > @@ +150,5 @@ > > + this._setDefaultButtonState(aTab, aTab.engine == gWebSearch.engine); > > + > > + // Set up onclick/oncommand listeners. > > + let self = this; > > + clone.getElementsByClassName("back")[0].addEventListener("click", > > This doesn't work for the back button - it always remains looking disabled > even though it isn't (strangely you can click it). This does work for me, though I agree that using supportsCommand et al would be better. > > I would suggest that you hook the buttons up to cmd_* and use the > supportsCommand/isCommandEnabled/doCommand. You should also be able to use > aTab.browser.canGoBack() and aTab.browser.canGoForward() to properly > enable/disable the buttons. > > ::: mail/base/content/webSearchTab.xul > @@ +37,5 @@ > > + - > > + - ***** END LICENSE BLOCK ***** --> > > + > > +<?xml-stylesheet href="chrome://messenger/skin/webSearch.css" type="text/css"?> > > +<?xml-stylesheet href="chrome://messenger/content/websearch-binding.css" type="text/css"?> > > This file isn't present in the patch. Removed. It's not needed since I put that code into messenger.css. Anything I haven't comment on I haven't fixed just yet.

This is just a prelude to factor out some common code that both content tabs and web search tabs use.

This should fix most (not all) of the issues, though I'm pretty sure I set up the module incorrectly. Standard8, do you mind taking another look at that part? Specifically, I'm not sure what you mean by "a javascript [module] that is registered under the XPCOM_DIRECTORY_PROVIDER_CATEGORY."

(In reply to Jim Porter (:squib) from comment #12) > This should fix most (not all) of the issues, though I'm pretty sure I set > up the module incorrectly. Standard8, do you mind taking another look at > that part? Specifically, I'm not sure what you mean by "a javascript > [module] that is registered under the XPCOM_DIRECTORY_PROVIDER_CATEGORY." Hmm, sorry, I should have said component rather than module - and just the nsIDirectoryServiceProvider bits into that component. The rest of it can stay in the UI. Does that make more sense?

Is this what you mean? I had to makes some guesses about how this is set up, but I think I got it right. Stuff left to do: * browser.search.order.* prefs * Fix the back and forward buttons * Fix filter.py For the back and forward buttons, do you think it would make sense to hook them up to cmd_goBack and cmd_goForward? This would let the back/forward buttons on the main toolbar go back and forth as well, e.g. in regular content tabs. That might be beyond the scope of this patch though.

Ok, this should have everything fixed (except for improving the "set default" UI). I opted to make new cmd_* items for back/forward, but in the future we should probably use cmd_go(Back|Forward). I didn't want to do that now, since I didn't think I'd have time to get the back/forward buttons on the main toolbar working quite right. Of course, we probably won't need to worry about that when we switch to tabs-on-top. One thing I'm not sure about is whether we should have 10 entries for browser.search.order.*. Firefox only has 3. Tests forthcoming in part 3 (I'm still working on them, and unfortunately haven't gotten super-far yet).

Comment on attachment 570618 [details] [diff] [review] Part 2: Add OpenSearch Review of attachment 570618 [details] [diff] [review]: ----------------------------------------------------------------- I partially reviewed this today. I need to take a bit more of a detailed look at the content based code tomorrow. ::: mail/base/content/nsContextMenu.js @@ +175,5 @@ > this.showItem("mailContext-addemail", this.onMailtoLink && !this.inThreadPane); > > + > + let searchTheWeb = document.getElementById("mailContext-searchTheWeb"); > + this.showItem(searchTheWeb, !this.inThreadPane && !this.onPlayableMedia && nice side effect is this is enabled for content tabs as well :-) @@ +180,5 @@ > + this.isContentSelected); > + > + if (!searchTheWeb.hidden) { > + let selection = document.commandDispatcher.focusedWindow.getSelection(); > + let bundle = Services.strings.createBundle( I wonder if we should cache the bundle here. People invoking the context menu a lot might hit this frequently. ::: mail/base/modules/webSearch.js @@ +78,5 @@ > + _initialized: false, > + _engineListeners: [], > + _callback: null, > + > + // Make ourselves an nsIDirectoryServiceProvider2. nit: redundant comment ::: mail/components/webSearchProvider.js @@ +46,5 @@ > +function WebSearchProvider() {} > +WebSearchProvider.prototype = { > + classDescription: "Web Search Provider", > + classID: Components.ID("{76a80bff-8c3f-4b78-ad2c-80099e35375d}"), > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIDirectoryServiceProvider2]), This should really QI to nsIDirectoryServiceProvider as well, as that's where the 2 version inherits from. It'll mean you also need to provide a getFile function. @@ +48,5 @@ > + classDescription: "Web Search Provider", > + classID: Components.ID("{76a80bff-8c3f-4b78-ad2c-80099e35375d}"), > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIDirectoryServiceProvider2]), > + > + getFiles: function(prop) { We need to make this function more like http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/dirprovider/DirectoryProvider.cpp#238 (DirectoryProvider::GetFiles), so that we can get searchplugins from the same places as Firefox does. ::: mail/installer/package-manifest.in @@ +28,4 @@ > @BINPATH@/chrome/@AB_CD@@JAREXT@ > @BINPATH@/chrome/@AB_CD@.manifest > @BINPATH@/@PREF_DIR@/all-l10n.js > +@BINPATH@/searchplugins/* This file also needs webSearchProvider.js adding to it (modules have a * inclusion, components don't).

This version should address everything in comment 16.

Requesting security review on Part 2 (attachment 571257 [details] [diff] [review] at the time of this posting). There might be some changes to the patch as Standard8 reviews it, but the content-based parts of the code should stay the same.

Comment on attachment 571257 [details] [diff] [review] Part 2: Add OpenSearch (v8) Review of attachment 571257 [details] [diff] [review]: ----------------------------------------------------------------- This got a bit more involved than I was expecting as I realised we should probably be using nsSearchService.js a lot more. The implications are that we will be using what Firefox calls the "selected" engine (within the search specific box in the search bar), as what we're currently calling the "default" engine. We won't actually change the default engine (indeed the search APIs don't allow for that afaict), but we'll just use that when the user required option isn't set. I think we'll still be able to cover all our bases with that though I'll try and get Blake to take a quick look as well. ::: mail/base/content/mailWindowOverlay.js @@ +3299,5 @@ > gShowFeedSummaryToggle = false; > } > } > + > +window.addEventListener("load", function(event) { Do we need to pass this callback function in here, or can we just define the callback in webSearch.js? Also you could probably just call this from InitMsgWindow which will work for both the 3-pane & standalone message windows, hence saving an additional load listener. I think this would be early enough. ::: mail/base/content/utilityOverlay.js @@ +296,5 @@ > + * @param where 'tab' to open in a new tab (default), 'window' to open in a > + * new window, or 'external' to open in the default browser > + */ > +function openSearchTab(query, where) { > + let url = WebSearch.getSearchURL(WebSearch.engine, query); So if we more use the search service, then what we really want here is something like: var submission = Services.search.currentEngine.getSubmission(query); For internal searches, then we need to take the submission's URL and postdata (which some of the search engines need) and open the url with that. Looking at Firefox's openUILinkIn function may help here. @@ +300,5 @@ > + let url = WebSearch.getSearchURL(WebSearch.engine, query); > + > + if (where == "external" || > + Services.prefs.getBoolPref("mail.websearch.open_externally")) { > + openLinkExternally(url); I think a follow-up could be to not use our default search engine for that, but just pass the query to the browser (e.g. -search to the firefox exe) - however I don't know if that'll work with all browsers, hence a follow-up. @@ +307,5 @@ > > + WebSearch.previousSearchTerm = query; > + let params = { > + background: false, > + contentPage: url, Actually, wouldn't it be better to just pass the query into the web search tab, and let the web search tab work out the url and engine itself? It seems more self-contained that way. You could possibly embed the clickHandler as default if there isn't an argument for it passed in as well. ::: mail/base/content/webSearchTab.js @@ +77,5 @@ > + > + aTab.panel.appendChild(clone); > + > + let engines = clone.getElementsByClassName("engines")[0]; > + for (let i = 0; i < engines.childNodes.length; i++) { This loop seems to want to set the default search engine in the UI, however the engines aren't loaded yet, and _setUpEngineListener already seems to do that properly. @@ +227,5 @@ > + }, > + > + _doSearch: function(aTab, engine) { > + aTab.engine = engine; > + aTab.browser.setAttribute("src", WebSearch.getSearchURL(engine, This will need to change based on what I said in utilityOverlay as well. ::: mail/base/modules/webSearch.js @@ +70,5 @@ > + onQueryCompleted: function(aCollection) { > + } > +}; > + > +var WebSearch = { I think with the further changes in this file and elsewhere, we're not going to be doing webSearch specific stuff in here, so we could rename it to glodaWebSearch or something like that. @@ +75,5 @@ > + bundle: Services.strings.createBundle( > + "chrome://messenger/locale/glodaComplete.properties"), > + > + _initialized: false, > + _engineListeners: [], AFAICT this is a write-only attribute (look at addEngines and where it is called from) and there's no need for it. @@ +110,5 @@ > + this._engineListeners.push(listener); > + }, > + > + set engine(value) { > + Services.prefs.setCharPref("mail.websearch.engine", value); This is actually wrong on a couple of levels. We need to be hooking into nsSearchService.js a bit more, so that we're functioning the same as Firefox (which makes it better for extensions, maintainability etc). So mail.websearch.engine is wrong - it is ignoring browser.search.defaultenginename and for the search engine the user has selected we should actually be using browser.search.selectedEngine. However, the easy way to fix this, is to use nsSearchServices.js. So set/get engine can be replaced by Services.search.currentEngine, which then does everything for us. We might just need to ensure we can cope with a null value, as per the documentation. @@ +123,5 @@ > + return "Google"; > + } > + }, > + > + getSearchURL: function(aEngine, searchterm) { This should go away based on my comments in utilityOverlay.js @@ +130,5 @@ > + return submission.uri.spec; > + return ""; > + }, > + > + isInEngine: function(aEngine, aPreUri, aPostUri) { we could just move this to webSearchTab.js as that seems to be the only place it is used from. @@ +157,5 @@ > + this._callback(aSubject); > + } > + }, > + > + QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsISupports, No need for this QI in here now. @@ +161,5 @@ > + QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsISupports, > + Components.interfaces.nsIDirectoryServiceProvider2]), > +}; > + > +XPCOMUtils.defineLazyServiceGetter(WebSearch, "protocol", I think this is unused.

(In reply to Mark Banner (:standard8) from comment #19) > This got a bit more involved than I was expecting as I realised we should > probably be using nsSearchService.js a lot more. > > The implications are that we will be using what Firefox calls the "selected" > engine (within the search specific box in the search bar), as what we're > currently calling the "default" engine. We won't actually change the default > engine (indeed the search APIs don't allow for that afaict), but we'll just > use that when the user required option isn't set. > > I think we'll still be able to cover all our bases with that though I'll try > and get Blake to take a quick look as well. I'll give the next patch a try (if you remember to ping me), but as long as the UX doesn't change, I think I'm happy with it. Thanks, Blake.

This should fix all the review comments. I also made a few names clearer so that no one gets confused years later.

Comment on attachment 571876 [details] [diff] [review] Part 2: Add OpenSearch (v9) Review of attachment 571876 [details] [diff] [review]: ----------------------------------------------------------------- Ok, This is good. Apart from these final two comments, I need a few confirmations as to the search engine stuff, then we'll be ready to land. ::: mail/base/content/webSearchTab.js @@ +66,5 @@ > + } > + }, > + > + openTab: function onTabOpened(aTab, aArgs) { > + if (!"contentPane" in aArgs || !"engine" in aArgs || !"query" in aArgs) Shouldn't we also check for postdata here? @@ +266,5 @@ > + this.progressListener = { > + QueryInterface: XPCOMUtils.generateQI([ > + Components.interfaces.nsIWebProgressListener, > + Components.interfaces.nsISupportsWeakReference, > + Components.interfaces.nsISupports generateQI already adds nsISupports.

(In reply to Mark Banner (:standard8) from comment #22) > Comment on attachment 571876 [details] [diff] [review] [diff] [details] [review] > Part 2: Add OpenSearch (v9) > > Review of attachment 571876 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > Ok, This is good. Apart from these final two comments, I need a few > confirmations as to the search engine stuff, then we'll be ready to land. > > ::: mail/base/content/webSearchTab.js > @@ +66,5 @@ > > + } > > + }, > > + > > + openTab: function onTabOpened(aTab, aArgs) { > > + if (!"contentPane" in aArgs || !"engine" in aArgs || !"query" in aArgs) > > Shouldn't we also check for postdata here? I left that off, since add-ons might conceivably want to open a special search tab using openTab(), and they shouldn't have to specify postData if it would be null. > > @@ +266,5 @@ > > + this.progressListener = { > > + QueryInterface: XPCOMUtils.generateQI([ > > + Components.interfaces.nsIWebProgressListener, > > + Components.interfaces.nsISupportsWeakReference, > > + Components.interfaces.nsISupports > > generateQI already adds nsISupports. Fixed.

Here's the updated patch for review.

Both patches checked in: http://hg.mozilla.org/comm-central/rev/d29730042bdf http://hg.mozilla.org/comm-central/rev/77d2995562e0 Marking as fixed, but this still wants a quick security review. We'll deal with any comments in follow-up bugs.

Comment on attachment 572747 [details] [diff] [review] Part 2: Add OpenSearch (v10) +# LOCALIZATION NOTE (glodaComplete.webSearch.label): The label used in the +# autocomplete widget to refer to a search on the web for a short string +# containing at most 15 characters. #1 is the string to search for. +glodaComplete.webSearch.label=Search the web for: #1 + +# LOCALIZATION NOTE (glodaComplete.webSearch.label.truncated): The label used in +# the autocomplete widget to refer to a search on the web for a short string +# containing more than 15 characters. #1 is the string to search for, truncated +# to 15 characters. +glodaComplete.webSearch.label.truncated=Search the web for: #1… I don't understand why we need two separate labels here. Can't there be just one label, with #1 being either full, or truncated to XX characters + the ellipsis? The ellipsis itself is already localizable elsewhere, isn't it?

bug 1120777 killed the need for sec-review

Can't add opensearch xml files to Thunderbird 60.3.1 So this should NOT be resolved ! reopen pls.