Support chrome_settings_overrides search engine

NEW
Assigned to
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Untriaged
P2
enhancement
8 months ago
4 days ago

People

(Reporter: Arunkumar, Assigned: mkaply, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged, URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/601.5.17 (KHTML, like Gecko) Version/9.1 Safari/601.5.17

Steps to reproduce:

The webextension dont have have support to manifest  API to use default search Engine 

"chrome_settings_overrides": {
    "search_provider": {
     "name": "DefaultSearch,
     "search_url": "https://www.google.co.in/search?q={searchTerms}",
     "prepopulated_id" : 445,
     "encoding": "UTF-8",
     "is_default": true
    }



Reference : https://developer.chrome.com/extensions/settings_override
(Reporter)

Updated

8 months ago
Severity: normal → critical
Priority: -- → P1

Updated

8 months ago
Severity: critical → enhancement
Priority: P1 → --
(Reporter)

Updated

8 months ago
Summary: Supports chrome_settings_overrides api in mozilla WebExtension → Supports chrome_settings_overrides api in mozilla WebExtension to change default search engine

Updated

8 months ago
Whiteboard: [design-decision-needed]triaged

Updated

8 months ago
Priority: -- → P2
Blocks: 1303905
(Assignee)

Comment 1

6 months ago
In addition to the Chrome API, we should support some additional parameter that points to one or more XML files.

We already have partners that have significant investments in opensearch XML, and we shouldn't require them to switch to this API.

In addition, this API doesn't cover functionality that we have in our opensearch (custom parameters and things).
Can we get an outline of what is not possible with the search provider settings?

It does seem it should be easy to support "opensearch": /path/to/search.xml in the overrides, but it may also be good to add more parameters to support options we need.  That may make the search engine extensions largely compatible.
Flags: needinfo?(mozilla)
(Assignee)

Comment 3

6 months ago
> Can we get an outline of what is not possible with the search provider settings?

1. Any MozParam specific stuff. So:
  a) Using a preference to fill in a search value.
  b) Having a different value based on the source of the search (keyword, searchbar, contextmenu, etc).

2. Separate URL for when you click on the icon without a search.

3. Different URL types for different searches:
https://dxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/searchplugins/bing.xml4. 

4. Description

We also use the filename as an internal identifier, so we'll have to figure out how to make that work (google, bing, ddg). It doesn't always match the lowercased search name.
Flags: needinfo?(mozilla)
The only thing I think couldn't be supported are the preferences for MozParam, since preferences wouldn't be supported in webextensions.  This could be addressed for distributions, but not for distribution via AMO.  Is MozParam used outside of distributions?
(Assignee)

Comment 5

6 months ago
> Is MozParam used outside of distributions?

It's used for source value of searches for extensions that install engines (web.de for example). I think Yahoo does as well.

Comment 6

6 months ago
We we need is:
* A way for Firefox to automatically add 1 or more search engines.
  * Currently, in the XPI, these are OSDs in the XPI directory searchplugins/ and are installed, uninstalled and deactivated automatically together with the extension. Particularly the uninstall/deactivation according to AMO rules proved problematic when doing this manually.)
  * Features should be similar to an OSD, particularly with icon (shipped as part of the ext) etc.
* A way for our extension code to programmatically set and unset one of the search engines as default engine.
  * We will present our own UI for the user to opt-in/out of it, as part of the setup dialog. We need this to be integrated with the rest of our UI, not a separate Firefox dialog (dialog fatigue).
  * I don't know whether this would covered by this bug or another one.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

