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.

Review URL: http://codereview.chromium.org/7054032

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@87229 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 213 insertions, 31 deletions

diff --git a/base/base.gyp b/base/base.gyp
index 8c3dfce..db1cd99 100644
--- a/base/base.gyp
+++ b/base/base.gyp
@@ -133,6 +133,7 @@
         'i18n/file_util_icu_unittest.cc',
         'i18n/icu_string_conversions_unittest.cc',
         'i18n/rtl_unittest.cc',
+        'i18n/time_formatting_unittest.cc',
         'json/json_reader_unittest.cc',
         'json/json_writer_unittest.cc',
         'json/string_escape_unittest.cc',
@@ -233,6 +234,8 @@
         'third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
+        '../third_party/icu/icu.gyp:icui18n',
+        '../third_party/icu/icu.gyp:icuuc',
       ],
       'conditions': [
         ['toolkit_uses_gtk==1', {
diff --git a/base/i18n/time_formatting.cc b/base/i18n/time_formatting.cc
index e52bd22..95d481e 100644
--- a/base/i18n/time_formatting.cc
+++ b/base/i18n/time_formatting.cc
@@ -26,6 +26,26 @@ string16 TimeFormat(const icu::DateFormat* formatter,
                   static_cast<size_t>(date_string.length()));
 }
 
+string16 TimeFormatWithoutAmPm(const icu::DateFormat* formatter,
+                               const Time& time) {
+  DCHECK(formatter);
+  icu::UnicodeString time_string;
+
+  icu::FieldPosition ampm_field(icu::DateFormat::kAmPmField);
+  formatter->format(
+      static_cast<UDate>(time.ToDoubleT() * 1000), time_string, ampm_field);
+  int ampm_length = ampm_field.getEndIndex() - ampm_field.getBeginIndex();
+  if (ampm_length) {
+    int begin = ampm_field.getBeginIndex();
+    // Doesn't include any spacing before the field.
+    if (begin)
+      begin--;
+    time_string.removeBetween(begin, ampm_field.getEndIndex());
+  }
+  return string16(time_string.getBuffer(),
+                  static_cast<size_t>(time_string.length()));
+}
+
 }  // namespace
 
 namespace base {
@@ -39,11 +59,12 @@ string16 TimeFormatTimeOfDay(const Time& time) {
 }
 
 string16 TimeFormatTimeOfDayWithHourClockType(const Time& time,
-                                              HourClockType type) {
+                                              HourClockType type,
+                                              AmPmClockType ampm) {
   // Just redirect to the normal function if the default type matches the
   // given type.
   HourClockType default_type = GetHourClockType();
-  if (default_type == type) {
+  if (default_type == type && (type == k24HourClock || ampm == kKeepAmPm)) {
     return TimeFormatTimeOfDay(time);
   }
 
@@ -63,7 +84,11 @@ string16 TimeFormatTimeOfDayWithHourClockType(const Time& time,
   // Then, format the time using the generated pattern.
   icu::SimpleDateFormat formatter(generated_pattern, status);
   CHECK(U_SUCCESS(status));
-  return TimeFormat(&formatter, time);
+  if (ampm == kKeepAmPm) {
+    return TimeFormat(&formatter, time);
+  } else {
+    return TimeFormatWithoutAmPm(&formatter, time);
+  }
 }
 
 string16 TimeFormatShortDate(const Time& time) {
diff --git a/base/i18n/time_formatting.h b/base/i18n/time_formatting.h
index 99d1911..85c0169 100644
--- a/base/i18n/time_formatting.h
+++ b/base/i18n/time_formatting.h
@@ -21,14 +21,22 @@ enum HourClockType {
   k24HourClock,  // Uses 0-23. e.g., "15:07"
 };
 
+// Argument type used to specify whether or not to include AM/PM sign.
+enum AmPmClockType {
+  kDropAmPm,  // Drops AM/PM sign. e.g., "3:07"
+  kKeepAmPm,  // Keeps AM/PM sign. e.g., "3:07 PM"
+};
+
 // Returns the time of day, e.g., "3:07 PM".
 string16 TimeFormatTimeOfDay(const Time& time);
 
 // Returns the time of day in the specified hour clock type. e.g.
-// "3:07 PM" (type == k12HourClock).
+// "3:07 PM" (type == k12HourClock, ampm == kKeepAmPm).
+// "3:07"    (type == k12HourClock, ampm == kDropAmPm).
 // "15:07"   (type == k24HourClock).
 string16 TimeFormatTimeOfDayWithHourClockType(const Time& time,
-                                              HourClockType type);
+                                              HourClockType type,
+                                              AmPmClockType ampm);
 
 // Returns a shortened date, e.g. "Nov 7, 2007"
 string16 TimeFormatShortDate(const Time& time);
@@ -36,8 +44,7 @@ string16 TimeFormatShortDate(const Time& time);
 // Returns a numeric date such as 12/13/52.
 string16 TimeFormatShortDateNumeric(const Time& time);
 
-// Formats a time in a friendly sentence format, e.g.
-// "Monday, March 6, 2008 2:44:30 PM".
+// Returns a numeric date and time such as "12/13/52 2:44:30 PM".
 string16 TimeFormatShortDateAndTime(const Time& time);
 
 // Formats a time in a friendly sentence format, e.g.
diff --git a/base/i18n/time_formatting_unittest.cc b/base/i18n/time_formatting_unittest.cc
new file mode 100644
index 0000000..eab12d9
--- /dev/null
+++ b/base/i18n/time_formatting_unittest.cc
@@ -0,0 +1,167 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/i18n/time_formatting.h"
+
+#include "base/time.h"
+#include "base/utf_string_conversions.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "unicode/locid.h"
+
+namespace {
+
+void SetICUDefaultLocale(const std::string& locale_string) {
+  icu::Locale locale(locale_string.c_str());
+  UErrorCode error_code = U_ZERO_ERROR;
+  icu::Locale::setDefault(locale, error_code);
+  EXPECT_TRUE(U_SUCCESS(error_code));
+}
+
+const base::Time::Exploded kTestDateTimeExploded = {
+  2011, 4, 6, 30, // Sat, Apr 30, 2011
+  15, 42, 7, 0    // 15:42:07.000
+};
+
+}  // namespace
+
+TEST(TimeFormattingTest, TimeFormatTimeOfDayDefault12h) {
+  // Test for a locale defaulted to 12h clock.
+  // As an instance, we use third_party/icu/source/data/locales/en.txt.
+  SetICUDefaultLocale("en_US");
+
+  base::Time time(base::Time::FromLocalExploded(kTestDateTimeExploded));
+  string16 clock24h(ASCIIToUTF16("15:42"));
+  string16 clock12h_pm(ASCIIToUTF16("3:42 PM"));
+  string16 clock12h(ASCIIToUTF16("3:42"));
+
+  // The default is 12h clock.
+  EXPECT_EQ(clock12h_pm,
+            base::TimeFormatTimeOfDay(time));
+  EXPECT_EQ(base::k12HourClock, base::GetHourClockType());
+  // k{Keep,Drop}AmPm should not affect for 24h clock.
+  EXPECT_EQ(clock24h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k24HourClock,
+                                                       base::kKeepAmPm));
+  EXPECT_EQ(clock24h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k24HourClock,
+                                                       base::kDropAmPm));
+  // k{Keep,Drop}AmPm affects for 12h clock.
+  EXPECT_EQ(clock12h_pm,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k12HourClock,
+                                                       base::kKeepAmPm));
+  EXPECT_EQ(clock12h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k12HourClock,
+                                                       base::kDropAmPm));
+}
+
+
+TEST(TimeFormattingTest, TimeFormatTimeOfDayDefault24h) {
+  // Test for a locale defaulted to 24h clock.
+  // As an instance, we use third_party/icu/source/data/locales/en_GB.txt.
+  SetICUDefaultLocale("en_GB");
+
+  base::Time time(base::Time::FromLocalExploded(kTestDateTimeExploded));
+  string16 clock24h(ASCIIToUTF16("15:42"));
+  string16 clock12h_pm(ASCIIToUTF16("3:42 PM"));
+  string16 clock12h(ASCIIToUTF16("3:42"));
+
+  // The default is 24h clock.
+  EXPECT_EQ(clock24h,
+            base::TimeFormatTimeOfDay(time));
+  EXPECT_EQ(base::k24HourClock, base::GetHourClockType());
+  // k{Keep,Drop}AmPm should not affect for 24h clock.
+  EXPECT_EQ(clock24h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k24HourClock,
+                                                       base::kKeepAmPm));
+  EXPECT_EQ(clock24h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k24HourClock,
+                                                       base::kDropAmPm));
+  // k{Keep,Drop}AmPm affects for 12h clock.
+  EXPECT_EQ(clock12h_pm,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k12HourClock,
+                                                       base::kKeepAmPm));
+  EXPECT_EQ(clock12h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k12HourClock,
+                                                       base::kDropAmPm));
+}
+
+TEST(TimeFormattingTest, TimeFormatTimeOfDayJP) {
+  // Test for a locale that uses different mark than "AM" and "PM".
+  // As an instance, we use third_party/icu/source/data/locales/ja.txt.
+  SetICUDefaultLocale("ja_JP");
+
+  base::Time time(base::Time::FromLocalExploded(kTestDateTimeExploded));
+  string16 clock24h(ASCIIToUTF16("15:42"));
+  string16 clock12h_pm(WideToUTF16(L"\x5348\x5f8c"L"3:42"));
+  string16 clock12h(ASCIIToUTF16("3:42"));
+
+  // The default is 24h clock.
+  EXPECT_EQ(clock24h,
+            base::TimeFormatTimeOfDay(time));
+  EXPECT_EQ(base::k24HourClock, base::GetHourClockType());
+  // k{Keep,Drop}AmPm should not affect for 24h clock.
+  EXPECT_EQ(clock24h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k24HourClock,
+                                                       base::kKeepAmPm));
+  EXPECT_EQ(clock24h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k24HourClock,
+                                                       base::kDropAmPm));
+  // k{Keep,Drop}AmPm affects for 12h clock.
+  EXPECT_EQ(clock12h_pm,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k12HourClock,
+                                                       base::kKeepAmPm));
+  EXPECT_EQ(clock12h,
+            base::TimeFormatTimeOfDayWithHourClockType(time,
+                                                       base::k12HourClock,
+                                                       base::kDropAmPm));
+}
+
+TEST(TimeFormattingTest, TimeFormatDateUS) {
+  // See third_party/icu/source/data/locales/en.txt.
+  // The date patterns are "EEEE, MMMM d, y", "MMM d, y", and "M/d/yy".
+  SetICUDefaultLocale("en_US");
+
+  base::Time time(base::Time::FromLocalExploded(kTestDateTimeExploded));
+
+  EXPECT_EQ(ASCIIToUTF16("Apr 30, 2011"),
+            base::TimeFormatShortDate(time));
+  EXPECT_EQ(ASCIIToUTF16("4/30/11"),
+            base::TimeFormatShortDateNumeric(time));
+  EXPECT_EQ(ASCIIToUTF16("4/30/11 3:42:07 PM"),
+            base::TimeFormatShortDateAndTime(time));
+  EXPECT_EQ(ASCIIToUTF16("Saturday, April 30, 2011 3:42:07 PM"),
+            base::TimeFormatFriendlyDateAndTime(time));
+  EXPECT_EQ(ASCIIToUTF16("Saturday, April 30, 2011"),
+            base::TimeFormatFriendlyDate(time));
+}
+
+TEST(TimeFormattingTest, TimeFormatDateGB) {
+  // See third_party/icu/source/data/locales/en_GB.txt.
+  // The date patterns are "EEEE, d MMMM y", "d MMM y", and "dd/MM/yyyy".
+  SetICUDefaultLocale("en_GB");
+
+  base::Time time(base::Time::FromLocalExploded(kTestDateTimeExploded));
+
+  EXPECT_EQ(ASCIIToUTF16("30 Apr 2011"),
+            base::TimeFormatShortDate(time));
+  EXPECT_EQ(ASCIIToUTF16("30/04/2011"),
+            base::TimeFormatShortDateNumeric(time));
+  EXPECT_EQ(ASCIIToUTF16("30/04/2011 15:42:07"),
+            base::TimeFormatShortDateAndTime(time));
+  EXPECT_EQ(ASCIIToUTF16("Saturday, 30 April 2011 15:42:07"),
+            base::TimeFormatFriendlyDateAndTime(time));
+  EXPECT_EQ(ASCIIToUTF16("Saturday, 30 April 2011"),
+            base::TimeFormatFriendlyDate(time));
+}
diff --git a/chrome/browser/chromeos/status/clock_menu_button.cc b/chrome/browser/chromeos/status/clock_menu_button.cc
index a6c72d2..062c0d3 100644
--- a/chrome/browser/chromeos/status/clock_menu_button.cc
+++ b/chrome/browser/chromeos/status/clock_menu_button.cc
@@ -88,30 +88,10 @@ void ClockMenuButton::UpdateText() {
   const bool use_24hour_clock =
       host_->GetProfile() &&
       host_->GetProfile()->GetPrefs()->GetBoolean(prefs::kUse24HourClock);
-  if (use_24hour_clock) {
-    SetText(UTF16ToWide(base::TimeFormatTimeOfDayWithHourClockType(
-        time, base::k24HourClock)));
-  } else {
-    // Remove the am/pm field if it's present.
-    scoped_ptr<icu::DateFormat> formatter(
-        icu::DateFormat::createTimeInstance(icu::DateFormat::kShort));
-    icu::UnicodeString time_string;
-    icu::FieldPosition ampm_field(icu::DateFormat::kAmPmField);
-    formatter->format(
-        static_cast<UDate>(time.ToDoubleT() * 1000), time_string, ampm_field);
-    int ampm_length = ampm_field.getEndIndex() - ampm_field.getBeginIndex();
-    if (ampm_length) {
-      int begin = ampm_field.getBeginIndex();
-      // Doesn't include any spacing before the field.
-      if (begin)
-        begin--;
-      time_string.removeBetween(begin, ampm_field.getEndIndex());
-    }
-    string16 time_string16 =
-        string16(time_string.getBuffer(),
-                 static_cast<size_t>(time_string.length()));
-    SetText(UTF16ToWide(time_string16));
-  }
+  SetText(UTF16ToWide(base::TimeFormatTimeOfDayWithHourClockType(
+      time,
+      use_24hour_clock ? base::k24HourClock : base::k12HourClock,
+      base::kDropAmPm)));
   SetTooltipText(UTF16ToWide(base::TimeFormatShortDate(time)));
   SchedulePaint();
 }