Closed
Bug 1040931
Opened 11 years ago
Closed 10 years ago
API addition: add extensionID parameter to addEngineWithDetails
Categories
(Firefox :: Search, defect)
Firefox
Search
Tracking
()
People
(Reporter: mconnor, Assigned: ttaubert)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 1 obsolete file)
7.69 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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?
Updated•11 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Updated•11 years ago
|
Points: --- → 3
Updated•11 years ago
|
QA Whiteboard: [qa-]
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 34.1
OS: Windows 8.1 → All
Hardware: x86_64 → All
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
Forgot to bump the UUID for nsIBrowserSearchService. Just imagine I did that and the next patch will include it.
Reporter | ||
Comment 3•11 years ago
|
||
(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.
Comment 4•11 years ago
|
||
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+
Assignee | ||
Comment 5•11 years ago
|
||
(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).
Comment 6•11 years ago
|
||
(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.
Updated•11 years ago
|
Iteration: 34.1 → 34.2
Comment 7•11 years ago
|
||
If this is just blocked by testing concerns, those can be split into another bug.
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 8•10 years ago
|
||
(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)
Assignee | ||
Comment 9•10 years ago
|
||
Had to back out unfortunately because I didn't push to try before:
https://hg.mozilla.org/integration/fx-team/rev/f39a5425d95d
Assignee | ||
Comment 10•10 years ago
|
||
Had to adapt a few addEngineWithDetails() calls.
Attachment #8475230 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 11•10 years ago
|
||
Updated•10 years ago
|
Iteration: 34.2 → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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-
Assignee | ||
Comment 14•10 years ago
|
||
Another push to try with the parameter being optional:
https://tbpl.mozilla.org/?tree=Try&rev=4f181f92ca70
Assignee | ||
Updated•10 years ago
|
Attachment #8475230 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in
before you can comment on or make changes to this bug.
Description
•