User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030331 Phoenix/0.5 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030331 Phoenix/0.5 This is a feature request, first off I love the google search box--I've added more than a few sites to my searchplugin directory. Currently, when a user highlights text, and right clicks it, you can search google for that text. I think that you should search using whatever searchplugin you're currently using. For instance, I've got a search php.net file written, and I'd love to be able to highlight a function name from a script I'm perusing on groups.google.com and search php.net using this functionality. Right now, I have to copy the text into the box and search that way--not too inconvenient, but could be better. Reproducible: Always Steps to Reproduce: 1. Go to a website with text 2. Highlight some text 3. Right-click and choose "Web Search for..." Actual Results: Searched google.com for the text Expected Results: Used the searchplugin site for finding the text

This makes a lot of sense, I wouldn't mind this at all (even though I use google there anyway). --> All/All, NEW

If this is implemented, shouldn't it be indicated somehow that the searchplugin in searchbar is actually used?! It would be cool if the contextmenu said Search <plugin name> for "...", i.e. Search Google for "..." or Search dmoz.org for "..."

I'm quite the novice, but may I make a suggestion? It seems to me that what you really want to do is link the contextual menu searches with your *Quick Searches*. In other words, first you define your quick searches in the Bookmarks manager as normal. Then, when you're reading a web page, you highlight text and right-click it. Currently, one of the items in the contextual menu is "Web Search for '<highlighted text>'." I think what you want instead is a contextual menu item that says: Search for "<highlighted text>" at ... Clicking that item in the contextual menu would bring up a submenu listing the sites you already defined in your quicksearches.

By the way, last time I checked, Mozilla suit had this feature working properly. In Mozilla suit it's not always google that does your web searches when you click on highlighted text.

Here's my go at this, the original function is so full of unused suite stuff that I just started from scratch. Basically it does this: * If selected text is an URL, open it. * If not and the search bar is available and the current engine is not "Find in this Page" use that engine. * Else use default search (i.e. google).

Comment on attachment 137151 [details] [diff] [review] Patch: Web Search uses current engine >+ var currentEngine = searchBar ? searchBar.getAttribute("searchengine") : null; Can't you just say var currentEngine = searchBar.getAttribute("searchengine"); ? Because you check for searchBar here again: >+ if (searchBar && currentEngine != "__PhoenixFindInPage") { >+ var defaultSearchURL = null; >+ try { >+ defaultSearchURL = gPrefService.getComplexValue("browser.search.defaulturl", >+ Components.interfaces.nsIPrefLocalizedString).data; >+ } catch (ex) { > } >+ openURL = defaultSearchURL + escapedSearchStr; I've just fixed a broken profile, where browser.search.defaulturl didn't exist (bug 227946). When searching for "foo", it tried to open http://nullfoo/, which couldn't be found of course.

->Pike. Pike, please have a loook at comment 6.

> Can't you just say var currentEngine = searchBar.getAttribute("searchengine"); But if the search bar has been removed from the toolbar, you would get an error like "searchBar has no properties", though maybe there's a better way to write that section to avoid the double check of searchBar, I'm not sure. > When searching for "foo", it tried to open http://nullfoo/, which couldn't be > found of course. Oh, whenever I have had this problem I get an error with navigatorRegionBundle not loading properly, which breaks the function. So it looks like I still need a secondary fallback to the URL from the stringbundle in case the pref fails.

Pike, you're right, I didn't try it with a removed searchbar. As for the second issue, you can simulate that by removing US.jar/locale/US/browser-region/region.properties. You can solve that by inserting a return in the curly brackets of the catch clause. This way just nothing happens. If you want to make some noise instead, insert a throw("browser.search.defaulturl can't be queried from chrome://browser-region/locale/region.properties."); that replaces one exception by another, but a more readable one.

Ah I see what you mean now, okay here is a version with the throw.

Comment on attachment 137318 [details] [diff] [review] Updated patch Fine! Is it ready for review?

I thought after creating that last patch the maybe there should be a pref to enable or disable this, i.e.: var escapedSearchStr = encodeURIComponent(searchStr); var useDefaultSearch = false; try { useDefaultSearch = gPrefService.getBoolPref("browser.search.usedefault"); } catch (ex) { } // If the search bar is available and "Find in this Page" is not selected then use it if (!useDefaultSearch && currentEngine && currentEngine != "__PhoenixFindInPage") { Then adding this to all.js: // always use the default search for Web Search pref("browser.search.usedefault", false); Is this a good idea or do you think I should just submit the existing patch for review?

I don't think we need a pref. Why should web search use a different (default) search engine than the search bar? I'd expect that web search always uses the engine shown in the search bar. Using another engine is confusing, and that's what this bug is about. But let the devs decide. :) I'd suggest Pierre (p_ch@verizon.net) since he made the last changes to this code: > //XXXpch: this routine needs to be cleaned up.

The idea of using the currently selected plugin is good, but wouldn't it be better (and a lot simpler) to follow the "submenu" concept outlined here: http://forums.mozillazine.org/viewtopic.php?p=322320#322320

