Closed Bug 1301315 Opened 8 years ago Closed 7 years ago

Support chrome_settings_overrides search engine

Categories

(WebExtensions :: Untriaged, enhancement, P2)

Product:
WebExtensions
Component:
Untriaged
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed
Blocking Flags:
webextensions ?

People

(Reporter: arun.emmafan, Assigned: mkaply)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file, 2 obsolete files)

Bug 1301315 - Add support for chrome_settings_overrides search engine.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
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
URL: https://developer.chrome.com/extensio...
Severity: normal → critical
Priority: -- → P1
Severity: critical → enhancement
Priority: P1 → --
Summary: Supports chrome_settings_overrides api in mozilla WebExtension → Supports chrome_settings_overrides api in mozilla WebExtension to change default search engine
Whiteboard: [design-decision-needed]triaged
Priority: -- → P2
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)
> 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?
> 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.
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
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/
ni? kev as the search engine expert
Flags: needinfo?(kev)
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.
Blocks: 1333828
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.
Is this a dupe of bug 1268401?
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.
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 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: nobody → mozilla
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 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-
> 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.
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 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-
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);
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 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 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 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.
(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
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.
> 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
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!
(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.
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
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)
> 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.
Attachment #8840090 - Attachment is obsolete: true
I decided to punt on 1042938.
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 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)
> 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?
(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.
webextensions: --- → ?
Anything we can do to help on this one Mike?
Sorry, Andy.

I just need to get 

https://bugzilla.mozilla.org/show_bug.cgi?id=1359538

finalized and checked in. I'll try to get to that this week. Pulled from every direction right now.

Once that is done, there isn't much to this patch.
Attachment #8861455 - Attachment is obsolete: true
Comment on attachment 8874014 [details]
Bug 1301315 - Add support for chrome_settings_overrides search engine.

https://reviewboard.mozilla.org/r/145464/#review149402

r+ given index fix and try run.

::: browser/components/extensions/ext-chrome-settings-overrides.js:45
(Diff revision 1)
> +          if (extension.startupReason === "ADDON_UPGRADE") {
> +            let engine = Services.search.getEngineByName(searchProvider.name.trim());
> +            if (isCurrent) {
> +              Services.search.currentEngine = engine;
> +            }
> +            Services.search.moveEngine(engine, index);

As discussed on irc, dont do this if index is -1
Attachment #8874014 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8874014 [details]
Bug 1301315 - Add support for chrome_settings_overrides search engine.

https://reviewboard.mozilla.org/r/145464/#review149406

::: browser/components/extensions/ext-chrome-settings-overrides.js:21
(Diff revision 1)
>        ExtensionPreferencesManager.setSetting(extension, "homepage_override",
>                                               manifest.chrome_settings_overrides.homepage);
>      }
> +    if (manifest.chrome_settings_overrides.search_provider) {
> +      let searchProvider = manifest.chrome_settings_overrides.search_provider;
> +      Services.search.init(() => {

Are we sure this will never run during the add-on manager startup? The search service shouldn't not be initialized before first paint, as it causes NSS to be initialized on the main thread. See bug 1359031 for a recent change we did to avoid this.

::: browser/components/extensions/ext-chrome-settings-overrides.js:22
(Diff revision 1)
>                                               manifest.chrome_settings_overrides.homepage);
>      }
> +    if (manifest.chrome_settings_overrides.search_provider) {
> +      let searchProvider = manifest.chrome_settings_overrides.search_provider;
> +      Services.search.init(() => {
> +        var isCurrent = false;

is there a reason for mixing 'var' and 'let' in this new code?

::: browser/components/extensions/ext-chrome-settings-overrides.js:58
(Diff revision 1)
> +  onShutdown(reason) {
> +    let {extension} = this;
> +    if (reason == "ADDON_DISABLE" ||
> +        reason == "ADDON_UNINSTALL") {
> +      if (extension.manifest.chrome_settings_overrides.search_provider) {
> +        let engines = Services.search.getEnginesByExtensionID(extension.id);

If we disable an add-on during the browser startup, this will trigger a sync init of the search service. Can we avoid it?

::: browser/components/extensions/ext-chrome-settings-overrides.js:60
(Diff revision 1)
> +    if (reason == "ADDON_DISABLE" ||
> +        reason == "ADDON_UNINSTALL") {
> +      if (extension.manifest.chrome_settings_overrides.search_provider) {
> +        let engines = Services.search.getEnginesByExtensionID(extension.id);
> +        if (engines.length > 0) {
> +          for (let i=0; i < engines.length; i++) {

nit: spaces around '='. Is eslint not catching this?

Actually: for (let engine of engines)

And what's the point of the 'if (engines.length > 0) {' check?
Florian:

This switches to a promise for checking search initializtion on startup and shutdown and all your other issues as well.
It also fixes all the eslint issues.

Shane:

This has your fix as well (the -1)
I'm happy, even happier if Florian is happy.
Flags: needinfo?(florian)
Comment on attachment 8874014 [details]
Bug 1301315 - Add support for chrome_settings_overrides search engine.

https://reviewboard.mozilla.org/r/145464/#review150990

::: browser/components/extensions/ext-chrome-settings-overrides.js:10
(Diff revisions 1 - 2)
>  "use strict";
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "ExtensionPreferencesManager",
>                                    "resource://gre/modules/ExtensionPreferencesManager.jsm");
>  
> +function waitForSearchInitialization() {

can we just call this searchServiceInialized?
Comment on attachment 8874014 [details]
Bug 1301315 - Add support for chrome_settings_overrides search engine.

https://reviewboard.mozilla.org/r/145464/#review151260

I'm happy, as far as startup performance is concerned.

I wonder if there's any risk of something else happening while we are waiting for the search service init, causing what happens next in the function to fail in undesirable ways.
Eg. is it possible that something else triggers a shutdown/restart of the browser while we are waiting? Or that the add-on manager decides to uninstall/disable the add-on for which we were trying to install a search plugin? It's OK if these edge cases cause failures, they should just be failures without side effects.

::: browser/components/extensions/ext-chrome-settings-overrides.js:78
(Diff revision 2)
> +    if (reason == "ADDON_DISABLE" ||
> +        reason == "ADDON_UNINSTALL") {
> +      if (extension.manifest.chrome_settings_overrides.search_provider) {
> +        await waitForSearchInitialization();
> +        let engines = Services.search.getEnginesByExtensionID(extension.id);
> +        for (let i = 0; i < engines.length; i++) {

I would still write this as:
  for (let engine of engines) {
but I guess it's just a matter of coding style preference.
Flags: needinfo?(florian)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/faf77d75b2b0
Add support for chrome_settings_overrides search engine. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/faf77d75b2b0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Hello, I'd like to ask if the changes from this ticket imply some update to this page > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/chrome_settings_overrides
Currently it states that "homepage" is the only setting that is currently supported.
Thanks!
Blocks: 1374312
Wanted to check if the bug is resolved now and is available in current version of FF. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/chrome_settings_overrides states that the search_provider.is_default	 is still not available.
This feature has not shipped yet (it is in Firefox 55). We have made no commitment to the is_default parameter.
Thanks! Is it available on nighty build? (In reply to Mike Kaply [:mkaply] from comment #74)
> This feature has not shipped yet (it is in Firefox 55). We have made no
> commitment to the is_default parameter.
> Thanks! Is it available on nighty build?

Yes, it is in nightly.
(In reply to Mike Kaply [:mkaply] from comment #76)
> > Thanks! Is it available on nighty build?
> 
> Yes, it is in nightly.

I tested my sample addon in debug mode on nightly build 56.0a1 (2017-07-20) (64-bit). The attached message mentions "Reading manifest: Error processing chrome_settings_overrides.search_provider.is_default: Unsupported on Firefox at this time."

Please let me know if I can provide screenshot or any further details from debugging.
As I said in comment 55, 

> We have made no commitment to the is_default parameter.

This bug is about adding support for adding a search engine at all.

Is the engine still loading but not being set as default? Or is the load failing completely?
(In reply to Mike Kaply [:mkaply] from comment #78)
> As I said in comment 55, 
> 
> > We have made no commitment to the is_default parameter.
> 
> This bug is about adding support for adding a search engine at all.
> 
> Is the engine still loading but not being set as default? Or is the load
> failing completely?

I realized that the message in addon debug screen is different for Mac and PC. 

In both Mac & PC the extension loads fine and added the search engine to the list but do not set it as default. 
In Mac the there is additional message on the addon debug screen - "Reading manifest: Error processing chrome_settings_overrides.search_provider.is_default: Unsupported on Firefox at this time."
(In reply to Deepak from comment #79)
> (In reply to Mike Kaply [:mkaply] from comment #78)
> > As I said in comment 55, 
> > 
> > > We have made no commitment to the is_default parameter.
> > 
> > This bug is about adding support for adding a search engine at all.
> > 
> > Is the engine still loading but not being set as default? Or is the load
> > failing completely?
> 
> I realized that the message in addon debug screen is different for Mac and
> PC. 
> 
> In both Mac & PC the extension loads fine and added the search engine to the
> list but do not set it as default. 
> In Mac the there is additional message on the addon debug screen - "Reading
> manifest: Error processing
> chrome_settings_overrides.search_provider.is_default: Unsupported on Firefox
> at this time."

Mike, 

Can you please clarify what do you mean by "We have made no commitment to the is_default parameter". Does it mean that mozilla will not support the feature to set search engine as default or this has not been prioritized yet.
(In reply to Deepak from comment #80)

> Can you please clarify what do you mean by "We have made no commitment to
> the is_default parameter". Does it mean that mozilla will not support the
> feature to set search engine as default or this has not been prioritized yet.

It means we haven't made any commitment to allowing extensions to set a default provider programmatically.
Flags: needinfo?(kev)
I've updated the compat data for this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/chrome_settings_overrides#Browser_compatibility, please let me know if you need anything else.
Flags: needinfo?(mozilla)
LGTM
Flags: needinfo?(mozilla)
I filed bug 1388052 as a follow up on the is_default property.
Depends on: 1452299
Product: Toolkit → WebExtensions
Depends on: 1493770
Depends on: 1525729
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: