508265 - Localized bookmarks backup are not replaced by new backups

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

This has been notified to me on the Mozillaitalia forums, looks like localized bookmarks backups are never removed, and never replaced by new backups.

Looking at my Library i just noticed i have the same issue, i have 4 old january backups, and only one August backup, that is replaced every day.

this is bad, and should be fixed in one of the next 1.9.1 releases, otherwise international users will have high possibility to incur in dataloss.

Comment 1

[Marco Bonardo [:mak]]

and when i choose Save, it tries to save a backup with localized name... sigh

Comment 2

[Marco Bonardo [:mak]]

looks like we do something stupid:
backupFileNames.sort();

In italian Bookmarks is translated with "Segnalibri", this means the new bookmark, once sorted, will always be the first (it starts with B), and thus considered the oldest.
we should provide a custom comparator for the date part, and possibly a valid unit test.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

this solves the issue, test coming.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.1 + test

Comment 5

[Marco Bonardo [:mak]]

asking in litmus too, checking which backups are available is quite trivial and errors are evident. Human support would be useful.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 392734 [details] [diff] [review]
patch v1.1 + test

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>@@ -551,29 +551,27 @@ var PlacesOrganizer = {
>     var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
>                  getService(Ci.nsIProperties);
>     var backupsDir = dirSvc.get("Desk", Ci.nsILocalFile);
>     fp.displayDirectory = backupsDir;
> 
>     // Use YYYY-MM-DD (ISO 8601) as it doesn't contain illegal characters
>     // and makes the alphabetical order of multiple backup files more useful.
>     var date = (new Date).toLocaleFormat("%Y-%m-%d");
>-    fp.defaultString = PlacesUIUtils.getFormattedString("bookmarksBackupFilenameJSON",
>-                                                        [date]);
>+    fp.defaultString = "bookmarks-" + date + ".json";

please take the code and comments in utils.js noted below, and put it in a PlacesUtils.getBackupFilename(aDate) and have callers in both files use the new method to generate the filename.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/utils.js#1778

r=me otherwise

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.2

m-c is closed, will push as soon as it reopens.

Comment 8

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/a1de621405d2

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 392905 [details] [diff] [review]
patch v1.2

please, either blocking or approval if possible.

This causes our intl users to only have 1 bookmarks backup for the current day, practically no protection against dataloss.

Comment 10

[Samuel Sidler (old account; do not CC)]

Comment on attachment 392905 [details] [diff] [review]
patch v1.2

Approved for 1.9.1.3. a=ss for release-drivers

Comment 11

[Daniel Veditz [:dveditz]]

Does this only happen in Firefox 3.5 and later, or does it happen on the 1.9.0 branch (Firefox 3.0) as well?

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/66c98b03857a

I don't have CVS access to 3.0 branch, Dietrich or Shawn can take care of that, since i suppose we need the fix on 3.0 due to bug 445704 being pushed there.

Comment 13

[Dietrich Ayala (:dietrich)]

patch doesn't apply on 3.0. please generate a new patch, and i can check it in once it gets a+.

Comment 14

[Marco Bonardo [:mak]]

comment 8 changeset is wrong, the correct one is:
http://hg.mozilla.org/mozilla-central/rev/9be4f809f2a7

Comment 15

[Marco Bonardo [:mak]]

Attachment: 1.9.0 branch patch

looks like 1.9.0.14 is closing today, i'm not sure if makes sense asking approval for it or better for 0.15

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1.3.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 393760 [details] [diff] [review]
1.9.0 branch patch

Approved for 1.9.0.15, a=dveditz for release-drivers

Comment 18

[Dietrich Ayala (:dietrich)]

checked into 1.9.0.

Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.169; previous revision: 1.168
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_utils_archiveBookmarksFile.js,v
done
Checking in toolkit/components/places/tests/unit/test_utils_archiveBookmarksFile.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_utils_archiveBookmarksFile.js,v  <--  test_utils_archiveBookmarksFile.js
initial revision: 1.1
done

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Verified fixed for 1.9.0 with Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9.0.15pre) Gecko/2009092805 GranParadiso/3.0.15pre (.NET CLR 3.5.30729).