base: Make TimeDurationFormat* report failures.

Update base::TimeDurationFormat() and
base::TimeDurationFormatWithSeconds() to return boolean
"success" values and use out-params to return formatted
strings. ICU errors were formerly swallowed, which makes it
impossible for the UI to handle them gracefully or for
developers to investigate their causes.

BUG=698802,677043

Review-Url: https://codereview.chromium.org/2734883003
Cr-Commit-Position: refs/heads/master@{#455144}
(cherry picked from commit efcd30c)

Review-Url: https://codereview.chromium.org/2736363002 .
Cr-Commit-Position: refs/branch-heads/3029@{#73}
Cr-Branched-From: 939b32e-refs/heads/master@{#454471}

Author: Daniel Erat

ash/common/system/chromeos/power/battery_notification.cc

@@ -62,9 +62,11 @@ std::unique_ptr<Notification> CreateNotification(
   } else if (PowerStatus::ShouldDisplayBatteryTime(time) &&
              !status.IsBatteryDischargingOnLinePower()) {
     if (status.IsBatteryCharging()) {
+      base::string16 duration;
+      if (!TimeDurationFormat(time, base::DURATION_WIDTH_NARROW, &duration))
+        LOG(ERROR) << "Failed to format duration " << time.ToInternalValue();
       time_message = l10n_util::GetStringFUTF16(
-          IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL,
-          TimeDurationFormat(time, base::DURATION_WIDTH_NARROW));
+          IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL, duration);
     } else {
       // This is a low battery warning prompting the user in minutes.
       time_message = ui::TimeFormat::Simple(ui::TimeFormat::FORMAT_REMAINING,

ash/common/system/chromeos/power/power_status_view.cc

@@ -124,11 +124,15 @@ void PowerStatusView::UpdateText() {
                                  : status.GetBatteryTimeToEmpty();
       if (PowerStatus::ShouldDisplayBatteryTime(time) &&
           !status.IsBatteryDischargingOnLinePower()) {
+        base::string16 duration;
+        if (!base::TimeDurationFormat(time, base::DURATION_WIDTH_NUMERIC,
+                                      &duration))
+          LOG(ERROR) << "Failed to format duration " << time.ToInternalValue();
         battery_time_status = l10n_util::GetStringFUTF16(
             status.IsBatteryCharging()
                 ? IDS_ASH_STATUS_TRAY_BATTERY_TIME_UNTIL_FULL_SHORT
                 : IDS_ASH_STATUS_TRAY_BATTERY_TIME_LEFT_SHORT,
-            TimeDurationFormat(time, base::DURATION_WIDTH_NUMERIC));
+            duration);
       }
     }
   }

base/i18n/time_formatting.cc

@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
+#include "third_party/icu/source/common/unicode/utypes.h"
 #include "third_party/icu/source/i18n/unicode/datefmt.h"
 #include "third_party/icu/source/i18n/unicode/dtitvfmt.h"
 #include "third_party/icu/source/i18n/unicode/dtptngen.h"
@@ -176,26 +177,51 @@ string16 TimeFormatWithPattern(const Time& time, const char* 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);
 
+  // TODO(derat): Delete the |status| checks and LOG(ERROR) calls throughout
+  // this function once the cause of http://crbug.com/677043 is tracked down.
   const icu::Measure measures[] = {
       icu::Measure(hours, icu::MeasureUnit::createHour(status), status),
       icu::Measure(minutes, icu::MeasureUnit::createMinute(status), status)};
+  if (U_FAILURE(status)) {
+    LOG(ERROR) << "Creating MeasureUnit or Measure for " << hours << "h"
+               << minutes << "m failed: " << u_errorName(status);
+    return false;
+  }
+
   icu::MeasureFormat measure_format(icu::Locale::getDefault(), u_width, status);
+  if (U_FAILURE(status)) {
+    LOG(ERROR) << "Creating MeasureFormat for "
+               << icu::Locale::getDefault().getName()
+               << " failed: " << u_errorName(status);
+    return false;
+  }
+
   icu::UnicodeString formatted;
   icu::FieldPosition ignore(icu::FieldPosition::DONT_CARE);
   measure_format.formatMeasures(measures, 2, formatted, ignore, status);
-  return base::string16(formatted.getBuffer(), formatted.length());
+  if (U_FAILURE(status)) {
+    LOG(ERROR) << "formatMeasures failed: " << u_errorName(status);
+    return false;
+  }
+
+  *out = base::string16(formatted.getBuffer(), formatted.length());
+  return true;
 }
 
-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;
@@ -211,7 +237,8 @@ string16 TimeDurationFormatWithSeconds(const TimeDelta time,
   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) == TRUE;
 }
 
 string16 DateIntervalFormat(const Time& begin_time,

base/i18n/time_formatting.h

@@ -8,6 +8,7 @@
 #ifndef BASE_I18N_TIME_FORMATTING_H_
 #define BASE_I18N_TIME_FORMATTING_H_
 
+#include "base/compiler_specific.h"
 #include "base/i18n/base_i18n_export.h"
 #include "base/strings/string16.h"
 
@@ -46,6 +47,10 @@ enum DateFormat {
   DATE_FORMAT_MONTH_WEEKDAY_DAY,
 };
 
+// TODO(derat@chromium.org): Update all of these functions to return boolean
+// "success" values and use out-params for formatted strings:
+// http://crbug.com/698802
+
 // Returns the time of day, e.g., "3:07 PM".
 BASE_I18N_EXPORT string16 TimeFormatTimeOfDay(const Time& time);
 
@@ -95,28 +100,31 @@ BASE_I18N_EXPORT string16 TimeFormatWithPattern(const Time& time,
                                                 const char* pattern);
 
 // Formats a time duration of hours and minutes into various formats, e.g.,
-// "3:07" or "3 hours, 7 minutes".  See DurationFormatWidth for details.
+// "3:07" or "3 hours, 7 minutes", and returns true on success. See
+// DurationFormatWidth for details.
 //
 // Please don't use width = DURATION_WIDTH_NUMERIC when the time duration
 // can possibly be larger than 24h, as the hour value will be cut below 24
 // after formatting.
 // TODO(chengx): fix function output when width = DURATION_WIDTH_NUMERIC
 // (http://crbug.com/675791)
-BASE_I18N_EXPORT string16 TimeDurationFormat(const TimeDelta time,
-                                             const DurationFormatWidth width);
+BASE_I18N_EXPORT bool TimeDurationFormat(const TimeDelta time,
+                                         const DurationFormatWidth width,
+                                         string16* out) WARN_UNUSED_RESULT;
 
 // Formats a time duration of hours, minutes and seconds into various formats,
-// e.g., "3:07:30" or "3 hours, 7 minutes, 30 seconds". See DurationFormatWidth
-// for details.
+// e.g., "3:07:30" or "3 hours, 7 minutes, 30 seconds", and returns true on
+// success. See DurationFormatWidth for details.
 //
 // Please don't use width = DURATION_WIDTH_NUMERIC when the time duration
 // can possibly be larger than 24h, as the hour value will be cut below 24
 // after formatting.
 // TODO(chengx): fix function output when width = DURATION_WIDTH_NUMERIC
 // (http://crbug.com/675791)
-BASE_I18N_EXPORT string16
-TimeDurationFormatWithSeconds(const TimeDelta time,
-                              const DurationFormatWidth width);
+BASE_I18N_EXPORT bool TimeDurationFormatWithSeconds(
+    const TimeDelta time,
+    const DurationFormatWidth width,
+    string16* out) WARN_UNUSED_RESULT;
 
 // Formats a date interval into various formats, e.g. "2 December - 4 December"
 // or "March 2016 - December 2016". See DateFormat for details.

base/i18n/time_formatting_unittest.cc

@@ -27,7 +27,7 @@ const Time::Exploded kTestDateTimeExploded = {
 // Returns difference between the local time and GMT formatted as string.
 // This function gets |time| because the difference depends on time,
 // see https://en.wikipedia.org/wiki/Daylight_saving_time for details.
-base::string16 GetShortTimeZone(const Time& time) {
+string16 GetShortTimeZone(const Time& time) {
   UErrorCode status = U_ZERO_ERROR;
   std::unique_ptr<icu::TimeZone> zone(icu::TimeZone::createDefault());
   std::unique_ptr<icu::TimeZoneFormat> zone_formatter(
@@ -37,7 +37,30 @@ base::string16 GetShortTimeZone(const Time& time) {
   zone_formatter->format(UTZFMT_STYLE_SPECIFIC_SHORT, *zone,
                          static_cast<UDate>(time.ToDoubleT() * 1000),
                          name, nullptr);
-  return base::string16(name.getBuffer(), name.length());
+  return string16(name.getBuffer(), name.length());
+}
+
+// Calls TimeDurationFormat() with |delta| and |width| and returns the resulting
+// string. On failure, adds a failed expectation and returns an empty string.
+string16 TimeDurationFormatString(const TimeDelta& delta,
+                                  DurationFormatWidth width) {
+  string16 str;
+  EXPECT_TRUE(TimeDurationFormat(delta, width, &str))
+      << "Failed to format " << delta.ToInternalValue() << " with width "
+      << width;
+  return str;
+}
+
+// Calls TimeDurationFormatWithSeconds() with |delta| and |width| and returns
+// the resulting string. On failure, adds a failed expectation and returns an
+// empty string.
+string16 TimeDurationFormatWithSecondsString(const TimeDelta& delta,
+                                             DurationFormatWidth width) {
+  string16 str;
+  EXPECT_TRUE(TimeDurationFormatWithSeconds(delta, width, &str))
+      << "Failed to format " << delta.ToInternalValue() << " with width "
+      << width;
+  return str;
 }
 
 #if defined(OS_ANDROID)
@@ -254,24 +277,24 @@ TEST(TimeFormattingTest, TimeDurationFormat) {
   // US English.
   i18n::SetICUDefaultLocale("en_US");
   EXPECT_EQ(ASCIIToUTF16("15 hours, 42 minutes"),
-            TimeDurationFormat(delta, DURATION_WIDTH_WIDE));
+            TimeDurationFormatString(delta, DURATION_WIDTH_WIDE));
   EXPECT_EQ(ASCIIToUTF16("15 hr, 42 min"),
-            TimeDurationFormat(delta, DURATION_WIDTH_SHORT));
+            TimeDurationFormatString(delta, DURATION_WIDTH_SHORT));
   EXPECT_EQ(ASCIIToUTF16("15h 42m"),
-            TimeDurationFormat(delta, DURATION_WIDTH_NARROW));
+            TimeDurationFormatString(delta, DURATION_WIDTH_NARROW));
   EXPECT_EQ(ASCIIToUTF16("15:42"),
-            TimeDurationFormat(delta, DURATION_WIDTH_NUMERIC));
+            TimeDurationFormatString(delta, DURATION_WIDTH_NUMERIC));
 
   // Danish, with Latin alphabet but different abbreviations and punctuation.
   i18n::SetICUDefaultLocale("da");
   EXPECT_EQ(ASCIIToUTF16("15 timer og 42 minutter"),
-            TimeDurationFormat(delta, DURATION_WIDTH_WIDE));
+            TimeDurationFormatString(delta, DURATION_WIDTH_WIDE));
   EXPECT_EQ(ASCIIToUTF16("15 t og 42 min."),
-            TimeDurationFormat(delta, DURATION_WIDTH_SHORT));
+            TimeDurationFormatString(delta, DURATION_WIDTH_SHORT));
   EXPECT_EQ(ASCIIToUTF16("15 t og 42 min"),
-            TimeDurationFormat(delta, DURATION_WIDTH_NARROW));
+            TimeDurationFormatString(delta, DURATION_WIDTH_NARROW));
   EXPECT_EQ(ASCIIToUTF16("15.42"),
-            TimeDurationFormat(delta, DURATION_WIDTH_NUMERIC));
+            TimeDurationFormatString(delta, DURATION_WIDTH_NUMERIC));
 
   // Persian, with non-Arabic numbers.
   i18n::SetICUDefaultLocale("fa");
@@ -285,10 +308,11 @@ TEST(TimeFormattingTest, TimeDurationFormat) {
       L"\x6f1\x6f5\x20\x633\x627\x639\x62a\x20\x6f4\x6f2\x20\x62f\x642\x6cc"
       L"\x642\x647");
   string16 fa_numeric = WideToUTF16(L"\x6f1\x6f5\x3a\x6f4\x6f2");
-  EXPECT_EQ(fa_wide, TimeDurationFormat(delta, DURATION_WIDTH_WIDE));
-  EXPECT_EQ(fa_short, TimeDurationFormat(delta, DURATION_WIDTH_SHORT));
-  EXPECT_EQ(fa_narrow, TimeDurationFormat(delta, DURATION_WIDTH_NARROW));
-  EXPECT_EQ(fa_numeric, TimeDurationFormat(delta, DURATION_WIDTH_NUMERIC));
+  EXPECT_EQ(fa_wide, TimeDurationFormatString(delta, DURATION_WIDTH_WIDE));
+  EXPECT_EQ(fa_short, TimeDurationFormatString(delta, DURATION_WIDTH_SHORT));
+  EXPECT_EQ(fa_narrow, TimeDurationFormatString(delta, DURATION_WIDTH_NARROW));
+  EXPECT_EQ(fa_numeric,
+            TimeDurationFormatString(delta, DURATION_WIDTH_NUMERIC));
 }
 
 TEST(TimeFormattingTest, TimeDurationFormatWithSeconds) {
@@ -300,44 +324,44 @@ TEST(TimeFormattingTest, TimeDurationFormatWithSeconds) {
   // Test different formats.
   TimeDelta delta = TimeDelta::FromSeconds(15 * 3600 + 42 * 60 + 30);
   EXPECT_EQ(ASCIIToUTF16("15 hours, 42 minutes, 30 seconds"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_WIDE));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_WIDE));
   EXPECT_EQ(ASCIIToUTF16("15 hr, 42 min, 30 sec"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_SHORT));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_SHORT));
   EXPECT_EQ(ASCIIToUTF16("15h 42m 30s"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_NARROW));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_NARROW));
   EXPECT_EQ(ASCIIToUTF16("15:42:30"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_NUMERIC));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_NUMERIC));
 
   // Test edge case when hour >= 100.
   delta = TimeDelta::FromSeconds(125 * 3600 + 42 * 60 + 30);
   EXPECT_EQ(ASCIIToUTF16("125 hours, 42 minutes, 30 seconds"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_WIDE));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_WIDE));
   EXPECT_EQ(ASCIIToUTF16("125 hr, 42 min, 30 sec"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_SHORT));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_SHORT));
   EXPECT_EQ(ASCIIToUTF16("125h 42m 30s"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_NARROW));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_NARROW));
 
   // Test edge case when minute = 0.
   delta = TimeDelta::FromSeconds(15 * 3600 + 0 * 60 + 30);
   EXPECT_EQ(ASCIIToUTF16("15 hours, 0 minutes, 30 seconds"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_WIDE));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_WIDE));
   EXPECT_EQ(ASCIIToUTF16("15 hr, 0 min, 30 sec"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_SHORT));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_SHORT));
   EXPECT_EQ(ASCIIToUTF16("15h 0m 30s"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_NARROW));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_NARROW));
   EXPECT_EQ(ASCIIToUTF16("15:00:30"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_NUMERIC));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_NUMERIC));
 
   // Test edge case when second = 0.
   delta = TimeDelta::FromSeconds(15 * 3600 + 42 * 60 + 0);
   EXPECT_EQ(ASCIIToUTF16("15 hours, 42 minutes, 0 seconds"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_WIDE));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_WIDE));
   EXPECT_EQ(ASCIIToUTF16("15 hr, 42 min, 0 sec"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_SHORT));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_SHORT));
   EXPECT_EQ(ASCIIToUTF16("15h 42m 0s"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_NARROW));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_NARROW));
   EXPECT_EQ(ASCIIToUTF16("15:42:00"),
-            TimeDurationFormatWithSeconds(delta, DURATION_WIDTH_NUMERIC));
+            TimeDurationFormatWithSecondsString(delta, DURATION_WIDTH_NUMERIC));
 }
 
 TEST(TimeFormattingTest, TimeIntervalFormat) {

chrome/browser/ui/task_manager/task_manager_table_model.cc

@@ -141,8 +141,11 @@ class TaskManagerValuesStringifier {
     if (cpu_time.is_zero())
       return n_a_string_;
 
-    return base::TimeDurationFormatWithSeconds(cpu_time,
-                                               base::DURATION_WIDTH_NARROW);
+    base::string16 duration;
+    return base::TimeDurationFormatWithSeconds(
+               cpu_time, base::DURATION_WIDTH_NARROW, &duration)
+               ? duration
+               : n_a_string_;
   }
 
   base::string16 GetMemoryUsageText(int64_t memory_usage, bool has_duplicates) {