Closed
Bug 344159
Opened 18 years ago
Closed 18 years ago
Ensure FF2 search service interacts appropriately with CCK extensions for partner and corporate deployment
Categories
(Firefox :: Search, defect)
Tracking
()
RESOLVED
FIXED
Firefox 2 beta2
People
(Reporter: benjamin, Assigned: benjamin)
Details
(Keywords: fixed1.8.1)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
moco
:
review+
mconnor
:
approval1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
mconnor
:
approval1.8.1+
|
Details | Diff | Splinter Review |
In FF 1.5 the CCK (and extensions in general) can modify many aspects of search engine behavior, including changing the default search plugin, changing branding parameters, and reordering search plugins. We need to ensure that similar (and enhanced) capabilities are available in FF2.
Assignee | ||
Comment 1•18 years ago
|
||
Basil, can you post detailed requirements here?
Flags: blocking-firefox2?
Updated•18 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2 beta2
Comment 2•18 years ago
|
||
The product team has been using Michael Kaply's CCK Wizard (https://addons.mozilla.org/firefox/2553/) to generate an XPI that includes the branding and customization required for various distribution partners. Most (but not all) of the requirements listed below are available from the CCK Wizard.
With regard to search plugins in particular, the requirements are:
Defaults:
- Extensions (such as the XPI generated from the CCK Wizard) can include one or more search plugins
- Select a new default search engine (from the existing list packaged with the browser)
- Install a new search engine and select it as the new default search engine
- Even when adding new search engines, do not change the default search engine
Install/Uninstall/Ordering:
- Search engines can be added at a particular location (e.g. slot 2)
- Allow extensions to replace existing search engines (e.g. in order to point to a different URL or modify the URL parameters)
- Allow extensions to remove existing search engines
- When an extension is uninstalled, the search plugin will no longer be present
URL Parameters:
- Allow extensions to add URL parameters to existing search engine URL parameters
(For modifications to URL parameters, it is suggested to replace th entire search engine plugin, rather than patch the parameters)
Assignee | ||
Comment 3•18 years ago
|
||
> branding and customization required for various distribution partners. Most
> (but not all) of the requirements listed below are available from the CCK
> Wizard.
But not for FF2, which will require a new version of the CCK ;-)
Comment 4•18 years ago
|
||
Removing existing search plugins could be problematic...
Also, the reordering is tricky as well.
The only thing that comes to mind here is a way in Firefox to say "ignore the installed search engines" and allow the CCK to do everything?
Updated•18 years ago
|
Whiteboard: [at risk]
Updated•18 years ago
|
Whiteboard: [at risk] → [at serious risk]
Comment 5•18 years ago
|
||
Is there a timeline on when we'll build a CCK for Fx2?
-> Benjamin for now, please reassign to the appropriate people if there are in fact issues to resolve.
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → benjamin
Assignee | ||
Comment 6•18 years ago
|
||
Attachment #231607 -
Flags: review?
Assignee | ||
Updated•18 years ago
|
Attachment #231607 -
Flags: review? → review?(sspitzer)
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #231625 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 8•18 years ago
|
||
With these two patches we're at parity with FF1.5. Removal and "insertion" via prefs doesn't look that hard, I'll take a stab at that once these land.
Comment 9•18 years ago
|
||
I'd rather not have two prefs to change when localizers need to change the default. Can't you avoid adding another default localized pref by comparing this._currentEngine to this.defaultEngine before calling setLocalizedPref, and calling clearUserPref if they're the same? Something like:
if (this._currentEngine == this.defaultEngine) {
if (hasUserPref(selectedEnginePref))
clearUserPref(selectedEnginePref)
} else {
setLocalizedPref(...)
}
in the currentEngine setter.
Updated•18 years ago
|
OS: Linux → All
Hardware: PC → All
Version: unspecified → 2.0 Branch
Comment 10•18 years ago
|
||
Put more clearly: move the check for equality to the setLocalizedPref caller, to avoid adding a default for browser.search.selectedEngine, and compare engines instead of engine names, since you have easy access to both engines.
Assignee | ||
Comment 11•18 years ago
|
||
Attachment #231625 -
Attachment is obsolete: true
Attachment #231642 -
Flags: review?(gavin.sharp)
Attachment #231625 -
Flags: review?(gavin.sharp)
Comment 12•18 years ago
|
||
Comment on attachment 231642 [details] [diff] [review]
Don't set user prefs which match the defaults, rev. 2
>Index: browser/components/search/nsSearchService.js
>+ if (this._currentEngine.name == this.defaultEngine.name) {
Remove the |.name|s and just compare the objects directly (these are just references to the JS objects).
>+ }
>+ else {
This file uses huggy else brackets :) |} else {|
Attachment #231642 -
Flags: review?(gavin.sharp) → review+
Comment 13•18 years ago
|
||
Comment on attachment 231607 [details] [diff] [review]
Make extension searchplugins override app-installed, rev. 1
r=sspitzer, this looks reasonable to me.
note, I think that AppendObject() can fail (but if this code fails, and we are out of memory, we have bigger problems.)
note, according to lxr, many callers of AppendObject() don't seem to check the return value either.
what about making AppendFileKey() return a nsresult?
return array.AppendObject(file) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
up to you, ben.
Attachment #231607 -
Flags: review?(sspitzer) → review+
Assignee | ||
Comment 14•18 years ago
|
||
Yeah, I thought about failing out, but I figured it was better to return a partially complete list than fail altogether.
Assignee | ||
Comment 15•18 years ago
|
||
Both patches fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #231607 -
Flags: approval1.8.1?
Assignee | ||
Updated•18 years ago
|
Attachment #231642 -
Flags: approval1.8.1?
Comment 16•18 years ago
|
||
Comment on attachment 231607 [details] [diff] [review]
Make extension searchplugins override app-installed, rev. 1
a=mconnor on behalf of drivers for checkin to 1.8 branch for 1.8.1
Attachment #231607 -
Flags: approval1.8.1? → approval1.8.1+
Comment 17•18 years ago
|
||
Comment on attachment 231642 [details] [diff] [review]
Don't set user prefs which match the defaults, rev. 2
a=mconnor on behalf of drivers for checkin to 1.8 branch for 1.8.1
Attachment #231642 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 18•18 years ago
|
||
Both patches landed on MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Whiteboard: [at serious risk]
You need to log in
before you can comment on or make changes to this bug.
Description
•