Refactored code for timezone settings.

chrome/browser/chromeos/system/timezone_settings.cc

Index: chrome/browser/chromeos/system/timezone_settings.cc
diff --git a/chrome/browser/chromeos/system/timezone_settings.cc b/chrome/browser/chromeos/system/timezone_settings.cc
index d5c505458c5aa6429eb87194fdf9a10d163641d7..c0c6068080633f03a88647c128bd99d55bbb6278 100644
--- a/chrome/browser/chromeos/system/timezone_settings.cc
+++ b/chrome/browser/chromeos/system/timezone_settings.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/system/timezone_settings.h"
 
+#include <string>
+
 #include "base/bind.h"
 #include "base/chromeos/chromeos_version.h"
 #include "base/file_path.h"
@@ -12,15 +14,13 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
 #include "base/observer_list.h"
+#include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
-namespace chromeos {
-namespace system {
-
 namespace {
 
 // The filepath to the timezone file that symlinks to the actual timezone file.
@@ -34,31 +34,104 @@ const char kTimezoneFilesDir[] = "/usr/share/zoneinfo/";
 // Fallback time zone ID used in case of an unexpected error.
 const char kFallbackTimeZoneId[] = "America/Los_Angeles";
 
-}  // namespace
-
-// The TimezoneSettings implementation used in production.
-class TimezoneSettingsImpl : public TimezoneSettings {
- public:
-  // TimezoneSettings implementation:
-  virtual const icu::TimeZone& GetTimezone();
-  virtual void SetTimezone(const icu::TimeZone& timezone);
-  virtual void AddObserver(Observer* observer);
-  virtual void RemoveObserver(Observer* observer);
-
-  static TimezoneSettingsImpl* GetInstance();
-
- private:
-  friend struct DefaultSingletonTraits<TimezoneSettingsImpl>;
-
-  TimezoneSettingsImpl();
-
-  scoped_ptr<icu::TimeZone> timezone_;
-  ObserverList<Observer> observers_;
-
-  DISALLOW_COPY_AND_ASSIGN(TimezoneSettingsImpl);
+// TODO(jungshik): Using Enumerate method in ICU gives 600+ timezones.
+// Even after filtering out duplicate entries with a strict identity check,
+// we still have 400+ zones. Relaxing the criteria for the timezone
+// identity is likely to cut down the number to < 100. Until we
+// come up with a better list, we hard-code the following list as used by
+// Android.
+static const char* kTimeZones[] = {
+    "Pacific/Majuro",
+    "Pacific/Midway",
+    "Pacific/Honolulu",
+    "America/Anchorage",
+    "America/Los_Angeles",
+    "America/Tijuana",
+    "America/Denver",
+    "America/Phoenix",
+    "America/Chihuahua",
+    "America/Chicago",
+    "America/Mexico_City",
+    "America/Costa_Rica",
+    "America/Regina",
+    "America/New_York",
+    "America/Bogota",
+    "America/Caracas",
+    "America/Barbados",
+    "America/Manaus",
+    "America/Santiago",
+    "America/St_Johns",
+    "America/Sao_Paulo",
+    "America/Araguaina",
+    "America/Argentina/Buenos_Aires",
+    "America/Godthab",
+    "America/Montevideo",
+    "Atlantic/South_Georgia",
+    "Atlantic/Azores",
+    "Atlantic/Cape_Verde",
+    "Africa/Casablanca",
+    "Europe/London",
+    "Europe/Amsterdam",
+    "Europe/Belgrade",
+    "Europe/Brussels",
+    "Europe/Sarajevo",
+    "Africa/Windhoek",
+    "Africa/Brazzaville",
+    "Asia/Amman",
+    "Europe/Athens",
+    "Asia/Beirut",
+    "Africa/Cairo",
+    "Europe/Helsinki",
+    "Asia/Jerusalem",
+    "Europe/Minsk",
+    "Africa/Harare",
+    "Asia/Baghdad",
+    "Europe/Moscow",
+    "Asia/Kuwait",
+    "Africa/Nairobi",
+    "Asia/Tehran",
+    "Asia/Baku",
+    "Asia/Tbilisi",
+    "Asia/Yerevan",
+    "Asia/Dubai",
+    "Asia/Kabul",
+    "Asia/Karachi",
+    "Asia/Oral",
+    "Asia/Yekaterinburg",
+    "Asia/Calcutta",
+    "Asia/Colombo",
+    "Asia/Katmandu",
+    "Asia/Almaty",
+    "Asia/Rangoon",
+    "Asia/Krasnoyarsk",
+    "Asia/Bangkok",
+    "Asia/Shanghai",
+    "Asia/Hong_Kong",
+    "Asia/Irkutsk",
+    "Asia/Kuala_Lumpur",
+    "Australia/Perth",
+    "Asia/Taipei",
+    "Asia/Seoul",
+    "Asia/Tokyo",
+    "Asia/Yakutsk",
+    "Australia/Adelaide",
+    "Australia/Darwin",
+    "Australia/Brisbane",
+    "Australia/Hobart",
+    "Australia/Sydney",
+    "Asia/Vladivostok",
+    "Pacific/Guam",
+    "Asia/Magadan",
+    "Pacific/Auckland",
+    "Pacific/Fiji",
+    "Pacific/Tongatapu",
 };
 
 std::string GetTimezoneIDAsString() {
+  // Compare with chromiumos/src/platform/init/ui.conf which fixes certain
+  // incorrect states of the timezone symlink on startup. Thus errors occuring
+  // here should be rather contrived.
+
   // Look at kTimezoneSymlink, see which timezone we are symlinked to.
   char buf[256];
   const ssize_t len = readlink(kTimezoneSymlink, buf,
@@ -115,16 +188,118 @@ void SetTimezoneIDFromString(const std::string& id) {
   }
 }
 
-const icu::TimeZone& TimezoneSettingsImpl::GetTimezone() {
+// Common code of the TimezoneSettings implementations.
+class TimezoneSettingsBaseImpl : public chromeos::system::TimezoneSettings {
+ public:
+  virtual ~TimezoneSettingsBaseImpl();
+
+  // TimezoneSettings implementation:
+  virtual const icu::TimeZone& GetTimezone();
+  virtual const string16 GetCurrentTimezoneID();
+  virtual void SetTimezoneFromID(const string16& timezone_id);
+  virtual void AddObserver(Observer* observer);
+  virtual void RemoveObserver(Observer* observer);
+  virtual const std::vector<icu::TimeZone*> &GetTimezoneList() const;

[Joao da Silva, 2012/07/12 18:34:09]

Mark all of these with OVERRIDE.

[pneubeck2, 2012/07/13 10:31:39]

Done.

+
+ protected:
+  TimezoneSettingsBaseImpl();
+
+  // Returns |timezone| if it is an element of |timezones_|.
+  // 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
+  // e.g.
+  //   US/Pacific == America/Los_Angeles
+  const icu::TimeZone *GetKnownTimezoneOrNull(

[Joao da Silva, 2012/07/12 18:34:09]

Nit: move * next to the return type

[pneubeck2, 2012/07/13 10:31:39]

Done.

+      const icu::TimeZone& timezone) const;
+
+  ObserverList<Observer> observers_;
+  std::vector<icu::TimeZone*> timezones_;
+  scoped_ptr<icu::TimeZone> timezone_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TimezoneSettingsBaseImpl);
+};
+
+TimezoneSettingsBaseImpl::~TimezoneSettingsBaseImpl() {
+  STLDeleteElements(&timezones_);
+}
+
+const icu::TimeZone& TimezoneSettingsBaseImpl::GetTimezone() {
   return *timezone_.get();
 }
 
+const string16 TimezoneSettingsBaseImpl::GetCurrentTimezoneID() {

[Joao da Silva, 2012/07/12 18:34:09]

No need for const when returning by value.

[pneubeck2, 2012/07/13 10:31:39]

Done.

+  return chromeos::system::TimezoneSettings::GetTimezoneID(GetTimezone());
+}
+
+void TimezoneSettingsBaseImpl::SetTimezoneFromID(const string16& timezone_id) {
+  scoped_ptr<icu::TimeZone> timezone(icu::TimeZone::createTimeZone(
+      icu::UnicodeString(timezone_id.c_str(), timezone_id.size())));
+  SetTimezone(*timezone);
+}
+
+void TimezoneSettingsBaseImpl::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void TimezoneSettingsBaseImpl::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+const std::vector<icu::TimeZone*> &
+TimezoneSettingsBaseImpl::GetTimezoneList() const {
+  return timezones_;
+}
+
+TimezoneSettingsBaseImpl::TimezoneSettingsBaseImpl() {
+  for (size_t i = 0; i < arraysize(kTimeZones); i++) {

[Joao da Silva, 2012/07/12 18:34:09]

Nit: ++i

[pneubeck2, 2012/07/13 10:31:39]

Done.

+    timezones_.push_back(icu::TimeZone::createTimeZone(
+        icu::UnicodeString(kTimeZones[i], -1, US_INV)));
+  }
+}
+
+const icu::TimeZone *TimezoneSettingsBaseImpl::GetKnownTimezoneOrNull(

[Joao da Silva, 2012/07/12 18:34:09]

Nit: move * next to the return type

[pneubeck2, 2012/07/13 10:31:39]

Done.

+    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;
+}
+
+// The TimezoneSettings implementation used in production.
+class TimezoneSettingsImpl : public TimezoneSettingsBaseImpl {

[Joao da Silva, 2012/07/12 18:34:09]

Put all the class declarations first in the file, and then the method
implementations.

[pneubeck2, 2012/07/13 10:31:39]

Done.

+ public:
+  // TimezoneSettings implementation:
+  virtual void SetTimezone(const icu::TimeZone& timezone);

[Joao da Silva, 2012/07/12 18:34:09]

OVERRIDE

[pneubeck2, 2012/07/13 10:31:39]

Done.

+
+  static TimezoneSettingsImpl* GetInstance();
+
+ private:
+
+  friend struct DefaultSingletonTraits<TimezoneSettingsImpl>;

[Joao da Silva, 2012/07/12 18:34:09]

Nit: remove the empty line above. Add DISALLOW_COPY_AND_ASSIGN at the end of the
class.

[pneubeck2, 2012/07/13 10:31:39]

Done.

+
+  TimezoneSettingsImpl();
+};
+
 void TimezoneSettingsImpl::SetTimezone(const icu::TimeZone& timezone) {
-  timezone_.reset(timezone.clone());
-  icu::UnicodeString unicode;
-  timezone.getID(unicode);
-  std::string id;
-  UTF16ToUTF8(unicode.getBuffer(), unicode.length(), &id);
+  // replace |timezone| by a known timezone with the same rules. if
+  // none exists go on with |timezone|.

[Joao da Silva, 2012/07/12 18:34:09]

Start both sentences in uppercase.

[pneubeck2, 2012/07/13 10:31:39]

Done.

+  const icu::TimeZone *known_timezone = GetKnownTimezoneOrNull(timezone);

[Joao da Silva, 2012/07/12 18:34:09]

Nit: * next to the type

[pneubeck2, 2012/07/13 10:31:39]

Done.

+  if (!known_timezone)
+    known_timezone = &timezone;
+
+  timezone_.reset(known_timezone->clone());
+  std::string id = UTF16ToUTF8(GetTimezoneID(*known_timezone));
   VLOG(1) << "Setting timezone to " << id;
   // Change the timezone config files on the FILE thread. It's safe to do this
   // in the background as the following operations don't depend on the
@@ -132,56 +307,50 @@ void TimezoneSettingsImpl::SetTimezone(const icu::TimeZone& timezone) {
   BrowserThread::PostTask(BrowserThread::FILE,

[Joao da Silva, 2012/07/12 18:34:09]

Post to the blocking pool instead:

BrowserThread::PostBlockingPoolTask(FROM_HERE, Bind(...));

[pneubeck2, 2012/07/13 10:31:39]

Done.

                           FROM_HERE,
                           base::Bind(&SetTimezoneIDFromString, id));
-  icu::TimeZone::setDefault(timezone);
-  FOR_EACH_OBSERVER(Observer, observers_, TimezoneChanged(timezone));
-}
-
-void TimezoneSettingsImpl::AddObserver(Observer* observer) {
-  observers_.AddObserver(observer);
+  icu::TimeZone::setDefault(*known_timezone);
+  FOR_EACH_OBSERVER(Observer, observers_, TimezoneChanged(*known_timezone));
 }
 
-void TimezoneSettingsImpl::RemoveObserver(Observer* observer) {
-  observers_.RemoveObserver(observer);
+TimezoneSettingsImpl* TimezoneSettingsImpl::GetInstance() {
+  return Singleton<TimezoneSettingsImpl,
+                   DefaultSingletonTraits<TimezoneSettingsImpl> >::get();
 }
 
 TimezoneSettingsImpl::TimezoneSettingsImpl() {
-  // Get Timezone
   std::string id = GetTimezoneIDAsString();
   if (id.empty()) {
     id = kFallbackTimeZoneId;
-    LOG(ERROR) << "Got an empty string for timezone, default to " << id;
+    LOG(ERROR) << "Got an empty string for timezone, default to '" << id;
   }
-  icu::TimeZone* timezone =
-      icu::TimeZone::createTimeZone(icu::UnicodeString::fromUTF8(id));
-  timezone_.reset(timezone);
-  icu::TimeZone::setDefault(*timezone);
-  VLOG(1) << "Timezone is " << id;
-}
 
-TimezoneSettingsImpl* TimezoneSettingsImpl::GetInstance() {
-  return Singleton<TimezoneSettingsImpl,
-                   DefaultSingletonTraits<TimezoneSettingsImpl> >::get();
+  timezone_.reset(icu::TimeZone::createTimeZone(
+      icu::UnicodeString::fromUTF8(id)));
+
+  // Store a known timezone equivalent to id in |timezone_|.
+  const icu::TimeZone *known_timezone = GetKnownTimezoneOrNull(*timezone_);

[Joao da Silva, 2012/07/12 18:34:09]

Nit: move * next to type

[pneubeck2, 2012/07/13 10:31:39]

Done.

+  if (known_timezone != NULL && *known_timezone != *timezone_)
+    // Not necessary to update the filesystem because |known_timezone| has the
+    // same rules.
+    timezone_.reset(known_timezone->clone());
+
+  icu::TimeZone::setDefault(*timezone_);
+  VLOG(1) << "Timezone initially set to " << id;
 }
 
 // The stub TimezoneSettings implementation used on Linux desktop.
-class TimezoneSettingsStubImpl : public TimezoneSettings {
+class TimezoneSettingsStubImpl : public TimezoneSettingsBaseImpl {

[Joao da Silva, 2012/07/12 18:34:09]

Declare all classes before defining method implementations.

[pneubeck2, 2012/07/13 10:31:39]

Done.

  public:
   // TimezoneSettings implementation:
-  virtual const icu::TimeZone& GetTimezone() {
-    return *timezone_.get();
-  }
-
   virtual void SetTimezone(const icu::TimeZone& timezone) {

[Joao da Silva, 2012/07/12 18:34:09]

OVERRIDE

[pneubeck2, 2012/07/13 10:31:39]

Done.

-    icu::TimeZone::setDefault(timezone);
-    FOR_EACH_OBSERVER(Observer, observers_, TimezoneChanged(timezone));
-  }
-
-  virtual void AddObserver(Observer* observer) {
-    observers_.AddObserver(observer);
-  }
-
-  virtual void RemoveObserver(Observer* observer) {
-    observers_.RemoveObserver(observer);
+    // replace |timezone| by a known timezone with the same rules. if
+    // none exists go on with |timezone|.

[Joao da Silva, 2012/07/12 18:34:09]

Start both sentences in uppercase.

[pneubeck2, 2012/07/13 10:31:39]

Done.

+    const icu::TimeZone *known_timezone = GetKnownTimezoneOrNull(timezone);
+    if (!known_timezone)
+      known_timezone = &timezone;
+
+    timezone_.reset(known_timezone->clone());
+    icu::TimeZone::setDefault(*known_timezone);
+    FOR_EACH_OBSERVER(Observer, observers_, TimezoneChanged(*known_timezone));
   }
 
   static TimezoneSettingsStubImpl* GetInstance() {
@@ -194,13 +363,16 @@ class TimezoneSettingsStubImpl : public TimezoneSettings {
 
   TimezoneSettingsStubImpl() {
     timezone_.reset(icu::TimeZone::createDefault());
+    const icu::TimeZone *known_timezone = GetKnownTimezoneOrNull(*timezone_);

[Joao da Silva, 2012/07/12 18:34:09]

Nit: move * next to the type

[pneubeck2, 2012/07/13 10:31:39]

Done.

+    if (known_timezone != NULL && *known_timezone != *timezone_)
+      timezone_.reset(known_timezone->clone());
   }

[Joao da Silva, 2012/07/12 18:34:09]

DISALLOW_COPY_AND_ASSIGN

[pneubeck2, 2012/07/13 10:31:39]

Done.

+};
 
-  scoped_ptr<icu::TimeZone> timezone_;
-  ObserverList<Observer> observers_;
+}  // namespace
 
-  DISALLOW_COPY_AND_ASSIGN(TimezoneSettingsStubImpl);
-};
+namespace chromeos {
+namespace system {
 
 TimezoneSettings* TimezoneSettings::GetInstance() {
   if (base::chromeos::IsRunningOnChromeOS()) {
@@ -210,5 +382,13 @@ TimezoneSettings* TimezoneSettings::GetInstance() {
   }
 }
 
+string16 TimezoneSettings::GetTimezoneID(

[Joao da Silva, 2012/07/12 18:34:09]

Add a comment before this line like this:

// static

[pneubeck2, 2012/07/13 10:31:39]

Done.

+    const icu::TimeZone& timezone) {

[Joao da Silva, 2012/07/12 18:34:09]

No need to break the previous line?

[pneubeck2, 2012/07/13 10:31:39]

Done.

+  icu::UnicodeString id;
+  timezone.getID(id);
+  return string16(id.getBuffer(), id.length());
+}
+
+

[Joao da Silva, 2012/07/12 18:34:09]

Nit: remove extra newline

[pneubeck2, 2012/07/13 10:31:39]

Done.

 }  // namespace system
 }  // namespace chromeos