Closed Bug 1344759 Opened 3 years ago Closed 3 years ago

Use insertTree API to import bookmarks from other browsers

Categories

(Firefox :: Migration, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox53 --- verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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?
Attachment #8845013 - Flags: review?(dao+bmo) → review+
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.
Attachment #8845014 - Flags: review?(dao+bmo) → 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

> 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
Blocks: 1347530
https://hg.mozilla.org/mozilla-central/rev/90e25222129c
https://hg.mozilla.org/mozilla-central/rev/ce47c3bc026a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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?
(to be clear, uplift request is for both csets.)
Depends on: 1348103
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+
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
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+
Comment on attachment 8845013 [details]
Bug 1344759 - add a unit test for Chrome bookmark imports,

[Triage Comment]
Attachment #8845013 - Flags: approval-mozilla-beta+
Flags: qe-verify+
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.
Depends on: 1353041
Depends on: 1353373
You need to log in before you can comment on or make changes to this bug.