The l10n hazard alluded to in bug 568656 can be fixed with the use of the Date's toLocaleString() method. The attached patch fixes this.

Comment on attachment 458844 [details] [diff] [review] Patch. Requires patch in bug 578658. This patch is wrong, because it no longer displays milliseconds.

New patch localizes timestamps including milliseconds as best I can, using nsIScriptableDateFormat and massaging the output. A test case is included. Feedback requested.

Requesting betaN blocking status for this bug because this is a localization issue with the Web Console.

Not a blocker - not primary UI, not a major regression, and the feature is not non-functional. Would take a low-risk patch w/ tests.

This patch looks wrong on many accounts. Stripping leading zeros sounds wrong. Not sure how the ms are coming into the date. The tests should pass for non-en-US locales. And I'm not sure what the logic around the last number does. Maybe just go for a plain localized strings that takes $1$i for hour, minute, second, and ms?

(In reply to comment #5) > Maybe just go for a plain localized strings that takes $1$i for hour, minute, > second, and ms? The problem with this is that it seems difficult to deal with AM/PM and 24-hour time.

I could totally live with the US all eating 24 hours :-). If you think you need AM/PM, yeah, I wouldn't know. Not sure how much you tested all the variants that nsIScriptableDateFormat yields on all those platforms.

(In reply to comment #7) > I could totally live with the US all eating 24 hours :-). > > If you think you need AM/PM, yeah, I wouldn't know. Not sure how much you > tested all the variants that nsIScriptableDateFormat yields on all those > platforms. How about two localization strings: one for AM and one for PM? In areas with a 24-hour clock, the localization strings will simply be the same.

you'd have to pass different hours, though.

(In reply to comment #9) > you'd have to pass different hours, though. I was planning on using "%I" in the string to mean "hours (12-hour clock)" and "%H" to mean "hours (24-hour clock)", just as strftime(3) does. The l10n string can just choose the appropriate one.

our printfs end up in http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prprf.h#41, which doesn't support I and H :-(.

Well, I'd do the "%I" and "%H" manually from JavaScript. Ugly, but works.

That is, I'd do the search-and-replace on those strings manually from JavaScript.

I am thinking AM/PM is a followup bug. Sholdn't we just force 24 hour clock in the console until we have more time to deal with it?

(In reply to comment #14) > I am thinking AM/PM is a followup bug. Sholdn't we just force 24 hour clock in > the console until we have more time to deal with it? You're right, let's do that.

(In reply to comment #14) FWIW, for locales that don't use AM/PM 12-hour format, we don't care about this to be localized or not, we only use 24-hour format. Sorry if I made noise in this bug for something not relevant.

New patch localizes the timestamps using a simple format string.

Comment on attachment 462267 [details] [diff] [review] Proposed patch, version 3. I guess I'd move the padding into the format string. That would allow the persians to even go for full text. I'm a bit surprised to see the hours padded with '0', fwiw.

(In reply to comment #18) > Comment on attachment 462267 [details] [diff] [review] > Proposed patch, version 3. > > I guess I'd move the padding into the format string. That would allow the > persians to even go for full text. What do you mean by full text?

"12:35 and 72 milliseconds" would be full text. I guess that "%02d:%02d.%03d" or something should be equivalent.

How do you use %d in locale strings? A quick grep through the tree shows that no .properties file contains a %d.

You're from js, right? I guess you'll be %S then anyway. I usually end up with trial and error what the code actually does.

New patch makes the padding part of the format string. This way, localizers can choose to omit it if they'd like.

Does this actually still depend on bug 578658? That one is not going to make it in. We should try to do this one without that dependency.

(In reply to comment #24) > Does this actually still depend on bug 578658? That one is not going to make it > in. We should try to do this one without that dependency. No, it doesn't. (But we should still land bug 578658 at some point, at the risk of going off-topic...)

Comment on attachment 464672 [details] [diff] [review] Proposed patch, version 4. <...> >+# LOCALIZATION NOTE (timestampFormat): %1$S = leading zeroes for hours, %2$S = hours (24-hour clock), %3$S = leading zeroes for minutes, %4$S = minutes, %5$S = leading zeroes for seconds, %6$S = seconds, %7$S = leading zeroes for milliseconds, %8$S = milliseconds >+timestampFormat=%S%S:%S%S:%S%S.%S%S Can't you do the padding of zeros wit %02S ? Don't have a build to test locally right now.

Comment on attachment 464672 [details] [diff] [review] Proposed patch, version 4. Noting a feedback- as long as I'm waiting for an answer on comment 26.

Sweet! %02S works. Patch rebased and updated per feedback.

Comment on attachment 469540 [details] [diff] [review] Proposed patch, version 5. That looks good. I wonder if timestampFormat=%2S:%02S:%02S.%03S would be better still, that should pad the hours with ' ', I think. Anyway, this is good to get real code review by someone, thanks for your questions.

New patch rebases to trunk and splits out the test into its own file.

running the HUDService tests with this installed, I get the following error: TEST-PASS | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | console exists JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/HUDService.jsm :: HS_getFormatStr :: line 1292" data: no] TEST-INFO | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Console message: [JavaScript Error: "uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/HUDService.jsm :: HS_getFormatStr :: line 1292" data: no]"] Line 1292 is in HS_getFormatStr(aName, aArray).

(In reply to comment #31) > running the HUDService tests with this installed, I get the following error: > > TEST-PASS | > chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js > | console exists > JavaScript error: , line 0: uncaught exception: [Exception... "Component > returned failure code: 0x80004005 (NS_ERROR_FAILURE) > [nsIStringBundle.formatStringFromName]" nsresult: "0x80004005 > (NS_ERROR_FAILURE)" location: "JS frame :: > resource://gre/modules/HUDService.jsm :: HS_getFormatStr :: line 1292" data: > no] > TEST-INFO | > chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js > | Console message: [JavaScript Error: "uncaught exception: [Exception... > "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) > [nsIStringBundle.formatStringFromName]" nsresult: "0x80004005 > (NS_ERROR_FAILURE)" location: "JS frame :: > resource://gre/modules/HUDService.jsm :: HS_getFormatStr :: line 1292" data: > no]"] > > Line 1292 is in HS_getFormatStr(aName, aArray). Did you rebuild from top level? The standard commands for incremental builds in toolkit/ won't work if there's a string change.

All tests pass for me.

(In reply to comment #32) > (In reply to comment #31) > > Line 1292 is in HS_getFormatStr(aName, aArray). > > Did you rebuild from top level? The standard commands for incremental builds in > toolkit/ won't work if there's a string change. oh. No I didn't. Sorry for the alarm. I am toolkit nub.

New version of the patch rebases to trunk.

Comment on attachment 473890 [details] [diff] [review] [checked-in] Proposed patch, version 5.2. http://hg.mozilla.org/mozilla-central/rev/224777dc8afe