Fix rounding of time interval strings

Fix rounding of time interval strings

Previously, text output for time intervals always was rounded
down. For example, a time interval of 1 min 59 sec was
displayed as "1 min", which was confusing users in some cases.
This CL implements rounding to nearest integer. Some example
outputs are given below.

0.49 sec --> "0 secs"
0.5  sec --> "1 sec"
59.4 sec --> "59 secs"
59.5 sec --> "1 min"
89.9 sec --> "1 min"
90.0 sec --> "2 mins"
59 min   --> "59 mins"
60 min   --> "1 hour"
89 min   --> "1 hour"
90 min   --> "2 hours"
...

BUG=338631
TEST=unit tests updated

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247948

[Thiemo Nagel, 2014-01-28 15:03:52]

Hi Jungshik,

could you please be so kind to take a look at this CL?

I'm a Noogler from Munich.

Best,

Thiemo

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

https://codereview.chromium.org/143633003/diff/1/ui/base/l10n/time_format.cc
File ui/base/l10n/time_format.cc (right):

https://codereview.chromium.org/143633003/diff/1/ui/base/l10n/time_format.cc#...
ui/base/l10n/time_format.cc:297: const int64 d2i = delta.ToInternalValue();
Nit: Wherever possible, we avoid abbreviations. |d2i| is not exactly a
self-explanatory name.

https://codereview.chromium.org/143633003/diff/1/ui/base/l10n/time_format.cc#...
ui/base/l10n/time_format.cc:300: if (d2i <
Time::kMicrosecondsPerMinute-Time::kMicrosecondsPerSecond/2) {
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))

https://codereview.chromium.org/143633003/diff/1/ui/base/l10n/time_format_uni...
File ui/base/l10n/time_format_unittest.cc (right):

https://codereview.chromium.org/143633003/diff/1/ui/base/l10n/time_format_uni...
ui/base/l10n/time_format_unittest.cc:32: const TimeDelta min  =
TimeDelta::FromMinutes(1);
Nit: Please avoid abbreviations where possible. There is plenty of space here to
write out "minutes," "seconds," etc.

[Thiemo Nagel, 2014-01-28 16:08:58]

Updated taking into account Bartosz' comments.

https://codereview.chromium.org/143633003/diff/1/ui/base/l10n/time_format_uni...
File ui/base/l10n/time_format_unittest.cc (right):

https://codereview.chromium.org/143633003/diff/1/ui/base/l10n/time_format_uni...
ui/base/l10n/time_format_unittest.cc:32: const TimeDelta min  =
TimeDelta::FromMinutes(1);
There might be plenty of space here, but it will cause ugly line wrapping below,
so I'd like to keep that, if I may.

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

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format.cc
File ui/base/l10n/time_format.cc (right):

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:285: if (delta < TimeDelta::FromSeconds(0)) {
Nit: The TimeDelta() constructor actually produces a zero delta that you can
test against. Your call which one is more legible.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:296: const TimeDelta minute    
(TimeDelta::FromMinutes(1));
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.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:305: // Less than 59.5 seconds gets "X seconds left"
Nit: Could you add a comment explaining that the .5 in these conditions is
because of rounding?

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:307: const int64 seconds64 =
(delta+half_second).InSeconds();
Here and further down: You are missing spaces around the +

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:308: const int seconds =
static_cast<int>(seconds64);
Nit: Why the intermediary int64 variable?

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
File ui/base/l10n/time_format_unittest.cc (right):

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:30: const TimeDelta day  =
TimeDelta::FromDays(1);
Please re-add the one_ prefixes.

Nit: Do not align by adding spaces. One space before =, even if this means
things will not align as nicely.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:35: const TimeDelta zero =
TimeDelta::FromMilliseconds(0);
Nit: You could also write const TimeDelta zero();

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:40: TestTimeFormats(499*msec, "0 secs");
You are missing spaces around the * operators.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:52: TestTimeFormats(23*hour + 29*min +
59*sec + 999*msec, "23 hours");
Nit: Here and elsewhere: You could get the same number much easier as |one_day -
one_msec|.

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

Hi Bartosz,

thank you for your comments!  Could you please be so kind to take another look
at the re-formatted code?

Thanks,
Thiemo

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format.cc
File ui/base/l10n/time_format.cc (right):

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:285: if (delta < TimeDelta::FromSeconds(0)) {
On 2014/01/29 15:40:12, bartfab wrote:
> Nit: The TimeDelta() constructor actually produces a zero delta that you can
> test against. Your call which one is more legible.

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

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:296: const TimeDelta minute    
(TimeDelta::FromMinutes(1));
On 2014/01/29 15:40:12, bartfab wrote:
> 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.

Done.

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

Done.

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

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

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:305: // Less than 59.5 seconds gets "X seconds left"
On 2014/01/29 15:40:12, bartfab wrote:
> Nit: Could you add a comment explaining that the .5 in these conditions is
> because of rounding?

Done.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:307: const int64 seconds64 =
(delta+half_second).InSeconds();
On 2014/01/29 15:40:12, bartfab wrote:
> Here and further down: You are missing spaces around the +

Done.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:308: const int seconds =
static_cast<int>(seconds64);
On 2014/01/29 15:40:12, bartfab wrote:
> Nit: Why the intermediary int64 variable?

Done.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
File ui/base/l10n/time_format_unittest.cc (right):

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:30: const TimeDelta day  =
TimeDelta::FromDays(1);
On 2014/01/29 15:40:12, bartfab wrote:
> Please re-add the one_ prefixes.
> 
> Nit: Do not align by adding spaces. One space before =, even if this means
> things will not align as nicely.

Done.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:35: const TimeDelta zero =
TimeDelta::FromMilliseconds(0);
On 2014/01/29 15:40:12, bartfab wrote:
> Nit: You could also write const TimeDelta zero();

Done.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:40: TestTimeFormats(499*msec, "0 secs");
On 2014/01/29 15:40:12, bartfab wrote:
> You are missing spaces around the * operators.

Done.

https://codereview.chromium.org/143633003/diff/20001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:52: TestTimeFormats(23*hour + 29*min +
59*sec + 999*msec, "23 hours");
On 2014/01/29 15:40:12, bartfab wrote:
> Nit: Here and elsewhere: You could get the same number much easier as |one_day
-
> one_msec|.

Done.

[jungshik at Google, 2014-01-29 22:43:59]

LGTM 

I'm with bartfab about the name scheme for constants (kOneMinute, etc), but it's
not super-critical. 
How about changing 'hour/minute/second' to 'one_{hour,minute,second}'?

[commit-bot: I haz the power, 2014-01-30 09:35:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tnagel@chromium.org/143633003/40001

[Thiemo Nagel, 2014-01-30 09:39:46]

On 2014/01/29 22:43:59, Jungshik Shin wrote:
> I'm with bartfab about the name scheme for constants (kOneMinute, etc), but
it's
> not super-critical.

I've discussed it with Bartosz: kOneMinute makes me think of a member constant
or a global constant.  Unless you really want it changed, I'd rather stay with
one_minute.

> How about changing 'hour/minute/second' to 'one_{hour,minute,second}'?

Done.

[commit-bot: I haz the power, 2014-01-30 13:39:38]

Change committed as 247948

[bartfab (slow), 2014-01-31 13:17:09]

https://codereview.chromium.org/143633003/diff/40001/ui/base/l10n/time_format.cc
File ui/base/l10n/time_format.cc (right):

https://codereview.chromium.org/143633003/diff/40001/ui/base/l10n/time_format...
ui/base/l10n/time_format.cc:308: const int seconds = static_cast<int>((delta +
half_second).InSeconds());
Nit: Is the cast needed? Will the compiler not implictly cast from int64 to int
for you?

https://codereview.chromium.org/143633003/diff/40001/ui/base/l10n/time_format...
File ui/base/l10n/time_format_unittest.cc (right):

https://codereview.chromium.org/143633003/diff/40001/ui/base/l10n/time_format...
ui/base/l10n/time_format_unittest.cc:42: TestTimeFormats(1 * one_second + 499 *
one_millisecond, "1 sec");
Nit: Is there much point in doing "1 *"? Does it makes things clearer in your
opinion? I think I would just leave out the "1 *".