Closed Bug 1116446 Opened 5 years ago Closed 5 years ago

Cannot restore bookmarks from backup JSON file.

Categories

(SeaMonkey :: Bookmarks & History, defect, major)

defect
Not set
major

Tracking

(seamonkey2.30 wontfix, seamonkey2.31 wontfix, seamonkey2.32+ fixed, seamonkey2.33+ fixed, seamonkey2.34+ fixed)

RESOLVED FIXED
seamonkey2.34
Tracking Status
seamonkey2.30 --- wontfix
seamonkey2.31 --- wontfix
seamonkey2.32 + fixed
seamonkey2.33 + fixed
seamonkey2.34 + fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(3 files, 2 obsolete files)

bug 852034 replaces PlacesUtils.restoreBookmarksFromJSONFile with BookmarkJSONUtils.importFromFile,
then bug 854288 removes PlacesUtils.restoreBookmarksFromJSONFile.

it breaks the SeaMonkey's bookmark restoration feature, which still uses PlacesUtils.restoreBookmarksFromJSONFile.
http://dxr.mozilla.org/comm-central/source/suite/common/bookmarks/bookmarksManager.js#436

Need to port some of bug 852034's patch to comm-central.

it's also reproducible on SeaMonkey 2.31.
Also, may need to port some of bug 852041's patch to make _initPlaces asynchronous.
currently jsonlz4 is used for bookmark backup format,
so this patch is needed to support restore from in-profile backup.
Attachment #8542532 - Flags: review?(iann_bugzilla)
This patch fixes Restore menu in bookmark manager.
Attachment #8542533 - Flags: review?(iann_bugzilla)
This patch fixes the automated bookmark restoration when database is corrupted.

Tested on Mac OS X 10.9.5 with following steps:
  1. run SeaMonkey with clean profile
  2. restore from JSON file which is exported from other profile (Part 2)

  3. backup to some file, to make backup in bookmarkbakcups folder
  4. remove some bookmark items
  5. restore from today's backup (Part 1)

  6. shutdown SeaMonkey
  7. remove places.sqlite
  8. run SeaMonkey, and confirm bookmark is restored (Part 3)
Attachment #8542535 - Flags: review?(iann_bugzilla)
See also, Bug 1061672 - Import json-file from Firefox (or even from SeaMonkey) do not work.
Comment on attachment 8542532 [details] [diff] [review]
Bug 1116446 Part 1: Support jsonlz4 format for restore in_bookmarksManager_js.patch

Neil may be a better reviewer.
Attachment #8542532 - Attachment description: 1-Bug_1116446___Part_1__Support_jsonlz4_format_for_restore_in_bookmarksManager_js.patch → 1 Bug_1116446 Part_1 Support jsonlz4 format for restore in_bookmarksManager_js.patch
Attachment #8542532 - Flags: review?(neil)
Attachment #8542533 - Flags: review?(neil)
Attachment #8542535 - Flags: review?(neil)
Attachment #8542532 - Attachment description: 1 Bug_1116446 Part_1 Support jsonlz4 format for restore in_bookmarksManager_js.patch → Bug 1116446 Part 1: Support jsonlz4 format for restore in_bookmarksManager_js.patch
Comment on attachment 8542532 [details] [diff] [review]
Bug 1116446 Part 1: Support jsonlz4 format for restore in_bookmarksManager_js.patch

Notwithstanding that the old code used match to test a string against a regexp when it should have used test instead, but not only did you compound the error by adding an endsWidth test instead of extending the existing regexp but you also copied Firefox's error of not testing for the . in the extension.
Attachment #8542532 - Flags: review?(neil) → review-
Comment on attachment 8542533 [details] [diff] [review]
Part 2: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in bookmarksManager.js.

