Closed Bug 356708 Opened 18 years ago Closed 18 years ago

Implement automatic bookmarks backup for 1.1


(Camino Graveyard :: Bookmarks, defect)

Not set


(Not tracked)



(Reporter: alqahira, Assigned: stuart.morgan+bugzilla)


(Keywords: fixed1.8.0.9, fixed1.8.1.1)


(2 files, 1 obsolete file)

Since we're not getting any useful data out of our logging for bug 337750 and people are continuing to see it (lots, recently, with 1.0.3), we probably need to implement some system of automatically backing up bookmarks when they are in the known-good state (a la Fx).

Something like: if, at startup, we find the bookmarks plist to be good, copy it to $profile/bookmarksbackup/bookmarks-datestamp.plist

In order to keep this folder from filling up with copies of 3 MB plists ;) it should only store the last 5 good revisions or something like that.

This way we at least stop the bleeding even if we can't find the wound yet....
Attached patch tourniquet (obsolete) — Splinter Review
Slows the bleeding.  Makes a backup on launch if the load succeeds and tries that backup if the bookmarks file is corrupt on a later launch.

Also makes a few other improvements to the failure case:
- installs the default plist if all else fails so the bookmarks won't all be completely empty
- tosses the alert dialog from the main thread, so we don't get weird visual corruption like we can currently

Also, our current dialog is very jargony:
"The bookmarks file could not be read, because its contents don’t conform to the expected format."
We should change it to:
"Your bookmarks file is damaged and could not be read."
Assignee: nobody → stuart.morgan
Attachment #245982 - Flags: review?
Flags: camino1.1a2?
Comment on attachment 245982 [details] [diff] [review]

>Index: src/bookmarks/
>RCS file: /cvsroot/mozilla/camino/src/bookmarks/,v
>retrieving revision 1.75
>diff -u -8 -r1.75
>--- src/bookmarks/	11 Nov 2006 20:58:49 -0000	1.75
>+++ src/bookmarks/	19 Nov 2006 23:44:12 -0000

>@@ -1233,54 +1234,81 @@
>   // figure out where Bookmarks.plist is and store it as mPathToBookmarkFile
>   // if there is a Bookmarks.plist, read it
>   // if there isn't a Bookmarks.plist, but there is a bookmarks.xml, read it.
>   // if there isn't either, move default Bookmarks.plist to profile dir & read it.
>   //
>   NSFileManager *fM = [NSFileManager defaultManager];
>   NSString *bookmarkPath = [profileDir stringByAppendingPathComponent:@"bookmarks.plist"];
>   [self setPathToBookmarkFile:bookmarkPath];
>+  BOOL bookmarksAreCorrupt = NO;
>   if ([fM isReadableFileAtPath:bookmarkPath]) {
>-    if ([self readPListBookmarks:bookmarkPath])
>+    NSString *backupPath = [bookmarkPath stringByAppendingString:@".bak"];
>+    if ([self readPListBookmarks:bookmarkPath]) {
>+      // since the bookmarks look good, save them aside as a backup in case something goes
>+      // wrong later (e.g., bug 337750) since users really don't like losing their bookmarks.
>+      NSString *backupPath = [bookmarkPath stringByAppendingString:@".bak"];

Why is this line in there twice? It seems like declaring it above would have been quite sufficient ;)

r=me with that addressed.
Attachment #245982 - Flags: review? → review+
Attached patch v2Splinter Review
Removed the double-declaration. Also fixed a potential NSLog crasher just for kicks.
Attachment #245982 - Attachment is obsolete: true
Attachment #246589 - Flags: superreview?(mikepinkerton)
Comment on attachment 246589 [details] [diff] [review]

Attachment #246589 - Flags: superreview?(mikepinkerton) → superreview+
Checked in on trunk and MOZILLA_1_8_BRANCH.
Closed: 18 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Flags: camino1.1a2?
Flags: camino1.1a2+
Flags: camino1.1?
Flags: camino1.1+
Flags: camino1.0.4?
Comment on attachment 246589 [details] [diff] [review]

This is going to need a new patch for 1.0.4 (that pulls it out from among all the logging, probably)
Attached patch 1.8.0 PatchSplinter Review
Patch for 1.8.0 branch.  Stuart, can you sanity check my porting?
Attachment #248793 - Flags: review?(stuart.morgan)
Comment on attachment 248793 [details] [diff] [review]
1.8.0 Patch

Looks fine; I'm assuming it's been tested.
Attachment #248793 - Flags: review?(stuart.morgan) → review+
1.8.0 patch seems to work OK here in contrived situations.

(We still need to make the string fix in comment 2 for trunk/1.1, too.)
Flags: camino1.0.4? → camino1.0.4+
Patch checked in on 1.8.0.
Strings checked in on 1.8 and trunk.
Keywords: fixed1.8.0.9
You need to log in before you can comment on or make changes to this bug.