move search service into toolkit

RESOLVED FIXED in mozilla1.9.1b1

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Neil Deakin (away until Feb 4), Assigned: Neil Deakin (away until Feb 4))

Tracking

Trunk
mozilla1.9.1b1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
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?
(Assignee)

Comment 1

10 years ago
Created attachment 329060 [details] [diff] [review]
patch
Attachment #329060 - Flags: review?(gavin.sharp)
(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.

Comment 5

10 years ago
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).

Comment 7

10 years ago
In that case, and as long as the backend unit tests move with the code, I'm fine with it.
(Assignee)

Comment 8

10 years ago
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?
(Assignee)

Comment 10

10 years ago
Created attachment 337880 [details] [diff] [review]
updated patch

Moving the directory service isn't needed. The default is <appdir>/searchplugins.
Attachment #329060 - Attachment is obsolete: true
Attachment #337880 - Flags: review?(gavin.sharp)
Attachment #329060 - Flags: review?(gavin.sharp)
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!
Attachment #337880 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 12

10 years ago
Created attachment 337950 [details] [diff] [review]
updated search service patch
Attachment #337880 - Attachment is obsolete: true
Attachment #337950 - Flags: review?(gavin.sharp)
Attachment #337950 - Flags: review?(gavin.sharp) → review+
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?
(Assignee)

Comment 14

10 years ago
Created attachment 338143 [details] [diff] [review]
fixed issues
[Checkin: Comment 15]

Fixed search.properties paths and added file toolkit/components/Makefile.in that I missed including in the previous patch.
Attachment #337950 - Attachment is obsolete: true
Attachment #338143 - Flags: review?(gavin.sharp)
Attachment #338143 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 338143 [details] [diff] [review]
fixed issues
[Checkin: Comment 15]

http://hg.mozilla.org/mozilla-central/rev/b6c3d94c2f66
Attachment #338143 - Attachment description: fixed issues → fixed issues [Checkin: Comment 15]
Blocks: 410613
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1b1
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.