>+    Task.spawn(function() {
Aren't generators supposed to use function*()?

>+        yield BookmarkJSONUtils.importFromFile(aFile, true);
This wants a path, not an nsIFile object.

>+    }.bind(this));
Rather than bind a generator to a task, might it be easier to catch the rejection on the returned Promise?
Attachment #8542535 - Flags: review?(neil) → review+
(In reply to comment #7)
> Notwithstanding that the old code used match to test a string against a
> regexp when it should have used test instead, but not only did you compound
> the error by adding an endsWidth test instead of extending the existing
> regexp but you also copied Firefox's error of not testing for the . in the
> extension.

PlacesBackups.getMostRecentBackup almost has the right regexp, but they still slipped up. Oops!
Thank you for reviewing :D
merged two tests, and use .test() method.
Attachment #8542532 - Attachment is obsolete: true
Attachment #8542532 - Flags: review?(iann_bugzilla)
Attachment #8542597 - Flags: review?(neil)
Fixed aFile to aFile.path, and use .catch().

(In reply to neil@parkwaycc.co.uk from comment #8)
> Comment on attachment 8542533 [details] [diff] [review]
> Part 2: Use BookmarkJSONUtils.importFromFile instead of
> PlacesUtils.restoreBookmarksFromJSONFile in bookmarksManager.js.
> 
> >+    Task.spawn(function() {
> Aren't generators supposed to use function*()?
Yeah, it also should be addressed in Firefox's side,
we should remove all legacy generator (bug 1083482).
Attachment #8542533 - Attachment is obsolete: true
Attachment #8542533 - Flags: review?(neil)
Attachment #8542533 - Flags: review?(iann_bugzilla)
Attachment #8542598 - Flags: review?(neil)
Attachment #8542597 - Flags: review?(neil) → review+
Attachment #8542598 - Flags: review?(neil) → review+
Attachment #8542535 - Flags: review?(iann_bugzilla)
Comment on attachment 8542598 [details] [diff] [review]
Part 2: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in bookmarksManager.js.

[Approval Request Comment]
Regression caused by (bug #): bug 852034 bug 854288
User impact if declined: Cannot restore bookmark from backup JSON file
Testing completed (on m-c, etc.): Port of bug 852034 bug 854288
Risk to taking this patch (and alternatives if risky): none bustage fix
String changes made by this patch: None.
Attachment #8542598 - Flags: approval-comm-beta?
Attachment #8542598 - Flags: approval-comm-aurora?
Comment on attachment 8542597 [details] [diff] [review]
Part 1: Support jsonlz4 format for restore in bookmarksManager.js

[Approval Request Comment]
Regression caused by (bug #): bug 852034 bug 854288
User impact if declined: Cannot restore bookmark from backup JSON file
Testing completed (on m-c, etc.): Port of bug 852034 bug 854288
Risk to taking this patch (and alternatives if risky): none bustage fix
String changes made by this patch: None.
Attachment #8542597 - Flags: approval-comm-beta?
Attachment #8542597 - Flags: approval-comm-aurora?
Comment on attachment 8542535 [details] [diff] [review]
Part 3: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in nsSuiteGlue.js.

[Approval Request Comment]
Regression caused by (bug #): bug 852034 bug 854288
User impact if declined: Cannot restore bookmark from backup JSON file
Testing completed (on m-c, etc.): Port of bug 852034 bug 854288
Risk to taking this patch (and alternatives if risky): none bustage fix
String changes made by this patch: None.
Attachment #8542535 - Flags: approval-comm-beta?
Attachment #8542535 - Flags: approval-comm-aurora?
Assignee: nobody → arai_a
Attachment #8542535 - Flags: approval-comm-beta?
Attachment #8542535 - Flags: approval-comm-beta+
Attachment #8542535 - Flags: approval-comm-aurora?
Attachment #8542535 - Flags: approval-comm-aurora+
Attachment #8542597 - Flags: approval-comm-beta?
Attachment #8542597 - Flags: approval-comm-beta+
Attachment #8542597 - Flags: approval-comm-aurora?
Attachment #8542597 - Flags: approval-comm-aurora+
Attachment #8542598 - Flags: approval-comm-beta?
Attachment #8542598 - Flags: approval-comm-beta+
Attachment #8542598 - Flags: approval-comm-aurora?
Attachment #8542598 - Flags: approval-comm-aurora+
Thank you for reviewing and approval :D
Keywords: checkin-needed
Blocks: 1117577
You need to log in before you can comment on or make changes to this bug.