Closed
Bug 878690
Opened 11 years ago
Closed 11 years ago
SafariProfileMigrator.js should use FormHistory.jsm
Categories
(Firefox :: Migration, defect)
Firefox
Migration
Tracking
()
RESOLVED
FIXED
Firefox 24
People
(Reporter: markh, Assigned: markh)
References
Details
Attachments
(1 file)
1.64 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
We are trying to remove nsIFormHistory2 which is used by SafariProfileMigrator.js - which should be updated to use FormHistory.jsm
Assignee | ||
Updated•11 years ago
|
OS: Windows 7 → Mac OS X
Hardware: x86_64 → All
Assignee | ||
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52b59291870
Comment 6•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a52b59291870
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in
before you can comment on or make changes to this bug.
Description
•