878690 - SafariProfileMigrator.js should use FormHistory.jsm

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

We are trying to remove nsIFormHistory2 which is used by SafariProfileMigrator.js - which should be updated to use FormHistory.jsm

Comment 1

[Mark Hammond [:markh] [:mhammond]]

This is for Windows and Mac

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 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...

Comment 3

[Marco Bonardo [:mak]]

(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 4

[Marco Bonardo [:mak]]

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

Comment 5

[Mark Hammond [:markh] [:mhammond]]

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

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a52b59291870