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 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?

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.

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

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!

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

https://hg.mozilla.org/mozilla-central/rev/90e25222129c https://hg.mozilla.org/mozilla-central/rev/ce47c3bc026a

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

(to be clear, uplift request is for both csets.)

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!

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d9b17867a0e https://hg.mozilla.org/releases/mozilla-aurora/rev/03017680b934

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 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).

Comment on attachment 8845013 [details] Bug 1344759 - add a unit test for Chrome bookmark imports, [Triage Comment]

remote: https://hg.mozilla.org/releases/mozilla-beta/rev/7233d987658b891e3c518302b7eca1179f13c508 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/1d65f4570b053341251db1477486c826a88f5769

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.