Closed Bug 1879625 Opened 6 months ago Closed 5 months ago

Set the saved settings on SearchEngine objects as soon as they are created, rather than doing it later

Categories

(Firefox :: Search, task, P2)

task

Tracking

()

VERIFIED FIXED
125 Branch
Tracking Status
firefox124 --- verified
firefox125 --- verified

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(4 files, 2 obsolete files)

Currently, when we load the search engines on initialisation, or when we reload the engines, we load the engines first and then we subsequently do an iteration over the settings file to load the settings into an individual engine.

This means a couple of things:

  • For engines that are loaded during loadEnginesFromSettings we actually load the settings data twice - once when we load the JSON, and the second when loadEnginesMetadataFromSettings subsequently gets called.
  • The settings aren't loaded into the engine object immediately. I have a case where I want to be able to use the saved settings before we've finished loading all the engines, so hence the need to re-arrange.

Additionally, I think it makes slightly more sense for the engines to have their settings immediately, rather than a two-step process to possibly load them later.

This could never have run properly as _alias isn't available on SearchEngine, only alias is.
Bug 1879555 has been filed to potentially address this in future.

For some other patches, we need to be able to have the user's saved engine settings loaded into the search engine objects striaght away.
Currently, they are generally not loaded until sometime later, and in the case of engines loaded in loadEnginesFromSettings,
they are actually loaded twice.

Additionally, I think it makes more sense to pass the data in when they are constructed.

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef7158de40a3
Remove incorrect search engine alias migration code. r=search-reviewers,jteow
https://hg.mozilla.org/integration/autoland/rev/4549cdedfb96
Load settings into search engines at the time of their creation. r=search-reviewers,jteow

Backed out for potential issues on configuration change

Backout link: https://hg.mozilla.org/integration/autoland/rev/09d8dbabfae0a5892714a2b101c57c550d21a275

Flags: needinfo?(standard8)

The issues were due to the potential for applying the wrong settings on a reload, because we were matching the settings by name, rather than identifier. This generally this wouldn't have mattered, but could have confused some situations where add-ons can override the application provided engine.

To handle this properly, we need to move to loading the saved settings according to the id rather than the name. This is something I've been intending for us to do, but hasn't happened yet. We will need to account for the upgrade case, but that should be easy to do and covered by existing tests.

This will cause one change - the different variants of eBay will be treated as separate engines wrt settings. We think that is acceptable, as eBay.fr is a different site/intention to eBay.co.uk.

Flags: needinfo?(standard8)

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

To handle this properly, we need to move to loading the saved settings according to the id rather than the name. This is something I've been intending for us to do, but hasn't happened yet. We will need to account for the upgrade case, but that should be easy to do and covered by existing tests.

The existing tests are test_settings.js and test_settings_migration_ids.js.

Blocks: 1882309
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b13b62667d5d
Remove incorrect search engine alias migration code. r=search-reviewers,jteow
https://hg.mozilla.org/integration/autoland/rev/698f53224906
Load settings into search engines at the time of their creation. r=search-reviewers,jteow
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 125 Branch

This could never have run properly as _alias isn't available on SearchEngine, only alias is.
Bug 1879555 has been filed to potentially address this in future.

Original Revision: https://phabricator.services.mozilla.com/D201269

Attachment #9389025 - Flags: approval-mozilla-beta?

For some other patches, we need to be able to have the user's saved engine settings loaded into the search engine objects striaght away.
Currently, they are generally not loaded until sometime later, and in the case of engines loaded in loadEnginesFromSettings,
they are actually loaded twice.

Additionally, I think it makes more sense to pass the data in when they are constructed.

This also switches to loading the settings by identifier, rather than name. This has been intended
for a while now as it will provide more accurate loading that is resilliant to name changes.

Original Revision: https://phabricator.services.mozilla.com/D201270

Attachment #9389026 - Flags: approval-mozilla-beta?
Attachment #9389026 - Attachment is obsolete: true
Attachment #9389026 - Flags: approval-mozilla-beta?
Attachment #9389025 - Attachment is obsolete: true
Attachment #9389025 - Flags: approval-mozilla-beta?

This could never have run properly as _alias isn't available on SearchEngine, only alias is.
Bug 1879555 has been filed to potentially address this in future.

Original Revision: https://phabricator.services.mozilla.com/D201269

Attachment #9389038 - Flags: approval-mozilla-beta?

For some other patches, we need to be able to have the user's saved engine settings loaded into the search engine objects striaght away.
Currently, they are generally not loaded until sometime later, and in the case of engines loaded in loadEnginesFromSettings,
they are actually loaded twice.

Additionally, I think it makes more sense to pass the data in when they are constructed.

This also switches to loading the settings by identifier, rather than name. This has been intended
for a while now as it will provide more accurate loading that is resilliant to name changes.

Original Revision: https://phabricator.services.mozilla.com/D201270

Attachment #9389039 - Flags: approval-mozilla-beta?

:standard8 these uplifts are missing the uplift request form, you can add this directly on each phabricator revision
turns out we just needed the form in the tip of the stack

Flags: needinfo?(standard8)
Attachment #9389039 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9389038 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(standard8)

This issue verification is part of a larger set of testing scope, caught in this ticket here -> QA-2238, Nightly 125 testing completed, following up with 124b8 testing and verification here test run.

Verified as part of QA-2238 on both Nightly 125/Beta 124.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: