Closed Bug 1237648 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(3 files)

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.
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).
Attachment #8705187 - Flags: review?(adw)
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.
Attachment #8705188 - Flags: review?(adw)
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.
Attachment #8705205 - Flags: review?(adw)
Attachment #8705187 - Flags: review?(adw) → review+
Attachment #8705188 - Flags: review?(adw) → review+
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!
Attachment #8705205 - Flags: review?(adw) → review+
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.
Depends on: 1238516
(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.
[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
Blocks: 1265881
You need to log in before you can comment on or make changes to this bug.