1203161 - remove the browser.search.cache.enabled pref

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Florian Quèze [:florian]]

This preference was introduced in bug 394979, but the comments there don't indicate why it was needed. I suspect it was added just so that the cache could be turned off on aurora if regressions were discovered. This pref doesn't seem to have any use case right now, and the cache being optional makes it unsuitable to store additional data about user-installed search engine.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch (diff -w) for easier review

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8659265 [details] [diff] [review]
Patch

Over to Drew

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8659265 [details] [diff] [review]
Patch

Review of attachment 8659265 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the -w patch!

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/55278c1dc745

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backing out Bug 1184220 wasn't enough to fix the bustage, apparently, so I'm backing out everything else from that push to hopefully get back to a clean slate.
https://hg.mozilla.org/integration/fx-team/rev/dde32e10af8b

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b5297f88b43f

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5297f88b43f

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Looks like this comment:

https://hg.mozilla.org/mozilla-central/rev/b5297f88b43f#l1.11

is now either wrong, or you should not have removed the "if (!this._readOnly)" conditional?

Comment 10

[Florian Quèze [:florian]]

I think the comment should be removed.

I haven't seen any data about how often the update feature is used, but I suspect it's very little.

In bug 1203167, I'm going to restrict the update feature to only user-installed engines (as app-shipped engines, and extension-shipped engines both already have a different update mechanism they can use), so engines with updates will never be read-only.

I think we should remove the update feature altogether eventually if my (and Mike's) guess that it isn't actually used in the wild is correct, but I think that'll require adding Telemetry probes to confirm, and so will have to wait a few cycles to have data.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> I think the comment should be removed.

Yes, I agree. It took me until I was in bed unable to sleep last night for me to realize it :)

> I haven't seen any data about how often the update feature is used, but I
> suspect it's very little.

It has certainly never been used with default engines (i.e. non-profile engines). Bug 511017 and associated bugs never went anywhere.

I suspect it is pretty much never used for profile engines either.

Your plan (disable now for readonly engines, eventually remove entirely) sounds good.