Closed
Bug 1344759
Opened 7 years ago
Closed 7 years ago
Use insertTree API to import bookmarks from other browsers
Categories
(Firefox :: Migration, defect)
Firefox
Migration
Tracking
()
VERIFIED
FIXED
Firefox 55
People
(Reporter: Gijs, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
dao
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
dao
:
review+
gchang
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
In bug 1344282 I'm creating an insertTree Bookmarks.jsm API that inserts multiple bookmarks in one go. Initial evidence suggests this dramatically speeds up bookmarks import and reduces jank/hangs while the import is ongoing. We should use it from the various importers.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8845013 [details] Bug 1344759 - add a unit test for Chrome bookmark imports, https://reviewboard.mozilla.org/r/118254/#review120462 Shouldn't you delete the imported bookmarks as well as chromefiles/ when you're done? ::: browser/components/migration/tests/unit/test_Chrome_bookmarks.js:77 (Diff revision 1) > + Assert.ok(migrator.sourceExists); > + > + let itemsSeen = {bookmarks: 0, folders: 0}; > + let bmObserver = { > + onItemAdded(aItemId, aParentId, aIndex, aItemType, aURI, aTitle) { > + if (!aTitle.includes("Chrome")) { What's the point of this check?
Attachment #8845013 -
Flags: review?(dao+bmo) → review+
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8845013 [details] Bug 1344759 - add a unit test for Chrome bookmark imports, https://reviewboard.mozilla.org/r/118254/#review120462 > What's the point of this check? Nevermind, I think I understand this now that I'm looking at the second patch.
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8845013 [details] Bug 1344759 - add a unit test for Chrome bookmark imports, https://reviewboard.mozilla.org/r/118254/#review120462 I don't think it matters for xpcshell tests - the safari one doesn't do it either. I can update both of them to get this addressed, though. > Nevermind, I think I understand this now that I'm looking at the second patch. It's because we wrap non-startup-migrated bookmarks in an "Imported <browser> bookmarks" kind of folder, and we don't want to count those folders here. :-)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8845014 [details] Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, https://reviewboard.mozilla.org/r/118256/#review120472 ::: browser/components/migration/EdgeProfileMigrator.js:199 (Diff revision 1) > if (!readingListItems.length) { > return; > } > > let destFolderGuid = yield this._ensureReadingListFolder(parentGuid); > - let exceptionThrown; > + readingListItems = readingListItems.map(item => { Instead of two loops (map then filter), this will likely be faster as a for-of loop creating the array... like you did in _fetchBookmarksFromDB, apparently. ::: browser/components/migration/EdgeProfileMigrator.js:328 (Diff revision 1) > - > - // Hacks! The bookmarks bar is special: > - if (folder.Title == "_Favorites_Bar_") { > - let toolbarGuid = PlacesUtils.bookmarks.toolbarGuid; > - if (!MigrationUtils.isStartupMigration) { > - toolbarGuid = > + bmToInsert = { > + dateAdded: bookmark.DateUpdated || new Date(), > + title: bookmark.Title, > + url: bookmark.URL, > + }; > + } else { A lot of indentation here. "} eles /* bookmark.IsFolder */ {" would make this a bit more readable. ::: browser/components/migration/MigrationUtils.jsm:1000 (Diff revision 1) > + let insertionPromise = PlacesUtils.bookmarks.insertTree(bookmarks, parent); > + return insertionPromise.then(insertedItems => { > + this._importQuantities.bookmarks += insertedItems.length; > + if (gKeepUndoData) { > + let bmData = gUndoData.get("bookmarks"); > + gUndoData.set("bookmarks", bmData.concat(insertedItems.map(bm => { Again this doesn't seem ideal. I suspect a for-of loop calling bmData.push would be faster.
Attachment #8845014 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8845014 [details] Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, https://reviewboard.mozilla.org/r/118256/#review120472 > A lot of indentation here. "} eles /* bookmark.IsFolder */ {" would make this a bit more readable. Added a comment. Thanks!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/90e25222129c add a unit test for Chrome bookmark imports, r=dao https://hg.mozilla.org/integration/autoland/rev/ce47c3bc026a batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, r=dao
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/90e25222129c https://hg.mozilla.org/mozilla-central/rev/ce47c3bc026a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8845014 [details] Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, Approval Request Comment [Feature/Bug causing the regression]: bookmarks import from other browsers [User impact if declined]: imports can cause multi-minute hangs [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: not yet, but there are automated tests. [Needs manual test from QE? If yes, steps to reproduce]: it would be a good idea 1. open the browser 2. open the library 3. import bookmarks (but not history/passwords/cookies) from Chrome/IE/Edge/Safari for cases where the Chrome/IE/Edge/Safari profiles have many (100s) of bookmarks) After the fixes in this bug, the time spent on the import should be significantly shorter. If you're importing a large number of bookmarks you can tell the difference, but you can also use about:telemetry to verify that the numbers for the FX_MIGRATION_BOOKMARKS_MS histogram are lower. Of course, the other thing that we should check is that bookmarks are imported correctly, especially whether bookmark hierarchies with folders are imported correctly. [List of other uplifts needed for the feature/fix]: bug 1344282 [Is the change risky?]: not very [Why is the change risky/not risky?]: because there are pretty reasonable automated tests for the relevant bits of code [String changes made/needed]: no
Attachment #8845014 -
Flags: approval-mozilla-beta?
Attachment #8845014 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 17•7 years ago
|
||
(to be clear, uplift request is for both csets.)
Updated•7 years ago
|
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Comment 18•7 years ago
|
||
Comment on attachment 8845014 [details] Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, According to comment #27 in bug 1344282, we can take this in Aurora54 and I would like it to ride the train on 54. Aurora54+ & Beta53-. Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Attachment #8845014 -
Flags: approval-mozilla-beta?
Attachment #8845014 -
Flags: approval-mozilla-beta-
Attachment #8845014 -
Flags: approval-mozilla-aurora?
Attachment #8845014 -
Flags: approval-mozilla-aurora+
Updated•7 years ago
|
Comment 19•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d9b17867a0e https://hg.mozilla.org/releases/mozilla-aurora/rev/03017680b934
Comment 20•7 years ago
|
||
I can confirm that the time spent on the importing a large number of bookmarks is significantly shorter on latest Nightly 55.0a1, Build ID 20170323030203, then on a build without the fix (Nightly 54.0a1 Build ID 20170302030206). Also, the value of FX_MIGRATION_BOOKMARKS_IMPORT_MS is lower: - on latest Nightly 55.0a1, Build ID 20170323030203 - 455 - on Nightly 54.0a1 Build ID 20170302030206 - 4588 Verified bug on Window 10 x64, Ubuntu 16.04 x64, Mac OS X 10.10
Comment 21•7 years ago
|
||
Comment on attachment 8845014 [details] Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, Let's give this a shot in beta (for beta 7).
Attachment #8845014 -
Flags: approval-mozilla-beta- → approval-mozilla-beta+
Updated•7 years ago
|
Comment 22•7 years ago
|
||
Comment on attachment 8845013 [details] Bug 1344759 - add a unit test for Chrome bookmark imports, [Triage Comment]
Attachment #8845013 -
Flags: approval-mozilla-beta+
Assignee | ||
Comment 23•7 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/7233d987658b891e3c518302b7eca1179f13c508 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/1d65f4570b053341251db1477486c826a88f5769
Updated•7 years ago
|
Flags: qe-verify+
Comment 24•7 years ago
|
||
Reproduced the issue on 54.0a1 (2017-03-02). The bookmark importing time is significantly shorter on 53.0b8 (20170330120906) and 54.0a2 (2017-03-31), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6. Marking this bug as verified fixed on above mentioned builds.
You need to log in
before you can comment on or make changes to this bug.
Description
•