The search suggestions code as landed in bug 337178 doesn't allow it to be disabled. This is IMHO bad for several reasons: (1) autocompleting from history is what the user has learned to expect from text box drop-downs - this might confuse them ("I've never searched for 'britney'"). (2) I often search several times for the same terms - history will be pretty successful in this use case - Google suggest probably not at all. (3) while the search terms will in the end be sent to Google anyway, I'd still prefer them not to know what I'm searching unless I've hit enter (or will the warn_submit_insecure alert be displayed for suggestions as well?). I'm sure that there will be some users who can profit from this feature (even users other than Google and Yahoo! ;-)), but I prefer not to be forced to. I suppose yet-another-hidden-pref would be the appropriate solution (which as a bonus might be exposed in the Search Engine Manager) - and maybe check for the pref in the Browser Metrics collection to see how many users didn't like it.

I'm not sure (1) is a reason to disable. As far as I can tell, most users aren't sophisticated enough to know or care whether we're using their history to guide autocomplete -- they just want good suggestions. (2) seems like a way the suggestion code code be enhanced, not a reason to disable. (3) seems like an actual reason to disable, but seems very weak to me given that, as you said, you're about to send the search term to your engine anyway. Now, if there's a (4) "suggest slows down browser responsiveness", THAT might be a reason to provide a pref to disable. I guess I'm just reluctant to add prefs in general -- I'd like to remove a good 75% of the prefs :)

Another reason from a consistency POV: (5) depending on which search engine is selected, you get different autocomplete results; this can make it more difficult to search different engines for the same terms - and to first enter the terms and then change the engine before searching. This might be solved together with (2) though. As for reason (1): users don't have to /know/ where the results come from, but they can still /feel/ that something's wrong. See e.g. this one reaction in MozillaZine's build forum: http://forums.mozillazine.org/viewtopic.php?t=415121

Why not include them both? At the moment Google Suggest overrides the default one, even if it is present in the profile. But with its original name people know why it behaves that way and they can choose.

