444735 - move search service into toolkit

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

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?

Comment 1

[Neil Deakin]

Attachment: patch

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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

Comment 5

[Benjamin Smedberg]

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.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 7

[Benjamin Smedberg]

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

Comment 8

[Neil Deakin]

Gavin, which files need to be moved here?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 10

[Neil Deakin]

Attachment: updated patch

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

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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 12

[Neil Deakin]

Attachment: updated search service patch

Comment 13

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 14

[Neil Deakin]

Attachment: fixed issues [Checkin: Comment 15]

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

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 338143 [details] [diff] [review]
fixed issues
[Checkin: Comment 15]

http://hg.mozilla.org/mozilla-central/rev/b6c3d94c2f66