Improve the timezone matching

GetKnownTimezoneOrNull() is supposed to find a matching timezone entry
to an input tz id in the CrOS tz list.  When looking for a match, the
canonical ids have to be taken into account.

Note: This CL started as a speculative fix to bug 466014, but
I couldn't verify that it fixes the bug on CrOS. It does fix the bug
when I tested 'Chrome with cros=1' on Linux (i.e. stub implementation.)
with tz changed on Linux.

BUG=466014
TEST=manual: see the bug

Review URL: https://codereview.chromium.org/1082323003

Cr-Commit-Position: refs/heads/master@{#326709}

6 files changed, 191 insertions, 13 deletions

diff --git a/chromeos/BUILD.gn b/chromeos/BUILD.gn
index afa1728..ff31dbc 100644
--- a/chromeos/BUILD.gn
+++ b/chromeos/BUILD.gn
@@ -140,6 +140,7 @@ test("chromeos_unittests") {
     "//net:test_support",
     "//testing/gmock",
     "//testing/gtest",
+    "//third_party/icu",
     "//url",
     ":cryptohome_proto",
     ":power_manager_proto",
diff --git a/chromeos/chromeos.gyp b/chromeos/chromeos.gyp
index a5df534..b98e2605 100644
--- a/chromeos/chromeos.gyp
+++ b/chromeos/chromeos.gyp
@@ -441,6 +441,8 @@
       'settings/cros_settings_provider.h',
       'settings/timezone_settings.cc',
       'settings/timezone_settings.h',
+      'settings/timezone_settings_helper.cc',
+      'settings/timezone_settings_helper.h',
       'system/name_value_pairs_parser.cc',
       'system/name_value_pairs_parser.h',
       'system/statistics_provider.cc',
@@ -519,6 +521,7 @@
       'network/shill_property_handler_unittest.cc',
       'process_proxy/process_output_watcher_unittest.cc',
       'process_proxy/process_proxy_unittest.cc',
+      'settings/timezone_settings_unittest.cc',
       'system/name_value_pairs_parser_unittest.cc',
       'system/version_loader_unittest.cc',
       'timezone/timezone_unittest.cc',
@@ -672,6 +675,8 @@
         '../net/net.gyp:net_test_support',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
+        '../third_party/icu/icu.gyp:icuuc',
+        '../third_party/icu/icu.gyp:icui18n',
         '../url/url.gyp:url_lib',
         'chromeos_test_support',
         'cryptohome_proto',
diff --git a/chromeos/settings/timezone_settings.cc b/chromeos/settings/timezone_settings.cc
index 99539ae..4d0b0b4 100644
--- a/chromeos/settings/timezone_settings.cc
+++ b/chromeos/settings/timezone_settings.cc
@@ -20,6 +20,7 @@
 #include "base/sys_info.h"
 #include "base/task_runner.h"
 #include "base/threading/worker_pool.h"
+#include "chromeos/settings/timezone_settings_helper.h"
 
 namespace {
 
@@ -166,6 +167,7 @@ static const char* kTimeZones[] = {
     "Asia/Singapore",
     "Asia/Manila",
     "Asia/Taipei",
+    "Asia/Ulaanbaatar",
     "Asia/Makassar",
     "Asia/Irkutsk",
     "Asia/Yakutsk",
@@ -275,7 +277,7 @@ class TimezoneSettingsBaseImpl : public chromeos::system::TimezoneSettings {
   // Otherwise, returns a timezone from |timezones_|, if such exists, that has
   // the same rule as the given |timezone|.
   // Otherwise, returns NULL.
-  // Note multiple timezones with the same time zone offset may exist
+  // Note multiple timezones with the same time zone rules may exist
   // e.g.
   //   US/Pacific == America/Los_Angeles
   const icu::TimeZone* GetKnownTimezoneOrNull(
@@ -362,18 +364,7 @@ TimezoneSettingsBaseImpl::TimezoneSettingsBaseImpl() {
 
 const icu::TimeZone* TimezoneSettingsBaseImpl::GetKnownTimezoneOrNull(
     const icu::TimeZone& timezone) const {
-  const icu::TimeZone* known_timezone = NULL;
-  for (std::vector<icu::TimeZone*>::const_iterator iter = timezones_.begin();
-       iter != timezones_.end(); ++iter) {
-    const icu::TimeZone* entry = *iter;
-    if (*entry == timezone)
-      return entry;
-    if (entry->hasSameRules(timezone))
-      known_timezone = entry;
-  }
-
-  // May return NULL if we did not find a matching timezone in our list.
-  return known_timezone;
+  return chromeos::system::GetKnownTimezoneOrNull(timezone, timezones_);
 }
 
 void TimezoneSettingsImpl::SetTimezone(const icu::TimeZone& timezone) {
@@ -419,6 +410,11 @@ TimezoneSettingsImpl::TimezoneSettingsImpl() {
 
   icu::TimeZone::setDefault(*timezone_);
   VLOG(1) << "Timezone initially set to " << id;
+  icu::UnicodeString resolvedId;
+  std::string resolvedIdStr;
+  timezone_->getID(resolvedId);
+  VLOG(1) << "Timezone initially resolved to "
+          << resolvedId.toUTF8String(resolvedIdStr);
 }
 
 void TimezoneSettingsStubImpl::SetTimezone(const icu::TimeZone& timezone) {
@@ -428,6 +424,8 @@ void TimezoneSettingsStubImpl::SetTimezone(const icu::TimeZone& timezone) {
   if (!known_timezone)
     known_timezone = &timezone;
 
+  std::string id = base::UTF16ToUTF8(GetTimezoneID(*known_timezone));
+  VLOG(1) << "Setting timezone to " << id;
   timezone_.reset(known_timezone->clone());
   icu::TimeZone::setDefault(*known_timezone);
   FOR_EACH_OBSERVER(Observer, observers_, TimezoneChanged(*known_timezone));
diff --git a/chromeos/settings/timezone_settings_helper.cc b/chromeos/settings/timezone_settings_helper.cc
new file mode 100644
index 0000000..781f774
--- /dev/null
+++ b/chromeos/settings/timezone_settings_helper.cc
@@ -0,0 +1,45 @@
+// Copyright 2015 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 "chromeos/settings/timezone_settings_helper.h"
+
+#include "base/logging.h"
+#include "chromeos/chromeos_export.h"
+
+namespace chromeos {
+namespace system {
+
+CHROMEOS_EXPORT const icu::TimeZone* GetKnownTimezoneOrNull(
+    const icu::TimeZone& timezone,
+    const std::vector<icu::TimeZone*>& timezone_list) {
+  const icu::TimeZone* known_timezone = NULL;
+  icu::UnicodeString id, canonical_id;
+  timezone.getID(id);
+  UErrorCode status = U_ZERO_ERROR;
+  icu::TimeZone::getCanonicalID(id, canonical_id, status);
+  DCHECK(U_SUCCESS(status));
+  for (const auto* entry : timezone_list) {
+    if (*entry == timezone)
+      return entry;
+    // Compare the canonical IDs as well.
+    // For instance, Asia/Ulan_Bator -> Asia/Ulaanbaatar or
+    // Canada/Pacific -> America/Vancouver
+    icu::UnicodeString entry_id, entry_canonical_id;
+    entry->getID(entry_id);
+    icu::TimeZone::getCanonicalID(entry_id, entry_canonical_id, status);
+    DCHECK(U_SUCCESS(status));
+    if (entry_canonical_id == canonical_id)
+      return entry;
+    // Last resort: If no match is found, the last timezone in the list
+    // with matching rules will be returned.
+    if (entry->hasSameRules(timezone))
+      known_timezone = entry;
+  }
+
+  // May return NULL if we did not find a matching timezone in our list.
+  return known_timezone;
+}
+
+}  // namespace system
+}  // namespace chromeos
diff --git a/chromeos/settings/timezone_settings_helper.h b/chromeos/settings/timezone_settings_helper.h
new file mode 100644
index 0000000..c8ed8ee
--- /dev/null
+++ b/chromeos/settings/timezone_settings_helper.h
@@ -0,0 +1,25 @@
+// Copyright 2015 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.
+
+#ifndef CHROMEOS_SETTINGS_TIMEZONE_SETTINGS_HELPER_H_
+#define CHROMEOS_SETTINGS_TIMEZONE_SETTINGS_HELPER_H_
+
+#include <vector>
+
+#include "third_party/icu/source/i18n/unicode/timezone.h"
+
+namespace chromeos {
+namespace system {
+
+// Return a timezone in the list matching |timezone| in terms of
+// id and canonical id. If both fail, return a timezone with
+// the same rules. Otherwise, return null.
+const icu::TimeZone* GetKnownTimezoneOrNull(
+    const icu::TimeZone& timezone,
+    const std::vector<icu::TimeZone*>& timezone_list);
+
+}  // namespace system
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SETTINGS_TIMEZONE_SETTINGS_HELPER_H_
diff --git a/chromeos/settings/timezone_settings_unittest.cc b/chromeos/settings/timezone_settings_unittest.cc
new file mode 100644
index 0000000..d8b5b5f
--- /dev/null
+++ b/chromeos/settings/timezone_settings_unittest.cc
@@ -0,0 +1,104 @@
+// Copyright 2015 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/memory/scoped_ptr.h"
+#include "base/stl_util.h"
+#include "chromeos/settings/timezone_settings_helper.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/icu/source/common/unicode/unistr.h"
+#include "third_party/icu/source/i18n/unicode/timezone.h"
+
+namespace chromeos {
+namespace system {
+
+using icu::TimeZone;
+using icu::UnicodeString;
+
+const char* kTimeZones[] = {
+    "America/Los_Angeles",
+    "America/Vancouver",
+    "America/Chicago",
+    "America/Winnipeg",
+    "America/Mexico_City",
+    "America/Buenos_Aires",
+    "Asia/Ho_Chi_Minh",
+    "Asia/Seoul",
+    "Europe/Athens",
+    "Asia/Ulaanbaatar",
+};
+
+class KnownTimeZoneTest : public testing::Test {
+ public:
+  KnownTimeZoneTest() {}
+  ~KnownTimeZoneTest() override {}
+
+  void SetUp() override {
+    for (const char* id : kTimeZones) {
+      timezones_.push_back(TimeZone::createTimeZone(UnicodeString(id)));
+    }
+  }
+
+  void TearDown() override { STLDeleteElements(&timezones_); }
+
+ protected:
+  std::vector<TimeZone*> timezones_;
+};
+
+TEST_F(KnownTimeZoneTest, IdMatch) {
+  static struct {
+    const char* id;
+    const char* matched;
+  } timezone_match_list[] = {
+      // Self matches
+      {"America/Los_Angeles", "America/Los_Angeles"},
+      {"America/Vancouver", "America/Vancouver"},  // Should not be Los_Angeles
+      {"America/Winnipeg", "America/Winnipeg"},
+      {"Asia/Seoul", "Asia/Seoul"},
+      // Canonical ID matches
+      {"Canada/Pacific", "America/Vancouver"},
+      {"US/Pacific", "America/Los_Angeles"},
+      {"US/Central", "America/Chicago"},
+      {"Mexico/General", "America/Mexico_City"},
+      {"Asia/Ulan_Bator", "Asia/Ulaanbaatar"},
+      // Asia/Saigon is canonical, but the list has Asia/Ho_Chi_Minh
+      {"Asia/Saigon", "Asia/Ho_Chi_Minh"},
+  };
+
+  for (const auto& pair : timezone_match_list) {
+    scoped_ptr<TimeZone> input(
+        TimeZone::createTimeZone(UnicodeString(pair.id)));
+    scoped_ptr<TimeZone> expected(
+        TimeZone::createTimeZone(UnicodeString(pair.matched)));
+    const TimeZone* actual = GetKnownTimezoneOrNull(*input, timezones_);
+    EXPECT_NE(nullptr, actual) << "input=" << pair.id;
+    if (actual == nullptr)
+      continue;
+    UnicodeString actual_id;
+    actual->getID(actual_id);
+    std::string actual_id_str;
+    actual_id.toUTF8String(actual_id_str);
+    EXPECT_EQ(*expected, *actual) << "input=" << pair.id << ", "
+                                  << "expected=" << pair.matched << ", "
+                                  << "actual=" << actual_id_str;
+  }
+}
+
+TEST_F(KnownTimeZoneTest, NoMatch) {
+  static const char* no_match_list[] = {
+      "Africa/Juba",                // Not in the list
+      "Africa/Tripoli",             //  UTC+2 with no DST != Europe/Athens
+      "America/Tijuana",            // Historically != America/Los_Angeles
+      "Europe/Sofia",               // Historically != Europe/Athens
+      "America/Argentina/Cordoba",  // Historically != America/Buenos_Aires
+      "Asia/Tokyo",                 // Historically != Asia/Seoul
+  };
+  for (const char* id : no_match_list) {
+    scoped_ptr<TimeZone> input(TimeZone::createTimeZone(UnicodeString(id)));
+    EXPECT_EQ(NULL, GetKnownTimezoneOrNull(*input, timezones_))
+        << "input=" << id;
+  }
+}
+
+}  // namespace system
+}  // namespace chromeos