1877721 - Remove nsISearchEngine.identifier

Status: RESOLVED FIXED

(Firefox :: Search, task, P3)

Description

[Mark Banner (:standard8)]

On nsISearchEngine we currently have three properties that all mention "identifier" in some form:

• id - the internal identifier of the search engine.
• telemetryId - the telemetry identifier of the search engine.
• identifier - if this is an application provided engine, this returns the telemetryId, otherwise it returns null.

This is confusing, as it really isn't clear what identifier is for.

Additionally, it isn't always obvious that identifier may return null, e.g. this code which returns an array containing nulls if third party search engines are installed.

Another example is filtering the identifier for falsy values without mentioning why this is or might be necessary.

Therefore I think we should remove identifier and replace its use with telemetryId. If we need to filter out third party engines, then we can check the isAppProvided property of the engine.

The other alternative would be to replace identifier with a function, e.g. getTelemetryId({ includeThirdParty: true}) so that it is explicit to include third party.

I'm not sure which method I like most at the moment.

Comment 1

[Mark Banner (:standard8)]

Looking at the production code, I think the only instances of code using this is in UITour & ASRouter, I've filed bug 1976703 and bug 1976706 about fixing those - which both seem viable.

However there may be some instances not picked up by the query, so we should be careful when removing this (maybe a try run with identifier throwing).

The only identifier like properties remaining should be id and telemetryId.

Comment 2

[Mark Banner (:standard8)]

We should also annotate telemetryId with being obsolete, to try and avoid its use in new code.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1877721 - In head_searchconfig.js, rename SearchConfigTest._config to #testDetails to reduce confusion with the configuration. r?#search-reviewers!

Also adds more detailed type definitions to aid with understanding/future development.

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1877721 - Improve type definitions and descriptions in head_searchconfig.js. r?#search-reviewers!

Also fixes/adds an explicit check for no client parameter in some situations.

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1877721 - Remove nsISearchEngine.identifier. r?#search-reviewers!

The identifier was a mixture of different fields, and the 'id' and other fields should be preferred.
The 'telemetryId' field is also deprecated in favour of other fields.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ba47ecc1962a
https://hg.mozilla.org/integration/autoland/rev/b6386efccc85
In head_searchconfig.js, rename SearchConfigTest._config to #testDetails to reduce confusion with the configuration. r=search-reviewers,scunnane
https://github.com/mozilla-firefox/firefox/commit/64a0c6b8d7fe
https://hg.mozilla.org/integration/autoland/rev/7b4e2ac938fc
Improve type definitions and descriptions in head_searchconfig.js. r=search-reviewers,scunnane
https://github.com/mozilla-firefox/firefox/commit/78d46938a551
https://hg.mozilla.org/integration/autoland/rev/7486ae59d969
Remove nsISearchEngine.identifier. r=search-reviewers,toolkit-telemetry-reviewers,TravisLong,scunnane,nimbus-reviewers,beth

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b6386efccc85
https://hg.mozilla.org/mozilla-central/rev/7b4e2ac938fc
https://hg.mozilla.org/mozilla-central/rev/7486ae59d969