1590803 - Replace some calls to getDefaultEngines with purpose-built specific nsISearchEngine.isAppProvided flag

Status: RESOLVED FIXED

(Firefox :: Search, task, P3)

Description

[Mark Banner (:standard8)]

Currently, there's two places in the code where we call getDefaultEngines to work out if a single engine is default (aka built-in) or not:

https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/browser/base/content/browser.js#4515,4519
https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/browser/components/extensions/parent/ext-chrome-settings-overrides.js#367,370

In both of these cases, we have easy access to the nsISearchEngine object.

Currently getDefaultEngines uses the engine._isDefault getter to filter the engines before sorting them. We should just expose the equivalent of _isDefault on nsISearchEngine, and then have the callers above access that direct.

This would save getting arrays and iterating over them.

Ideally, we should use an attribute name other than isDefault as that could be confused with being the "default" engine for a user. Unfortunately we can't use isBuiltIn as that is for built-in engines that aren't distribution engines.

Comment 1

[Mark Banner (:standard8)]

Bug 1621892 is removing one instance of this.

Comment 2

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #1)

Bug 1621892 is removing one instance of this.

I was wrong, that's a slightly different bit of code. I'll start looking at this next.

Comment 3

[Mark Banner (:standard8)]

I've got a couple of patches that do this change. I've gone with isAppProvided for now since that feels like a much better term than using something that references "default" (which is overloaded), or "built-in" (also overloaded, and not necessarily true all the time).

I'm open to even better naming though. For now, isAppProvided just replaces the _isDefault that we had before, however, I think we can work towards replacing _builtIn with it as well, but I'm still investigating those and working through it, so I'll leave those to a later bug.

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1590803 - Replace SearchEngine._isBuiltIn with a public nsISearchEngine.isAppProvided function. r?daleharvey!

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1590803 - Clean up uses of SearchService.getDefaultEngines where we can now use engine.isAppProvided. r?daleharvey!

Depends on D69233

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efd1d1868d70
Replace SearchEngine._isBuiltIn with a public nsISearchEngine.isAppProvided function. r=daleharvey
https://hg.mozilla.org/integration/autoland/rev/6256694a9047
Clean up uses of SearchService.getDefaultEngines where we can now use engine.isAppProvided. r=daleharvey

Comment 7

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1590803) for browser-chrome failures in browser/browser_placeholder.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=295947400&repo=autoland&lineNumber=2951

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=6256694a90479b46badb88534d23306f01b9771f

Backout:
https://hg.mozilla.org/integration/autoland/rev/661ff57b823733f13f5544780c93a2eefad52aff

Comment 8

[Mark Banner (:standard8)]

I fixed the failure, this will re-land once the tree is open again.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e31c9f38d6d2
Replace SearchEngine._isBuiltIn with a public nsISearchEngine.isAppProvided function. r=daleharvey
https://hg.mozilla.org/integration/autoland/rev/2ff264d68b14
Clean up uses of SearchService.getDefaultEngines where we can now use engine.isAppProvided. r=daleharvey

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e31c9f38d6d2
https://hg.mozilla.org/mozilla-central/rev/2ff264d68b14