pref to disable dated bookmark backup

RESOLVED FIXED in Firefox 2 beta1

Status

()

--
enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

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

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 beta1
fixed1.8.1
Points:
---
Bug Flags:
blocking1.8b5 -
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

13 years ago
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
(Assignee)

Comment 1

13 years ago
Created attachment 194953 [details] [diff] [review]
patch v1

Maybe something like this?

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

Updated

13 years ago
Flags: blocking1.8b5?
(Assignee)

Updated

13 years ago
Attachment #194953 - Flags: review?(vladimir)

Comment 2

13 years ago
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-
(Assignee)

Comment 3

13 years ago
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-
(Assignee)

Comment 5

13 years ago
Created attachment 195980 [details] [diff] [review]
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
Attachment #194953 - Attachment is obsolete: true
Attachment #195980 - Flags: review?(mconnor)
(Assignee)

Updated

13 years ago
Attachment #195980 - Attachment description: patch v1 → patch v2
(Assignee)

Comment 7

13 years ago
(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.
(Assignee)

Comment 9

13 years ago
Created attachment 196048 [details] [diff] [review]
cvs diff
(Assignee)

Comment 10

13 years ago
Created attachment 196849 [details] [diff] [review]
mozilla1.8 patch
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.)
(Assignee)

Comment 12

13 years ago
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
(Assignee)

Comment 14

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

Comment 16

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

Comment 17

13 years ago
logan, does your patch still apply/hasn't it bitrotted? It would be nice to have this for Firefox 2.
Flags: blocking-firefox2?
(Assignee)

Comment 18

13 years ago
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

13 years ago
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

Updated

13 years ago
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
(Assignee)

Updated

13 years ago
Flags: blocking-firefox2+ → blocking-firefox2?
Whiteboard: [checkin needed]
Target Milestone: Firefox 2 beta1 → ---

Updated

13 years ago
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)
(Assignee)

Comment 22

13 years ago
(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
(Assignee)

Updated

13 years ago
Attachment #196849 - Attachment description: cvs diff -up8 → mozilla1.8 patch
(Assignee)

Comment 23

13 years ago
Created attachment 223988 [details] [diff] [review]
trunk patch
(Assignee)

Updated

13 years ago
Attachment #196849 - Flags: approval-branch-1.8.1?(mconnor)

Updated

13 years ago
Attachment #196849 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+

Comment 24

13 years ago
Landed on branch and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Comment 25

13 years ago
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

Comment 27

13 years ago
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

13 years ago
why landed on Trunk ?
no bookmark-backup on trunk.
Where/which is bookmark-backup file on trunk ?
(Assignee)

Comment 29

13 years ago
(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.