423154 - off-by-one error for browser.bookmarks.max_backups

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Simon Bünzli]

First all but max_backups files are deleted, but then a new one is created...

NB: This same issue exists in two files, this bug's URL and
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp&rev=1.60&mark=2575,2587#2575

Besides: Setting max_backups to 0 is broken (should mean no backups at all, now means as many as possible).

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: fix

moves browser.bookmarks.max_backups to firefox.js (orig added in bug 307135)

now supports:

0: no backups created, old ones deleted (privacy mode)
-1: no cap on the number of backups created

Comment 2

[Asaf Romano (gone)]

Comment on attachment 309821 [details] [diff] [review]
fix

>Index: toolkit/components/places/src/utils.js
>===================================================================

leNames.length >= aNumberOfBackups) {
>-        var numberOfBackupsToDelete = backupFileNames.length - aNumberOfBackups;
>+      var numberOfBackupsToDelete = 0;
>+      if (aNumberOfBackups == -1)
>+        numberOfBackupsToDelete  = 0;
>+      else if (aNumberOfBackups == 0)
>+        numberOfBackupsToDelete  = backupFileNames.length;
>+      else if (backupFileNames.length >= aNumberOfBackups)
>+        numberOfBackupsToDelete = backupFileNames.length - aNumberOfBackups;
>+

that should probably be

>+      var numberOfBackupsToDelete = 0;
>+      if (aNumberOfBackups != -1) {
>+        if (aNumberOfBackups == 0)
>+          numberOfBackupsToDelete  = backupFileNames.length;
>+        else if (backupFileNames.length >= aNumberOfBackups)
>+          numberOfBackupsToDelete = backupFileNames.length - aNumberOfBackups;
>+      }

r=mano.

Comment 3

[Simon Bünzli]

(In reply to comment #2)
Or even simpler:
> >+      var numberOfBackupsToDelete = 0;
> >+      if (aNumberOfBackups > -1)
> >+        numberOfBackupsToDelete = backupFileNames.length - aNumberOfBackups;

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: as checked in

thanks, and thanks

Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.7; previous revision: 1.6
done

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

a=beltzner for follow-up checkin ...

Comment 6

[Dietrich Ayala (:dietrich)]

forgot to check in the removal of the html archival code (unused since JSON landed)

Checking in browser/components/places/public/nsIPlacesImportExportService.idl;
/cvsroot/mozilla/browser/components/places/public/nsIPlacesImportExportService.idl,v  <--  nsIPlacesImportExportService.idl
new revision: 1.4; previous revision: 1.3
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.61; previous revision: 1.60
done
Checking in browser/components/places/src/nsPlacesImportExportService.h;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.h,v  <--  nsPlacesImportExportService.h
new revision: 1.10; previous revision: 1.9
done

Comment 7

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv