base: Make TimeDurationFormat* report failures.

base/i18n/time_formatting.cc

 // 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 <stddef.h>
 
 #include <memory>
 
@@ skipping 158 matching lines @@
   std::unique_ptr<icu::DateFormat> formatter(
       icu::DateFormat::createDateInstance(icu::DateFormat::kFull));
   return TimeFormat(formatter.get(), time);
 }
 
 string16 TimeFormatWithPattern(const Time& time, const char* pattern) {
   icu::SimpleDateFormat formatter = CreateSimpleDateFormatter(pattern);
   return TimeFormat(&formatter, time);
 }
 
-string16 TimeDurationFormat(const TimeDelta time,
-                            const DurationFormatWidth width) {
+bool TimeDurationFormat(const TimeDelta time,
+                        const DurationFormatWidth width,
+                        string16* out) {
+  DCHECK(out);
   UErrorCode status = U_ZERO_ERROR;
   const int total_minutes = static_cast<int>(time.InSecondsF() / 60 + 0.5);
   const int hours = total_minutes / 60;
   const int minutes = total_minutes % 60;
   UMeasureFormatWidth u_width = DurationWidthToMeasureWidth(width);
 
   const icu::Measure measures[] = {
       icu::Measure(hours, icu::MeasureUnit::createHour(status), status),
       icu::Measure(minutes, icu::MeasureUnit::createMinute(status), status)};

[Daniel Erat, 2017/03/06 20:14:58]

it seems to be undocumented, but from my reading of the ICU source, ICU methods
and constructors generally (and hopefully always) bail out without overwriting
the passed-in UErrorCode if it already contains an error.

[Greg Levin, 2017/03/06 21:40:06]

jshin@ would know the ICU code better, but that's certainly how it looks.

Insofar as this CL is for debugging unexpected ICU errors like crbug.com/677043,
it might be helpful to add one (or three?) debugging logs here as well, to
preserve and pinpoint the ICU error code, e.g.,

  if (U_FAILURE(status))
    LOG(ERROR) << "Failed to construct measures[], status = " << status;

   icu::MeasureFormat measure_format(icu::Locale::getDefault(), u_width, status);

[Greg Levin, 2017/03/06 21:40:06]

  if (U_FAILURE(status))
    LOG(ERROR) << "Failed to construct measure_format[], status = " << status;

[Daniel Erat, 2017/03/06 23:19:34]

Done.

   icu::UnicodeString formatted;
   icu::FieldPosition ignore(icu::FieldPosition::DONT_CARE);
   measure_format.formatMeasures(measures, 2, formatted, ignore, status);

[Greg Levin, 2017/03/06 21:40:06]

  if (U_FAILURE(status))
    LOG(ERROR) << "formatMeasures() failed, status = " << status;

Or, if you think three LOGs is too aggressive, then how about just putting

  if (U_FAILURE(status))
    LOG(ERROR) << "ICU error in TimeDurationFormat() failed, status = " <<
status;

here?  (Normally I'd say 3 LOGs was overkill, but since this function currently
seems to be the source of unexpected, mysterious errors, maybe it's warranted
for now?)

[Daniel Erat, 2017/03/06 23:19:34]

sure, added temporary logging throughout.

-  return base::string16(formatted.getBuffer(), formatted.length());
+  *out = base::string16(formatted.getBuffer(), formatted.length());
+  return U_SUCCESS(status);
 }
 
-string16 TimeDurationFormatWithSeconds(const TimeDelta time,
-                                       const DurationFormatWidth width) {
+bool TimeDurationFormatWithSeconds(const TimeDelta time,
+                                   const DurationFormatWidth width,
+                                   string16* out) {
+  DCHECK(out);
   UErrorCode status = U_ZERO_ERROR;
   const int64_t total_seconds = static_cast<int>(time.InSecondsF() + 0.5);
   const int hours = total_seconds / 3600;
   const int minutes = (total_seconds - hours * 3600) / 60;
   const int seconds = total_seconds % 60;
   UMeasureFormatWidth u_width = DurationWidthToMeasureWidth(width);
 
   const icu::Measure measures[] = {
       icu::Measure(hours, icu::MeasureUnit::createHour(status), status),
       icu::Measure(minutes, icu::MeasureUnit::createMinute(status), status),
       icu::Measure(seconds, icu::MeasureUnit::createSecond(status), status)};
   icu::MeasureFormat measure_format(icu::Locale::getDefault(), u_width, status);
   icu::UnicodeString formatted;
   icu::FieldPosition ignore(icu::FieldPosition::DONT_CARE);
   measure_format.formatMeasures(measures, 3, formatted, ignore, status);
-  return base::string16(formatted.getBuffer(), formatted.length());
+  *out = base::string16(formatted.getBuffer(), formatted.length());
+  return U_SUCCESS(status);
 }
 
 string16 DateIntervalFormat(const Time& begin_time,
                             const Time& end_time,
                             DateFormat format) {
   UErrorCode status = U_ZERO_ERROR;
 
   std::unique_ptr<icu::DateIntervalFormat> formatter(
       icu::DateIntervalFormat::createInstance(DateFormatToString(format),
                                               status));
@@ skipping 42 matching lines @@
   // See http://userguide.icu-project.org/formatparse/datetime for details
   // about the date/time format syntax.
   if (pattern_unicode.indexOf('a') == -1) {
     return k24HourClock;
   } else {
     return k12HourClock;
   }
 }
 
 }  // namespace base