Fixit: Factor out common base::Time* math operator overloads.

Fixit: Factor out common base::Time* math operator overloads.

This is part 1 of a 2-part change to fork base::TimeTicks into three
type-checked time classes (TimeTicks + ThreadTicks + TraceTicks).  The
forking of TimeTicks will ensure values from different clocks are not
erroneously being mixed together when doing time math.

In this change, the identical comparison and math operator overloads
found in base::Time and base::TimeTicks are being factored-out into a
templated base class.  In a following change, this base class will also
be used to de-dupe this common functionality in the two new time
classes.

BUG=467417
Committed: https://crrev.com/7ca717095b4758cb76e53e904b775852e46d646d
Cr-Commit-Position: refs/heads/master@{#328696}

[miu, 2015-05-05 01:39:40]

miu@chromium.org changed reviewers:
+ thestig@chromium.org

[miu, 2015-05-05 01:39:41]

thestig: PTAL.  As requested in https://codereview.chromium.org/1121463005, I've
split the change into two parts.

[Lei Zhang, 2015-05-05 21:52:42]

https://codereview.chromium.org/1122443004/diff/1/base/time/time.cc
File base/time/time.cc (right):

https://codereview.chromium.org/1122443004/diff/1/base/time/time.cc#newcode315
base/time/time.cc:315: if (interval_offset != base::TimeDelta() && tick_phase <
*this)
There's a lot of places where we have "foo == base::TimeDelta()". I wonder if
TimeDelta() should have an is_zero() method.

Also, no need for base:: in namespace base.

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h#newcode62
base/time/time.h:62: namespace time_internal {
Please add a big comment telling readers not to use this code outside of
base/time/time.{cc,h}. Ditto on line 243.

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h#newcode65
base/time/time.h:65: // are are in terms of a microsecond timebase.
typo: "are are", ditto below on line 70.

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h#newcode197
base/time/time.h:197: TimeDelta operator%(TimeDelta a) const {
This is rarely needed. Can we just do this internally in SnappedToNextTick()?

[miu, 2015-05-06 18:26:06]

Patchset #2 (id:20001) has been deleted

[miu, 2015-05-06 18:29:17]

https://codereview.chromium.org/1122443004/diff/1/base/time/time.cc
File base/time/time.cc (right):

https://codereview.chromium.org/1122443004/diff/1/base/time/time.cc#newcode315
base/time/time.cc:315: if (interval_offset != base::TimeDelta() && tick_phase <
*this)
On 2015/05/05 21:52:41, Lei Zhang wrote:
> There's a lot of places where we have "foo == base::TimeDelta()". I wonder if
> TimeDelta() should have an is_zero() method.

Done.  Good idea.  I come across this all the time.

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h#newcode62
base/time/time.h:62: namespace time_internal {
On 2015/05/05 21:52:41, Lei Zhang wrote:
> Please add a big comment telling readers not to use this code outside of
> base/time/time.{cc,h}. Ditto on line 243.

Done.

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h#newcode65
base/time/time.h:65: // are are in terms of a microsecond timebase.
On 2015/05/05 21:52:42, Lei Zhang wrote:
> typo: "are are", ditto below on line 70.

Done.

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h#newcode197
base/time/time.h:197: TimeDelta operator%(TimeDelta a) const {
On 2015/05/05 21:52:41, Lei Zhang wrote:
> This is rarely needed. Can we just do this internally in SnappedToNextTick()?

I have a strong preference to keep this for the following reasons:

1. The lack of the operator incentivizes people to write code that converts from
the TimeDelta type to a raw int or double to do math.  The math on the raw types
subverts the compile-time checking of valid time math operations.  The code that
was written in SnappedToNextTick() is a perfect example of a developer choosing
to subvert the type system.

2. The operator works as expected, and has the obvious meaning (i.e., do a
division and return the "left over" amount).

3. While I understand it may not be widely used, there are certain parts of the
code base that could/should make heavy use of the operator to simplify.  I
myself write and maintain lots of code in the media space where this pattern has
shown up repeatedly:

  TimeDelta remainder = a - ((a / b) * b);

...so this operator has been on my personal wish-list for a while.  ;-)

WDYT?

https://codereview.chromium.org/1122443004/diff/40001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122443004/diff/40001/base/time/time.h#newcod...
base/time/time.h:282: // Warning: Be careful when writing code that performs
math on time values,
BTW--As I was looking through the code again, I remembered I ran into the issue
described here.  It caused a serious bug and was difficult to root-cause.  So, I
added a warning here for others.

[Lei Zhang, 2015-05-06 21:41:31]

lgtm

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122443004/diff/1/base/time/time.h#newcode197
base/time/time.h:197: TimeDelta operator%(TimeDelta a) const {
On 2015/05/06 18:29:17, miu wrote:
> On 2015/05/05 21:52:41, Lei Zhang wrote:
> > This is rarely needed. Can we just do this internally in
SnappedToNextTick()?
> 
> I have a strong preference to keep this for the following reasons:
> 
> 1. The lack of the operator incentivizes people to write code that converts
from
> the TimeDelta type to a raw int or double to do math.  The math on the raw
types
> subverts the compile-time checking of valid time math operations.  The code
that
> was written in SnappedToNextTick() is a perfect example of a developer
choosing
> to subvert the type system.
> 
> 2. The operator works as expected, and has the obvious meaning (i.e., do a
> division and return the "left over" amount).
> 
> 3. While I understand it may not be widely used, there are certain parts of
the
> code base that could/should make heavy use of the operator to simplify.  I
> myself write and maintain lots of code in the media space where this pattern
has
> shown up repeatedly:
> 
>   TimeDelta remainder = a - ((a / b) * b);
> 
> ...so this operator has been on my personal wish-list for a while.  ;-)
> 
> WDYT?

Ok, fair enough.

[miu, 2015-05-07 02:02:33]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-05-07 02:03:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1122443004/40001

[commit-bot: I haz the power, 2015-05-07 03:45:03]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-05-07 03:46:30]

Patchset 2 (id:??) landed as
https://crrev.com/7ca717095b4758cb76e53e904b775852e46d646d
Cr-Commit-Position: refs/heads/master@{#328696}

[Kunihiko Sakamoto, 2015-05-07 04:32:36]

A revert of this CL (patchset #2 id:40001) has been created in
https://codereview.chromium.org/1130953002/ by ksakamoto@chromium.org.

The reason for reverting is: Broke iOS build.

http://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/5621/step...
.