307135 - pref to disable dated bookmark backup

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[logan]

I saw a few pref requests in bug 305004, but none to disable the automatic dated
bookmark backup completely. I'm sure this won't be a common request as most
users will appreciate the feature, but I'd prefer to handle this myself and
without a pref you'd have to edit the source or screw with the permissions on
the bookmarkbackups/ directory or something. :P

Comment 1

[logan]

Attachment: patch v1

Maybe something like this?

about:config -> browser.bookmarks.dated_backup.enabled -> false

Comment 2

[Asa Dotzler [:asa]]

I can't see us blocking the release for this enhancement but if you get a safe
and fully reviewed patch, please request approval and we'll evaluate it then.

Comment 3

[logan]

Comment on attachment 194953 [details] [diff] [review]
patch v1

The indenting change accounts for most of the patch, the core of it is only a
few lines, pretty simple.

vlad might be busy, so I'll change review to shaver.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 194953 [details] [diff] [review]
patch v1

It'd be better to do an integer pref so people can keep extra/less backups, as
desired, with 0 disabling the feature entirely.

This also breaks the safe mode dialog to revert bookmarks in the preffed off
case, which shouldn't be necessary.  If you check the restore pref, and make
the block you reindented |if (numberOfBackups > 0 || restoreDefaultBookmarks)|
you should be fine.

Comment 5

[logan]

Attachment: patch v2

The pref is now browser.bookmarks.max_backups and takes an int val. The default
is 5 (same behavior as the old #define), 0 to disable.

Hopefully this one is better. :P

Comment 6

[Steve Russell (RaiseMachine)]

With all due respect the old #define is 4 not 5.
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksService.cpp#4390

Comment 7

[logan]

(In reply to comment #6)
> With all due respect the old #define is 4 not 5.

and my patch tests >=, not > ...

Comment 8

[Mike Connor [:mconnor]]

can I get a diff -up8 version of this, or maybe more like -up12 since its not a
CVS diff and thus the patchview part of bugzilla can't give me more context.

Comment 9

[logan]

Attachment: cvs diff

Comment 10

[logan]

Attachment: mozilla1.8 patch

Comment 11

[Matt Nordhoff (aka Peng on IRC & forums)]

I'm sorry for the bugspam (a little), but has this been abandoned? It's been almost two months since anything happened. A decent number of people would like this to be checked in.

(Note: I have no idea if the patch is totally broken or anything. I don't have CVS set up at the moment to check. But still.)

Comment 12

[logan]

The patch should still be good (trunk or branch), but it's never been reviewed. The last time I spoke with mconnor, it didn't sound like this had any priority. It's definitely too late for 1.5...

In the meantime, this can be disabled by making the bookmarkbackups directory read-only or creating an empty file with the same name. I'm content with this as it's not a big hassle or completely aweful, but there's no joy for those looking to actually adjust the amount of backups kept.

Comment 13

[Matt Nordhoff (aka Peng on IRC & forums)]

I know, I do have it read-only. Hadn't thought about creating an empty file. I think that should've caused crashes before bug 313990 was fixed.

Eh, even if it is only of enhancement severity, it still annoys me that mconnor hasn't looked at it in two months. I doubt he's really *that* busy. (Then again, it does take a while to patch, compile and test a new build, I guess...)

Meh.

I'll stop the bugspam now, I think. :P

Comment 14

[logan]

I was unaware of bug 313990, but the patch should still cleanly apply with a little fuzz. An unreadable directory or a file would have cause a problem before that was fixed. Never tested, just assumed...

I've actually heard WONTFIX mentioned a few times in relation to this bug, but I think that was more to the idea of simply disabling the feature. There's more value in being able to control the amount of and since a setting of 0 would disable it, everyone should be happy.

The staff/drivers/reviewers have alot to do, so I wouldn't be too hard on 'em. A conversation on irc gave me the impression that he looked at this in some depth  at one time or another and was more or less satisfied.

Comment 15

[Matt Nordhoff (aka Peng on IRC & forums)]

I don't want to be **** mconnor, since I'm sure he has a lot of work to do, especially now when Firefox 1.5 is a few weeks away, but I am annoyed that this has been neglected. Making bookmarkbackups read-only is a hack.

Comment 16

[Jo Hermans]

*** Bug 317388 has been marked as a duplicate of this bug. ***

Comment 17

[Simon Bünzli]

logan, does your patch still apply/hasn't it bitrotted? It would be nice to have this for Firefox 2.

Comment 18

[logan]

looks ok on MOZILLA_1_8_BRANCH, some fuzz:

-(~/tmp) patch -p 0 < ~/src/mozilla/307135/firefox-1.5-datedbackup-2.patch 
patching file mozilla/browser/components/bookmarks/src/nsBookmarksService.cpp
Hunk #1 succeeded at 4331 (offset 8 lines).
Hunk #2 succeeded at 4383 (offset 9 lines).
Hunk #3 succeeded at 4392 (offset 9 lines).
Hunk #4 succeeded at 4414 (offset 9 lines).
Hunk #5 succeeded at 4596 (offset 9 lines).
patching file mozilla/browser/components/bookmarks/src/nsBookmarksService.h
patching file mozilla/modules/libpref/src/init/all.js

Comment 19

[Lee_Dailey]

howdy y'all,

[1] my tbird info ...
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.2) Gecko/20060308 Thunderbird/1.5.0.2 Mnenhy/0.7.4.666 - Build ID: 2006030803

[2] a reason for this pref i have NOT seen mentioned here is portable firefox. for usb versions of apps, every un-needed write is a real problem. both for speed reasons AND cuz flash memory  has a limited number of writes before it dies.

that's a _really_ good reason to have this pref.

[3] one of the mozillazine threads about the portable app situation ...
- Give Me My Freedom Back, Please!
http://forums.mozillazine.org/viewtopic.php?t=413390

too-cutesy title, but still covers the subject.

take care,
lee

Comment 20

[Mike Connor [:mconnor]]

The portable firefox case is a good one, let's get this on trunk and land on branch after a3 ships.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 195980 [details] [diff] [review]
patch v2

This is obsolete now, right?

Comment 22

[logan]

(In reply to comment #21)
> (From update of attachment 195980 [details] [diff] [review] [edit])
> This is obsolete now, right?
> 

ya, although it's the same code as the cvs diff

Comment 23

[logan]

Attachment: trunk patch

Comment 24

[Joe Hughes]

Landed on branch and trunk.

Comment 25

[Felix Miata]

This bug version is currently marked Version: 1.5.0.x Branch, which seems to be its original state. Yet, from what I can tell here, it's only been fixed for 2.0 and 3.0. Can this be fixed for 1.5.0.5 as well?

Comment 26

[:Gavin Sharp [email: [e-mail]]]

It's not really something that's within the scope of a security release, in my opinion, so I really doubt it.

Comment 27

[Felix Miata]

From what I've read in various Linux forums, it seems a lot of non-security things have gone in between 1.5 and 1.5.0.4. Seems like this might qualify for security related anyway, having impact on disk space and backup procedures, and maybe causing hang or crash when profiles are located where disk space is particularly precious.

Comment 28

[pal-moz]

why landed on Trunk ?
no bookmark-backup on trunk.
Where/which is bookmark-backup file on trunk ?

Comment 29

[logan]

(In reply to comment #28)
> why landed on Trunk ?

bookmarks.html and bookmarkbackups are used if places is disabled