1344759 - Use insertTree API to import bookmarks from other browsers

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (out for now; he/him)]

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 1

[:Gijs (out for now; he/him)]

Attachment: Bug 1344759 - add a unit test for Chrome bookmark imports,

Review commit: https://reviewboard.mozilla.org/r/118254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118254/

Comment 2

[:Gijs (out for now; he/him)]

Attachment: Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome,

Review commit: https://reviewboard.mozilla.org/r/118256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118256/

Comment 3

[Dão Gottwald [:dao]]

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 4

[Dão Gottwald [:dao]]

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 5

[:Gijs (out for now; he/him)]

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

[Dão Gottwald [:dao]]

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 7

[:Gijs (out for now; he/him)]

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 8

[:Gijs (out for now; he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118254/diff/1-2/

Comment 9

[:Gijs (out for now; he/him)]

Comment on attachment 8845014 [details]
Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118256/diff/1-2/

Comment 10

[:Gijs (out for now; he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118254/diff/2-3/

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8845014 [details]
Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118256/diff/2-3/

Comment 12

[:Gijs (out for now; he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118254/diff/3-4/

Comment 13

[:Gijs (out for now; he/him)]

Comment on attachment 8845014 [details]
Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118256/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[:Gijs (out for now; he/him)]

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

Comment 17

[:Gijs (out for now; he/him)]

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

Comment 18

[Gerry Chang [:gchang]]

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!

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Brindusa Tot, Desktop QA]

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

[Liz Henry (:lizzard) (relman/hg->git project)]

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 22

[Liz Henry (:lizzard) (relman/hg->git project)]

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

[Triage Comment]

Comment 23

[:Gijs (out for now; he/him)]

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

Comment 24

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.