4 months ago
As Qwant partnering closely with Mozilla, we are looking for this bug to move forward. Our current extension is not compatible with Electrolysis and this bug is missing for us to migrate our extension to WebExtensions. Any chance to prioritize this as high ?
(In reply to david from comment #7)
> As Qwant partnering closely with Mozilla, we are looking for this bug to
> move forward. Our current extension is not compatible with Electrolysis and
> this bug is missing for us to migrate our extension to WebExtensions. Any
> chance to prioritize this as high ?

Qwant shouldn't be blocked by this. In the interim use a hybrid addon[1] with the bootstrap providing the override[2].  This way you can migrate your addon to webextension while still keep functionality that is not yet finished.  The hybrid approach also gives you a way to migrate any local data you may have.

[1]https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Embedded_WebExtensions
[2]https://github.com/mixedpuppy/newtaboverride/

Comment 9

4 months ago
ni? kev as the search engine expert
Flags: needinfo?(kev)
(Reporter)

Comment 10

3 months ago
In chrome the  API chrome_settings_overrides wont change the preference , if the extension installed then  it just overrides the search url to the url specified in manifest of the extension.
(Assignee)

Updated

3 months ago
Blocks: 1333828
(Assignee)

Comment 11

3 months ago
One more thought on this.

We'll need to properly migrate an add-on that had search plugins as part of that add-on. So if an add-on installed OSD files and those were made default for the user, when going to a WebExtension, it should be seamless.

Updated

3 months ago
Duplicate of this bug: 1334232

Comment 13

2 months ago
Is this a dupe of bug 1268401?
(Assignee)

Comment 14

2 months ago
No, there are two separate things.

There is supporting the chrome_settings_overrides in manifest.json as is, and then there is creating additional APIs to address the things that need to be done with search (which is bug 1268401).

As a side note, we really need to get some movement on these. This is going to be major for partner migration.
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 months ago
This is step one which is just getting as close to chrome parity as we can. A lot of the things in the chrome manifest we don't support. Next I'm going to add support for putting engines into the XPI.

Comment 17

2 months ago
mozreview-review
Comment on attachment 8840090 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/114628/#review116132

Initial feedback.

::: browser/components/extensions/ext-chrome-settings-overrides.js:11
(Diff revision 1)
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");
> +
> +let gAlias = null;

Use a weakmap here, similar to what was done in toolkit/components/extensions/ext-idle.js

::: browser/components/extensions/ext-chrome-settings-overrides.js:17
(Diff revision 1)
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_chrome_settings_overrides", (type, directive, extension, manifest) => {
> +  if ("search_provider" in manifest.chrome_settings_overrides) {
> +    let searchProvider = manifest.chrome_settings_overrides.search_provider;
> +    if (!("name" in searchProvider)) {

These required checks are better done in the schema.  As long as you dont have optional: true the extension should fail to install without required values.

::: browser/components/extensions/ext-chrome-settings-overrides.js:28
(Diff revision 1)
> +    if (!("search_url" in searchProvider)) {
> +        extension.manifestError("search_url is required in search_provider");
> +    }
> +    gAlias = searchProvider.keyword;
> +    let engine = Services.search.getEngineByAlias(gAlias);
> +    if (engine) {

This should throw an error rather than overwriting an existing engine.

::: browser/components/extensions/ext-chrome-settings-overrides.js:38
(Diff revision 1)
> +      }
> +    }
> +    let method = "search_url_post_params" in searchProvider ? "POST" : "GET";
> +    let iconURL = "favicon_url" in searchProvider ? searchProvider.favicon_url : null;
> +    try {
> +      Services.search.addEngineWithDetails(searchProvider.name, iconURL, gAlias, null, method, searchProvider.search_url);

Will this overwrite existing engines with the same name?  Should we do some extra checking here to ensure there are not duplicate names?

::: browser/components/extensions/ext-chrome-settings-overrides.js:39
(Diff revision 1)
> +    }
> +    let method = "search_url_post_params" in searchProvider ? "POST" : "GET";
> +    let iconURL = "favicon_url" in searchProvider ? searchProvider.favicon_url : null;
> +    try {
> +      Services.search.addEngineWithDetails(searchProvider.name, iconURL, gAlias, null, method, searchProvider.search_url);
> +      if ("is_default" in searchProvider &&

Lets split this out for now so we can move forward.

::: browser/components/extensions/schemas/chrome_settings_overrides.json:17
(Diff revision 1)
> +              "search_provider": {
> +                "type": "object",
> +                "optional": true,
> +                "properties": {
> +                  "name": {
> +                    "type": "string",

I'm thinking we could use a regex "pattern" here to ensure some of the crazy stuff is not done (e.g. unicode space at end of name, bug 1154835)

::: browser/components/extensions/schemas/chrome_settings_overrides.json:41
(Diff revision 1)
> +                    "type": "string",
> +                    "format": "url",
> +                    "optional": true,
> +                    "preprocess": "localize"
> +                  },
> +                  "is_default": {

Add "unsupported": true when splitting out isDefault support.
(Assignee)

Updated

2 months ago
Assignee: nobody → mozilla

Comment 18

2 months ago
mozreview-review
Comment on attachment 8840090 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/114628/#review121644

till the next version.
Attachment #8840090 - Flags: review?(mixedpuppy) → review-
Comment hidden (mozreview-request)

Comment 20

2 months ago
mozreview-review
Comment on attachment 8840090 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/114628/#review122216

In reviewboard, can you fix or discard issues from the prior review (and this one) so I can see what is fixed or what you feel shouldn't be done (and why)?

::: browser/components/extensions/ext-chrome-settings-overrides.js:22
(Diff revision 2)
>                                             manifest.chrome_settings_overrides.homepage);
>    }
> +  if ("search_provider" in manifest.chrome_settings_overrides) {
> +    let searchProvider = manifest.chrome_settings_overrides.search_provider;
> +    let method = "search_url_post_params" in searchProvider ? "POST" : "GET";
> +    let iconURL = "favicon_url" in searchProvider ? searchProvider.favicon_url : null;

if is optional, it should already be null if not provided.

::: browser/components/extensions/ext-chrome-settings-overrides.js:24
(Diff revision 2)
> +  if ("search_provider" in manifest.chrome_settings_overrides) {
> +    let searchProvider = manifest.chrome_settings_overrides.search_provider;
> +    let method = "search_url_post_params" in searchProvider ? "POST" : "GET";
> +    let iconURL = "favicon_url" in searchProvider ? searchProvider.favicon_url : null;
> +    try {
> +      Services.search.addEngineWithDetails(searchProvider.name, iconURL,

Are we going to deal with "similar" name issues?

::: browser/components/extensions/ext-chrome-settings-overrides.js:25
(Diff revision 2)
> +    let searchProvider = manifest.chrome_settings_overrides.search_provider;
> +    let method = "search_url_post_params" in searchProvider ? "POST" : "GET";
> +    let iconURL = "favicon_url" in searchProvider ? searchProvider.favicon_url : null;
> +    try {
> +      Services.search.addEngineWithDetails(searchProvider.name, iconURL,
> +                                           searchProvider.keyword + "_" + extension.id, null,

`${searchProvider.keyword}_${extension.id}`  dito where used again

::: browser/components/extensions/ext-chrome-settings-overrides.js:27
(Diff revision 2)
> +    let iconURL = "favicon_url" in searchProvider ? searchProvider.favicon_url : null;
> +    try {
> +      Services.search.addEngineWithDetails(searchProvider.name, iconURL,
> +                                           searchProvider.keyword + "_" + extension.id, null,
> +                                           method, searchProvider.search_url);
> +      engine = Services.search.getEngineByAlias(searchProvider.keyword);

where is engine used?  how does searchProvider.keyword work in this call? since it doesn't seem to be added in the prior call.
Attachment #8840090 - Flags: review?(mixedpuppy) → review-
(Assignee)

Comment 21

2 months ago
> Are we going to deal with "similar" name issues?

I think I should do a very basic trim to avoid trailing spaces, but since WebExtensions won't be able to hide engines, I don't think this is as serious a problem as it in regular extensions.

In addition, work is going on in regular Firefox to solve this problem.
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 months ago
I've made some other changes in the new patch.

1. Removed support for post_params. What chrome does and what we could do aren't compatible. I'll be providing a way to use XML files directly in firefox.

2. Added code to trim spaces plus a test.

3. Other general code cleanup (unnecessary if statements).

4. Allowed for additional parameters so we don't fail.

Comment 24

2 months ago
mozreview-review
Comment on attachment 8840090 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/114628/#review122550

::: browser/components/extensions/schemas/chrome_settings_overrides.json
(Diff revisions 2 - 3)
>                    "search_url": {
>                      "type": "string",
>                      "format": "url",
>                      "preprocess": "localize"
>                    },
> -                  "search_url_post_params": {

If this is part of the documented api, you should leave it but add unsupported: true
Attachment #8840090 - Flags: review?(mixedpuppy) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 months ago
I'm learning as I go here.

1. I went with deprecated because I could add a message. onerror: warn didn't work.
2. My understanding of keyword/alias was wrong. It's supposed to work like a search engine keyword in Firefox (where the user types it). So we can't use it as a unique identifier.
3. There's really no way to do a unique identifier here at all.

I thought about during shutdown trying to make sure that the engine we got was the one we added, but it's non trivial and I'm not sure how important it is. We'd have to create a fake search submission and take apart the original URL for the search.
(In reply to Mike Kaply [:mkaply] from comment #26)
> I'm learning as I go here.
> 
> 1. I went with deprecated because I could add a message. onerror: warn
> didn't work.

sgtm

> 2. My understanding of keyword/alias was wrong. It's supposed to work like a
> search engine keyword in Firefox (where the user types it). So we can't use
> it as a unique identifier.
> 3. There's really no way to do a unique identifier here at all.

We can add optional properties beyond the API that google uses, so if it's useful for us, lets do that.

> I thought about during shutdown trying to make sure that the engine we got
> was the one we added

use a weakmap, we do this in several api implementations: 

// WeakMap[Extension -> engine]
let engineMap = new WeakMap()
engineMap.set(extension, engine);

Updated

2 months ago
Summary: Supports chrome_settings_overrides api in mozilla WebExtension to change default search engine → Support chrome_settings_overrides api in mozilla WebExtension to change default search engine
Comment hidden (mozreview-request)

Comment 29

2 months ago
mozreview-review
Comment on attachment 8840090 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/114628/#review122662

::: browser/components/extensions/ext-chrome-settings-overrides.js:49
(Diff revision 5)
> +    }
> +  }
> +});
> +
> +extensions.on("shutdown", (type, extension) => {
> +  if ("search_provider" in extension.manifest.chrome_settings_overrides) {

isn't this always true since search_provider is null if one wasn't supplied?

::: browser/components/extensions/ext-chrome-settings-overrides.js:50
(Diff revision 5)
> +  }
> +});
> +
> +extensions.on("shutdown", (type, extension) => {
> +  if ("search_provider" in extension.manifest.chrome_settings_overrides) {
> +    let searchProvider = extension.manifest.chrome_settings_overrides.search_provider;

looks like searchProvider is unused

::: browser/components/extensions/schemas/chrome_settings_overrides.json:35
(Diff revision 5)
> +                    "optional": true,
> +                    "preprocess": "localize"
> +                  },
> +                  "search_url": {
> +                    "type": "string",
> +                    "format": "url",

I think we should enforce https here

::: browser/components/extensions/schemas/chrome_settings_overrides.json:41
(Diff revision 5)
> +                    "preprocess": "localize"
> +                  },
> +                  "favicon_url": {
> +                    "type": "string",
> +                    "optional": true,
> +                    "format": "url",

also https here, maybe less important

::: browser/components/extensions/schemas/chrome_settings_overrides.json:101
(Diff revision 5)
> +                    "deprecated": "Unsupported on Firefox."
> +                  },
> +                  "is_default": {
> +                    "type": "boolean",
> +                    "optional": true,
> +                    "unsupported": true

Should this use deprected instead of unsupported?

::: browser/components/extensions/test/browser/browser_ext_settings_overrides_search.js:46
(Diff revision 5)
> +  });
> +
> +  yield ext1.startup();
> +
> +  let engine = Services.search.getEngineByName("MozSearch");
> +  isnot(engine, null, "Engine doesn't exist");

text here seems backwards, a PASS here should be engine exists.  dito on the following test, also seems backwards.  actually dito in the above test as well.
Attachment #8840090 - Flags: review?(mixedpuppy) → review-
Comment hidden (mozreview-request)

Comment 31

a month ago
mozreview-review
Comment on attachment 8840090 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/114628/#review124052

::: browser/components/extensions/ext-chrome-settings-overrides.js:30
(Diff revision 6)
> +                                           searchProvider.keyword, null,
> +                                           "GET", searchProvider.search_url);
> +      let engine = Services.search.getEngineByName(searchProvider.name.trim());
> +      searchEngineMap.set(extension, engine);
> +    } catch (e) {
> +      // It's possible we got restarted but we don't have a reference to our

IIRC via IRC a test was going to be added to check this block.
Attachment #8840090 - Flags: review?(mixedpuppy) → review-

Comment 32

a month ago
mozreview-review
Comment on attachment 8840090 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/114628/#review124858

::: browser/components/extensions/ext-chrome-settings-overrides.js:26
(Diff revision 6)
> +  if (manifest.chrome_settings_overrides.search_provider) {
> +    let searchProvider = manifest.chrome_settings_overrides.search_provider;
> +    try {
> +      Services.search.addEngineWithDetails(searchProvider.name.trim(), searchProvider.favicon_url,
> +                                           searchProvider.keyword, null,
> +                                           "GET", searchProvider.search_url);

This should provide the extension ID parameter of addEngineWithDetails. And we should change the addEngineWithDetails implementation in the search service to include the extension id inside the loadpath.
(Assignee)

Comment 33

a month ago
(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> Comment on attachment 8840090 [details]
> Bug 1301315 - Add support for chrome_settings_overrides/search_provider.
> 
> https://reviewboard.mozilla.org/r/114628/#review124858
> 
> ::: browser/components/extensions/ext-chrome-settings-overrides.js:26
> (Diff revision 6)
> > +  if (manifest.chrome_settings_overrides.search_provider) {
> > +    let searchProvider = manifest.chrome_settings_overrides.search_provider;
> > +    try {
> > +      Services.search.addEngineWithDetails(searchProvider.name.trim(), searchProvider.favicon_url,
> > +                                           searchProvider.keyword, null,
> > +                                           "GET", searchProvider.search_url);
> 
> This should provide the extension ID parameter of addEngineWithDetails. And
> we should change the addEngineWithDetails implementation in the search
> service to include the extension id inside the loadpath.

We should really get the addEngineWithDetails/extension ID stuff implemented. Then we wouldn't need the code I'm adding.

That being said, for loadpath, are you thinking:

if (aExtensionID) {
engine._loadPath = "[" + aExtensionID + "]addEngineWithDetails";

Mike
(In reply to Mike Kaply [:mkaply] from comment #33)

> We should really get the addEngineWithDetails/extension ID stuff
> implemented. Then we wouldn't need the code I'm adding.

Do you mean code to remove automatically search plugins that were installed by add-ons the user has disabled/uninstalled? (ie. bug 1042938)

In comment 32 I was only thinking about ensuring our telemetry environment data is correct.

> That being said, for loadpath, are you thinking:
> 
> if (aExtensionID) {
> engine._loadPath = "[" + aExtensionID + "]addEngineWithDetails";

I would prefer the string between [] to remain a constant, so I would like something like:
[extension]addEngineWithDetails:" + aExtensionID
(Assignee)

Comment 35

a month ago
If we get bug 1042938 fixed, this code becomes incredibly simple and much more reliable.

So I'm going to focus on getting that one in and then finish this up.
Depends on: 1042938
> Do you mean code to remove automatically search plugins that were installed
> by add-ons the user has disabled/uninstalled? (ie. bug 1042938)


(In reply to Mike Kaply [:mkaply] from comment #35)
> If we get bug 1042938 fixed, this code becomes incredibly simple and much
> more reliable.
> 
> So I'm going to focus on getting that one in and then finish this up.

I'm not sure what the point of bug 1042938 is in a webextension-only world, and at that point, it feels less obtuse to me to have the remove on uninstall/upgrade/disable/etc in the same place as the install of the search engine.
(Assignee)

Comment 37

a month ago
> I'm not sure what the point of bug 1042938 is in a webextension-only world, and at that point, it feels less obtuse to me to have the remove on uninstall/upgrade/disable/etc in the same place as the install of the search engine.

Even in a Webextensions world, it makes more sense to have the search engine manager manage the relationship.

Then we don't have to worry about any sync issues between extension ID, engine, etc. Because the search engine manager has that information.

So we add an engine with an extension ID, and the search engine manager knows about it and does the right thing on enable, disable and uninstall.

It ends up much cleaner in the Webextensions code (and avoids my hack to try to find the engine if we somehow end up out of sync).
removing design-decision-needed, I think this belongs with Kev/PM not with the community group.
Whiteboard: [design-decision-needed]triaged → triaged

Comment 39

10 days ago
Is there a place where users can track the progress of this task? In our effort to convert addons to WebExtensions, this is one of the capability that is missing for us to have equivalent features. Thanks!

Comment 40

10 days ago
(In reply to Francois from comment #39)
> Is there a place where users can track the progress of this task? In our
> effort to convert addons to WebExtensions, this is one of the capability
> that is missing for us to have equivalent features. Thanks!

Same here. Would be great to see progress of this task.

Comment 41

10 days ago
I have been trying to get status update for this as well. It will be great if we can establish some timelines.
ping @kaply @flo, this bug feels quite stalled now...
Flags: needinfo?(mozilla)
Flags: needinfo?(florian)
Summary: Support chrome_settings_overrides api in mozilla WebExtension to change default search engine → Support chrome_settings_overrides search engine
(Assignee)

Comment 43

10 days ago
The reason the bug is stalled is because of the amount of work required for bug	1042938.

My concern about my original patch here is that I think things can very quickly get out of sync.

For instance, if the WebExtension is blocklisted or disabled somehow outside of the normal add-on flow.

There's also issues with the engine moving around when it is enabled/disabled.

I'm trying to move things back to the front but I'm getting sidetracked by a lot of other things.
Flags: needinfo?(mozilla)
(In reply to Mike Kaply [:mkaply] from comment #43)
> The reason the bug is stalled is because of the amount of work required for
> bug	1042938.

IMO we shouldn't block on this and get the functionality in, it can always be updated later.

> My concern about my original patch here is that I think things can very
> quickly get out of sync.
> 
> For instance, if the WebExtension is blocklisted or disabled somehow outside
> of the normal add-on flow.

The only way I can think the webext shutdown code wouldn't (or shouldn't) be called is if firefox is not running and you delete the addon from the filesystem.

> There's also issues with the engine moving around when it is
> enabled/disabled.

I haven't looked back at the code to refresh my memory, but I would assume that the engine is added during install/enable, and removed during uninstall/disable, but left in place on app_shutdown.  So it shouldn't move around just from a restart.  I think we can live with it moving around through a disable/uninstall.
(In reply to Shane Caraveo (:mixedpuppy) from comment #44)
> (In reply to Mike Kaply [:mkaply] from comment #43)

> > My concern about my original patch here is that I think things can very
> > quickly get out of sync.
> > 
> > For instance, if the WebExtension is blocklisted or disabled somehow outside
> > of the normal add-on flow.
> 
> The only way I can think the webext shutdown code wouldn't (or shouldn't) be
> called is if firefox is not running and you delete the addon from the
> filesystem.

Which I think is something an antivirus would be likely to do.
Flags: needinfo?(florian)
(Assignee)

Comment 46

10 days ago
> The only way I can think the webext shutdown code wouldn't (or shouldn't) be called is if firefox is not running and you delete the addon from the filesystem.

What if the add-on is disabled before startup via the AMO blocklist. 

I don't think we would call the shutdown code then because the add-on hasn't started up yet.
I think the issues raised here can all be handled within the existing webextensions framework without requiring any special handling in the search component.  We went through a bunch of the same stuff with ExtensionPreferencesManager.  A couple of things:
- the AddonManager should take care of balancing calls to enable/disable (which map to startup/manifest_* events and shutdown events respctively), and we have pretty thorough unit tests covering different cases.
- by using the ExtensionSettingsStore (which is what ExtensionPreferencesManager is built on top of), information about an extension's settings is stored in extension-settings.json so it can be removed when the extension is disabled, even if the manifest isn't available at that time (ie, if the extension gets removed from disk)
- enable and disable are the right events to pay attention to since an extension can be uninstalled but disabled if it is blocklisted, unsigned, etc., which are states where nothing the extension tries to do should be applied.  the one tricky case is upgrades (and downgrades), but you can check for that by consulting extension.shutdownReason in a shutdown handler.
- ExtensionSettingsStore is not a perfect fit here since it is built to handle the case where there is a single underlying setting that multiple extensions might fight over (e.g., the new tab page or specific preferences).  But I think it could be made to work nevertheless...
> ExtensionSettingsStore is not a perfect fit here since it is built to handle
> the case where there is a single underlying setting that multiple extensions
> might fight over (e.g., the new tab page or specific preferences).

Isn't the default search engine exactly such a setting?
(In reply to Ben Bucksch (:BenB) from comment #48)
> > ExtensionSettingsStore is not a perfect fit here since it is built to handle
> > the case where there is a single underlying setting that multiple extensions
> > might fight over (e.g., the new tab page or specific preferences).
> 
> Isn't the default search engine exactly such a setting?

Sure but setting the default search engine is not in the scope of this specific bug.
> setting the default search engine is not in the scope of this specific bug.

The default search engine is the purpose of this bug. See initial description. That's why I'm here, and what I need.

There's no point for me to add a search engine, if it's not made default as well.
(Assignee)

Updated

4 days ago
Attachment #8840090 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 52

4 days ago
I decided to punt on 1042938.

Comment 53

4 days ago
mozreview-review
Comment on attachment 8861455 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/133450/#review136284

Forgive the drive-by, but I saw that a patch was added to the bug and I was interested to see it. I added a couple of comments.

::: browser/components/extensions/ext-chrome-settings-overrides.js:24
(Diff revision 1)
>                                               manifest.chrome_settings_overrides.homepage);
>      }
> +    if (manifest.chrome_settings_overrides.search_provider) {
> +      let searchProvider = manifest.chrome_settings_overrides.search_provider;
> +      try {
> +        Services.search.addEngineWithDetails(searchProvider.name.trim(),

What happens if two extensions use the same name? Would that cause a problem?

::: browser/components/extensions/ext-chrome-settings-overrides.js:49
(Diff revision 1)
> +        }
> +        Components.utils.reportError(e);
> +      }
> +    }
> +  }
> +  onShutdown(reason) {

Shouldn't the `reason` be used here to ensure this only happens when the extension is removed or disabled? Won't this cause it to be removed when the browser is shut down?

Comment 54

4 days ago
mozreview-review
Comment on attachment 8861455 [details]
Bug 1301315 - Add support for chrome_settings_overrides/search_provider.

https://reviewboard.mozilla.org/r/133450/#review136310

::: browser/components/extensions/ext-chrome-settings-overrides.js:49
(Diff revision 1)
> +        }
> +        Components.utils.reportError(e);
> +      }
> +    }
> +  }
> +  onShutdown(reason) {

dito.  The engine shouldn't be removed on APP_SHUTDOWN.  Need to also make sure addon upgrade works as expected.

::: browser/components/extensions/test/browser/browser_ext_settings_overrides_search.js:45
(Diff revision 1)
> +    }
> +  });
> +
> +  yield ext1.startup();
> +
> +  let engine = Services.search.getEngineByName("MozSearch");

Also check that name w/spaces does not exist?
Attachment #8861455 - Flags: review?(mixedpuppy)
(Assignee)

Comment 55

4 days ago
> Need to also make sure addon upgrade works as expected.

That could get really interesting.

What if the manifest changes the engine name/URL as part of an upgrade?

We have no way to find the old engine to remove it.
(In reply to Mike Kaply [:mkaply] from comment #55)
> > Need to also make sure addon upgrade works as expected.
> 
> That could get really interesting.
> 
> What if the manifest changes the engine name/URL as part of an upgrade?
> 
> We have no way to find the old engine to remove it.

If you're storing the information in ExtensionSettingsStore, you can look it up with getAllForExtension()
(In reply to Mike Kaply [:mkaply] from comment #55)

> We have no way to find the old engine to remove it.

Can you get it by add-on id from the search service?
(Assignee)

Comment 58

4 days ago
(In reply to Florian Quèze [:florian] [:flo] from comment #57)
> (In reply to Mike Kaply [:mkaply] from comment #55)
> 
> > We have no way to find the old engine to remove it.
> 
> Can you get it by add-on id from the search service?

I'm adding that capability in now.
You need to log in before you can comment on or make changes to this bug.