Closed Bug 307135 Opened 15 years ago Closed 14 years ago

pref to disable dated bookmark backup

Categories

(Firefox :: Bookmarks & History, enhancement)

2.0 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 2 beta1

People

(Reporter: logan+mozilla-bmo, Assigned: logan+mozilla-bmo)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 3 obsolete files)

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
Attached patch patch v1 (obsolete) — Splinter Review
Maybe something like this?

about:config -> browser.bookmarks.dated_backup.enabled -> false
Flags: blocking1.8b5?
Attachment #194953 - Flags: review?(vladimir)
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.
Flags: blocking1.8b5? → blocking1.8b5-
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.
Attachment #194953 - Flags: review?(vladimir) → review?(shaver)
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.
Attachment #194953 - Flags: review?(shaver) → review-
Attached patch patch v2 (obsolete) — Splinter Review
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
Attachment #194953 - Attachment is obsolete: true
Attachment #195980 - Flags: review?(mconnor)
Attachment #195980 - Attachment description: patch v1 → patch v2
(In reply to comment #6)
> With all due respect the old #define is 4 not 5.

and my patch tests >=, not > ...
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.
Attached patch cvs diff (obsolete) — Splinter Review
Attached patch mozilla1.8 patchSplinter Review
Attachment #196048 - Attachment is obsolete: true
Attachment #196849 - Flags: review?(mconnor)
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.)
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.
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
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.
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.
*** Bug 317388 has been marked as a duplicate of this bug. ***
logan, does your patch still apply/hasn't it bitrotted? It would be nice to have this for Firefox 2.
Flags: blocking-firefox2?
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
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

Attachment #196849 - Flags: review?(mconnor) → review+
The portable firefox case is a good one, let's get this on trunk and land on branch after a3 ships.
Assignee: nobody → mozilla
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1
Flags: blocking-firefox2+ → blocking-firefox2?
Whiteboard: [checkin needed]
Target Milestone: Firefox 2 beta1 → ---
Status: NEW → ASSIGNED
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1
Comment on attachment 195980 [details] [diff] [review]
patch v2

This is obsolete now, right?
Attachment #195980 - Attachment is obsolete: true
Attachment #195980 - Flags: review?(mconnor)
(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
Attachment #196849 - Attachment description: cvs diff -up8 → mozilla1.8 patch
Attached patch trunk patchSplinter Review
Attachment #196849 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #196849 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Landed on branch and trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [checkin needed]
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?
It's not really something that's within the scope of a security release, in my opinion, so I really doubt it.
Version: 1.5.0.x Branch → 2.0 Branch
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.
why landed on Trunk ?
no bookmark-backup on trunk.
Where/which is bookmark-backup file on trunk ?
(In reply to comment #28)
> why landed on Trunk ?

bookmarks.html and bookmarkbackups are used if places is disabled
You need to log in before you can comment on or make changes to this bug.