Fix rounding of time interval strings

ui/base/l10n/time_format.cc

Index: ui/base/l10n/time_format.cc
diff --git a/ui/base/l10n/time_format.cc b/ui/base/l10n/time_format.cc
index f3b16a3f969ac1115b642550d5146660ce1b7ea3..6f3532315d76e579ef6056b658983869ea23c2ce 100644
--- a/ui/base/l10n/time_format.cc
+++ b/ui/base/l10n/time_format.cc
@@ -282,41 +282,46 @@ icu::PluralFormat* TimeFormatter::createFallbackFormat(
 }
 
 base::string16 FormatTimeImpl(const TimeDelta& delta, FormatType format_type) {
-  if (delta.ToInternalValue() < 0) {
+  if (delta < TimeDelta::FromSeconds(0)) {

[bartfab (slow), 2014/01/29 15:40:12]

Nit: The TimeDelta() constructor actually produces a zero delta that you can
test against. Your call which one is more legible.

[Thiemo Nagel, 2014/01/29 17:40:12]

I think "FromSeconds(0)" is clearer than both "TimeDelta()" and "0".

     NOTREACHED() << "Negative duration";
     return base::string16();
   }
 
-  int number;
-
   const std::vector<icu::PluralFormat*>& formatters =
     g_time_formatter.Get().formatter(format_type);
 
   UErrorCode error = U_ZERO_ERROR;
   icu::UnicodeString time_string;
-  // Less than a minute gets "X seconds left"
-  if (delta.ToInternalValue() < Time::kMicrosecondsPerMinute) {
-    number = static_cast<int>(delta.ToInternalValue() /
-                              Time::kMicrosecondsPerSecond);
-    time_string = formatters[0]->format(number, error);
-
-  // Less than 1 hour gets "X minutes left".
-  } else if (delta.ToInternalValue() < Time::kMicrosecondsPerHour) {
-    number = static_cast<int>(delta.ToInternalValue() /
-                              Time::kMicrosecondsPerMinute);
-    time_string = formatters[1]->format(number, error);
-
-  // Less than 1 day remaining gets "X hours left"
-  } else if (delta.ToInternalValue() < Time::kMicrosecondsPerDay) {
-    number = static_cast<int>(delta.ToInternalValue() /
-                              Time::kMicrosecondsPerHour);
-    time_string = formatters[2]->format(number, error);
+
+  const TimeDelta minute     (TimeDelta::FromMinutes(1));

[bartfab (slow), 2014/01/29 15:40:12]

Chrome code does not use this kind of formatting to align braces. There should
be no spaces between variable name and braces, even if it looks less neat.

Nit 1: Could you prefix these with one_, e.g. one_minute?

Nit 2: Constants in Chrome code tend to have names of the form kOneMinute,
kOneHour, etc.

[Thiemo Nagel, 2014/01/29 17:40:12]

Done.
Done.
I think I'd rather stick with the current naming scheme.

+  const TimeDelta hour       (TimeDelta::FromHours(1));
+  const TimeDelta day        (TimeDelta::FromDays(1));
+
+  const TimeDelta half_second(TimeDelta::FromMilliseconds(500));
+  const TimeDelta half_minute(TimeDelta::FromSeconds(30));
+  const TimeDelta half_hour  (TimeDelta::FromMinutes(30));
+  const TimeDelta half_day   (TimeDelta::FromHours(12));
+
+  // Less than 59.5 seconds gets "X seconds left"

[bartfab (slow), 2014/01/29 15:40:12]

Nit: Could you add a comment explaining that the .5 in these conditions is
because of rounding?

[Thiemo Nagel, 2014/01/29 17:40:12]

Done.

+  if (delta < minute - half_second) {
+    const int64 seconds64 = (delta+half_second).InSeconds();

[bartfab (slow), 2014/01/29 15:40:12]

Here and further down: You are missing spaces around the +

[Thiemo Nagel, 2014/01/29 17:40:12]

Done.

+    const int seconds = static_cast<int>(seconds64);

[bartfab (slow), 2014/01/29 15:40:12]

Nit: Why the intermediary int64 variable?

[Thiemo Nagel, 2014/01/29 17:40:12]

Done.

+    time_string = formatters[0]->format(seconds, error);
+
+  // Less than 59.5 minutes gets "X minutes left".
+  } else if (delta < hour - half_minute) {
+    const int minutes = (delta+half_minute).InMinutes();
+    time_string = formatters[1]->format(minutes, error);
+
+  // Less than 23.5 hours gets "X hours left"
+  } else if (delta < day - half_hour) {
+    const int hours = (delta+half_hour).InHours();
+    time_string = formatters[2]->format(hours, error);
 
   // Anything bigger gets "X days left"
   } else {
-    number = static_cast<int>(delta.ToInternalValue() /
-                              Time::kMicrosecondsPerDay);
-    time_string = formatters[3]->format(number, error);
+    const int days = (delta+half_day).InDays();
+    time_string = formatters[3]->format(days, error);
   }
 
   // With the fallback added, this should never fail.