Fix ChromeOS clock menu so that it can show 12-hour clock in all locales.

Fix ChromeOS clock menu so that it can show 12-hour clock in all locales.

BUG=chromium-os:15183
TEST=Set "OS Language" to "Japanese" and open "System" setting.
Click the check box of "Use 24-hour clock".
Verify the top-right menu clock toggles (12 <--> 24).

The cause of the issue was that, the 12/24h setting is not
referred at all when time strings are formatted with AM/PM
sign dropped.

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

[Yusuke Sato, 2011-05-26 06:33:48]

LGTM, but you might also have to get lgtm from one of base/ OWNERs:
http://src.chromium.org/viewvc/chrome/trunk/src/base/OWNERS?revision=81630&vi...

[kinaba, 2011-05-26 07:44:14]

I added OWNER base/ (brettw) as a reviewer. Brett, could you take a look?
Thanks.

[brettw, 2011-05-26 17:12:54]

It seems like this would be easy to write a unit test for, and some of the
complexity in the new function seems worthwhile testing. It looks like we don't
have a time_formatting_unittest, so now would be a great time to add one. Can
you look into this?

[kinaba, 2011-05-29 22:30:19]

Thanks for your comment.
I agree that we should have a test for this part. I take care of it.

On 2011/05/26 17:12:54, brettw wrote:
> It seems like this would be easy to write a unit test for, and some of the
> complexity in the new function seems worthwhile testing. It looks like we
don't
> have a time_formatting_unittest, so now would be a great time to add one. Can
> you look into this?

[kinaba, 2011-05-30 08:10:28]

Added unittests for functions in time_formatting.cc, and fixed one apparently
incorrect comment.

[brettw, 2011-05-30 08:34:13]

LGTM, thanks!

http://codereview.chromium.org/7054032/diff/5002/base/i18n/time_formatting_un...
File base/i18n/time_formatting_unittest.cc (right):

http://codereview.chromium.org/7054032/diff/5002/base/i18n/time_formatting_un...
base/i18n/time_formatting_unittest.cc:29: class TimeFormattingTest : public
PlatformTest {
I don't think you need this (and you can delete the platform_test.h include).
Since you're not using TEST_F, I don't think this is even getting used.

[kinaba, 2011-05-30 08:43:38]

http://codereview.chromium.org/7054032/diff/5002/base/i18n/time_formatting_un...
File base/i18n/time_formatting_unittest.cc (right):

http://codereview.chromium.org/7054032/diff/5002/base/i18n/time_formatting_un...
base/i18n/time_formatting_unittest.cc:29: class TimeFormattingTest : public
PlatformTest {
On 2011/05/30 08:34:13, brettw wrote:
> I don't think you need this (and you can delete the platform_test.h include).
> Since you're not using TEST_F, I don't think this is even getting used.

Done. Thanks for pointing it out!