If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SafariProfileMigrator.js should use FormHistory.jsm

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Migration
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
Firefox 24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
We are trying to remove nsIFormHistory2 which is used by SafariProfileMigrator.js - which should be updated to use FormHistory.jsm
(Assignee)

Updated

4 years ago
OS: Windows 7 → Mac OS X
Hardware: x86_64 → All
(Assignee)

Comment 1

4 years ago
This is for Windows and Mac
OS: Mac OS X → All
(Assignee)

Comment 2

4 years ago
Created attachment 757238 [details] [diff] [review]
Use FormHistory.jsm in the Safari migrator

This patch uses FormHistory instead of nsIFormHistory2.  There don't seem to be tests for this, so I tested it by:

* Installing Safari on a Window box and performing a search in the Safari searchbox for "safari history".
* Started Firefox in a new profile on the machine and indicating it should import data from Safari.
* In Firefox, went to the searchbar and entered "s"
* In the dropdown for the searchbar, verified that "safari history" appeared as a previous search (ie, it was "above the line")

Which I *think* is appropriate...
Assignee: nobody → mhammond
Attachment #757238 - Flags: review?(mak77)
(In reply to Mark Hammond (:markh) from comment #2)
> There don't seem to
> be tests for this

yes, we don't have a good test harness to verify interactions between different browsers. there some bugs filed to make one, but no ETA.

> * Installing Safari on a Window box and performing a search in the Safari
> searchbox for "safari history".
> * Started Firefox in a new profile on the machine and indicating it should
> import data from Safari.
> * In Firefox, went to the searchbar and entered "s"
> * In the dropdown for the searchbar, verified that "safari history" appeared
> as a previous search (ie, it was "above the line")

This sounds good, the only "negative" here is that Safari for Windows is a discontinued product, the Mac version may have changed, though since the scope here is not to verify if migration works, but rather to verify if the code change is equivalent, I think it's ok.
Comment on attachment 757238 [details] [diff] [review]
Use FormHistory.jsm in the Safari migrator

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

this looks good to me, thank you. Removing synchronous form history consumers has a great value.

::: browser/components/migration/src/SafariProfileMigrator.js
@@ +540,5 @@
>          if (aDict.has("RecentSearchStrings")) {
>            let recentSearchStrings = aDict.get("RecentSearchStrings");
>            if (recentSearchStrings && recentSearchStrings.length > 0) {
> +            let formHistory = Cu.import("resource://gre/modules/FormHistory.jsm", {})
> +                              .FormHistory;

I'd rather put this as a defineLazyModuleGetter at the top of this file, near other modules
Attachment #757238 - Flags: review?(mak77) → review+
(Assignee)

Comment 5

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52b59291870

Comment 6

4 years ago
https://hg.mozilla.org/mozilla-central/rev/a52b59291870
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.