The three search service script files were moved to toolkit/components/search with 'hg rename' but they don't show up in the patch. In order for search to fully work, the dirprovider for searchplugins must also be provided. Thoughts on this? Move to toolkit, make it part of the main dir service, or duplicate this each time?

(In reply to comment #0) > In order for search to fully work, the dirprovider for searchplugins must also > be provided. Thoughts on this? Move to toolkit, make it part of the main dir > service, or duplicate this each time? > Moving it to toolkit sounds right. Kinda like the spellchecker stuff. Applications can extend the dirprovider if needed, but the toolkit will cover core searchproviders.

bsmedberg should approve, have you asked him about it? If we do this I think it should probably be enabled/disabled in configure/confvars.sh the same way MOZ_PLACES is handled (as opposed to the Thunderbird/SeaMonkey Makefile ifdefs in that patch).

bsmedberg: the motivation behind this is that we want to use the search service in fennec without forking it.

I'm a little skeptical of this: how decoupled are the search service backend and UI? I presume that Fennec only wants the backend and not the UI. Unless this is cleanly decoupled, I would prefer that this were not in toolkit, and is maintained as "forked" code. You can make exact copies of the code I think and even use comparison scripts to make sure it stays up to date, if that's desirable.

The backend is completely decoupled from the UI (the UI is in browser/components/search/content/, but is just a consumer of the service in browser/components/search/). Fennec just wants the backend, and that's all we're going to be moving, so this is essentially just an |hg mv| (modulo the dirprovider changes that will be required per comment 0).

In that case, and as long as the backend unit tests move with the code, I'm fine with it.

Gavin, which files need to be moved here?

Ryan's working on tests as part of bug 394979. I'm not sure when he's planning on having those ready - perhaps we should do this before taking that patch?

Moving the directory service isn't needed. The default is <appdir>/searchplugins.

Comment on attachment 337880 [details] [diff] [review] updated patch >diff --git a/browser/components/search/Makefile.in b/browser/components/search/Makefile.in > MODULE = browsersearch\n> XPIDL_MODULE = browsersearch This can be removed (and the reference to browsersearch.xpt in both packages-statics, too). > ifneq (,$(BUILD_OFFICIAL)$(MOZILLA_OFFICIAL)) > DEFINES += -DOFFICIAL_BUILD=1 > endif > > DEFINES += -DMOZ_DISTRIBUTION_ID=$(MOZ_DISTRIBUTION_ID) These are only used in the components, so you can remove them. >diff --git a/toolkit/components/search/Makefile.in b/toolkit/components/search/Makefile.in >+XPIDL_MODULE = toolkitsearch You'll need to add toolkitsearch.xpt to packages-static. We probably want to duplicate the browser directory provider that allows extensions to ship search plugins in Fennec, but that's less critical (file a followup?). I just noticed that nsBrowserSearchService depends on browser/locales/en-US/chrome/browser/search.properties, we'll need to split the strings it needs into a toolkit file (followup bug if you want). Ryan: beware of bitrot!

Comment on attachment 337950 [details] [diff] [review] updated search service patch >diff --git a/browser/locales/en-US/chrome/browser/search.properties b/browser/locales/en-US/chrome/browser/search.properties >-error_invalid_engine_title=Install Error >-# LOCALIZATION NOTE (error_invalid_engine_msg): %S = brandShortName >-error_invalid_engine_msg=This search engine isn't supported by %S and can't be installed. These are used in nsSidebar.js, so you'll have to update its reference to search.properties. You need to update SEARCH_BUNDLE in both search components as well, right?

Fixed search.properties paths and added file toolkit/components/Makefile.in that I missed including in the previous patch.

Comment on attachment 338143 [details] [diff] [review] fixed issues [Checkin: Comment 15] http://hg.mozilla.org/mozilla-central/rev/b6c3d94c2f66