1165341 - No "engine-default" is notified when removing a default engine

Status: RESOLVED FIXED

(Firefox :: Search, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

In bug 1138503, we've noticed that no "browser-search-engine-modified" topic with "engine-default" data field is notified when a default engine is removed using the search preferences panel.

A notification should probably be dispatched in [1], as the default engine is changed internally.

[1] - https://hg.mozilla.org/mozilla-central/annotate/25c5525a1000/toolkit/components/search/nsSearchService.js#l4368

Comment 1

[Alessio Placitelli [:Dexter]]

Florian, is there a particular reason why an "engine-default" notification shouldn't be fired when removing a default engine in nsSearchService?

Comment 2

[Florian Quèze [:florian]]

(In reply to Alessio Placitelli [:Dexter] from comment #1)
> Florian, is there a particular reason why an "engine-default" notification
> shouldn't be fired when removing a default engine in nsSearchService?

I don't see any obvious reason for that. If we change it for the default engine, we should likely also change it for the current engine 4 lines above in the code.

Comment 3

[Alessio Placitelli [:Dexter]]

Once this gets fixed, we should remove the comment here: https://dxr.mozilla.org/mozilla-central/rev/daa7d98525e859d32a3b3e97101e129a897192a1/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#1120

Comment 4

[Florian Quèze [:florian]]

I'm wondering if the issue here isn't that the engine-removed notification isn't fired. There's a patch in bug 1122618 that (among other things) fixes that engine-removed notification.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> I'm wondering if the issue here isn't that the engine-removed notification
> isn't fired. There's a patch in bug 1122618 that (among other things) fixes
> that engine-removed notification.

Mh, doesn't seem so unfortunately :( Commenting out [0] still makes the test fail.

[0] - https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#1122

Comment 6

[Mark Banner (:standard8)]

Bug 1575649 fixed this.