1903247 - Remove SearchEngine.searchForm

Status: RESOLVED FIXED

(Firefox :: Search, task)

Comment 1

[Moritz Beier [:mbeier]]

Attachment: Bug 1903247 - Remove SearchEngine.searchForm. r?standard8

Comment 2

[Shane Hughes [:aminomancer]]

Can we add a bit more context? Why are we doing this? Where is the work being tracked, and is this part of a broader cleanup? Meta bug? How is this going to impact existing search engines? I've created several search engine extensions like this:

"search_provider": {
  "name": "Google Translate",
  "search_url": "https://translate.google.com",
  "search_form": "https://translate.google.com/?source=osdd&sl=auto&tl=auto&text={searchTerms}&op=translate",
  "search_url_get_params": "source=osdd&sl=auto&tl=auto&text={searchTerms}&op=translate",
  "suggest_url": "https://www.google.com/complete/search?client=firefox&output=firefox&q={searchTerms}",
  "suggest_url_get_params": "client=firefox&q={searchTerms}"
}

I no longer see search_form nor search_url_get_params in the documentation, so I guess the standard has changed. Is my extension going to stop working now and I need to update it? I have like 15 of these, between extensions and engines I've added directly to search.json.mozlz4. Several on AMO with many users relying on them. Do we have some kind of migration or backwards compatibility to avoid breaking all these old engines?

Are we supposed to use just search_url, putting all the search params in that one URL? I was under the impression that Firefox used search_url to determine the domain for the search engine, so that we could offer search engine history urlbar results (i.e. previous searches made with the search engine) when the urlbar query matches the domain, and other things of that nature. So I put just the domain in search_url so that Firefox would know what the base domain is and be able to connect the search engine to that site's history and other operations associated with that site. And then I relied on search_form and/or search_url_get_params to handle the actual search. Is that incorrect? Unfortunately the documentation does not explain the intended pattern.

Comment 3

[Mark Banner (:standard8)]

(In reply to Shane Hughes [:aminomancer] from comment #2)

Can we add a bit more context? Why are we doing this? Where is the work being tracked, and is this part of a broader cleanup? Meta bug? How is this going to impact existing search engines?

This is not particularly part of any effort, it is more just clean-up of things we don't really use / aren't worth it - although it happens to be being grouped into the tidy up after switching to a new configuration format (bug 1870686).

I've just added the dependencies from the other clean-ups we've done relating to searchForm. The only UI change there is that pressing enter on the search bar no longer loads the empty search form - this makes it more consistent with the functionality of the address bar (We're intending on making the search bar more similar to the address bar over time).

The other couple of changes are really behind the scenes and basically have no effect for search engine use, since they were pretty much just using the hostname from the search url anyway.

I no longer see search_form nor search_url_get_params in the documentation, so I guess the standard has changed. Is my extension going to stop working now and I need to update it? I have like 15 of these, between extensions and engines I've added directly to search.json.mozlz4. Several on AMO with many users relying on them. Do we have some kind of migration or backwards compatibility to avoid breaking all these old engines?

As far as I know, search_form and search_url_get_params have never been part of the "official" WebExtension standard. For example, Google Chrome does not support them. I believe the extra fields were added by the search team when we moved to using WebExtensions for application-provided engines.

Whilst we will deprecate the use of search_form, we won't be changing the schema to disallow it. That would break the intention behind the manifest versioning system.

search_url_get_params is unaffected here.

I guess for both of them if we'd been earlier in the v3 manifest cycle then I might have suggested removing them in v3 as clean-up and so that we'd be more consistent with Google Chrome, but it doesn't cost us much to continue to have them there (e.g. there's also other fields that can be specified but aren't supported). We can always consider it for a v4 if it ever happens.

Are we supposed to use just search_url, putting all the search params in that one URL? I was under the impression that Firefox used search_url to determine the domain for the search engine, so that we could offer search engine history urlbar results (i.e. previous searches made with the search engine) when the urlbar query matches the domain, and other things of that nature.

As far as I know we should be treating the URLs the same regardless of if the params are bundled in search_url or if they are in search_url_get_params. If we treated them differently in the past, that was probably a bug. Most of the third party search add-ons I've seen use only the search_url.

I did just scan the code and nothing obvious jumps out for differences between the two, though it is a little complicated to be sure until we get some more of the clean-up from the previous search configuration done.

Comment 4

[Shane Hughes [:aminomancer]]

Thanks for the overview, that's much appreciated. I think what I may have been remembering is not history results, but the behavior you just described where searchForm could be used to navigate directly to the engine's base domain if the search query was blank. If that no longer exists, then I guess there's no longer a reason for my engines to use search_form.

Now I'm thinking the reason I did it this way was because I started making these extensions when I wanted to change the icons for some of the built-in search engines that are provided by extensions. Since they do it this way and I copied from them, that's the pattern I reproduced. This might have been what you were referring to in your comment in the revision about tidying up other uses of search_form. I suppose all these built-in engines should technically be updated, but that might be pretty involved since many of them are localized.

Comment 5

[Mark Banner (:standard8)]

(In reply to Shane Hughes [:aminomancer] from comment #4)

Since they do it this way and I copied from them, that's the pattern I reproduced. ... I suppose all these built-in engines should technically be updated, but that might be pretty involved since many of them are localized.

Those extensions are pretty much unused with the new configuration, but we have to leave them in there for now so we can uninstall them from profiles. Hence we're not going to worry about updating them.

Comment 6

[Rob Wu [:robwu]]

(In reply to Mark Banner (:standard8) from comment #3)

Whilst we will deprecate the use of search_form, we won't be changing the schema to disallow it. That would break the intention behind the manifest versioning system.

Because the search_provider key already defines "additionalProperties": { "$ref": "UnrecognizedProperty" }, at https://searchfox.org/mozilla-central/rev/cb1060f7b4581e6c2d30f1accc84c7d807132d82/browser/components/extensions/schemas/chrome_settings_overrides.json#22, you can safely drop properties from the schema if they are really unused.

Comment 7

[Shane Hughes [:aminomancer]]

(In reply to Mark Banner (:standard8) from comment #5)

Those extensions are pretty much unused with the new configuration

Out of curiosity, where are the built-in extensions coming from now? If they reflect the current standards, I would try to keep mine basically in sync with them. I found this but I'm guessing that is just an automated dump of an RS collection?

Comment 8

[Mark Banner (:standard8)]

We are no longer using extensions for application provided engines. All the details for application provided engines are served from that collection and a couple of others.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bd158ed8528
Remove SearchEngine.searchForm. r=Standard8,omc-reviewers,home-newtab-reviewers,aminomancer,urlbar-reviewers,nbarrett

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1bd158ed8528