Added a test checking for TimeFormat:: producing NaN, rolled icu version

Added a test verifying that TimeFormat:: functions convert
numbers to "NaN" on certain locales. This is a bug on icu.

Rolled third_party/icu version to @88321.

BUG=60476
TEST=browser_tests:TimeFormat.DecimalPointNotDot

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88329

[Joao da Silva, 2011-05-11 14:32:18]

A fix for ICU is at http://codereview.chromium.org/7004021. Once ICU is fixed,
the FAILS_ prefix can be removed from this test.

Please review, thanks!

[Paweł Hajdan Jr., 2011-05-11 14:47:13]

Drive-by with a testing comment.

http://codereview.chromium.org/7003028/diff/1/chrome/common/time_format_unitt...
File chrome/common/time_format_unittest.cc (right):

http://codereview.chromium.org/7003028/diff/1/chrome/common/time_format_unitt...
chrome/common/time_format_unittest.cc:85: locale = setlocale(LC_ALL,
locales[i]);
And now all following tests run with the changed locale. Ideally we should have
something like ScopedChangeLocale to make sure it is always set back to the
original value.

[Joao da Silva, 2011-05-11 15:35:07]

Paweł: updated to restore the locale after the test, please have another look.

http://codereview.chromium.org/7003028/diff/1/chrome/common/time_format_unitt...
File chrome/common/time_format_unittest.cc (right):

http://codereview.chromium.org/7003028/diff/1/chrome/common/time_format_unitt...
chrome/common/time_format_unittest.cc:85: locale = setlocale(LC_ALL,
locales[i]);
On 2011/05/11 14:47:13, Paweł Hajdan Jr. wrote:
> And now all following tests run with the changed locale. Ideally we should
have
> something like ScopedChangeLocale to make sure it is always set back to the
> original value.
Yikes, true. Fixed by adding a ScopedChangeLocale.
Does it make sense to have that class available to other tests? In that case,
where should it be placed?

[Paweł Hajdan Jr., 2011-05-11 19:10:44]

http://codereview.chromium.org/7003028/diff/5001/chrome/common/time_format_un...
File chrome/common/time_format_unittest.cc (right):

http://codereview.chromium.org/7003028/diff/5001/chrome/common/time_format_un...
chrome/common/time_format_unittest.cc:23: class ScopedChangeLocale {
Sorry for not finding it earlier (I tried, but failed), but now I found an
existing class which does more or less the same:
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/base/sys_string_con...

The code using it is in base/ so I'd suggest extracting it to base/test.

http://codereview.chromium.org/7003028/diff/5001/chrome/common/time_format_un...
chrome/common/time_format_unittest.cc:33: const char *stored_locale_;
nit: Star on the wrong side, should be const char* stored_locale_;

http://codereview.chromium.org/7003028/diff/5001/chrome/common/time_format_un...
chrome/common/time_format_unittest.cc:38: } // namespace
nit: Two spaces between "}" and "// namespace".

[jungshik at Google, 2011-05-12 01:20:08]

Hmm... I must be missing something.   This is not about parsing but about
formatting. And, even for Parsing, with the locale set correctly, ICU should
work correctly. I'll take a look at our number formatting code.

[Joao da Silva, 2011-05-12 09:20:46]

I've traced this behavior down to a bug in ICU, please have a look at
http://codereview.chromium.org/7004021. I don't believe this is caused from our
side, like failing to set some locale setting.

Here's a trace of how the NaN is created.

The TimeFormat calls internally use FormatTimeImpl, which calls
PluralFormat::format(). The PluralFormat used has been loaded with the default
locale in chrome/common/time_format.cc:196. The number passed in the test is 1,
and the plural pattern is "# min".

The format() call casts the int to double, and ends up at
icu/source/i18n/plurfmt.cpp:307, where insertFormattedNumber() is called. This
calls numberFormat->format to convert the number to a string. numberFormat is a
DecimalFormat, and the call ends up at icu/source/i18n/decimfmt.cpp:973.

That call leads to _format() internally, which converts the double (1.0) to a
DigitList, using DigitList::set(double) in icu/source/i18n/digitlst.cpp. This is
where the problem is: DigitList::set(double) just calls sprintf() to convert the
double to a string, but sprintf() can use a ',' (comma) instead of a '.' (dot)
as a decimal separator in certain locales. The string is then passed to
uprv_decNumberFromString(), which fails to parse it because it is looking for
the dot, and sets the reprensentation to NaN.

