Closed Bug 1094876 Opened 7 years ago Closed 7 years ago

Migrators should use new Bookmarks.jsm API

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox40 --- verified

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Chrome, IE and Safari migrators should be using the new async API.
Flags: qe-verify+
Flags: firefox-backlog+
No longer depends on: 1094875
This is very trivial. The terrible migration UI is async-friendly :) We'll just need to test this very carefully (there are no automated tests), and make sure QA focuses on testing the migrators.
Assignee: nobody → mano
Points: 5 → 3
Assignee: mano → nobody
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Attached patch patch v1 (obsolete) — Splinter Review
Attaching current patch, I must still do manual testing of the migration.
Attached patch patch v1.1 (obsolete) — Splinter Review
IE and Chrome are OK. now testing Safari.
Attachment #8593427 - Attachment is obsolete: true
Depends on: 1155705
Attached patch patch v1.2 (obsolete) — Splinter Review
tested Safari (clearly with the dependency bug fixed)
Attachment #8594028 - Attachment is obsolete: true
Attachment #8594032 - Flags: review?(ttaubert)
Comment on attachment 8594032 [details] [diff] [review]
patch v1.2

Review of attachment 8594032 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks for all the manual testing!

::: browser/components/migration/ChromeProfileMigrator.js
@@ +62,2 @@
>  
> +        insertBookmarkItems(newFolderGuid, item.children);

yield insertBookmarkItems(...);

@@ +192,5 @@
> +                             },
> +                             (inputStream, resultCode) => {
> +                               if (!Components.isSuccessCode(resultCode))
> +                                 reject(new Error("Could not read Bookmarks file"));
> +                               resolve(inputStream);

I know we can't resolve a rejected promise but maybe use "else" to the code a little clearer?

::: browser/components/migration/IEProfileMigrator.js
@@ +209,5 @@
> +            folderGuid = yield PlacesUtils.bookmarks.insert({
> +              type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +              parentGuid: aDestFolderGuid,
> +              title: entry.leafName
> +            });

Shouldn't that set folderGuid to the .guid property of the bookmark?

::: browser/components/migration/SafariProfileMigrator.js
@@ +153,5 @@
>        if (type == "WebBookmarkTypeList" && entry.has("Children")) {
>          let title = entry.get("Title");
> +        let newFolderGuid = yield PlacesUtils.bookmarks.insert({
> +          parentGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER, title
> +        });

Shouldn't newFolderGuid be set to the .guid property?
Attachment #8594032 - Flags: review?(ttaubert) → review+
Attached patch patch v1.3Splinter Review
yep, good catches
Attachment #8594032 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ea414edf26e3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Regression testing was performed today using Firefox 40 Beta 4 on: Win 7 x86, Win 10 x64, Mac OS X 10.9.5, Ubuntu 14.04 x64. No new issues were found during testing. Detailed test results can be found at: https://docs.google.com/spreadsheets/d/1ZGOcdnEVy8RC1OPf2YPzKNPY3Df9lLmrdKarwjPcbFA/edit#gid=34497851.

Marking this as Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.