[date] Add ICU backend for timezone info behind a flag

src/i18n.cc

Index: src/i18n.cc
diff --git a/src/i18n.cc b/src/i18n.cc
index d2245ef34a9a319a53b4cf4b4ea05ec095fef2d5..30d649e45c95977047b7092034b0c80b62518e78 100644
--- a/src/i18n.cc
+++ b/src/i18n.cc
@@ -899,5 +899,87 @@ void V8BreakIterator::DeleteBreakIterator(
   GlobalHandles::Destroy(reinterpret_cast<Object**>(data.GetParameter()));
 }
 
+ICUTimezoneCache::ICUTimezoneCache() { Clear(); }

[jgruber, 2017/03/03 08:35:29]

I'm surprised this doesn't fail. Clear() deletes timezone_, which is
uninitialized at that point. Pointer class members aren't auto-initialized by
the compiler AFAIK, so it can contain random junk.

[Dan Ehrenberg, 2017/03/03 13:20:27]

Wow, not sure how I got this so wrong; I guess I started with initializing them
more eagerly, and forgot to insert the default initialization, and then was
lucky in tests.. Fixed.

+
+ICUTimezoneCache::~ICUTimezoneCache() { Clear(); }
+
+const char* ICUTimezoneCache::LocalTimezone(double time_ms) {
+  bool is_dst = DaylightSavingsOffset(time_ms) != 0;
+  char* name = is_dst ? dst_timezone_name_ : timezone_name_;

[jgruber, 2017/03/03 08:35:29]

The cache is never filled, not sure if that's on purpose.

[Dan Ehrenberg, 2017/03/03 13:20:27]

What do you mean? Why is it never filled? I thought it normally would be. I
locally put in some print statements and verified that the 'cached' case is hit.

When used in conjunction with https://codereview.chromium.org/2726253002/ , the
caching part is not so important. The really important part is the memory
ownership--the protocol returns a const char* which is supposed to just stick
around (as OSes provide), so the solution here is to put it in the object. Might
as well keep it around once it's there, but I'm also fine with removing it.

[jgruber, 2017/03/03 13:29:08]

Yup you're right. Sorry, my mistake.

+  if (name[0] == '\0') {
+    icu::UnicodeString result;
+    GetTimeZone()->getDisplayName(is_dst, icu::TimeZone::SHORT, result);
+
+    // With the SHORT format and the default locale, the result should be of
+    // the form PST or CET, short and within ASCII range.
+    int32_t length = result.length();
+    CHECK(length < kMaxTimezoneChars);
+    for (int32_t i = 0; i < length; i++) {
+      UChar ch = result[i];
+      CHECK(ch == static_cast<UChar>(static_cast<char>(ch)));
+      name[i] = static_cast<char>(ch);
+    }
+    name[length] = '\0';
+  }
+  return const_cast<const char*>(name);
+}
+
+icu::TimeZone* ICUTimezoneCache::GetTimeZone() {
+  if (timezone_ == nullptr) {
+    timezone_ = icu::TimeZone::createDefault();
+  }
+  return timezone_;
+}
+
+bool ICUTimezoneCache::GetOffsets(double time_ms, int32_t* raw_offset,
+                                  int32_t* dst_offset) {
+  UErrorCode status = U_ZERO_ERROR;
+  GetTimeZone()->getOffset(time_ms, false, *raw_offset, *dst_offset, status);
+  if (!U_SUCCESS(status)) return false;

[jgruber, 2017/03/03 08:35:29]

Nit: return U_SUCCESS(status)

[Dan Ehrenberg, 2017/03/03 13:20:27]

Done, but I would be interested in more feedback here--what's the correct
behavior for an ICU failure like this--silently slog on with UTC values, crash,
throw an "illegal access" error, increment a UseCounter, something else?

+  return true;
+}
+
+double ICUTimezoneCache::DaylightSavingsOffset(double time_ms) {
+  int32_t raw_offset, dst_offset;
+  if (!GetOffsets(time_ms, &raw_offset, &dst_offset)) return 0;
+  return dst_offset;
+}
+
+double ICUTimezoneCache::LocalTimeOffset() {
+  int32_t raw_offset, dst_offset;
+  if (!GetOffsets(icu::Calendar::getNow(), &raw_offset, &dst_offset)) return 0;
+  return raw_offset;
+}
+
+void ICUTimezoneCache::Clear() {
+  delete timezone_;
+  timezone_ = nullptr;
+  timezone_name_[0] = '\0';
+  dst_timezone_name_[0] = '\0';
+}
+
+ICUOSTimezoneCache::ICUOSTimezoneCache() : os_tz_cache_(nullptr) {}
+
+ICUOSTimezoneCache::~ICUOSTimezoneCache() { delete os_tz_cache_; }
+
+const char* ICUOSTimezoneCache::LocalTimezone(double time_ms) {
+  const char* result = ICUTimezoneCache::LocalTimezone(time_ms);
+  // ICU often returns timezones which are not as descriptive as they
+  // could be, e.g., GMT+1 for Europe/Madrid when in en_US. When a timezone
+  // starts with GMT, call out to the OS to get the 'real' name.

[ulan, 2017/03/03 13:20:27]

This seems hacky as it breaks the "cache" semantics of the class. Can we fix it
one the ICU side and keep this class in sync with ICU?

[Dan Ehrenberg, 2017/03/03 15:06:54]

I agree that it's hacky, but I'm not sure we'll get a fix into ICU any time
soon.

I'm not sure the caching is so important. It turns out that the redundant
calculation of whether we're in DST or not was expensive enough that the cache
in https://codereview.chromium.org/2726253002/ was necessary in addition anyway
to not leave Date with a performance regression when the flag is flipped on.

I took a look at the underlying data files. the tz database
(https://www.iana.org/time-zones) has all the relevant names. These files are
translated into timezone information files in /usr/share/zoneinfo , which has a
representation of all names from the original database.

By contrast, ICU seems to be getting its information from CLDR. CLDR doesn't
represent all of these different time zones, just the ones that are relevant for
the locale (and doesn't have historical name information either).

One reason that these are taken from CLDR rather than just the tz database is
because the three-letter acronyms actually are locale-dependent. For example,
CLDR represents "CET" as either CET, MET or SEČ depending on the locale
(http://www.unicode.org/cldr/charts/latest/by_type/timezones.europe.html#47cbf...).
Here, we only care about the "root locale" however, and aren't trying to
localize the output.

Speculating, another reason to limit the number of short acronyms could be that
ICU implements date parsing, and the acronyms for timezone names are ambiguous
(reused with multiple meanings). If a more limited set of timezone names are
considered relevant for the locale, then parsing is more possible.

Jungshik, do you have any more thoughts, or a deeper understanding of why these
short timezone names aren't more available through CLDR? What I don't understand
is why the tz database names are not taken into the root locale. Or, absent
that, maybe ICU could have a mode fall back to these names in the logic in
TimeZone::getDisplayName that currently falls back to "localized" GMT +- the
current offset.

+  if (result[0] == 'G' && result[1] == 'M' && result[2] == 'T') {

[jgruber, 2017/03/03 08:35:29]

Nit: Another option would be to use strncmp. 

[Dan Ehrenberg, 2017/03/03 13:20:27]

Done.

+    if (os_tz_cache_ == nullptr) {
+      os_tz_cache_ = base::OS::CreateTimezoneCache();
+    }
+    return os_tz_cache_->LocalTimezone(time_ms);
+  }
+  return result;
+}
+
+void ICUOSTimezoneCache::Clear() {
+  ICUTimezoneCache::Clear();
+  if (os_tz_cache_) os_tz_cache_->Clear();
+}
+
 }  // namespace internal
 }  // namespace v8