Add tests for TimeRemainingLong() and TimeDurationLong()

ui/base/l10n/time_format_unittest.cc

Index: ui/base/l10n/time_format_unittest.cc
diff --git a/ui/base/l10n/time_format_unittest.cc b/ui/base/l10n/time_format_unittest.cc
index 6f6acc42cd6008e9ebef23af354c585b88490d89..129fd795893e6a284aef982bab7c6b3483f133ba 100644
--- a/ui/base/l10n/time_format_unittest.cc
+++ b/ui/base/l10n/time_format_unittest.cc
@@ -18,12 +18,31 @@ namespace {
 using base::TimeDelta;
 
 void TestTimeFormats(const TimeDelta& delta, const char* expected_ascii) {
-  base::string16 expected = ASCIIToUTF16(expected_ascii);
-  base::string16 expected_left = expected + ASCIIToUTF16(" left");
-  base::string16 expected_ago = expected + ASCIIToUTF16(" ago");
+  const base::string16 expected(ASCIIToUTF16(expected_ascii));
+  const base::string16 sec(ASCIIToUTF16("sec"));
+  const base::string16 min(ASCIIToUTF16("min"));
+  const base::string16 ond(ASCIIToUTF16("ond"));

[bartfab (slow), 2014/01/31 12:58:23]

Nit: No longer used.

[Thiemo Nagel, 2014/01/31 14:10:36]

Done.

+  const base::string16 ute();

[bartfab (slow), 2014/01/31 12:58:23]

Nit: No longer used and actually completely wrong as ute == "" now, not ute ==
"ute" :)

[Thiemo Nagel, 2014/01/31 14:10:36]

That's embarrassing. I forgot to remove ond and ute. It seems that the compiler
warns about simple types that are unused, but not objects. Too bad.

+
+  base::string16 expected_long = expected;        // sec, min --> second, minute
+  base::string16 expected_long_minute = expected; // min --> minute
+
+  if (expected_long.find(sec) != base::string16::npos)
+    expected_long.insert(expected_long.find(sec) + sec.length(),
+                         ASCIIToUTF16("ond"));
+  if (expected_long.find(min) != base::string16::npos)
+    expected_long.insert(expected_long.find(min) + min.length(),
+                         ASCIIToUTF16("ute"));
+  if (expected_long_minute.find(min) != base::string16::npos)

[bartfab (slow), 2014/01/31 12:58:23]

Nit: Not sure whether it would make things cleaner/easier, but you could remove
one of the blocks if you did the substitution in the opposite order:

* take |expected| and s/min/minute/, producing |expected_long_minute|
* take |expected_long_minute| and s/sec/seconds/, producing |expected_long|

[Thiemo Nagel, 2014/01/31 14:10:36]

Good idea! I have the impression that this makes the code more readable.

+    expected_long_minute.insert(expected_long_minute.find(min) + min.length(),
+                                ASCIIToUTF16("ute"));
+
+  EXPECT_EQ(expected + ASCIIToUTF16(" ago"), TimeFormat::TimeElapsed(delta));
+  EXPECT_EQ(expected + ASCIIToUTF16(" left"), TimeFormat::TimeRemaining(delta));
+  EXPECT_EQ(expected_long_minute + ASCIIToUTF16(" left"),
+            TimeFormat::TimeRemainingLong(delta));
   EXPECT_EQ(expected, TimeFormat::TimeRemainingShort(delta));
-  EXPECT_EQ(expected_left, TimeFormat::TimeRemaining(delta));
-  EXPECT_EQ(expected_ago, TimeFormat::TimeElapsed(delta));
+  EXPECT_EQ(expected_long, TimeFormat::TimeDurationLong(delta));

[bartfab (slow), 2014/01/31 12:58:23]

Is there a reason that TimeDurationLong() expands "min" and "sec" but
TimeRemainingLong() expands "min" only? It seems awfully inconsistent.

[Thiemo Nagel, 2014/01/31 14:10:36]

I fully agree.  However, this behaviour is documented in the header file, so I
assume it's intentional:

 // Returns times in remaining-long-format: "3 minutes left", "2 days left".
 // Currently, this only affects the minutes in long format, the rest
 // of the time units are formatted the same as TimeRemaining does.

 }
 
 TEST(TimeFormat, FormatTime) {