356708 - Implement automatic bookmarks backup for 1.1

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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....

Comment 1

[Stuart Morgan]

Attachment: tourniquet

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."

Comment 2

[Chris Lawson (gone)]

Comment on attachment 245982 [details] [diff] [review]
tourniquet

>Index: src/bookmarks/BookmarkManager.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/bookmarks/BookmarkManager.mm,v
>retrieving revision 1.75
>diff -u -8 -r1.75 BookmarkManager.mm
>--- src/bookmarks/BookmarkManager.mm	11 Nov 2006 20:58:49 -0000	1.75
>+++ src/bookmarks/BookmarkManager.mm	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.

Comment 3

[Stuart Morgan]

Attachment: v2

Removed the double-declaration. Also fixed a potential NSLog crasher just for kicks.

Comment 4

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 246589 [details] [diff] [review]
v2

sr=pink

Comment 5

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 246589 [details] [diff] [review]
v2

This is going to need a new patch for 1.0.4 (that pulls it out from among all the logging, probably)

Comment 7

[froodian (Ian Leue)]

Attachment: 1.8.0 Patch

Patch for 1.8.0 branch.  Stuart, can you sanity check my porting?

Comment 8

[Stuart Morgan]

Comment on attachment 248793 [details] [diff] [review]
1.8.0 Patch

Looks fine; I'm assuming it's been tested.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.)

Comment 10

[froodian (Ian Leue)]

Patch checked in on 1.8.0.
Strings checked in on 1.8 and trunk.