Fix rounding of time interval strings

ui/base/l10n/time_format.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 "ui/base/l10n/time_format.h"
 
 #include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ skipping 264 matching lines @@
     pattern += UNICODE_STRING_SIMPLE("one{# ") + kUnits[index][0] + suffix;
   }
   pattern += UNICODE_STRING_SIMPLE(" other{# ") + kUnits[index][1] + suffix;
   UErrorCode err = U_ZERO_ERROR;
   icu::PluralFormat* format = new icu::PluralFormat(rules, pattern, err);
   DCHECK(U_SUCCESS(err));
   return format;
 }
 
 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);
+  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));
 
-  // 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 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.
   DCHECK(U_SUCCESS(error));
   int capacity = time_string.length() + 1;
   DCHECK_GT(capacity, 1);
   base::string16 result;
   time_string.extract(static_cast<UChar*>(WriteInto(&result, capacity)),
                       capacity, error);
   DCHECK(U_SUCCESS(error));
@@ skipping 41 matching lines @@
   if (time >= tomorrow)
     return base::string16();
   else if (time >= midnight_today)
     return l10n_util::GetStringUTF16(IDS_PAST_TIME_TODAY);
   else if (time >= yesterday)
     return l10n_util::GetStringUTF16(IDS_PAST_TIME_YESTERDAY);
   return base::string16();
 }
 
 }  // namespace ui