Not really, that would be a more complex user experience (having to select which plugin to use). As an extension it has potential, though.

Comment on attachment 137318 [details] [diff] [review] Updated patch This patch no longer applies, I'll update it shortly. While I'm at it, should I change the menu item to say "Open %S" when a plain URL is selected so it is obvious what will happen when clicked or is it more consistent to always keep it as "Search Web for %S"?

Pike, I haven't even looked at the patch yet, but if highlighted text is going to open in as a URL, and not be a search term, there should absolutely be an option like "Open Selected Text as Link" What's the result if Find in this Page is selected? Do we just fail with feedback, or use Google, or what?

(In reply to comment #17) > Pike, I haven't even looked at the patch yet, but if highlighted text is going > to open in as a URL, and not be a search term, there should absolutely be an > option like "Open Selected Text as Link" The code to open plain links is already built into the existing web search feature, it's just broken at the moment so I fixed it in this patch. Now, I'm wondering whether instead I should have split that part out into a separate bug since this bug doesn't explicitly cover that. The other issue is whether to have a single menu item and rename as appropriate (i.e. "Search Web for" or "Open") or to have two items and hide one (I'm assuming you'd never want to search against a plain text URL, only open it). I'll have to check the existing code to see if there are any similar situations and see what they do. > What's the result if Find in this Page is selected? Do we just fail with > feedback, or use Google, or what? If Find in Page is selected the code falls back to using the default search engine as defined in the current locale package (i.e. Google). In terms of feedback I did think about changing the menu item to say "Search Google for %S" and so on, the only obvious problem is that for the life of me I can't find a way to find the name of the currently selected search engine (I suppose I could tweak search.xml to save the value somewhere when one is selected) or maybe I'm just missing something obvious.

I'd go for "Open Selected Text as Link" as well. > In terms of feedback I did think about changing the menu item to say "Search > Google for %S" and so on, That would be really nice since a lot of search plugins don't search the "entire web", but only a certain site, e.g. bugzilla.m.o.

This patch is updated to the latest version of the source tree and also improves it in a couple of ways. 1. Now shows an "Open Selected Text as Link" menu item in place of the search item when a plain URL is selected. 2. The "Search Web for %S" item now includes the name of the current search engine, e.g. "Search IMDB for %S" (it defaults back to "Web" anytime the current search engine name is unknown). One thing to note is that because the name of the current Search Engine is not currently stored anywhere (I couldn't find it anyway), when you first apply the patch it will still say "Search Web". The first time that you select a different engine in the search box the correct name will be stored and from then on it will work as expected (p.s. this is not a problem on new profiles only existing ones).

Updated to latest source version again.

Comment on attachment 148012 [details] [diff] [review] Patch: Web Search uses current engine v2 (updated) moving the review request to myself so I don't forget about this. in my cursory look it seems to be good

I just filed a bug ( bug 248173 ) to request the incorporation of context search extension http://forums.mozillazine.org/viewtopic.php?t=75294 in FF. Isn't that a better option than the patch that is proposed overhere??? Selecting the search engine in the search bar and then dueing the context search does not seem really intuitive to me

(In reply to comment #23) > I just filed a bug ( bug 248173 ) to request the incorporation of context search > extension http://forums.mozillazine.org/viewtopic.php?t=75294 in FF. Isn't that > a better option than the patch that is proposed overhere??? > > Selecting the search engine in the search bar and then dueing the context search > does not seem really intuitive to me This was discussed above and at least I agree with the notion that a context menu here is unnecessary. My vote would be to keep the extension as it is, and finish implementing the feature laid out here.

I really expect to have this reviewed tomorrow night, i haven't forgotten, work is my main focus at this moment though.

Comment on attachment 148012 [details] [diff] [review] Patch: Web Search uses current engine v2 (updated) This needs a bit of work now with the Find Bar/Locale reshuffle, not sure if it also needs to support Jesse's tab opening patch.

I like the idea of this as a context menu. Maybe a few ideas can be borrowed from the extension, and let the extension handle the extras?

Comment on attachment 148012 [details] [diff] [review] Patch: Web Search uses current engine v2 (updated) pike, if this is going to go into 1.0, that update needs to happen soon, due to the l10n impact.

Updated to latest Aviary (FastFind, localisation changes, etc), sorry about the delay. So are the peers happy about this change considering there are quite a few people against it (or who'd prefer alternate systems)? p.s. I've left worrying about whether this should use the new tab/window opening system to another bug.

(In reply to comment #27) > I like the idea of this as a context menu. Maybe a few ideas can be borrowed > from the extension, and let the extension handle the extras? This was the extension I was referring to: http://forums.mozillazine.org/viewtopic.php?t=75294

Sas, read comment 14 and comment 15... a full incorporation of Context Search is unlikely to happen, and I am quite happy to keep it open as an extension for the forseeable future. Also, I'd like to raise a point which I think could improve the intuitiveness of this patch... Taken from bug 248173#7, I agree that it might be a good idea to use the favicon supplied by the search plugin in the context menu.

Pike, you should've requested review back in July (and kept reminding mconnor). Now the patch is bitrotten again. :( We've only got around 2 weeks left until the localization freeze if we want to do it before 1.0. Can you update it once more, please?

(In reply to comment #33) > Pike, you should've requested review back in July (and kept reminding mconnor). > Now the patch is bitrotten again. :( > We've only got around 2 weeks left until the localization freeze if we want to > do it before 1.0. Can you update it once more, please? Not sure if it's too late, but will attach updated patch sometime later today or tomorrow.

Why let it search the currently selected search engine? Why not add a cascaded context menu with all installed search plugins? That would be of even more use.

(In reply to comment #36) > Why let it search the currently selected search engine? Why not add a cascaded > context menu with all installed search plugins? That would be of even more use. See comment 31.

Oops, sorry for the spam ;)

*** Bug 287444 has been marked as a duplicate of this bug. ***

*** Bug 294681 has been marked as a duplicate of this bug. ***

pike, is this still working or does it need an update? yes, I suck!

Why not add an additional entry to the context menu of seleceted text. One would be "Search Web for '<text>'", where <text> stands in for the text that is currently selected. This could be linked to Google by default. This is what is currently implemented in FF. Underneath this in the context menu, there could be another entry, "Search *** for '<text.'", where *** stands for the currently active search engine in the search toolbar. It is importnat to maintain a distinction between a general web search, like a normal Google search, and the oftentimes more specialized searches done in the search toolbar. I.e., many people don't use the search toolbar for general web searches, but have like the Google toolbar or Yahoo Companion extnesions where they do such searches, and leave the search toolbar for more speciality searches. The functionality of the context menu should mimic this user behabior, having one entry to a general web search of selected text, and another for a "speciality" search of selected text, by whatever search engine is currently active in the search bar.

Comment on attachment 160177 [details] [diff] [review] Patch: Web Search uses current engine v2.1 >Index: mozilla/browser/base/content/browser.js >-function OpenSearch(tabName, searchStr, newTabFlag) Nice cleanup here, this function has been bloated for a while now :). >+ var currentEngine = searchBar ? searchBar.getAttribute("searchengine") : null; ... >+ // If the search bar is available then use it >+ if (currentEngine) { >+ var iSearchService = Components.classes["@mozilla.org/rdf/datasource;1?name=internetsearch"] >+ .getService(Components.interfaces.nsIInternetSearchService); >+ openUrl = iSearchService.GetInternetSearchURL(currentEngine, escapedSearchStr, 0, 0, {value: 0}); >+ } else { // Else use default search engine Why not always just use the browser.search.selectedEngine pref? Especially since once bug 235204 is fixed, using the default when the searchbar is hidden won't be right. >+ var searchBar = document.getElementById("searchbar"); >+ var searchEngineName = searchBar ? searchBar.getAttribute("enginename") : null; >+ if (!searchEngineName) >+ searchEngineName = gNavigatorBundle.getString("defaultSearchName"); >+ var searchSelectText = gNavigatorBundle.getFormattedString("searchText", [searchEngineName, selectText]); Same point about using currentlySelected. >+ var selectStr = focusedWindow.__proto__.getSelection.call(focusedWindow); >+ selectStr = selectStr.toString(); The __proto__ hack isn't necessary thanks to the new built in XPCNativeWrapper. >Index: mozilla/browser/base/content/search.xml Some of the changes here somewhat conflict with the patch in bug 235204. Depending on which goes in first, changes will need to be made to one or the other. If you'd like look over and comment on that patch, I'd appreciate the feedback :). Also, I'm guessing this no longers applies cleanly to the trunk, so a fresh patch would probably be a good idea. Mike should be able to get to it a little quicker now, so hopefully it doesn't sit around rotting as it has for a while now.

(In reply to comment #43) > Why not always just use the browser.search.selectedEngine pref? Especially > since once bug 235204 is fixed, using the default when the searchbar is hidden > won't be right. AFAIK that pref didn't exist when the patch was attached. > >+ var selectStr = focusedWindow.__proto__.getSelection.call(focusedWindow); > >+ selectStr = selectStr.toString(); > > The __proto__ hack isn't necessary thanks to the new built in XPCNativeWrapper. That line is moved, not added by the patch so would be fixed in an updated version. > Also, I'm guessing this no longers applies cleanly to the trunk, so a fresh > patch would probably be a good idea. Mike should be able to get to it a little > quicker now, so hopefully it doesn't sit around rotting as it has for a while > now. It sounds like it might be better if this were handled together with bug 235204, please feel free to take over this bug, I'd rather leave it to someone who knows what they're doing. :-)

not a blocker, but hopefully we'll sneak in some more search improvements before beta

Comment on attachment 160177 [details] [diff] [review] Patch: Web Search uses current engine v2.1 Obsoleting rotted patch.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.522&mark=2775#2771 Indeed! ;)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060323 Firefox/1.6a1 ID:2006032305 now these are same.

This has been fixed by bug 317107.

Verified fixed in recent Bon echo builds