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

Status: VERIFIED FIXED

(Firefox :: Search, task, P2)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1879625 - Remove incorrect search engine alias migration code. r?#search-reviewers!

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.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1879625 - Load settings into search engines at the time of their creation. r?#search-reviewers!

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
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

Comment 4

[Narcis Beleuzu [:NarcisB]]

Backed out for potential issues on configuration change

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

Comment 5

[Mark Banner (: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.

Comment 6

[Mark Banner (: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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b13b62667d5d
https://hg.mozilla.org/mozilla-central/rev/698f53224906

Comment 9

[Mark Banner (:standard8)]

Attachment: Bug 1879625 - Remove incorrect search engine alias migration code.

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

Comment 10

[Mark Banner (:standard8)]

Attachment: Bug 1879625 - Load settings into search engines at the time of their creation.

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

Comment 11

[Mark Banner (:standard8)]

Attachment: Bug 1879625 - Remove incorrect search engine alias migration code.

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

Comment 12

[Mark Banner (:standard8)]

Attachment: Bug 1879625 - Load settings into search engines at the time of their creation.

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

Comment 13

[Dianna Smith [:diannaS]]

: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

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/b156916898e6
https://hg.mozilla.org/releases/mozilla-beta/rev/66ae0f7c126b

Comment 15

[Adrian Florinescu [:aflorinescu]]

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.

Comment 16

[Adrian Florinescu [:aflorinescu]]

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