What ever happened to respect for the users? This is a very annoying feature. For instance, I work behind a proxy. If I open Firefox and then try to type in something I want Google to search for (not what Google wants me to search for), then right in the middle of typing, up pops the Proxy authentication box, and my typing continues in there, instead of in the Google Search. This is about me doing what I want with Firefox, not what Google wants me to do. Give me back my control.(In reply to comment #0) > The search suggestions code as landed in bug 337178 doesn't allow it to be > disabled. This is IMHO bad for several reasons: > > (1) autocompleting from history is what the user has learned to expect from > text box drop-downs - this might confuse them ("I've never searched for > 'britney'"). > (2) I often search several times for the same terms - history will be pretty > successful in this use case - Google suggest probably not at all. > (3) while the search terms will in the end be sent to Google anyway, I'd still > prefer them not to know what I'm searching unless I've hit enter (or will the > warn_submit_insecure alert be displayed for suggestions as well?). > > I'm sure that there will be some users who can profit from this feature (even > users other than Google and Yahoo! ;-)), but I prefer not to be forced to. > > I suppose yet-another-hidden-pref would be the appropriate solution (which as a > bonus might be exposed in the Search Engine Manager) - and maybe check for the > pref in the Browser Metrics collection to see how many users didn't like it. > (In reply to comment #0) > The search suggestions code as landed in bug 337178 doesn't allow it to be > disabled. This is IMHO bad for several reasons: > > (1) autocompleting from history is what the user has learned to expect from > text box drop-downs - this might confuse them ("I've never searched for > 'britney'"). > (2) I often search several times for the same terms - history will be pretty > successful in this use case - Google suggest probably not at all. > (3) while the search terms will in the end be sent to Google anyway, I'd still > prefer them not to know what I'm searching unless I've hit enter (or will the > warn_submit_insecure alert be displayed for suggestions as well?). > > I'm sure that there will be some users who can profit from this feature (even > users other than Google and Yahoo! ;-)), but I prefer not to be forced to. > > I suppose yet-another-hidden-pref would be the appropriate solution (which as a > bonus might be exposed in the Search Engine Manager) - and maybe check for the > pref in the Browser Metrics collection to see how many users didn't like it. >

(In reply to comment #1) > I'm not sure (1) is a reason to disable. As far as I can tell, most users > aren't sophisticated enough to know or care whether we're using their history > to guide autocomplete -- they just want good suggestions. Which raises the question whether Suggest returns useful results. There're only two cases where suggest is useful, one is playing around and the other is if you don't have a clue how a word is spelled correctly or if you don't know what you are searching for. In 99.9% of the cases I know what I am looking for and I know how to spell most words so the only advantage would be that I don't have to type the query if the Suggest actually brings up a good result. However, most queries aren't that long and I am faster typing than stopping going through the results and select the one I need. Probably because I'm an advanced user. Somewhen Google's Suggest extension came it I installed it on my parents' profile - they don't even use autocomplete and if I told them "look it already guessed what you are looking for" it took them longer to grab the mouse and select the result than just finish typing. (6) Moreover think about privacy. Even the off-by-default ping attribute didn't get any good reviews though it doesn't actually add any new spy-functionally that wasn't possible before. IE 7's phising filter didn't raise trust either. Remember the ebay search plugin fiasco? This is all but good publicity - and a good reason for many people (including me) to mistrust Firefox. Rather make it opt-in, e.g. ask on first use if the suggest feature should be used for this engine or just make it off by default and make it available via a hidden pref or just provide the interface so that the devs can develop suggest extensions very easily. xeen

I'm certainly willing to believe that the suggested results should take search history into account, probably with higher priority than the search engine's default suggestions. That's still not a request to disable "suggest", that's a suggestion on how to enhance it to make it better so that the suggestions are actually what you want. "Let me do it the old way" isn't a use case. "I'm typing in a query I've typed before, the suggestions aren't appropriate" is. Incidentally, autocomplete results are generally meant to be arrowed down to, not selected with the mouse...

(In reply to comment #4) > What ever happened to respect for the users? This is a very annoying feature. > For instance, I work behind a proxy. If I open Firefox and then try to type in > something I want Google to search for (not what Google wants me to search for), > then right in the middle of typing, up pops the Proxy authentication box, and > my typing continues in there, instead of in the Google Search. That sounds like a request that proxy authentication boxes not steal your focus when you're typing. A reasonable solution might be that suggest should check when authentication would be needed, and silently fail in such cases. Suggest isn't "what Google wants you to search for" any more than a history-based set of suggestions is "what your browser wants you to search for" or the urlbar autocomplete list is "where your browser wants you to go". The goal of presenting a list of likely choices to users is to be helpful. Specific cases where suggestions fail to be helpful, such as using history-based suggestions in preference to search engine-suggested ones, can help make the browser more useful and usable. I have filed bug 338061 on using history-based suggestions in conjunction with (and in preference to) suggest-based suggestions. I suspect most people would be better served by that bug being fixed than this one.

This is already something we want to do as a global toggle, in the search manager and as a right-click option on the field.

Adds a new pref, browser.search.suggest.enabled. Exposes this pref in the search engine manager and in the context menu for the searchbar text field. This is a branch patch, but trunk should be pretty much identical.

Why create the menuitem onpopupshowing? The menu item can be added in initialize, just like clear history, right? I think it should be a type="checkbox" menuitem instead of alternating between "don't" and "do", the checked state can be updated onpopupshowing. I'd also rather avoid creating a global variable for the prefservice in engineManager.js, since it's only used twice, and not frequently (onLoad, and onOK). There's no need for getSuggestEnabled, IMO.

Comment on attachment 227750 [details] [diff] [review] Adds pref and UI to change it >+const BROWSER_SUGGEST_PREF = "browser.search.suggest.enabled"; >+ > /** > * Metadata describing the Web Search suggest mode > */ > const SEARCH_SUGGEST_CONTRACTID = > "@mozilla.org/autocomplete/search;1?name=search-autocomplete"; > const SEARCH_SUGGEST_CLASSNAME = "Remote Search Suggestions"; > const SEARCH_SUGGEST_CLASSID = > Components.ID("{aa892eb4-ffbf-477d-9f9a-06c995ae9f27}"); >@@ -423,16 +425,28 @@ SuggestAutoComplete.prototype = { > * > * @private > */ > _okToRequest: function SAC__okToRequest() { > return Date.now() > this._nextRequestTime; > }, > > /** >+ * >+ * This checks whether search suggestions are enabled by global preference. >+ * >+ * @private >+ */ >+ _suggestionsEnabled: function SAC__suggestionsDisabled() { >+ var prefService = Cc["@mozilla.org/preferences-service;1"] >+ .getService(Ci.nsIPrefBranch); >+ return prefService.getBoolPref(BROWSER_SUGGEST_PREF); >+ }, >+ this isn't really necessary, and actually is pretty inefficient since you're checking the pref on every search It is far better to cache the value and observe the pref, given how often we'll check the pref vs. how often it will change > if (!searchString || >+ !this._suggestionsEnabled() || > !engine.supportsResponseType(SEARCH_RESPONSE_SUGGESTION_JSON) || > !this._okToRequest()) { > // We have an empty search string (user pressed down arrow to see > // history), or the current engine has no suggest functionality, > // or we're in backoff mode; so just use local history. > this._sentSuggestRequest = false; > this._startHistorySearch(searchString, searchParam, previousResult); > return; you'd want to fix the comment here too >+const BROWSER_SUGGEST_PREF = "browser.search.suggest.enabled"; > > var gEngineView = null; >+var gPrefService = null; > > var gEngineManagerDialog = { > init: function engineManager_init() { > gEngineView = new EngineView(new EngineStore()); > >+ gPrefService = Cc["@mozilla.org/preferences-service;1"] >+ .getService(Ci.nsIPrefBranch); >+ nit, when using the Cc prefix, period should be trailing. Gavin is right that caching the prefservice is unnecessary and probably the wrong thing, but in this case I'm inclined to be ok with it since its a modal dialog, and I want you to cache the pref value as well (see below) >+ getSuggestEnabled: function engineManager_getSuggestEnabled() { >+ return gPrefService.getBoolPref(BROWSER_SUGGEST_PREF); >+ }, this is unnecessary given that it has a single caller, and should be replaced with a direct call in the init call > onOK: function engineManager_onOK() { > // Remove the observer > var os = Cc["@mozilla.org/observer-service;1"]. > getService(Ci.nsIObserverService); > os.removeObserver(this, "browser-search-engine-modified"); > >+ // Set the suggest preference >+ var enabled = document.getElementById("enableSuggest").checked; >+ gPrefService.setBoolPref(BROWSER_SUGGEST_PREF, enabled); I'd rather not set this every time, but only if its changed, since we should be observing the pref changes >Index: components/search/content/search.xml=================================================================== >RCS file: /cvsroot/mozilla/browser/components/search/content/search.xml,v >retrieving revision 1.37.2.41 >diff -u -8 -p -r1.37.2.41 search.xml >--- components/search/content/search.xml 26 Jun 2006 21:56:33 -0000 1.37.2.41 >+++ components/search/content/search.xml 30 Jun 2006 22:55:32 -0000 >@@ -385,16 +385,48 @@ > if (this._engines[i].iconURI) > menuitem.setAttribute("src", this._engines[i].iconURI.spec); > popup.insertBefore(menuitem, popup.firstChild); > menuitem.engine = this._engines[i]; > } > ]]></body> > </method> > >+ <!-- Removes any existing Show Suggestions menu item and appends the >+ appropriate wording depending on the current state of the preference. >+ --> >+ <method name="appendSuggestToggle"> >+ <parameter name="aMenu"/> >+ <body><![CDATA[ >+ // Remove last item, if it is the toggle-suggest item. >+ var lastItem = aMenu.lastChild; >+ if (lastItem.getAttribute("anonid") == "toggle-suggest-item") >+ aMenu.removeChild(lastItem); >+ >+ var prefService = this._textbox._prefBranch; >+ var enabled = >+ prefService.getBoolPref("browser.search.suggest.enabled"); >+ var label; >+ if (enabled) >+ label = this._stringBundle.getString("cmd_disableSuggestions"); >+ else >+ label = this._stringBundle.getString("cmd_enableSuggestions"); >+ const akey = >+ this._stringBundle.getString("cmd_toggleSuggestions_accesskey"); >+ >+ var element = document.createElementNS(XUL_NS, "menuitem"); >+ element.setAttribute("anonid", "toggle-suggest-item"); >+ element.setAttribute("label", label); >+ element.setAttribute("accesskey", akey); >+ element.setAttribute("cmd", "cmd_togglesuggest"); >+ >+ aMenu.appendChild(element); >+ ]]></body> >+ </method> >+ > <method name="openManager"> > <parameter name="aEvent"/> > <body><![CDATA[ > var wm = > Components.classes["@mozilla.org/appshell/window-mediator;1"] > .getService(Components.interfaces.nsIWindowMediator); > > var window = wm.getMostRecentWindow("Browser:SearchManager"); >@@ -529,17 +561,24 @@ > this.handleSearchCommand(event); > else if (anonid == "open-engine-manager") > this.openManager(event); > else if (target.getAttribute("class").indexOf("addengine-item") != -1 || > target.engine) > this.onEnginePopupCommand(target); > ]]></handler> > >- <handler event="popupshowing" action="this.rebuildPopupDynamic();"/> >+ <handler event="popupshowing"><![CDATA[ >+ const target = event.originalTarget; >+ var anonid = target.getAttribute("anonid"); >+ if (anonid == "searchbar-popup") >+ this.rebuildPopupDynamic(); >+ else if (anonid == "input-box-contextmenu") >+ this.appendSuggestToggle(target); >+ ]]></handler> I'll second gavin's comment that this is the wrong approach for now. We should look at doing the Mac-like thing you're doing here throughout the UI in Fx3 for Mac, but for now our primary target is Windows and this is a confusing convention for Windows. Update the checkbox onpopupshowing and keep the singular label, like we do for spellcheck >@@ -651,28 +690,41 @@ > this._getParentSearchbar().handleSearchCommand(evt); > ]]></body> > </method> > > <!-- nsIController --> > <field name="searchbarController" readonly="true"><![CDATA[({ > _self: this, > supportsCommand: function(aCommand) { >- return aCommand == "cmd_clearhistory"; >+ return aCommand == "cmd_clearhistory" || >+ aCommand == "cmd_togglesuggest"; > }, nit: align aCommand with the one with the line above > isCommandEnabled: function(aCommand) { >- return this._self._formHistSvc.nameExists( >+ if (aCommand == "cmd_clearhistory") >+ return this._self._formHistSvc.nameExists( > this._self.getAttribute("autocompletesearchparam")); this is super-ugly, long nested calls just suck, but at least align this so it seems a little less random. Even better is refactoring to read cleaner, like this (still ugly, less oddball wrapping) var attr = this._self.getAttribute("autocompletesearchparam") return this._self._formHistSvc.nameExists(attr); >+ else >+ return true; else after return is evil, just return >+cmd_enableSuggestions=Show Suggestions >+cmd_disableSuggestions=Don't Show Suggestions >+cmd_toggleSuggestions_accesskey=S I know I said we're not doing this approach at present, but please don't assume that because label X and label Y can share the same accesskey in en-US, that the same is true for all locales.

(In reply to comment #12) All comments addressed, except: > >+ // Set the suggest preference > >+ var enabled = document.getElementById("enableSuggest").checked; > >+ gPrefService.setBoolPref(BROWSER_SUGGEST_PREF, enabled); > > I'd rather not set this every time, but only if its changed, since we should be > observing the pref changes I'm a bit confused by this. It's a modal dialog, so the pref shouldn't normally be changing out from under us; and if it did, I wouldn't think we'd want to change the state of the checkbox after opening the dialog anyway. So I don't see why we'd be observing the pref changes in engineManager.js, and instead I did what I assume you must have meant: cache the starting state and update the pref if it's changed from that.

Comment on attachment 228377 [details] [diff] [review] Patch addressing review comments >Index: components/search/content/engineManager.js >+ // Set the preference if it has changed >+ var newSuggestEnabled = document.getElementById("enableSuggest").checked; >+ if (newSuggestEnabled != this._suggestEnabled) { >+ var prefService = Cc["@mozilla.org/preferences-service;1"]. >+ getService(Ci.nsIPrefBranch); >+ prefService.setBoolPref(BROWSER_SUGGEST_PREF, newSuggestEnabled); >+ } The dialog may be modal, but it's only window-modal, so if you have two windows open you can still have the dialog open in one window while toggling the pref with the context menu in the other. I think you should set the pref unconditionally as in your original patch (I don't think it's worth the effort to have a pref observer in the dialog). People having the dialog open while toggling the pref in another window is an edge case, anyways, and onOK isn't called often enough to make setting a pref in it each time prohibitively expensive. >Index: components/search/content/search.xml > <destructor><![CDATA[ >+ var ps2 = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch2); >+ ps2.removeObserver("browser.search.suggest.enabled", this); nit: fix the indentation here. >+ this._suggestMenuItem = element; >+ cxmenu.appendChild(element); This could just be: this._suggestMenuItem = cxmenu.appendChild(element); r=me if you change onOK to set the pref unconditionally (and remove _suggestEnabled).

(In reply to comment #15) > I think you should set the pref > unconditionally as in your original patch (I don't think it's worth the effort > to have a pref observer in the dialog). Agh, dueling reviewers! But I'm with you on this one for simplicity's sake, so I'll revert to the original plan and let mconnor say something if he strongly disagrees. > >+ this._suggestMenuItem = element; > >+ cxmenu.appendChild(element); > > This could just be: this._suggestMenuItem = cxmenu.appendChild(element); It's more readable as two lines, to me, since we already have |element| handy and the return value of appendChild isn't transparently obvious.

Comment on attachment 229846 [details] [diff] [review] Patch addressing Gavin's comments I have a UI question - what happens if suggest isn't available for the currently selected engine? Showing the option seems weird then. But it's not per-engine, it's a global setting. Maybe just disable the option in that case? The easiest possible outcome is "Show search suggestions when available" text in place of what's there already. Thoughts? The patch looks fine to me otherwise. r=ben@mozilla.org

That's a good point, but I don't think we can make it all that much clearer with different text. Disabling doesn't make a lot of sense to me for the reasons you point out, and also, there are cases where we won't show suggestions for engines that *can* return them, such as when latency is high. I think we're good to leave this for now, and we can re-examine if it's reported as a huge problem. FWIW, I think users will be happy to see them when they're there, and consider them a bonus when they see them as opposed to getting confused about them not being available.

Landed on trunk to bake.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060720 Minefield/3.0a1 ID:2006072016 [cairo] assertion error appears. see : http://forums.mozillazine.org/viewtopic.php?p=2382120#2382120

(In reply to comment #20) > assertion error appears. > see : http://forums.mozillazine.org/viewtopic.php?p=2382120#2382120 That is bug 345404, which I just fixed. It was a bug caused by the patch for bug 327932.

Comment on attachment 229846 [details] [diff] [review] Patch addressing Gavin's comments a=mconnor on behalf of drivers

Landed on 1.8.1 branch.

I know this is old but one NEW reason that has some currency that was not mentioned when this was dealt with. Google estimates that each search uses the same energy as boiling two cups of water. Now if I search for "solar energy savings" it get to type about 20 chars before I get a match. (ie I typed just about the whole phrase). That means these hints just burnt enough energy for me to take a shower !! Now multiply that by every FF user that has not explicitly turned this off and you are probably keeping several major power stations in business just with this one , probably near useless, feature. browser.search.suggest.enabled SHOULD be FALSE by default.