1226595 - The _isDefault implementation is fragile and not really needed

Status: RESOLVED WONTFIX

(Firefox :: Search, task, P3)

Description

[Florian Quèze [:florian]]

Attachment: remove-_isDefault WIP

Follow-up to bug 1221513 where Gavin pointed out that the _isDefault implementation we have since bug 1203167 is quite fragile, and could be simplified.

Looking at this some more, it seems in most cases the _isDefault calls could just be replaced by using the _readOnly field. However, there are some callers that shouldn't treat all read-only engines as default, so the patch isn't as trivial as I would have hoped.

I think the attached patch works, but I'm attaching it as a WIP because I haven't fully thought through all possible cases. I'm also not sure yet if I want to pursue this or wontfix.

Comment 1

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

Comment on attachment 8690077 [details] [diff] [review]
remove-_isDefault WIP

Using "_readonly" in different contexts like this without renaming it seems potentially confusing. I'm not sure what to suggest because I forget what it now means exactly, but perhaps something like "_isBuiltIn"? As in "is a built-in engine"?

Expanding the use of the confusing loadPath-matching regex is kind of against the point I was trying to make in the other bug - better to save the metadata directly rather than using the indirect loadPath-matching. Save a boolean flag on the engine object (and persist it) when the engine is given a loadPath for the first time.

I probably should just step away from giving you feedback on search service work in general, because I don't have the time to do it in-depth, and it's not really fair to you to have to deal with my superficial griping :)

Comment 2

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #1)
> Comment on attachment 8690077 [details] [diff] [review]
> remove-_isDefault WIP
> 
> Using "_readonly" in different contexts like this without renaming it seems
> potentially confusing. I'm not sure what to suggest because I forget what it
> now means exactly, but perhaps something like "_isBuiltIn"? As in "is a
> built-in engine"?

It means readOnly. This flag is set for all engines except the engines that only exist in the search service's "cache" file. It's the current equivalent of what used to be isDefault, ie. it's true for built-in engines, for add-on shipped engines and for distribution shipped engines.

> Expanding the use of the confusing loadPath-matching regex is kind of
> against the point I was trying to make in the other bug - better to save the
> metadata directly rather than using the indirect loadPath-matching. Save a
> boolean flag on the engine object (and persist it) when the engine is given
> a loadPath for the first time.

The reason why I'm reluctant to go this way is that we would then have 2 different values stored, and what to do when they contradict themselves will be ambiguous. Currently the loadPath value is more or less the single source of truth. I'm saying "more or less" in the previous sentence because it would be fair to argue that _readOnly is already a second value, but its current use is significantly different, and mostly contained to determining if an engine would be blown away or preserved when refreshing the cache.

Comment 3

[Mark Banner (:standard8)]

In bug 1622245 and bug 1590803 the implementation of what was _isDefault vs _readOnly has been refined and improved, especially with the simplified code we now have in the search service.

I believe the new flag, isAppProvided, is reliable and is needed for a number of cases.