Closed Bug 1359538 Opened 8 years ago Closed 7 years ago

Provide an API for getting engines by extension ID

Categories

(Firefox :: Search, enhancement)

Product:
Firefox
Component:
Search
Version:
51 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

Details

Attachments

(1 file)

Bug 1359538 - Add API to retrieve engines by extension ID.
59 bytes, text/x-review-board-request
florian
: review+
Details 
Bug 1042938 isn't going to be done any time soon, but in researching work for WebExtensions, it would be very useful to simply query the engines that were added by an extension (assuming an extension ID was passed in).

We whould add that.
Comment on attachment 8862025 [details]
Bug 1359538 - Add API to retrieve engines by extension ID.

https://reviewboard.mozilla.org/r/133996/#review138616

The code looks reasonable, but I'm curious about how you intend to use this. This method relies on the search service being already initialized, or will initialize it synchronously. So you can't expect to call this during the add-on manager startup.

::: netwerk/base/nsIBrowserSearchService.idl:10
(Diff revision 1)
>  #include "nsISupports.idl"
>  
>  interface nsIURI;
>  interface nsIInputStream;
>  
> -[scriptable, uuid(5799251f-5b55-4df7-a9e7-0c27812c469a)]
> +[scriptable, uuid(d4754b72-e403-476a-b777-e5578ff5cf9c)]

I think we no longer rev uuids these days.

::: netwerk/base/nsIBrowserSearchService.idl:491
(Diff revision 1)
> +  /**
> +   * Returns an array of search engines installed by a given extension.
> +   *
> +   * @returns an array of nsISearchEngine objects.
> +   */
> +  void getEnginesByExtensionID(in AString extensionID,

This should go next to similar methods, eg after 'getEngines'.
> The code looks reasonable, but I'm curious about how you intend to use this. This method relies on the search service being already initialized, or will initialize it synchronously. So you can't expect to call this during the add-on manager startup.

I thought I replied to this. Sorry.

We're using it at add-on install, not add-on manager startup. So it should be initialized. I don't see any way around the initialization problem if it did happen at startup. Do you?
(In reply to Mike Kaply [:mkaply] from comment #3)
> > The code looks reasonable, but I'm curious about how you intend to use this. This method relies on the search service being already initialized, or will initialize it synchronously. So you can't expect to call this during the add-on manager startup.
> 
> I thought I replied to this. Sorry.
> 
> We're using it at add-on install, not add-on manager startup. So it should
> be initialized. I don't see any way around the initialization problem if it
> did happen at startup. Do you?

You could wait for a promise of the search service being initialized, and load the add-on asynchronously (ie. not during startup). That should be fine as webextension add-ons are restartless, right?
> You could wait for a promise of the search service being initialized, and load the add-on asynchronously (ie. not during startup). That should be fine as webextension add-ons are restartless, right?

Yes, although I realized this will primarily be used in shutdown not startup.

I've attached a new patch addressing your comments.
Comment on attachment 8862025 [details]
Bug 1359538 - Add API to retrieve engines by extension ID.

https://reviewboard.mozilla.org/r/133996/#review145100

r=me because the code here looks good, but I would still like to understand how you can do something useful with this if you only call it during shutdown (it would be preferable to avoid removing search engines during shutdown, as that would block shutdown until the search service cache has been rewritten to disk).

::: toolkit/components/search/tests/xpcshell/test_addEngineWithExtensionID.js:11
(Diff revision 2)
> +const kSearchEngineID = "addEngineWithDetails_test_engine";
> +const kSearchEngineURL = "http://example.com/?search={searchTerms}";
> +const kSearchTerm = "foo";
> +const kExtensionID = "extension@mozilla.com";
> +
> +add_task(function* test_addEngineWithExtensionID() {

async function

::: toolkit/components/search/tests/xpcshell/test_addEngineWithExtensionID.js:14
(Diff revision 2)
> +const kExtensionID = "extension@mozilla.com";
> +
> +add_task(function* test_addEngineWithExtensionID() {
> +  do_check_false(Services.search.isInitialized);
> +
> +  yield asyncInit();

await
Attachment #8862025 - Flags: review?(florian) → review+
> (it would be preferable to avoid removing search engines during shutdown, as that would block shutdown until the search service cache has been rewritten to disk).

It's shutdown of the extension, not shutdown of the browser (unless somehow the add-on was removed in a way that only goes away at shutdown).
(In reply to Mike Kaply [:mkaply] from comment #8)
> > (it would be preferable to avoid removing search engines during shutdown, as that would block shutdown until the search service cache has been rewritten to disk).
> 
> It's shutdown of the extension, not shutdown of the browser (unless somehow
> the add-on was removed in a way that only goes away at shutdown).

How do you handle search plugins from blocklisted add-ons, or add-ons that are gone for another unrelated reason at the next startup?
> How do you handle search plugins from blocklisted add-ons, or add-ons that are gone for another unrelated reason at the next startup?

The WebExtensions team has said that extension shutdown would be called in these cases. We would have these same problems for any other WebExtensions specific modifications (change homepage, new tab, etc).
Comment on attachment 8862025 [details]
Bug 1359538 - Add API to retrieve engines by extension ID.

https://reviewboard.mozilla.org/r/133996/#review138616

It will be used during the upgrade of an extension (where search should already be intialized) or during shutdown of an extension.

It's primarily for finding the search engines so that we can uninstall them. This is a simpler solution than trying to fix 1042938 for now.

> I think we no longer rev uuids these days.

I was wondering about that. I'll check.
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/fa0107fba537
Add API to retrieve engines by extension ID. r=florian
https://hg.mozilla.org/mozilla-central/rev/fa0107fba537
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: