Bookmarks backup suggests invalid file name

VERIFIED FIXED in Firefox 3 beta1

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 3 beta1
Points:
---
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: l10n litmus test? comment 34)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 280461 [details] [diff] [review]
patch

> the default name suggested by the Bookmarks Manager backup function contains
> forward slashes (Bookmarks 09/11/07.html). This can't be saved because the
> slash is the path separator on a Linux/Unix system.
(http://forums.mozillazine.org/viewtopic.php?p=3051402#3051402)

Note that this isn't a Unix-only issue. File names mustn't contain slashes on Windows, too.

My proposal is to fall back to ISO 8601 if the localized date contains slashes.
Attachment #280461 - Flags: review?(sspitzer)
(Assignee)

Comment 1

11 years ago
Created attachment 280462 [details] [diff] [review]
patch v2

ISO 8601 for real
Assignee: nobody → dao
Attachment #280461 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #280462 - Flags: review?(sspitzer)
Attachment #280461 - Flags: review?(sspitzer)
(Assignee)

Updated

11 years ago
Flags: in-litmus?
Dão, what about just replaceing / with - instead, that way at least YY MM DD will be in the correct order (for the current locale)
Comment on attachment 280462 [details] [diff] [review]
patch v2

clearing review request, I think replacing / with - (in the date string) would be better.
Attachment #280462 - Flags: review?(sspitzer)
if m8 is not out the door, this would be a good one to sneak in, as this feature is new to m8.
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M8
Created attachment 280470 [details] [diff] [review]
patch v3, thanks to dao for getting this started
Attachment #280462 - Attachment is obsolete: true
Attachment #280470 - Flags: review?(dietrich)

Comment 6

11 years ago
Thanks a lot for the fix, Seth.
Do you really need the bug # in the code, though? :)
Comment on attachment 280470 [details] [diff] [review]
patch v3, thanks to dao for getting this started

r=me. however, it'd be nice to add a separator option to nsIScriptableDateFormatter, can you please file a followup for that if you think the idea has merit?
Attachment #280470 - Flags: review?(dietrich) → review+
Comment on attachment 280470 [details] [diff] [review]
patch v3, thanks to dao for getting this started

I can remove the bug # upon checkin, if you'd like.

(I like doing that, as moving code around or changing whitespace makes cvsblame less useful over time.  but maybe that's just me?)
Attachment #280470 - Flags: approval1.9?
I missed m8, so m9 (but relnote for m8, per mconnor)
Keywords: relnote
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Dao, my apologies for stealing this from under you.  

I was hoping to sneak something into m8 this morning, but I missed m8.
> can you please file a followup for that if you think the idea has merit?

see bug #395794, but I think it might be wontfix, but we can follow up there.
(Assignee)

Comment 12

11 years ago
(In reply to comment #2)
> Dão, what about just replaceing / with - instead, that way at least YY MM DD
> will be in the correct order (for the current locale)

That's bad imho, because you can mix up the components too easily. It might work for US dates, but can you ensure that it'll work as well for other locales with slashes and that the result won't be confused with an international or English date format? As a German, something like 07-10-06 causes me headaches. That's why I chose the standard format right away.
Using "-" is consistent with how we currently name the backups in the bookmarkbackups dir in the profile (going back to Fx2 at least).
(Assignee)

Comment 14

11 years ago
I'm not against using "-" (see attachment 280462 [details] [diff] [review]). I'm against invalidating the local time format.

In fact, the dates in the bookmarkbackups dir seem to match ISO 8601.
should we fix this bug by doing this:

# note, %1$S is year, %2$S is month, %3$S is day
# don't use : / ? (etc) because those are illegal for some operating systems
bookmarksBackupFilename=Bookmarks %1$S-%2$S-%#$S.html

note, we'd need a localization note to warn the localizer against using any illegal characters even if we don't change bookmarksBackupFilename

as for what dietrich points out, see http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.cpp#2541

PR_FormatTime(timeString, 128, "bookmarks-%Y-%m-%d.html", &nowInfo);

perhaps "bookmarks-%Y-%m-%d.html" should be a string property, with a note like:

# do not localize %Y %m and %d because they are year, month and date.
# do not use : / ? (etc) because those are illegal for some operating systems
autobackupFileName=bookmarks-%Y-%m-%d.html
> In fact, the dates in the bookmarkbackups dir seem to match ISO 8601.

What about fixing this code to always produce something like:

Bookmarks YYYY-MM-DD.html

we still need a localization note about : / ? (etc)
Comment on attachment 280470 [details] [diff] [review]
patch v3, thanks to dao for getting this started

clearing approval, review on this patch, based on dao's comments.
Attachment #280470 - Attachment is obsolete: true
Attachment #280470 - Flags: review+
Attachment #280470 - Flags: approval1.9?
(Assignee)

Comment 18

11 years ago
(In reply to comment #16)
> > In fact, the dates in the bookmarkbackups dir seem to match ISO 8601.
> 
> What about fixing this code to always produce something like:
> 
> Bookmarks YYYY-MM-DD.html

Why not use the local date format if it doesn't contain "/"? That's what patch 2 does.
I can't find the bug right now, but Axel's vote on whether or not you can expect localizers to know legal filename characters on all (or even all supported) platforms is in a Thunderbird RSS bug, a slightly more gently expressed version of "think again, bucko, not going to happen."

Comment 20

11 years ago
As a side note, without too much diving into the code in question, the localization of Firefox you're currently running has nothing to do with the encodings of your filesystem.

But yes, that entity should have a localization note. I personally prefer to use 2007-05-03, as the leading year is a globally unique sign of year, month, day, and it sorts nicely in directory listings.

Simon, do you know any date formats that have a leading year other than YYYY-MM-DD?

I'm not so worried about the date format here overall, the file has a chance to be named like the feature. The concept of back-ups itself is geeky enough for folks to remember that it's year-month-date.

The localization note should mention both that %S (why the 1$ in a single argument?) is a date formatted as ..., as well as that this is going to be a file path, so legal characters should be preferred.

Did anybody try what happens in the file dialog when you try to enter an invalid suggestion?
(Assignee)

Comment 21

11 years ago
I have nothing against YYYY-MM-DD. (Well, I proposed it in the first place!) The thing is, the code to use the local format if it doesn't contain "/" is already there -- why change it?
> Did anybody try what happens in the file dialog when you try to enter an
> invalid suggestion?

on windows, the file picker replaces illegal chars with -.

See http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsFilePicker.cpp#479

(Thank you my old friend JFD, see bug #100591)

I'm not sure about other platforms.

> The thing is, the code to use the local format if it doesn't contain "/" is
> already there -- why change it?

Again, if the local format uses : or any other illegal char, we'd have the same problem.

FYI:

as for the existing code at http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.cpp#2541,

PR_FormatTime(timeString, 128, "bookmarks-%Y-%m-%d.html", &nowInfo);

see bug #305004
for this bug, I think we should do:

1) fix the code to produce "Bookmarks YYYY-MM-DD.htm"l (note, not "bookmarks-YYYY-MM-DD.html", which is the format of the auto backup.)

2) add the localizations notes and fix the nit that axel found:

# NOTE TO LOCALIZERS
# do not localize %S, as it a date string of the format YYYY-MM-DD (ISO 8601)
# don't use : / ? (etc) because those are illegal for some operating systems
bookmarksBackupFilename=Bookmarks %S.html

3) add a comment to http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.cpp#2541 about that format being ISO 8601 as well.

dao, what do you think?
(Assignee)

Comment 24

11 years ago
(In reply to comment #22)
> > Did anybody try what happens in the file dialog when you try to enter an
> > invalid suggestion?
> 
> on windows, the file picker replaces illegal chars with -.
> 
> See
> http://mxr.mozilla.org/seamonkey/source/widget/src/windows/nsFilePicker.cpp#479

Is that file picker in use, actually? I get an "illegal file name" alert (in German).

> > The thing is, the code to use the local format if it doesn't contain "/" is
> > already there -- why change it?
> 
> Again, if the local format uses : or any other illegal char, we'd have the same
> problem.

Not an issue with dates, as far as I can tell. The separator always seems to be ".", "-" or "/".
> Is that file picker in use, actually? I get an "illegal file name" alert (in
> German).

yes, on windows, at least.

I am seeing us convert "Bookmarks 9/11/2007.html" to "Bookmarks 9-11-2007.html"

stack trace:

>	gkwidget.dll!nsFilePicker::SetDefaultString(const nsAString_internal & aString={...})  Line 479	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000007, unsigned int methodIndex=1, unsigned int paramCount=1238420, nsXPTCVariant * params=0x00000000)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=7)  Line 2326 + 0x1e bytes	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_SETTER)  Line 2326 + 0x1e bytes	C++
 	xpc3250.dll!XPCWrappedNative::SetAttribute(XPCCallContext & ccx={...})  Line 2081 + 0xe bytes	C++
 	xpc3250.dll!XPC_WN_GetterSetter(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, unsigned int argc=1, long * argv=0x06298f4c, long * vp=0x0012e848)  Line 1491 + 0xc bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x04d361e8, unsigned int argc=1, unsigned int flags=2)  Line 1378 + 0x20 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, long fval=103339904, unsigned int flags=0, unsigned int argc=1, long * argv=0x0012ef9c, long * rval=0x0012ef9c)  Line 1474 + 0x14 bytes	C
 	js3250.dll!js_InternalGetOrSet(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, long id=68511260, long fval=103339904, JSAccessMode mode=JSACC_WRITE, unsigned int argc=1, long * argv=0x0012ef9c, long * rval=0x0012ef9c)  Line 1546 + 0x1f bytes	C
 	js3250.dll!js_NativeSet(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, JSScopeProperty * sprop=0x0626a10c, long * vp=0x0012ef9c)  Line 3490 + 0x32 bytes	C
 	js3250.dll!js_SetProperty(JSContext * cx=0x04d361e8, JSObject * obj=0x0628cf20, long id=68511260, long * vp=0x0012ef9c)  Line 3749 + 0x15 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x04d361e8, unsigned char * pc=0x00dd6b07, long * result=0x0012f0ac)  Line 3865 + 0x147 bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x04d361e8, unsigned int argc=1, unsigned int flags=2)  Line 1399 + 0x13 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x04d361e8, JSObject * obj=0x0628ce40, long fval=103337568, unsigned int flags=0, unsigned int argc=1, long * argv=0x06298e50, long * rval=0x0012f234)  Line 1474 + 0x14 bytes	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x04d361e8, JSObject * obj=0x0628ce40, long fval=103337568, unsigned int argc=1, long * argv=0x06298e50, long * rval=0x0012f234)  Line 4940 + 0x1f bytes	C
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x04e91388, void * aScope=0x04e05080, void * aHandler=0x0628ce60, nsIArray * aargv=0x06297020, nsIVariant * * arv=0x0012f3a8)  Line 1842 + 0x24 bytes	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x06296f3c)  Line 224 + 0x67 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x04e91488, nsIDOMEventListener * aListener=0x04e91438, nsIDOMEvent * aDOMEvent=0x06296f3c, nsISupports * aCurrentTarget=0x04e91388, unsigned int aPhaseFlags=6)  Line 1096 + 0x12 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x043eb288, nsEvent * aEvent=0x0012f718, nsIDOMEvent * * aDOMEvent=0x0012f678, nsISupports * aCurrentTarget=0x04e91388, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012f67c)  Line 1216	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 207	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 266	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x04e91388, nsPresContext * aPresContext=0x043eb288, nsEvent * aEvent=0x0012f718, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f760, nsDispatchingCallback * aCallback=0x00000000)  Line 479 + 0x12 bytes	C++
 	gklayout.dll!nsXULElement::PreHandleEvent(nsEventChainPreVisitor & aVisitor={...})  Line 1509 + 0x29 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::PreHandleEvent(nsEventChainPreVisitor & aVisitor={...})  Line 186 + 0x1c bytes	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x061aa500, nsPresContext * aPresContext=0x043eb288, nsEvent * aEvent=0x0012f934, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f928, nsDispatchingCallback * aCallback=0x00000000)  Line 438	C++
 	gklayout.dll!PresShell::HandleDOMEventWithTarget(nsIContent * aTargetContent=0x061aa500, nsEvent * aEvent=0x0012f934, nsEventStatus * aStatus=0x0012f928)  Line 5772 + 0x1c bytes	C++
 	gklayout.dll!nsXULMenuCommandEvent::Run()  Line 1865	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f9e8)  Line 491	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00bfc390, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 154 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 170 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00bf8b00, const nsXREAppData * aAppData=0x00bf8ee0)  Line 3069 + 0x25 bytes	C++
 	firefox.exe!main(int argc=1, char * * argv=0x00bf8b00)  Line 153 + 0x12 bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	firefox.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816fd7() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	

Comment 26

11 years ago
(In reply to comment #23)
> for this bug, I think we should do:
> 
> 1) fix the code to produce "Bookmarks YYYY-MM-DD.htm"l (note, not
> "bookmarks-YYYY-MM-DD.html", which is the format of the auto backup.)

Ah, that's what v1.1 of the patch in bug 374528 did:

https://bugzilla.mozilla.org/attachment.cgi?id=280157

:-)

I like your idea of putting that string in the properties file!
(Assignee)

Comment 27

11 years ago
(In reply to comment #25)
Yeah, I misread the code -- it processes the default name, but I entered it manually since my system locale is DE. Also not that this doesn't work on Linux or OS X, where FILE_ILLEGAL_CHARACTERS doesn't contain "/".

(In reply to comment #23)
> dao, what do you think?

I'm still in favor of patch v2, which satisfies most users, I think.
> I'm still in favor of patch v2, which satisfies most users, I think.

dao, one additional reason to favor ISO 8601 is that if you backup many times into a folder, it is easy to spot the "latest" by searching alphabetically.

our current format (ex:  "Bookmarks 9-11-2007") is not good for this reason.
(Assignee)

Comment 29

11 years ago
Created attachment 280532 [details] [diff] [review]
patch v4
Attachment #280532 - Flags: review?
(Assignee)

Comment 30

11 years ago
Comment on attachment 280532 [details] [diff] [review]
patch v4

as suggested in comment 23
Attachment #280532 - Flags: review? → review?(sspitzer)
(Assignee)

Comment 31

11 years ago
Created attachment 280533 [details] [diff] [review]
patch v4.1

removing a superfluous backslash
Attachment #280532 - Attachment is obsolete: true
Attachment #280533 - Flags: review?(sspitzer)
Attachment #280532 - Flags: review?(sspitzer)
Comment on attachment 280533 [details] [diff] [review]
patch v4.1

r=sspitzer

note in two places you misspelled "illagel" (should be "illegal"

(fwiw, I tested from the js console, and (new Date).toLocaleFormat("%Y-%m-%d") generates what we want, specifically, 09 for september, not just 9.

Thanks again, Dao!
Attachment #280533 - Flags: review?(sspitzer)
Attachment #280533 - Flags: review+
Attachment #280533 - Flags: approval1.9?

Comment 33

11 years ago
Comment on attachment 280533 [details] [diff] [review]
patch v4.1

<...>
>Index: browser/locales/en-US/chrome/browser/places/places.properties

<...>
>+# %S is the current date string of the format YYYY-MM-DD (ISO 8601)
>+# Don't use any of those characters as they are illegal for some operating systems: /\:*?"<>|

Two comments on the comment, firstly, please use the localization note format, http://developer.mozilla.org/en/docs/Localization_notes. Secondly, the date is coming in already formatted, so the "§($ stuff won't matter that much.

I'd suggest
# LOCALIZATION NOTE (bookmarksBackupFilename) :
# %S will be replaced by the current date in ISO 8601 format, YYYY-MM-DD.
# The resulting string will be suggested as a filename, so make sure that you're
# only using characters legal for file names. Consider falling back to the
# en-US value if you have to use non-ascii characters.

<...>

That might not be the best English, but it should get the message across.

Comment 34

11 years ago
Dao already requested the litmus test, I'd like to add that we should get this into the l10n-targeted litmus tests, too. AFAICT, this test will be platform dependent as well, and be a functionality test much more than a localization test.
Whiteboard: l10n litmus test? comment 34

Comment 35

11 years ago
Phil was probably talking about bug 313525. Dropping a reference to that here.
(Assignee)

Comment 36

11 years ago
Created attachment 280578 [details] [diff] [review]
patch v4.2

comment typo and l10n note fixed
Attachment #280533 - Attachment is obsolete: true
Attachment #280578 - Flags: approval1.9?
Attachment #280533 - Flags: approval1.9?
Litmus Triage Team: Adding tchung because of Comment 34 - l10n request.
Comment on attachment 280578 [details] [diff] [review]
patch v4.2

carrying over my r=, thanks again Dao!
Attachment #280578 - Flags: review+

Updated

11 years ago
Attachment #280578 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.102; previous revision: 1.101
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v  <--  places.properties
new revision: 1.28; previous revision: 1.27
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Verified fixed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1 for the beta.
Status: RESOLVED → VERIFIED
Keywords: relnote
It seems like the date numbering aspect of "Export HTML" has been removed. Does this bug need a test case on litmus anymore?
(In reply to comment #41)
> It seems like the date numbering aspect of "Export HTML" has been removed. Does
> this bug need a test case on litmus anymore?

Still needs a testcase since this is for the bookmark JSON backup, not export HTML.
Cool, thanks for the advice, Marcia. The following test cases were updated on litmus for the 3.x test runs in the Library subgroup.

For 3.0,
https://litmus.mozilla.org/show_test.cgi?id=5234

For 3.1,
https://litmus.mozilla.org/show_test.cgi?id=6852
Flags: in-litmus? → in-litmus+
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.