Add support for localized time strings with two units, eg. "2 hours 17 minutes"

ui/base/l10n/time_format.h

 // 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.
 
 #ifndef UI_BASE_L10N_TIME_FORMAT_H_
 #define UI_BASE_L10N_TIME_FORMAT_H_
 
 #include "base/basictypes.h"
 #include "base/strings/string16.h"
 #include "ui/base/ui_base_export.h"
 
 namespace base {
 class Time;
 class TimeDelta;
 }
 
 namespace ui {
+class FormatterContainer;
 
 // Methods to format time values as strings.
 class UI_BASE_EXPORT TimeFormat {
  public:
-  // TimeElapsed, TimeRemaining* and TimeDuration*:
-  // These functions return a localized string of approximate time duration.
+  enum { kNType = 3 };

[bartfab (slow), 2014/02/18 12:04:04]

This is not how we do enums in Chrome:

1. Enum entries should be UPPER_CASE_HACKER_STYLE, no kCamelCaps.
2. If you want to know how many entries there are, define your enum as:

enum Type {
  TYPE_DURATION = 0,
  TYPE_REMAINING,
  TYPE_ELAPSED,
  TYPE_COUNT  // Must be last.
};

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  enum Type {
+    kDuration,   // plain duration, eg. in English: "2 minutes"

[bartfab (slow), 2014/02/18 12:04:04]

Nit: Capitalize the first letter of comments.

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+    kRemaining,  // remaining time, eg. in English: "2 minutes left"
+    kElapsed     // elapsed time, eg. in English: "2 minutes ago"
+  };
 
-  // Returns times in elapsed-format: "3 mins ago", "2 days ago".
-  static base::string16 TimeElapsed(const base::TimeDelta& delta);
+  enum { kNLength = 2 };

[bartfab (slow), 2014/02/18 12:04:04]

See above. This is not how we do enums in Chrome.

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  enum Length {
+    kShort,  // short format, eg. in English: sec/min/hour/day

[bartfab (slow), 2014/02/18 12:04:04]

Nit 1: Capitalize the first letter of comments.
Nit 2: s/eg./e.g.,/

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+    kLong    // long format, eg. in English: second/minute/hour/day
+  };
 
-  // Returns times in remaining-format: "3 mins left", "2 days left".
-  static base::string16 TimeRemaining(const base::TimeDelta& delta);
+  // Return a localized string of approximate time duration, formatted as a
+  // single number, eg. in English "2 hours ago".  Currently, all combinations

[bartfab (slow), 2014/02/18 12:04:04]

Nit: s/eg./e.g.,/

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  // of type and length are implemented except (kElapsed,kLong).

[bartfab (slow), 2014/02/18 12:04:04]

Nit 1: Add space after comma.
Nit 2: Reorder sentence "of type and length except (kElapsed, kLong) are
implemented."

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  static base::string16 Simple(Type type,
+                               Length length,
+                               const base::TimeDelta& delta);
 
-  // Returns times in remaining-long-format: "3 minutes left", "2 days left".
-  static base::string16 TimeRemainingLong(const base::TimeDelta& delta);
-
-  // Returns times in short-format: "3 mins", "2 days".
-  static base::string16 TimeDurationShort(const base::TimeDelta& delta);
-
-  // Return times in long-format: "2 hours", "25 minutes".
-  static base::string16 TimeDurationLong(const base::TimeDelta& delta);
+  // Return a localized string of approximate time duration, conditionally
+  // formatted as a single number, eg. in English "2 hours ago", or as two

[bartfab (slow), 2014/02/18 12:04:04]

Nit: s/eg./e.g.,/

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  // numbers, eg. in English "2 hours and 13 minutes ago".  The two-number

[bartfab (slow), 2014/02/18 12:04:04]

Nit: s/eg./e.g.,/

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  // format is chosen whenever the single-number format would yield a numeric
+  // value less than cutoff.  (Applied to the examples above, cutoff=2 would

[bartfab (slow), 2014/02/18 12:04:04]

Nit 1: Put pipes around symbol names referenced in a comment, e.g., |cutoff|.
Nit 2: Add spaces around =

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  // yield the first string and cutoff=3 would return the second string.)

[bartfab (slow), 2014/02/18 12:04:04]

Your example is correct but it took me a lot of thinking and remembering our
prior offline conversation to figure out what cutoff=2 and cutoff=3 do and why.
I think this sentence needs some rephrasing or maybe a clearer explanation that
2 respectively 3 would be compared against the number of hours here.

[Thiemo Nagel, 2014/02/19 17:08:44]

I've tried to improve the comment.

+  // Currently, the only combination of type and length that is implemented is
+  // (kDuration,kLong).

[bartfab (slow), 2014/02/18 12:04:04]

Nit: Add space after comma.

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  //
+  // Note: This is implemented with two translation strings (IDS_TIME_*_1ST and
+  // IDS_TIME_*_2ND) per [plurality x pair of units] which are contatenated

[bartfab (slow), 2014/02/18 12:04:04]

Nit: s/contatenated/concatenated/

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+  // after having been formatted individually.  The separator between first unit
+  // and second unit (presently a blank) is included in IDS_TIME_*_1ST.

[bartfab (slow), 2014/02/18 12:04:04]

I think "a blank in the case of English" would be more accurate than "presently
a blank". It is not like we are planning to change the separator in English but
other languages may choose something different.

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

(I wanted to express that someday someone might implement a different format
that uses " and " instead of a blank.)

+  static base::string16 Detailed(Type type,
+                                 Length length,
+                                 int cutoff,
+                                 const base::TimeDelta& delta);
 
   // For displaying a relative time in the past.  This method returns either
   // "Today", "Yesterday", or an empty string if it's older than that.  Returns
   // the empty string for days in the future.
   //
   // TODO(brettw): This should be able to handle days in the future like
   //    "Tomorrow".
   // TODO(tc): This should be able to do things like "Last week".  This
   //    requires handling singluar/plural for all languages.
   //
   // The second parameter is optional, it is midnight of "Now" for relative day
   // computations: Time::Now().LocalMidnight()
   // If NULL, the current day's midnight will be retrieved, which can be
   // slow. If many items are being processed, it is best to get the current
   // time once at the beginning and pass it for each computation.
   static base::string16 RelativeDate(const base::Time& time,
                                      const base::Time* optional_midnight_today);
 
+ protected:
+  // Implemented in header file to avoid code generation in production builds.
+  static base::string16 SimpleForTesting(const FormatterContainer& container,

[bartfab (slow), 2014/02/18 12:04:04]

AFAICT, the only difference between these ForTesting methods and their
production counterparts is that these ones allow a |container| to be injected
that forces you down the fallback code path. Why not provide a method that calls
g_container.Get()->SetFallbackForTesting() instead, setting up |g_container| so
that there is no need to inject a |container| for testing?

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+                                         Type type,
+                                         Length length,
+                                         const base::TimeDelta& delta) {
+    return DetailedForTesting(container, type, length, 0, delta);
+  }
+  static base::string16 DetailedForTesting(const FormatterContainer& container,

[bartfab (slow), 2014/02/18 12:04:04]

Nit: Add blank line before this.

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

+                                           Type type,
+                                           Length length,
+                                           int cutoff,
+                                           const base::TimeDelta& delta) {
+    return FormatTimeImpl(container, type, length, cutoff, delta);
+  }
+
  private:
+  static base::string16 FormatTimeImpl(const FormatterContainer& container,

[bartfab (slow), 2014/02/18 12:04:04]

Nit: The suffix |Impl| is used in Chrome for classes that implement a
corresponding interface (e.g. UserManagerImpl implements UserManager). For
method names, |Impl| is not a suffix we tend to use.

[Thiemo Nagel, 2014/02/19 17:08:44]

I've removed the method anyways.

+                                       Type type,
+                                       Length length,
+                                       int cutoff,
+                                       const base::TimeDelta& delta);
+
+#ifndef TIME_FORMAT_UNITTEST

[bartfab (slow), 2014/02/18 12:04:04]

There is no precedent for this kind of construction in Chrome code. As commented
in time_forma_unittest.cc, I do not think that TimeFormatTest should be
inheriting from TimeFormat.

[Thiemo Nagel, 2014/02/19 17:08:44]

Done.

   DISALLOW_IMPLICIT_CONSTRUCTORS(TimeFormat);
+#endif
 };
 
 }  // namespace ui
 
 #endif  // UI_BASE_L10N_TIME_FORMAT_H_