1237648 - The defaultEngine attribute should just be an alias to the currentEngine attribute

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Florian Quèze [:florian]]

A lot of confusion happened in the past due to the search service's defaultEngine and currentEngine attributes being almost identical but subtly different.

There has been some attempts to keep these properties in sync for Firefox-only in the past (bug 860560, bug 885351). So in theory these 2 properties always have the same value for Firefox.

Fennec seems to only use the defaultEngine property and to ignore the currentEngine one.

The subtle (unintentional) difference between these 2 properties, in addition to consistently confusing developers, is suspected to be the cause of bugs like bug 1113692.

The way forward I'm proposing is to keep both attributes (to avoid breaking add-ons), but with a single implementation behind them, so that they are guaranteed to always have matching values.

Comment 1

[Florian Quèze [:florian]]

Attachment: remove the defaultEngine implementation and forward the getter/setter to currentEngine

One behavior difference that was actually enforced by the tests is that when the original default engine is removed (ie. hidden), defaultEngine fallbacks to the next visible engine until the original default is unhidden, whereas the currentEngine attribute won't switch back automatically (intentional behavior implemented in bug 1227045).

Comment 2

[Florian Quèze [:florian]]

Attachment: remove the browser/ code sync'ing currentEngine and defaultEngine

This removes the sync'ing code that has been added in bug 860560 and bug 885351 (this may turn out to be a perf win, as this code was running at startup, and visible during my investigations for bug 1230661), and cleans up some code that was listening to both the engine-default and engine-current notifications, which are now fully identical.

Comment 3

[Florian Quèze [:florian]]

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

Comment 4

[Florian Quèze [:florian]]

Attachment: Cleanup usage of the search service in nsContextMenu.js and nsBrowserContentHandler.js

There are several other places in nsBrowserContentHandler.js where using Services.* instead of getService would be cleaner, but I'll keep that for a mentored follow-up bug.

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8705205 [details] [diff] [review]
Cleanup usage of the search service in nsContextMenu.js and nsBrowserContentHandler.js

Review of attachment 8705205 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work Florian!

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/716ab904d3f3499135f04541d5d0366a7dc81621
Bug 1237648 - remove the defaultEngine implementation and forward the getter/setter to currentEngine, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/fae7cd42e651bbdf80ed7a9d15cf79f851bf64c8
Bug 1237648 - remove the browser/ code sync'ing currentEngine and defaultEngine, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/e12dc3542ba00415e1b2cb316141e9d170a23270
Bug 1237648 - Cleanup usage of the search service in nsContextMenu.js and nsBrowserContentHandler.js, r=adw.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/716ab904d3f3
https://hg.mozilla.org/mozilla-central/rev/fae7cd42e651
https://hg.mozilla.org/mozilla-central/rev/e12dc3542ba0

Comment 8

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)

> There are several other places in nsBrowserContentHandler.js where using
> Services.* instead of getService would be cleaner, but I'll keep that for a
> mentored follow-up bug.

I filed bug 1238537 for this.

Comment 9

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected