JSON bookmarks backup has localized filename (and can't be easily restored)

VERIFIED FIXED in Firefox 3.1b2

Status

()

defect
P1
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: u81239, Assigned: dietrich)

Tracking

(4 keywords)

unspecified
Firefox 3.1b2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has patch][has review])

Attachments

(1 attachment, 4 obsolete attachments)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9) Gecko/2008052906 Firefox/3.0

Hi,

Using the Dutch localisation, when I look in the bookmarkbackups folder of my profile I see the following files:

bladwijzers-2008-07-16.json
bookmarks-2008-03-13.html

The .html version is a left-over from before the JSON bookmark backups were put into effect. However, this made me notice that the file name changed along with the file format.

It seems that for the JSON version of the bookmarks backup, the localized name is used? (But not a localised date!) That looks like a bug to me. The filename should not contain any locale-specific parts. There is no precedent for that in the rest of the Firefox profile’s filenames, and no good reason to either.

The file name of the JSON file should be the same as the HTML one was. Having a localised part in the filename can lead to unexpected bugs (e.g. does the part that restores a bookmarks backup also use the localised name?).

~Grauw

Reproducible: Always

Steps to Reproduce:
1. Install and use a non-English version of Firefox
2. Go to C:\Users\…\AppData\Roaming\Mozilla\Firefox\Profiles\…\bookmarkbackups
3. Observe how the JSON bookmark backups contain a localised name, e.g. bladwijzers-2008-07-16.json
I guess the even bigger problem is that our own code expects the files to start with "bookmarks-" <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/utils.js#1621> which prevents them from showing up in the Library...
Status: UNCONFIRMED → NEW
Component: Bookmarks & History → Places
Ever confirmed: true
Flags: wanted1.9.0.x?
Flags: blocking-firefox3.1?
Keywords: regression
QA Contact: bookmarks → places
Summary: JSON bookmarks backup has localized filename → JSON bookmarks backup has localized filename (and can't be easily restored)
Flags: wanted1.9.0.x?
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1+
OS: Windows Vista → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → Firefox 3.1b2
Blocks: 450363
Target Milestone: Firefox 3.1b2 → Firefox 3.1
Blocks: 448969
Blocks: 447652
Posted patch v1 (obsolete) — Splinter Review
de-localize bookmark backup filenames. also removes an unused string.
Assignee: nobody → dietrich
Attachment #347095 - Flags: review?(mano)
This will leave backup files with a localized name lying around indefinitely. Won't this be a potential privacy risk?
Attachment #347095 - Flags: review?(mano)
Comment on attachment 347095 [details] [diff] [review]
v1

per comment #3, yes. will add code to clean those up.
Posted patch v2 (obsolete) — Splinter Review
this displays existing localized and un-localized backups in the menu and removes both per the normal archive-removal policy.
Attachment #347095 - Attachment is obsolete: true
Attachment #347470 - Flags: review?(zeniko)
Whiteboard: [has patch][needs review zeniko]
Comment on attachment 347470 [details] [diff] [review]
v2

>+      var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json$");

AFAICT this will catch all but the mn translation (where there seems to be text after .json). And nobody's been using parentheses and friends, so we should be fine without escaping the prefix.

>+        if (backupName.substr(0, backupFilenamePrefix.length) == backupFilenamePrefix ||
>+            backupName.substr(0, localizedFilenamePrefix.length) == localizedFilenamePrefix) {

Why not use the same RegExp as in places.js for consistency?

>+# Do not change this string!
>+bookmarksArchiveFilename=beluga-%S.json

Please, listen to yourself! ;-)
If you want to make our L10n community aware of this change, rather change the property's name to legacyBookmarksArchiveFilename or similar.

r+=me and thanks with this fixed.
Attachment #347470 - Flags: review?(zeniko) → review+
(In reply to comment #6)
> >+# Do not change this string!
> >+bookmarksArchiveFilename=beluga-%S.json
> 
> Please, listen to yourself! ;-)
> If you want to make our L10n community aware of this change, rather change the
> property's name to legacyBookmarksArchiveFilename or similar.

Er, that was left in from testing the change, not at all intended as a new l10n notification mechanism :)

http://bit.ly/GYSR
(In reply to comment #6)
> (From update of attachment 347470 [details] [diff] [review])
> >+      var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json$");
> 
> AFAICT this will catch all but the mn translation (where there seems to be text
> after .json).

how about: "^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json"

> And nobody's been using parentheses and friends, so we should be
> fine without escaping the prefix.

are you referring to the "."?
Posted patch v2.1 (obsolete) — Splinter Review
fixes comments
Attachment #347470 - Attachment is obsolete: true
Whiteboard: [has patch][needs review zeniko] → [has patch]
(In reply to comment #8)
> how about: "^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json"

That should do it, yes. Next time, please don't let the file extension be localizable... ;-)

> are you referring to the "."?

No, I was referring to the various translations of the word "bookmark".

On second thought, the following could remain localizable, as AFAICT we won't try to automatically restore from these:

-bookmarksBackupFilename=Bookmarks %S.html
-bookmarksBackupFilenameJSON=Bookmarks %S.json

Should we keep these, the file extension should still be moved into code, though.
Posted patch v2.2 (obsolete) — Splinter Review
yes, those strings can stay localized, and we'd probably get complaints if we de-localized them. i'm leaving the file extension in for now, since we're long past string freeze.
Attachment #347562 - Attachment is obsolete: true
Attachment #347697 - Flags: approval1.9.1b2?
Whiteboard: [has patch] → [has patch][has review]
Posted patch v2.3Splinter Review
... and this one actually contains the regexp change mentioned above.
Attachment #347697 - Attachment is obsolete: true
Attachment #347698 - Flags: approval1.9.1b2?
Attachment #347697 - Flags: approval1.9.1b2?
Comment on attachment 347698 [details] [diff] [review]
v2.3

a=beltzner
Attachment #347698 - Flags: approval1.9.1b2? → approval1.9.1b2+
Pushed: http://hg.mozilla.org/mozilla-central/rev/8c92737298f6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.1b2
I'd like to get this backported to the 1.9.0.x branch.

Dietrich, can you check if this patch would work as is or needs real backporting effort?
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.5?
Just to make things clear: if I'm reading to code properly - we should not de-localize bookmarksArchiveFilename if already localized, because Firefox won't find localized backups then. If so, you should definetely mention this in the localization note.
Wanted for branch, but definitely not blocking the release whose codefreeze is in a couple days. We'll want trunk baking time and branch tests for this.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.5?
Keywords: privacy
Attachment #347698 - Flags: review?(mano)
Comment on attachment 347698 [details] [diff] [review]
v2.3

grr, i just realized i got initial review from simon, and no review from a valid browser or toolkit peer.
(In reply to comment #16)
> Just to make things clear: if I'm reading to code properly - we should not
> de-localize bookmarksArchiveFilename if already localized, because Firefox
> won't find localized backups then. If so, you should definetely mention this in
> the localization note.

the latest version of the patch finds both localized and non-localized files in all scenarios (menu population, getting latest backup, etc), but only generates non-localized files.
Duplicate of this bug: 457665
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2
Status: RESOLVED → VERIFIED
Duplicate of this bug: 481157
Comment on attachment 347698 [details] [diff] [review]
v2.3

i think this should be fixed for 3.0.9, since actually we save backups with localized names and go looking for unlocalized names.
Attachment #347698 - Flags: approval1.9.0.9?
Attachment #347698 - Flags: approval1.9.0.9? → approval1.9.0.10?
Attachment #347698 - Flags: approval1.9.0.10? → approval1.9.0.10+
Comment on attachment 347698 [details] [diff] [review]
v2.3

Approved for 1.9.0.10, a=dveditz for release-drivers
Can we get automated tests for this on all branches and trunk?
Flags: in-testsuite?
i have a 1.9.0 patch for this, will check in when the tree is clean
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.168; previous revision: 1.167
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.25; previous revision: 1.24
done
Checking in toolkit/locales/en-US/chrome/places/places.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/places/places.properties,v  <--  places.properties
new revision: 1.8; previous revision: 1.7
done
Keywords: fixed1.9.0.10
Verified for 1.9.0.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9.0.11pre) Gecko/2009051304 GranParadiso/3.0.11pre. This is fixed now. New names are non-localized but the older, localized, files are viewable as options in the Bookmarks Library.
Blocks: 508265
No longer blocks: 508265
Depends on: 508265
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.