Closed Bug 1238516 Opened 4 years ago Closed 4 years ago

Migrate the defaultenginename preference

Categories

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

46 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed
fennec 46+ ---

People

(Reporter: florian, Assigned: Margaret)

References

Details

Attachments

(1 file)

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;
tracking-fennec: --- → ?
Assignee: nobody → 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?
Duplicate of this bug: 1240382
(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.
Attachment #8709146 - Flags: review?(florian) → review+
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.
(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.
(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.
(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.
tracking-fennec: ? → 46+
https://hg.mozilla.org/mozilla-central/rev/4abbb197ebfe
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.