Replace some calls to getDefaultEngines with purpose-built specific nsISearchEngine.isAppProvided flag
Categories
(Firefox :: Search, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox76 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
Details
Attachments
(2 files)
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.
Assignee | ||
Comment 2•5 years ago
|
||
(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.
Assignee | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D69233
Comment 7•5 years ago
|
||
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
Assignee | ||
Comment 8•5 years ago
|
||
I fixed the failure, this will re-land once the tree is open again.
Comment 10•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e31c9f38d6d2
https://hg.mozilla.org/mozilla-central/rev/2ff264d68b14
Description
•