Implement automatic bookmarks backup for 1.1

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
--
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Stuart Morgan)

Tracking

({fixed1.8.0.9, fixed1.8.1.1})

Details

Attachments

(2 attachments, 1 obsolete attachment)

v2
4.31 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
5.04 KB, patch
Stuart Morgan
: review+
Details | Diff | Splinter Review
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....
Flags: camino1.1?
(Assignee)

Comment 1

11 years ago
Created attachment 245982 [details] [diff] [review]
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."
Assignee: nobody → stuart.morgan
Status: NEW → ASSIGNED
Attachment #245982 - Flags: review?

Updated

11 years ago
Flags: camino1.1a2?

Comment 2

11 years ago
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.
Attachment #245982 - Flags: review? → review+
(Assignee)

Comment 3

11 years ago
Created attachment 246589 [details] [diff] [review]
v2

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]
v2

sr=pink
Attachment #246589 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 5

11 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 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]
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

11 years ago
Created attachment 248793 [details] [diff] [review]
1.8.0 Patch

Patch for 1.8.0 branch.  Stuart, can you sanity check my porting?
Attachment #248793 - Flags: review?(stuart.morgan)
(Assignee)

Comment 8

11 years ago
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+

Comment 10

11 years ago
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.