1238516 - Migrate the defaultenginename preference

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Florian Quèze [:florian]]

In bug 1237648 I removed the search service's defaultEngine implementation in favor of the currentEngine implemention (see attachment 8705187 [details] [diff] [review]). Unfortunately, it seems that Fennec was using defaultEngine, so in order to avoid losing the user's selected default browser, some migration code may be needed.

In pseudo code, I think the migration needed is something like:

let name;
if ("browser.search.defaultenginename" has user value)
  name = getCharPref("browser.search.defaultenginename");
if (!name && "browser.search.defaultenginename.US" has user value)
  name = getCharPref("browser.search.defaultenginename.US");
let engine;
if (name)
  engine = Services.search.getEngineByName(name);
if (engine)
  Services.search.defaultEngine = engine;

Comment 1

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1238516 - Migrate the defaultenginename preference. r=florian

Review commit: https://reviewboard.mozilla.org/r/31301/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31301/

Comment 2

[:Margaret Leibovic]

I had a tough time coming up with the best way to test this. Let me know if you think this works.

Also, should we be updating any defaultEngine API consumers to use currentEngine instead?

Comment 4

[Florian Quèze [:florian]]

(In reply to :Margaret Leibovic from comment #2)

> Also, should we be updating any defaultEngine API consumers to use
> currentEngine instead?

I used to want to remove one of the two attributes, but given the burden this would impose on add-on authors for very little benefit, I changed my mind. I currently have no plan to remove either of these attributes. The key benefit of bug 1237648 (in addition to removing some code) is that bugs caused by inconsistent usage of currentEngine/defaultEngine won't exist anymore.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8709146 [details]
MozReview Request: Bug 1238516 - Migrate the defaultenginename preference. r=florian

https://reviewboard.mozilla.org/r/31301/#review28125

Looks reasonable. I'm assuming you have tested this.

::: mobile/android/chrome/content/browser.js:1099
(Diff revision 1)
> +        engine = Services.search.getEngineByName(name);

This will force the search service to initialize itself synchronously at startup. This is unfortunate, but given that this code will run only once and only for the users who have customized their default engine, this may be acceptable.

Comment 6

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a48d2ef3626d

Comment 7

[:Margaret Leibovic]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Comment on attachment 8709146 [details]
> MozReview Request: Bug 1238516 - Migrate the defaultenginename preference.
> r=florian
> 
> https://reviewboard.mozilla.org/r/31301/#review28125
> 
> Looks reasonable. I'm assuming you have tested this.
> 
> ::: mobile/android/chrome/content/browser.js:1099
> (Diff revision 1)
> > +        engine = Services.search.getEngineByName(name);
> 
> This will force the search service to initialize itself synchronously at
> startup. This is unfortunate, but given that this code will run only once
> and only for the users who have customized their default engine, this may be
> acceptable.

I updated the patch to use the async init method instead. We don't need the new search engine immediately, and even if this only happens once, it's worth avoiding the sync call if we can.

To make the test pass, I decided to use an observer notification, but I still think that's better than the synchronous initialization.

I'll wait on the try push before landing this.

Comment 8

[Florian Quèze [:florian]]

(In reply to :Margaret Leibovic from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #5)

> > ::: mobile/android/chrome/content/browser.js:1099
> > (Diff revision 1)
> > > +        engine = Services.search.getEngineByName(name);
> > 
> > This will force the search service to initialize itself synchronously at
> > startup. This is unfortunate, but given that this code will run only once
> > and only for the users who have customized their default engine, this may be
> > acceptable.
> 
> I updated the patch to use the async init method instead. We don't need the
> new search engine immediately, and even if this only happens once, it's
> worth avoiding the sync call if we can.
> 
> To make the test pass, I decided to use an observer notification, but I
> still think that's better than the synchronous initialization.
> 
> I'll wait on the try push before landing this.

Two comments on the patch you pushed to try:
- if |name| isn't truthy (ie. the user hasn't customized the default engine), I think it would be better to avoid initializing the search service from the migration code.
- If the browser quits before the search service has finished initializing, the user's default engine choice will be lost, as the browser.migration.version pref will have already been bumped. That's probably acceptable.

Comment 9

[:Margaret Leibovic]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #5)
> 
> > > ::: mobile/android/chrome/content/browser.js:1099
> > > (Diff revision 1)
> > > > +        engine = Services.search.getEngineByName(name);
> > > 
> > > This will force the search service to initialize itself synchronously at
> > > startup. This is unfortunate, but given that this code will run only once
> > > and only for the users who have customized their default engine, this may be
> > > acceptable.
> > 
> > I updated the patch to use the async init method instead. We don't need the
> > new search engine immediately, and even if this only happens once, it's
> > worth avoiding the sync call if we can.
> > 
> > To make the test pass, I decided to use an observer notification, but I
> > still think that's better than the synchronous initialization.
> > 
> > I'll wait on the try push before landing this.
> 
> Two comments on the patch you pushed to try:
> - if |name| isn't truthy (ie. the user hasn't customized the default
> engine), I think it would be better to avoid initializing the search service
> from the migration code.

Good catch, thanks. I updated that.

> - If the browser quits before the search service has finished initializing,
> the user's default engine choice will be lost, as the
> browser.migration.version pref will have already been bumped. That's
> probably acceptable.

I'm fine with living with that edge case. It's not impossible to update your default search engine again if you hit this issue.

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/4abbb197ebfefe61315229a55ab57482ab50108d
Bug 1238516 - Migrate the defaultenginename preference. r=florian

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4abbb197ebfe