Closed Bug 848420 Opened 7 years ago Closed 7 years ago

Distribution support for additional default search engines

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(2 files, 1 obsolete file)

Desktop appears to already do this, probably by looking for search engine XML files in the distribution folder. We should look into that and do something similar.
Assignee: nobody → margaret.leibovic
Attached patch WIP (obsolete) — Splinter Review
This ports over the logic from desktop's DirectoryProvider.cpp, except I omitted the logic for extension search plugin locations, since I don't think we have those (and logic for that isn't in the core implementation [1] anyway).

I just have to add some logic to deal with the fact that the distribution might be in a system location. Right now in browser.js we deal with this by passing the system distribution path along from Java, but maybe there's a way we can avoid that. Is there a way to get at the app id from JS, or maybe I should set a pref with the path when I get it in browser.js?

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsAppFileLocationProvider.cpp#547
Attachment #725192 - Flags: feedback?(mark.finkle)
Comment on attachment 725192 [details] [diff] [review]
WIP

Looks good. Nice and clean. We could use your pref idea to set something like "distribution.searchplugins.location".

How would it work? 

We still need to always look in the NS_XPCOM_CURRENT_PROCESS_DIR rooted folder first, in case local overrides exist (like we do in Distribution.java). But if it does not and "distribution.searchplugins.location" exists, we have a /system distribution?

If we knew the "org.mozilla.xxx" part we could just built it here. On second thought, maybe keeping that logic in Distribution.java is best, and sending out the /system path to browser.js, then a pref is the better approach.
Attachment #725192 - Flags: feedback?(mark.finkle) → feedback+
Attached patch patchSplinter Review
I decided to store a pref to the distribution path if we passed a custom one from Java, and this seems to work well.

I tested the following configurations:
1) no distribution (good to make sure this still works ;)
2) distribution in APK
3) distribution in /system 
4) different distributions in APK and /system (the one in the APK wins)
Attachment #725192 - Attachment is obsolete: true
Attachment #725502 - Flags: review?(mark.finkle)
Attachment #725502 - Flags: review?(mark.finkle) → review+
Simple idea for a test:
1. Add a simple, non-functional "search engine" XML file to the distribution folder
2. Use the JSON "SearchEngines:Get" message to pull the list of search engines
3. Look for the new engine in the list
Attached patch testSplinter Review
Attachment #726299 - Flags: review?(gbrown)
Comment on attachment 726299 [details] [diff] [review]
test

LGTM
Attachment #726299 - Flags: review?(gbrown) → review+
https://hg.mozilla.org/mozilla-central/rev/f49cd7c9b766
https://hg.mozilla.org/mozilla-central/rev/7dafe64aa7f7
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Depends on: 1091728
You need to log in before you can comment on or make changes to this bug.