Added support to convert double to timedelta's without loss of data during type conversion from dou…

Added support to convert double to timedelta's without loss of data during type conversion from double to int64

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

[patro, 2014-04-11 15:20:22]

Please review

[Mark Mentovai, 2014-04-15 21:54:33]

LGTM

[mithro-old, 2014-04-16 00:32:30]

On 2014/04/15 21:54:33, Mark Mentovai wrote:
> LGTM

Hi shyamp,

Can you add a reference to bug
https://code.google.com/p/chromium/issues/detail?id=349328 ?

I wrote a similar patch to this at https://codereview.chromium.org/192433002/
(but that was specifically for interfacing to Blink code).

Where is this conversion needed? Almost all code inside Chrome should be using
TimeTicks as it is significantly safer. Any chance we could put this behind some
type of guard which means things need to be specifically approved to use it?
(This can be done by making an inner class which has private static methods and
a friend declaration needed for each user.)

Couple of things you might want to consider;
 * Outside the range of -2**53 < x < 2**53 (doubles can only represent 53 bits
of whole values) the conversion doesn't roundtrip. IE
      int64 x;
      double y = (int64)x;
      int64 z = (double)y;
      x != z

   It might be a good idea to check / warn when this is happening (should
probably only be in debug builds)?

 * Similarly, when converting from a double it might be worth warning / checking
if we are discarding any information.

 * You don't seem to deal with the special "null" TimeTicks?

 * Could we have a basic unit test? (You can copy mine if you want.)

Happy to chat if you have any questions.

Tim 'mithro' Ansell

[patro, 2014-04-16 12:30:10]

Hi mithro,

On 2014/04/16 00:32:30, mithro wrote:
> On 2014/04/15 21:54:33, Mark Mentovai wrote:
> > LGTM
> 
> Hi shyamp,
> 
> Can you add a reference to bug
> https://code.google.com/p/chromium/issues/detail?id=349328 ?
> 
> I wrote a similar patch to this at https://codereview.chromium.org/192433002/
> (but that was specifically for interfacing to Blink code).
> 
On example for using this 
You can refer to
https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
where it is required to convert floating point to TimeDelta without losing
precision.

> Where is this conversion needed? Almost all code inside Chrome should be using
> TimeTicks as it is significantly safer. Any chance we could put this behind
some
> type of guard which means things need to be specifically approved to use it?
> (This can be done by making an inner class which has private static methods
and
> a friend declaration needed for each user.)
> 

Yes this range should be taken into consideration by programmer while using the
functions that is the reason i wrote separate functions for double.

> Couple of things you might want to consider;
>  * Outside the range of -2**53 < x < 2**53 (doubles can only represent 53 bits
> of whole values) the conversion doesn't roundtrip. IE
>       int64 x;
>       double y = (int64)x;
>       int64 z = (double)y;
>       x != z
> 
>    It might be a good idea to check / warn when this is happening (should
> probably only be in debug builds)?
> 
>  * Similarly, when converting from a double it might be worth warning /
checking
> if we are discarding any information.
> 

I will add them in next patch along with the unittest.

>  * You don't seem to deal with the special "null" TimeTicks?
> 
>  * Could we have a basic unit test? (You can copy mine if you want.)
> 
> Happy to chat if you have any questions.
> 
> Tim 'mithro' Ansell

[mithro-old, 2014-04-16 13:38:44]

Hi,

Just FYI, generally you reply under a question rather than above it.

> > Where is this conversion needed? Almost all code inside Chrome should be
using
> > TimeTicks as it is significantly safer. Any chance we could put this behind
> some
> > type of guard which means things need to be specifically approved to use it?
> > (This can be done by making an inner class which has private static methods
> and
> > a friend declaration needed for each user.)
> > 
>
> On example for using this 
> You can refer to
>
https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
> where it is required to convert floating point to TimeDelta without losing
> precision.

Actually that is probably an example of where you should be using TimeDelta and
TimeTicks directly rather than doubles.

I believe that only systems which interface with legacy or external code should
ever need to convert to doubles. Inside Chromium we should make an effort to use
the safer types.

> > Couple of things you might want to consider;
> >  * Outside the range of -2**53 < x < 2**53 (doubles can only represent 53
bits
> > of whole values) the conversion doesn't roundtrip. IE
> >       int64 x;
> >       double y = (int64)x;
> >       int64 z = (double)y;
> >       x != z
> > 
> >    It might be a good idea to check / warn when this is happening (should
> > probably only be in debug builds)?
> > 
> >  * Similarly, when converting from a double it might be worth warning /
> checking
> > if we are discarding any information.
> > 
> 
> Yes this range should be taken into consideration by programmer while using
the
> functions that is the reason i wrote separate functions for double.

It is easy to get them wrong without realizing it, so checks here are good.

> >  * You don't seem to deal with the special "null" TimeTicks?
> > 
> >  * Could we have a basic unit test? (You can copy mine if you want.)
> 
> I will add them in next patch along with the unittest.

Great!

Tim 'mithro' Ansell

[patro, 2014-04-21 11:50:21]

Please review

[mithro-old, 2014-04-22 07:05:11]

lgtm

LGTM

https://codereview.chromium.org/235233003/diff/20001/base/time/time_unittest.cc
File base/time/time_unittest.cc (right):

https://codereview.chromium.org/235233003/diff/20001/base/time/time_unittest....
base/time/time_unittest.cc:738: EXPECT_EQ(13.3,
TimeDelta::FromMillisecondsD(13.3).InMillisecondsF());
Technically anything EXPECT_EQ with floating point numbers should be using the
gtest floating point comparisons (
https://code.google.com/p/googletest/wiki/AdvancedGuide#Floating-Point_Compar...
) but since the rest of the file already has this issue we should fix it in a
different CL.

[commit-bot: I haz the power, 2014-04-22 07:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/behara.ms@samsung.com/235233003/20001

[commit-bot: I haz the power, 2014-04-22 09:00:58]

Change committed as 265218