Closed Bug 1040931 Opened 11 years ago Closed 10 years ago

API addition: add extensionID parameter to addEngineWithDetails

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: mconnor, Assigned: ttaubert)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

Because packed extensions aren't supported for bundled extensions, it's become common to use addEngineWithDetails as a workaround. However, this effectively means those engines won't be removed when the add-on is, which isn't the right user experience (and is a potential form of abuse). To manage this with minimal pain, we should add (and require by policy for now) that add-ons calling this method must also pass in their correct add-on ID. The search service should then watch for add-on notifications and remove added plugins if/when the plugin is removed or blocked. In addition, we should validate that the add-on ID is still present whenever we're regenerating search.json. Nominating for backlog based on discussion with Gavin, and cc-ing Jorge/Kris so they know this is coming. Probably won't happen in 33, but should ideally be in an early 34 iteration.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Points: --- → 3
QA Whiteboard: [qa-]
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 34.1
OS: Windows 8.1 → All
Hardware: x86_64 → All
I assume that the observer should be added in a separate bug? (In reply to Mike Connor [:mconnor] from comment #0)In addition, > we should validate that the add-on ID is still present whenever we're > regenerating search.json. How can we validate that a search engine in the JSON file should have an extension ID? AFAICT we keep all search engines (default and non-default ones) in search.json, no? Should we compare it to the list of default search engines? Or is search.json the list of engines we ship?
Attachment #8460778 - Flags: review?(mconnor)
Forgot to bump the UUID for nsIBrowserSearchService. Just imagine I did that and the next patch will include it.
(In reply to Tim Taubert [:ttaubert] from comment #1) > Created attachment 8460778 [details] [diff] [review] > 0001-Bug-1040931-Add-extensionID-parameter-to-addEngineWi.patch > > I assume that the observer should be added in a separate bug? Yeah, that makes sense to me. I can file it. > Should we compare it to the list of default search engines? Or is > search.json the list of engines we ship? search.json is a cache of all current search engines (to avoid scanning directories, I assume), it gets regenerated when the buildID changes (maybe other times). I'll include the behaviour in the followup bug.
Blocks: 1042938
Comment on attachment 8460778 [details] [diff] [review] 0001-Bug-1040931-Add-extensionID-parameter-to-addEngineWi.patch We should have some test coverage that this attribute gets serialized/deserialized correctly (both in the .xml and in the json cache). r=me with that. I think the next steps here are: a) validating the passed-in extension ID (i.e. checking that it corresponds to a currently installed add-on, if specified) b) bug 1042938 c) code that uninstalls/removes any engines whose associated add-ons are no longer installed when the cache is rebuilt (i.e. on upgrade, in the common case) I think b)/c) can be in followups, but a) should probably land along side this patch (whether or not it gets a new bug).
Attachment #8460778 - Flags: review?(mconnor) → review+
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #4) > We should have some test coverage that this attribute gets > serialized/deserialized correctly (both in the .xml and in the json cache). > r=me with that. I'm not at all familiar with the search code and its tests. I managed to write a test that checks that extensionID ends up in search.json but I have no idea how to convince the search service to read from that file after it has already been initialized. I couldn't find any tests for the .xml files either (I think).
(In reply to Tim Taubert [:ttaubert] from comment #5) > I'm not at all familiar with the search code and its tests. I managed to > write a test that checks that extensionID ends up in search.json but I have > no idea how to convince the search service to read from that file after it > has already been initialized. I couldn't find any tests for the .xml files > either (I think). For search.json, you should be able to just tweak test_json_cache.js (and toolkit/components/search/tests/xpcshell/data/search.json) to test both reading and writing that property from the cache. For testing the XML serialization, you should be able to copy test_serialize_file.js but extend it to use addEngineWithDetails, and then actually check the XML for the attribute, rather than just checking that the file exists.
Iteration: 34.1 → 34.2
If this is just blocked by testing concerns, those can be split into another bug.
Flags: needinfo?(ttaubert)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #7) > If this is just blocked by testing concerns, those can be split into another > bug. Sorry, this slipped off my radar for too long. (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6) > For search.json, you should be able to just tweak test_json_cache.js (and > toolkit/components/search/tests/xpcshell/data/search.json) to test both > reading and writing that property from the cache. Thanks for the hints, did that and caught a bug (yay). Will file a follow-up to test XML serialization. https://hg.mozilla.org/integration/fx-team/rev/836c52cc3c05
Flags: needinfo?(ttaubert)
Depends on: 1055492
Had to back out unfortunately because I didn't push to try before: https://hg.mozilla.org/integration/fx-team/rev/f39a5425d95d
Had to adapt a few addEngineWithDetails() calls.
Attachment #8475230 - Flags: review?(gavin.sharp)
Iteration: 34.2 → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Comment on attachment 8475230 [details] [diff] [review] 0002-Bug-1040931-Fix-places-that-call-addEngineWithDetail.patch Hrm, why didn't we actually make this optional in IDL? It's confusing to have a comment that says "optional" but having the parameter not actually be. I could see argument for both ways, actually...
Comment on attachment 8475230 [details] [diff] [review] 0002-Bug-1040931-Fix-places-that-call-addEngineWithDetail.patch I think we should make this an optional parameter for now in IDL.
Attachment #8475230 - Flags: review?(gavin.sharp) → review-
Another push to try with the parameter being optional: https://tbpl.mozilla.org/?tree=Try&rev=4f181f92ca70
Attachment #8475230 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
This was never added to the docs.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: