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 276 matching lines @@
     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;
+  const int64 d2i = delta.ToInternalValue();

[bartfab (slow), 2014/01/28 15:16:46]

Nit: Wherever possible, we avoid abbreviations. |d2i| is not exactly a
self-explanatory name.

+
   // Less than a minute gets "X seconds left"
-  if (delta.ToInternalValue() < Time::kMicrosecondsPerMinute) {
-    number = static_cast<int>(delta.ToInternalValue() /
+  if (d2i < Time::kMicrosecondsPerMinute-Time::kMicrosecondsPerSecond/2) {

[bartfab (slow), 2014/01/28 15:16:46]

1) Here and elsewhere in the file: Add spaces around the mathematical operators
(see style guide).

2) The comment and the math disagree. If you wanted to handle "less than a
second," it would be sufficient to do |d1i < Time::MicrosecondsPerMinute|. What
is the |Time::MicrosecondsPerSecond / 2| for?

3) Doing math with internal values is a bad idea. The internal value is supposed
to be an opaque thing that you do not interpret. Why not do something like:

if (delta < base::TimeDelta::FromSeconds(1))

+    number = static_cast<int>((d2i+Time::kMicrosecondsPerSecond/2) /
                               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() /
+  } else if (d2i < Time::kMicrosecondsPerHour-Time::kMicrosecondsPerMinute/2) {
+    number = static_cast<int>((d2i+Time::kMicrosecondsPerMinute/2) /
                               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() /
+  } else if (d2i < Time::kMicrosecondsPerDay-Time::kMicrosecondsPerHour/2) {
+    number = static_cast<int>((d2i+Time::kMicrosecondsPerHour/2) /
                               Time::kMicrosecondsPerHour);
     time_string = formatters[2]->format(number, error);
 
   // Anything bigger gets "X days left"
   } else {
-    number = static_cast<int>(delta.ToInternalValue() /
+    number = static_cast<int>((d2i+Time::kMicrosecondsPerDay/2) /
                               Time::kMicrosecondsPerDay);
     time_string = formatters[3]->format(number, 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)),
@@ skipping 43 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