In bug 410613, I'm working on bringing OpenSearch to SeaMonkey, and reviews as well as review-inspired work found some cleanups in search and sidebar code that are worth porting back to Firefox to improve all our lives. :)

And here's the patch to port back those fixes, I hope they look good for your side as well.

Comment on attachment 479809 [details] [diff] [review] v1: port back fixes >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >- openUILinkIn(Services.search.defaultEngine.searchForm, "current"); >+ loadURI(Services.search.defaultEngine.searchForm); Don't want this change - openUILinkIn handles the "current tab is an app tab" case differently (uses a new tab). >- gBrowser.loadOneTab(submission.uri.spec, { >- postData: submission.postData, >- relatedToCurrent: true}); >+ gBrowser.loadOneTab(submission.uri.spec, >+ { postData: submission.postData, >+ relatedToCurrent: true, >+ inBackground: false }); What's the justification for the addition of inBackground? Even though it might make sense, I'd rather not include the behavior change in a "cleanup" patch.

(In reply to comment #2) > >- openUILinkIn(Services.search.defaultEngine.searchForm, "current"); > >+ loadURI(Services.search.defaultEngine.searchForm); > > Don't want this change - openUILinkIn handles the "current tab is an app tab" > case differently (uses a new tab). Ah, OK, interesting to know! Without that info, it sounds strange to go through openUILink() there. ;-) > What's the justification for the addition of inBackground? Even though it might > make sense, I'd rather not include the behavior change in a "cleanup" patch. Ah, right, I almost forgot that was in there when naming this bug as it is. I surely can split this off to yet another bug. It made not much sense to me or Neil that searches one opens intentionally via direct user interaction would appear in background tabs.

That mirrors our behavior for "open link in new tab", fwiw.

(In reply to comment #4) > That mirrors our behavior for "open link in new tab", fwiw. True, though opening tabs from the UI is different from opening them from a web page in that you are not reading content when issuing the page load from the UI, like a search or opening a "UI tab" like Add-ons Manager (where we actually always oping in the foreground even in Firefox, I think). You're right though in that it probably something to be thinking about more closely, I'll split this off into a separate bug - do we need a word from UX on this?

Here's a new patch updated for the review comments, I've split off the background/foreground matter into bug 612965 instead.

Comment on attachment 491270 [details] [diff] [review] v1.1: update for review comments >@@ -3248,19 +3248,19 @@ const BrowserSearch = { > // getSubmission can return null if the engine doesn't have a URL > // with a text/html response type. This is unlikely (since > // SearchService._addEngineToStore() should fail for such an engine), > // but let's be on the safe side. > if (!submission) > return; > > if (useNewTab) { >- gBrowser.loadOneTab(submission.uri.spec, { >- postData: submission.postData, >- relatedToCurrent: true}); >+ gBrowser.loadOneTab(submission.uri.spec, >+ { postData: submission.postData, >+ relatedToCurrent: true }); > } else > loadURI(submission.uri.spec, null, submission.postData, false); This code doesn't exist anymore, see bug 641090.

Comment on attachment 491270 [details] [diff] [review] v1.1: update for review comments >diff --git a/browser/components/sidebar/src/nsSidebar.js b/browser/components/sidebar/src/nsSidebar.js >- this.searchService.addEngine(aDescriptionURL, typeXML, iconURL, true); >+ Servcies.search.addEngine(aDescriptionURL, typeXML, iconURL, true); typo: Servcies There's a lot of bitrot here, but all of the changes look fine. I'll quickly review an updated patch (ideally one that's been tested :), sorry for taking forever.

Here's an unbitrotted version that I tested for the things I knew how to test (sidebars working, installation of search plugins working).

No time to watch the tree right now, so setting checkin-needed.

As for the checkin-needed, this has the user name and comment in a "hg export" format.