1575649 - Change nsISearchService.removeEngine to set the new defaults straight away if removing the default engine

Status: RESOLVED FIXED

(Firefox :: Search, task, P3)

Description

[Mark Banner (:standard8)]

removeEngine currently removes the engine from the search service, but it does not update the default engine(s) if one of those is being removed. All it currently does is set this._currentEngine to null (and/or this._currentPrivateEngine after bug 1562922).

Once a call to get the default is then made, we then figure it out.

It looks like removing search engines only currently works in the Firefox UI because we're either displaying the preferences UI or it is a WebExtension being removed, both of which are triggering gets of the current engine.

One of the potential side-effects of this, is for tests, the cache may still have a value that's out of date, causing confusing results.

I also suspect that bug 1113692 and bug 1165341 are results of this as well. Bug 1113692 does seem like it was fixed with the work around.

I think we should be calling getDefault(Private) straight away during or at the end of the remove to ensure the new item is set up fully.

Comment 1

[Mark Banner (:standard8)]

There's a few places we might need to allow for a batch remove, though that might not be a bad thing anyway.

Comment 2

[Mark Banner (:standard8)]

Note that if we do this, we need to check that we don't loose the default search engine on WebExtension upgrades, xref bug 1610870 comment 1.

Comment 3

[Mark Banner (:standard8)]

This turned out simpler than what I was originally expecting. Rather than clearing the current engine we can simply do the call to set the default to the original default engine. That does all the calculations for us.

WebExtensions are handled correctly on upgrade due to special handling.

I decided we should do this now, as the current implementation relies on ContentSearch to request updates once the existing default engine's removed notification is received. That's dangerous as we should be driving it from the search service, and there's also potential ContentSearch simplifications on the horizon that would stop this working potentially without being noticed straight away.

Comment 4

[Mark Banner (:standard8)]

Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=abab1374ab50cc162c4e6c371624f313896666bb

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1575649 - Remove browser_addEngine.js as it is covered by other tests. r?mak!

toolkit/components/search/tests/xpcshell/test_notifications.js and the test_opensearch* tests already test this functionality. There's nothing extra gained by running this as a mochitest.

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1575649 - When removing a default engine, calculate and notify the new default engine straight away. r?mak!

Depends on D105030

Comment 7

[Mark Banner (:standard8)]

Attachment: Bug 1575649 - Rewrite default search engine fallback tests to make them clearer, and add docs for removing the default engine. r?mak!

Depends on D105030

Comment 8

[Mark Banner (:standard8)]

Attachment: Bug 1575649 - Add notification checks to default engine fallback tests. r?mak!

Depends on D106625

Comment 9

[Phabricator Automation]

Comment on attachment 9202901 [details]
Bug 1575649 - Remove browser_addEngine.js as it is covered by other tests. r?mak!

Revision D105030 was moved to bug 1695608. Setting attachment 9202901 [details] to obsolete.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab2844c65f85
Rewrite default search engine fallback tests to make them clearer, and add docs for removing the default engine. r=mak
https://hg.mozilla.org/integration/autoland/rev/7d0b05364045
Add notification checks to default engine fallback tests. r=mak
https://hg.mozilla.org/integration/autoland/rev/2e53dfdab965
When removing a default engine, calculate and notify the new default engine straight away. r=mak

Comment 11

[Mark Banner (:standard8)]

Marking as qe-verify+. It would be nice if we could test the following:

• Removing an engine via preferences.
• Removing an engine via the ignore list.
• Removing a WebExtension from the profile (I think that should remove the engine).

In all cases the engine should be set as default. Try different combinations of the other engines being hidden. For this bug we expect to fallback to the region/locale default if it is not hidden, then the first visible one, then we unhide the region/locale default.

Note that bug 1695262 will change the fallback behaviour slightly, so if that has landed, it will change the expectations here.

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ab2844c65f85
https://hg.mozilla.org/mozilla-central/rev/7d0b05364045
https://hg.mozilla.org/mozilla-central/rev/2e53dfdab965