This problem is present in the current version of ICU too:
http://bugs.icu-project.org/trac/browser/icu/trunk/source/i18n/digitlst.cpp#L741

Note that other parts of DigitList handle the comma as a decimal separator (for
example, DigitList::getDouble()). From what I've seen, all the calls leading up
here do not depend on any locale settings, so I don't think we can configure
PluralFormat to avoid this code path.

A comment at the top of digitlst.cpp mentions that the file was converted from
the java version, where Double.toString() is used (see
http://bugs.icu-project.org/trac/browser/icu4j/trunk/main/classes/core/src/co...).
Double.toString() ignores the locale, but sprintf() doesn't.

Please let me know what you think, and how to proceed. Thanks for reviewing!

[jungshik at Google, 2011-06-07 21:46:45]

LGTM 

After checking in the ICU patch, include DEPS file update to roll in the ICU
revision with your patch in this CL and check in.

[jungshik at Google, 2011-06-07 21:47:16]

On 2011/06/07 21:46:45, Jungshik Shin wrote:
> LGTM 
> 
> After checking in the ICU patch, include DEPS file update to roll in the ICU
> revision with your patch in this CL and check in.

And, change TEST= line accordingly (no more 'FAILS_' :-))

[jungshik at Google, 2011-06-07 21:48:01]

http://codereview.chromium.org/7003028/diff/12001/chrome/common/time_format_b...
File chrome/common/time_format_browsertest.cc (right):

http://codereview.chromium.org/7003028/diff/12001/chrome/common/time_format_b...
chrome/common/time_format_browsertest.cc:30:
IN_PROC_BROWSER_TEST_F(TimeFormatBrowserTest, FAILS_DecimalPointNotDot) {
If you include ICU roll (DEPS change) in this CL, you can drop 'FAILS_' above.

[commit-bot: I haz the power, 2011-06-08 13:54:18]

Can't process patch for file chrome/chrome_tests.gypi.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-06-08 14:14:53]

Presubmit check for 7003028-18001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
base/test/test_util.h,base/sys_string_conversions_unittest.cc

[Paweł Hajdan Jr., 2011-06-09 08:43:22]

Thank you for working on this. Please fix the issues below _and_ let me take
another look before this gets committed (base/test/OWNERS).

http://codereview.chromium.org/7003028/diff/18001/base/test/test_util.h
File base/test/test_util.h (right):

http://codereview.chromium.org/7003028/diff/18001/base/test/test_util.h#newcode9
base/test/test_util.h:9: // Generic utilities used only by tests.
Please don't add yet another _util file. How about naming it scoped_locale.h?
Also, please split it into .h and .cc file, with most of the implementation in
the .cc file.

http://codereview.chromium.org/7003028/diff/18001/base/test/test_util.h#newco...
base/test/test_util.h:23: explicit ScopedSetLocale(const char* locale) {
nit: Could you change the type of the parameter to const std::string& or maybe
StringPiece to make it easier to pass strings there without c_str?

http://codereview.chromium.org/7003028/diff/18001/base/test/test_util.h#newco...
base/test/test_util.h:25: setlocale(LC_ALL, locale);
Please check the return value. NULL would indicate failure.

http://codereview.chromium.org/7003028/diff/18001/base/test/test_util.h#newco...
base/test/test_util.h:26: }
nit: Add empty line below.

http://codereview.chromium.org/7003028/diff/18001/chrome/common/time_format_b...
File chrome/common/time_format_browsertest.cc (right):

http://codereview.chromium.org/7003028/diff/18001/chrome/common/time_format_b...
chrome/common/time_format_browsertest.cc:19: #if defined(OS_POSIX)
Please handle only running on POSIX in gyp, not in the .cc file.

http://codereview.chromium.org/7003028/diff/18001/chrome/common/time_format_b...
chrome/common/time_format_browsertest.cc:22: class TimeFormatBrowserTest: public
InProcessBrowserTest {
nit: Space before ":".

http://codereview.chromium.org/7003028/diff/18001/chrome/common/time_format_b...
chrome/common/time_format_browsertest.cc:24: TimeFormatBrowserTest():
scoped_locale_("fr_FR.utf-8") {}
nit: Space before ":", put the ending brace } on its own line.

http://codereview.chromium.org/7003028/diff/18001/chrome/common/time_format_b...
chrome/common/time_format_browsertest.cc:38: EXPECT_EQ(ASCIIToUTF16("1 min"),
one_min) << "fr_FR.utf-8 locale generates "
nit: No need for the << message, EXPECT_EQ handles that.