Change ui::Event::time_stamp from TimeDelta to TimeTicks

Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  => TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) => TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()

BUG=453559
Committed: https://crrev.com/9b3bda8e667c7e0767476cef4728ffa93f3efdc2
Cr-Commit-Position: refs/heads/master@{#398947}

[majidvp, 2016-05-12 22:01:04]

Description was changed from

==========
Chanve ui::Event::time_stamp_ type from TimeDelta to TimeTicks

BUG=453559
==========

to

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using 
base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to
make it clear that these values are expected to come from the same clock and
remove some crufty code.

The changes in the CL are pretty simple and mechanical:


- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between 
WebInputEvent::timeStampSeconds (double) and ui::Event::timeStamp (TimeTicks)

- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()


BUG=453559
==========

[majidvp, 2016-05-12 22:01:54]

Description was changed from

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using 
base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to
make it clear that these values are expected to come from the same clock and
remove some crufty code.

The changes in the CL are pretty simple and mechanical:


- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between 
WebInputEvent::timeStampSeconds (double) and ui::Event::timeStamp (TimeTicks)

- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()


BUG=453559
==========

to

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:


- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)

- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()


BUG=453559
==========

[majidvp, 2016-05-16 12:49:50]

https://codereview.chromium.org/1975533002/diff/20001/ash/touch/touch_hud_deb...
File ash/touch/touch_hud_debug.cc (right):

https://codereview.chromium.org/1975533002/diff/20001/ash/touch/touch_hud_deb...
ash/touch/touch_hud_debug.cc:99: timestamp((touch.time_stamp() -
base::TimeTicks()).InMillisecondsF()),
This is the pattern I use when it is needed to convert a TimeTicks to TimeDelta:
delta = ticks - base::TimeTicks()

There are a few of such cases where a conversion is needed. 
Few of them are artifact of |MenuRunner::closing_event_time()| being a delta
which
should get fixed in a separate patch.

https://codereview.chromium.org/1975533002/diff/20001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1975533002/diff/20001/base/time/time.h#newcod...
base/time/time.h:314: }
Pulled up from Time so that it is applicate to all TimeCalss instances.

[majidvp, 2016-05-16 12:50:50]

Description was changed from

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:


- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)

- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()


BUG=453559
==========

to

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()


BUG=453559
==========

[miu, 2016-05-16 20:05:31]

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

[miu, 2016-05-16 20:05:35]

Hey. Saw this change come up on one of my watchlists. Some comments:

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1877: closing_event_time_ = base::TimeDelta();
IIUC, closing_event_time_ needs to be changed to be of type base::TimeTicks.

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1880: return (delta.InMilliseconds() == 0);
The comment suggests this is a bug. Shouldn't this code be:

  base::TimeTicks last_closing_event_time = closing_event_time_;
  closing_event_time_ = base::TimeTicks();
  // <comment>
  return event.time_stamp() == last_closing_event_time;

https://codereview.chromium.org/1975533002/diff/160001/ui/events/base_event_u...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/base_event_u...
ui/events/base_event_utils.h:25: EVENTS_BASE_EXPORT double
EventTimeStampToSeconds(base::TimeTicks time_stamp);
Please add some comments to explain the usage of these:

1. These should rarely be used (i.e., only when interfacing with platform code
that does not use Chromium's base::TimeXXX type system). Otherwise, client code
should maintain the time values in TimeTicks form.

2. The time origin of the doubles is not fixed between runs of the application
(and is certainly not any sort of UNIX epoch, or some such).

In fact, do you absolutely need these? And, if you do, can they be made private
or otherwise placed in an "internal" namespace to discourage outside use?

https://codereview.chromium.org/1975533002/diff/160001/ui/events/cocoa/events...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/cocoa/events...
ui/events/cocoa/events_mac.mm:18: 
Please remove blank line here.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/cocoa/events...
ui/events/cocoa/events_mac.mm:88: return ui::EventTimeStampFromSeconds(seconds)
+
This feels brittle. How will you ensure this remains in-line with the
base/time/time_mac.mm implementation? Perhaps you should write a unit test (or
at least some DCHECKing) to ensure this translation is always correct?

https://codereview.chromium.org/1975533002/diff/160001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/event.h#newc...
ui/events/event.h:80: const base::TimeTicks& time_stamp() const { return
time_stamp_; }
nit (Chromium style): Pass-by-value instead of by-ref here (and for
set_time_stamp() below).

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/event_converter_evdev.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/event_converter_evdev.cc:149: return
ui::EventTimeStampFromSeconds(event.time.tv_sec) +
Same thing here: How will you ensure this does not break if the implementation
in base/time/time_posix.cc changes?

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/event_converter_evdev_impl.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/event_converter_evdev_impl.h:56: const base::TimeTicks&
timestamp);
Please pass-by-value here (and OnButtonChange() below).

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc
(right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc:83:
return ui::EventTimeStampFromSeconds(timestamp);
Same thing here too (regarding this remaining in-line with changes to
base/time/time_posix.cc).

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/touch_event_converter_evdev.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/touch_event_converter_evdev.h:76: const base::TimeTicks&
delta);
Pass-by-value here too.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.h
(right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.h:30: bool
is_valid() const { return start != base::TimeTicks(); }
!start.is_null()

https://codereview.chromium.org/1975533002/diff/160001/ui/events/x/events_x_u...
File ui/events/x/events_x_utils.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/x/events_x_u...
ui/events/x/events_x_utils.cc:316: // Takes Xlib Time and returns a time delta
that is immune to timer rollover.
comment need edit: s/time delta/base::TimeTicks/

[majidvp, 2016-05-21 01:38:00]

> Hey. Saw this change come up on one of my watchlists.
Thanks for drive by comments. See responses below.

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1877: closing_event_time_ = base::TimeDelta();
On 2016/05/16 20:05:35, miu wrote:
> IIUC, closing_event_time_ needs to be changed to be of type base::TimeTicks.

It should be eventually but I prefer to make that change in a
separate patch. 

This value get initialized from MenuRunner::closing_event_time.
I din't want to change that interface in this patch to contain the
scope. But I have a TODO in menu_controller.h about
the need for this change.

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1880: return (delta.InMilliseconds() == 0);
On 2016/05/16 20:05:35, miu wrote:
> The comment suggests this is a bug. Shouldn't this code be:
> 
>   base::TimeTicks last_closing_event_time = closing_event_time_;
>   closing_event_time_ = base::TimeTicks();
>   // <comment>
>   return event.time_stamp() == last_closing_event_time;

I don't see how this logic is different from the existing logic.
The result is the same.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/base_event_u...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/base_event_u...
ui/events/base_event_utils.h:25: EVENTS_BASE_EXPORT double
EventTimeStampToSeconds(base::TimeTicks time_stamp);
On 2016/05/16 20:05:35, miu wrote:
> Please add some comments to explain the usage of these:
> 
> 1. These should rarely be used (i.e., only when interfacing with platform code
> that does not use Chromium's base::TimeXXX type system). Otherwise, client
code
> should maintain the time values in TimeTicks form.
> 
> 2. The time origin of the doubles is not fixed between runs of the application
> (and is certainly not any sort of UNIX epoch, or some such).

I will add a comment. As you mentioned these should only
be used when interfacing with platform code. I am planing
to eventually get rid of that usecase as well.

> In fact, do you absolutely need these? And, if you do, can they be made
private
> or otherwise placed in an "internal" namespace to discourage outside use?

These are being used by mojo/converter as well so I cannot make them private. Do
you still think it is useful to make them internal
given that  they will be used outside ui/events?

https://codereview.chromium.org/1975533002/diff/160001/ui/events/cocoa/events...
File ui/events/cocoa/events_mac.mm (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/cocoa/events...
ui/events/cocoa/events_mac.mm:18: 
On 2016/05/16 20:05:35, miu wrote:
> Please remove blank line here.

Done.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/cocoa/events...
ui/events/cocoa/events_mac.mm:88: return ui::EventTimeStampFromSeconds(seconds)
+
On 2016/05/16 20:05:35, miu wrote:
> This feels brittle. How will you ensure this remains in-line with the
> base/time/time_mac.mm implementation? Perhaps you should write a unit test (or
> at least some DCHECKing) to ensure this translation is always correct?

Good point. I added a DCHECK to ensure it is within a reasonable distance of
TimeTicks::Now.

Note that we already track this in UMA and it is pretty clear that we should
never hit that DCHECK on Mac OS:
https://uma.googleplex.com/p/chrome/histograms/?endDate=05-18-2016&dayCount=2...
 

Also will look into how I can get Mac OS to generate event for testing purpose
and see if I can write a test for that.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/event.h#newc...
ui/events/event.h:80: const base::TimeTicks& time_stamp() const { return
time_stamp_; }
On 2016/05/16 20:05:35, miu wrote:
> nit (Chromium style): Pass-by-value instead of by-ref here (and for
> set_time_stamp() below).

Done. Though I couldn't find the directive to prefer pass by-value over by-ref
but I agree it is better.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/event_converter_evdev.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/event_converter_evdev.cc:149: return
ui::EventTimeStampFromSeconds(event.time.tv_sec) +
On 2016/05/16 20:05:35, miu wrote:
> Same thing here: How will you ensure this does not break if the implementation
> in base/time/time_posix.cc changes?

Added a validation check. If we find out that it gets triggered then we can work
on a fallback.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/touch_event_converter_evdev.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/touch_event_converter_evdev.h:76: const base::TimeTicks&
delta);
On 2016/05/16 20:05:35, miu wrote:
> Pass-by-value here too.

Done.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.h
(right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.h:30: bool
is_valid() const { return start != base::TimeTicks(); }
On 2016/05/16 20:05:35, miu wrote:
> !start.is_null()

Done.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/x/events_x_u...
File ui/events/x/events_x_utils.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/x/events_x_u...
ui/events/x/events_x_utils.cc:316: // Takes Xlib Time and returns a time delta
that is immune to timer rollover.
On 2016/05/16 20:05:35, miu wrote:
> comment need edit: s/time delta/base::TimeTicks/

Done.

[majidvp, 2016-05-24 15:47:05]

majidvp@chromium.org changed reviewers:
+ sadrul@chromium.org

[majidvp, 2016-05-24 15:47:06]

miu@: PTAL.

+ sadrul@: Please review the proposed changes.

[majidvp, 2016-05-24 15:52:33]

Description was changed from

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()


BUG=453559
==========

to

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- Introduce a new assertion to help detect cases where an event timestamp is not
comparable to underlying platform tick clock.
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()



BUG=453559
==========

[majidvp, 2016-05-24 21:01:38]

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/event_converter_evdev.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/event_converter_evdev.cc:149: return
ui::EventTimeStampFromSeconds(event.time.tv_sec) +
On 2016/05/21 01:38:00, majidvp wrote:
> On 2016/05/16 20:05:35, miu wrote:
> > Same thing here: How will you ensure this does not break if the
implementation
> > in base/time/time_posix.cc changes?
> 
> Added a validation check. If we find out that it gets triggered then we can
work
> on a fallback. 

After chatting with sadrul@, I decided to add this validation in a separate CL
as it
is rather independent of this change.

I have created:  https://codereview.chromium.org/2007083002/

[miu, 2016-05-25 01:42:32]

lgtm % minor things. Good stuff! :)

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1880: return (delta.InMilliseconds() == 0);
On 2016/05/21 01:38:00, majidvp wrote:
> On 2016/05/16 20:05:35, miu wrote:
> > The comment suggests this is a bug. Shouldn't this code be:
> > 
> >   base::TimeTicks last_closing_event_time = closing_event_time_;
> >   closing_event_time_ = base::TimeTicks();
> >   // <comment>
> >   return event.time_stamp() == last_closing_event_time;
> 
> I don't see how this logic is different from the existing logic.
> The result is the same.

What if |delta| is 999 usec? Then |delta.InMilliseconds() == 0| will be true
(integer truncation after usec are divided by 1000), but clearly there were two
distinct timestamps almost a full millisecond apart; and this indicates the
event is not a "repost event" as the comment states.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/base_event_u...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/base_event_u...
ui/events/base_event_utils.h:25: EVENTS_BASE_EXPORT double
EventTimeStampToSeconds(base::TimeTicks time_stamp);
On 2016/05/21 01:38:00, majidvp wrote:
> On 2016/05/16 20:05:35, miu wrote:
> > Please add some comments to explain the usage of these:
> > 
> > 1. These should rarely be used (i.e., only when interfacing with platform
code
> > that does not use Chromium's base::TimeXXX type system). Otherwise, client
> code
> > should maintain the time values in TimeTicks form.
> > 
> > 2. The time origin of the doubles is not fixed between runs of the
application
> > (and is certainly not any sort of UNIX epoch, or some such).
> 
> I will add a comment. As you mentioned these should only
> be used when interfacing with platform code. I am planing
> to eventually get rid of that usecase as well.
> 
> > In fact, do you absolutely need these? And, if you do, can they be made
> private
> > or otherwise placed in an "internal" namespace to discourage outside use?
> 
> These are being used by mojo/converter as well so I cannot make them private.
Do
> you still think it is useful to make them internal
> given that  they will be used outside ui/events?

IIRC, mojo includes a serializer for the base::TimeTicks type. If not, it
should. ;)

My main concern here is that external code will attempt to interpret the meaning
of the double values (e.g., assuming some specific time origin, or relating the
value to other clocks, or to some calendar point-in-time). While this is not the
case in your change here--since you're only using these to translate to/from
native platform UI toolkit values--it does set a possible starting point for
unintentional "code rot" in the future.

I'll leave it up to you and ui/events/OWNERS to decide whether it's acceptable
for these functions to be publicly visible like they are. You may simply want to
comment them with a "WARNING: Only for native platform value translation use
within ui/events code!"

https://codereview.chromium.org/1975533002/diff/160001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/event.h#newc...
ui/events/event.h:80: const base::TimeTicks& time_stamp() const { return
time_stamp_; }
On 2016/05/21 01:38:00, majidvp wrote:
> On 2016/05/16 20:05:35, miu wrote:
> > nit (Chromium style): Pass-by-value instead of by-ref here (and for
> > set_time_stamp() below).
> 
> Done. Though I couldn't find the directive to prefer pass by-value over by-ref
> but I agree it is better.

Oh, sorry. Here's a link to the relevant header file comment:
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l=24

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/event_converter_evdev.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/event_converter_evdev.cc:149: return
ui::EventTimeStampFromSeconds(event.time.tv_sec) +
On 2016/05/24 21:01:38, majidvp wrote:
> On 2016/05/21 01:38:00, majidvp wrote:
> > On 2016/05/16 20:05:35, miu wrote:
> > > Same thing here: How will you ensure this does not break if the
> implementation
> > > in base/time/time_posix.cc changes?
> > 
> > Added a validation check. If we find out that it gets triggered then we can
> work
> > on a fallback. 
> 
> After chatting with sadrul@, I decided to add this validation in a separate CL
> as it
> is rather independent of this change.
> 
> I have created:  https://codereview.chromium.org/2007083002/

Acknowledged. Looks good.

[sadrul, 2016-05-25 20:22:11]

One nit, one comment. lgtm

https://codereview.chromium.org/1975533002/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner_cocoa_unittest.mm (right):

https://codereview.chromium.org/1975533002/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner_cocoa_unittest.mm:168:
TEST_F(MenuRunnerCocoaTest, RunMenkjuAndCancel) {
unrename?

https://codereview.chromium.org/1975533002/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner_cocoa_unittest.mm:179: (ui::EventTimeForNow()
- base::TimeTicks()));
I assume addressing the TODO in menu_controller.h will allow us going back to
just EventTimeForNow()?

[majidvp, 2016-05-27 14:34:16]

sadrul@, miu@: Thanks for review.

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/1975533002/diff/160001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1880: return (delta.InMilliseconds() == 0);
On 2016/05/25 01:42:31, miu wrote:
> On 2016/05/21 01:38:00, majidvp wrote:
> > On 2016/05/16 20:05:35, miu wrote:
> > > The comment suggests this is a bug. Shouldn't this code be:
> > > 
> > >   base::TimeTicks last_closing_event_time = closing_event_time_;
> > >   closing_event_time_ = base::TimeTicks();
> > >   // <comment>
> > >   return event.time_stamp() == last_closing_event_time;
> > 
> > I don't see how this logic is different from the existing logic.
> > The result is the same.
> 
> What if |delta| is 999 usec? Then |delta.InMilliseconds() == 0| will be true
> (integer truncation after usec are divided by 1000), but clearly there were
two
> distinct timestamps almost a full millisecond apart; and this indicates the
> event is not a "repost event" as the comment states.

Doh! I totally missed the milliseconds conversion. Fixed.

https://codereview.chromium.org/1975533002/diff/160001/ui/events/base_event_u...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/1975533002/diff/160001/ui/events/base_event_u...
ui/events/base_event_utils.h:25: EVENTS_BASE_EXPORT double
EventTimeStampToSeconds(base::TimeTicks time_stamp);
On 2016/05/25 01:42:31, miu wrote:
> On 2016/05/21 01:38:00, majidvp wrote:
> > On 2016/05/16 20:05:35, miu wrote:
> > > Please add some comments to explain the usage of these:
> > > 
> > > 1. These should rarely be used (i.e., only when interfacing with platform
> code
> > > that does not use Chromium's base::TimeXXX type system). Otherwise, client
> > code
> > > should maintain the time values in TimeTicks form.
> > > 
> > > 2. The time origin of the doubles is not fixed between runs of the
> application
> > > (and is certainly not any sort of UNIX epoch, or some such).
> > 
> > I will add a comment. As you mentioned these should only
> > be used when interfacing with platform code. I am planing
> > to eventually get rid of that usecase as well.
> > 
> > > In fact, do you absolutely need these? And, if you do, can they be made
> > private
> > > or otherwise placed in an "internal" namespace to discourage outside use?
> > 
> > These are being used by mojo/converter as well so I cannot make them
private.
> Do
> > you still think it is useful to make them internal
> > given that  they will be used outside ui/events?
> 
> IIRC, mojo includes a serializer for the base::TimeTicks type. If not, it
> should. ;)

Mojo has convertor for TimeTicks but we are converting TimeTicks to seconds.
It has a utility function for that as well but I intentionally removed in favor
of 
consolidating all such event time conversions in these pair of functions.

> My main concern here is that external code will attempt to interpret the
meaning
> of the double values (e.g., assuming some specific time origin, or relating
the
> value to other clocks, or to some calendar point-in-time). While this is not
the
> case in your change here--since you're only using these to translate to/from
> native platform UI toolkit values--it does set a possible starting point for
> unintentional "code rot" in the future.
> 
> I'll leave it up to you and ui/events/OWNERS to decide whether it's acceptable
> for these functions to be publicly visible like they are. You may simply want
to
> comment them with a "WARNING: Only for native platform value translation use
> within ui/events code!"

I added a WARNING to the comment.

https://codereview.chromium.org/1975533002/diff/280001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner_cocoa_unittest.mm (right):

https://codereview.chromium.org/1975533002/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner_cocoa_unittest.mm:168:
TEST_F(MenuRunnerCocoaTest, RunMenkjuAndCancel) {
On 2016/05/25 20:22:10, sadrul wrote:
> unrename?

Done.

https://codereview.chromium.org/1975533002/diff/280001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner_cocoa_unittest.mm:179: (ui::EventTimeForNow()
- base::TimeTicks()));
On 2016/05/25 20:22:10, sadrul wrote:
> I assume addressing the TODO in menu_controller.h will allow us going back to
> just EventTimeForNow()?

Yes indeed.

[majidvp, 2016-05-27 14:40:08]

majidvp@chromium.org changed reviewers:
+ reveman@chromium.org, sky@chromium.org

[majidvp, 2016-05-27 14:40:10]

+reveman@: For components/exo review
+sky@: For components/mus, ash/, ui/views, mojo/converters review

[sky, 2016-05-27 15:51:36]

Meta question. Is timeticks going to be roughly the same across processes?

[sky, 2016-05-27 15:56:03]

https://codereview.chromium.org/1975533002/diff/320001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right):

https://codereview.chromium.org/1975533002/diff/320001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner_impl_cocoa.mm:98:
closing_event_time_(base::TimeDelta()) {
Change to TimeTicks?

[majidvp, 2016-05-27 16:45:38]

On 2016/05/27 15:51:36, sky wrote:
> Meta question. Is timeticks going to be roughly the same across processes?

TimeTick::Now() should be for the most part. I think there is some exceptions to
this for Windows in cases where we have to fallback to TSC. If there is a need
to
convert timestamp across processes on Windows there is 
|InterProcessTimeTicksConverter|.


See here for details on that: 
https://docs.google.com/document/d/1ypBZPZnshJ612FWAjkkpSBQiII7vYEEDsRkDOkVjQ...

[sky, 2016-05-27 16:47:30]

Do we use said function in IPCs with timestamps?

On Fri, May 27, 2016 at 9:45 AM,  <majidvp@chromium.org> wrote:
> On 2016/05/27 15:51:36, sky wrote:
>> Meta question. Is timeticks going to be roughly the same across processes?
>
> TimeTick::Now() should be for the most part. I think there is some
> exceptions to
> this for Windows in cases where we have to fallback to TSC. If there is a
> need
> to
> convert timestamp across processes on Windows there is
> |InterProcessTimeTicksConverter|.
>
>
> See here for details on that:
>
https://docs.google.com/document/d/1ypBZPZnshJ612FWAjkkpSBQiII7vYEEDsRkDOkVjQ...
>
>
>
> https://codereview.chromium.org/1975533002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[majidvp, 2016-05-27 16:49:22]

https://codereview.chromium.org/1975533002/diff/320001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right):

https://codereview.chromium.org/1975533002/diff/320001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner_impl_cocoa.mm:98:
closing_event_time_(base::TimeDelta()) {
On 2016/05/27 15:56:03, sky wrote:
> Change to TimeTicks?

I have a TODO in menu_controller.h to do this in a separate patch
if that is alright. It is my attempt to keep the scope smaller.

[majidvp, 2016-05-27 17:53:43]

On 2016/05/27 16:47:30, sky wrote:
> Do we use said function in IPCs with timestamps?

No we don't as far as I can tell. It is used in a few specific cases such as
when resource timing informations are computed.

+charliea@ in case he has more information on this.

[majidvp, 2016-05-30 14:19:34]

On 2016/05/27 17:53:43, majidvp wrote:
> On 2016/05/27 16:47:30, sky wrote:
> > Do we use said function in IPCs with timestamps?
> 
> No we don't as far as I can tell. It is used in a few specific cases such as
> when resource timing informations are computed.
> 
> +charliea@ in case he has more information on this.

@sky, I have opened a separate discussion with charliea@
on how best to address potential skew when passing timeticks
between processes.

I don't think that should block this patch as it is only making
it clear that these timestamp values are timeticks rather than 
timedelta and does not worsen/improve any inter-process
clock skew issue that exists.

[majidvp, 2016-05-30 14:20:12]

Description was changed from

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- Introduce a new assertion to help detect cases where an event timestamp is not
comparable to underlying platform tick clock.
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()



BUG=453559
==========

to

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()



BUG=453559
==========

[sky, 2016-05-31 17:57:34]

On Mon, May 30, 2016 at 7:19 AM,  <majidvp@chromium.org> wrote:
> On 2016/05/27 17:53:43, majidvp wrote:
>> On 2016/05/27 16:47:30, sky wrote:
>> > Do we use said function in IPCs with timestamps?
>>
>> No we don't as far as I can tell. It is used in a few specific cases such
>> as
>> when resource timing informations are computed.
>>
>> +charliea@ in case he has more information on this.
>
> @sky, I have opened a separate discussion with charliea@
> on how best to address potential skew when passing timeticks
> between processes.
>
> I don't think that should block this patch as it is only making
> it clear that these timestamp values are timeticks rather than
> timedelta and does not worsen/improve any inter-process
> clock skew issue that exists.

With a timeticks don't consumers that want to track deltas now need to
do the subtraction themselves? I'm worried that we have code sending
the timeticks over IPC and now doing something like event_time_ticks -
cached_time_ticks_, which could give a different result than looking
at the old timedelta of the event. As you had to do the conversion do
we have places like that?

  -Scott

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[majidvp, 2016-06-01 21:08:13]

On 2016/05/31 17:57:34, sky wrote:
> On Mon, May 30, 2016 at 7:19 AM,  <mailto:majidvp@chromium.org> wrote:
> > On 2016/05/27 17:53:43, majidvp wrote:
> >> On 2016/05/27 16:47:30, sky wrote:
> >> > Do we use said function in IPCs with timestamps?
> >>
> >> No we don't as far as I can tell. It is used in a few specific cases such
> >> as
> >> when resource timing informations are computed.
> >>
> >> +charliea@ in case he has more information on this.
> >
> > @sky, I have opened a separate discussion with charliea@
> > on how best to address potential skew when passing timeticks
> > between processes.
> >
> > I don't think that should block this patch as it is only making
> > it clear that these timestamp values are timeticks rather than
> > timedelta and does not worsen/improve any inter-process
> > clock skew issue that exists.
> 
> With a timeticks don't consumers that want to track deltas now need to
> do the subtraction themselves? I'm worried that we have code sending
> the timeticks over IPC and now doing something like event_time_ticks -
> cached_time_ticks_, which could give a different result than looking
> at the old timedelta of the event.

The result will not be any different given that the underlying value has not
changed
only its type. In another word, the old  delta was not the difference between
consecutive
events but actually an absolute timestamp that was encoded in a timedelta.

So if the code was comparing the old timedelta with a cached ticks value
from a different process it was going to suffer from inter-process inconsistency
anyways. The change does not improve or worsen the situation.


> As you had to do the conversion do we have places like that?

We do. I know at least of a case in
InputHandlerProxy::ReportInputEventLatencyUma
which means some of our event latency metrics for Windows are bogus.

I will file a bug to track fixing these cases if possible.

[sky, 2016-06-01 21:13:04]

LGTM

[majidvp, 2016-06-02 20:11:15]

majidvp@chromium.org changed reviewers:
+ danakj@chromium.org, tdresser@chromium.org

[majidvp, 2016-06-02 20:11:17]

+ tdresse@r: for content/common/input review
+ danakj@: for base/time review

[danakj, 2016-06-02 20:18:22]

On Thu, Jun 2, 2016 at 1:11 PM, <majidvp@chromium.org> wrote:

> + tdresse@r: for content/common/input review
> + danakj@: for base/time review
>

Why did you move the definition to the header?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[majidvp, 2016-06-02 21:04:30]

On 2016/06/02 20:18:22, danakj wrote:
> On Thu, Jun 2, 2016 at 1:11 PM, <mailto:majidvp@chromium.org> wrote:
> 
> > + tdresse@r: for content/common/input review
> > + danakj@: for base/time review
> >
> 
> Why did you move the definition to the header?

I moved the definition from Time class to its parent TimeClass (a template
class)
so that the function is available on both Time and TimeTicks.

TimeTicks::Max() is used in
components/mus/common/event_param_traits_unittest.cc.

[danakj, 2016-06-02 21:10:24]

Ah yah, thanks. That seems ok given is_max() it already has this notion. LGTM

[reveman, 2016-06-03 13:06:00]

https://codereview.chromium.org/1975533002/diff/400001/components/exo/wayland...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/1975533002/diff/400001/components/exo/wayland...
components/exo/wayland/server.cc:112: int64_t InMilliseconds(base::TimeTicks
ticks) {
Can you make this "uint32_t EventTimeStampToMilliseconds(base::TimeTicks
ticks)"?

uint32_t as that's what wayland interfaces use for event time stamps. It's fine
for this to potentially overflow as long as overflow is well defined as when
using uint32_t.

[majidvp, 2016-06-03 19:14:21]

https://codereview.chromium.org/1975533002/diff/400001/components/exo/wayland...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/1975533002/diff/400001/components/exo/wayland...
components/exo/wayland/server.cc:112: int64_t InMilliseconds(base::TimeTicks
ticks) {
On 2016/06/03 13:06:00, reveman wrote:
> Can you make this "uint32_t EventTimeStampToMilliseconds(base::TimeTicks
> ticks)"?
> 
> uint32_t as that's what wayland interfaces use for event time stamps. It's
fine
> for this to potentially overflow as long as overflow is well defined as when
> using uint32_t.

Done. I used TimeTicksToMilleseconds because it was not limited to event
timestamps.

[majidvp, 2016-06-03 19:16:12]

majidvp@chromium.org changed reviewers:
+ yfriedman@chromium.org

[majidvp, 2016-06-03 19:16:14]

+yfriedman@: please review  content/browser/android/content_view_core_impl.cc

[reveman, 2016-06-03 19:19:44]

On 2016/06/03 at 19:14:21, majidvp wrote:
>
https://codereview.chromium.org/1975533002/diff/400001/components/exo/wayland...
> File components/exo/wayland/server.cc (right):
> 
>
https://codereview.chromium.org/1975533002/diff/400001/components/exo/wayland...
> components/exo/wayland/server.cc:112: int64_t InMilliseconds(base::TimeTicks
ticks) {
> On 2016/06/03 13:06:00, reveman wrote:
> > Can you make this "uint32_t EventTimeStampToMilliseconds(base::TimeTicks
> > ticks)"?
> > 
> > uint32_t as that's what wayland interfaces use for event time stamps. It's
fine
> > for this to potentially overflow as long as overflow is well defined as when
> > using uint32_t.
> 
> Done. I used TimeTicksToMilleseconds because it was not limited to event
> timestamps.

Thanks. LGTM.

[majidvp, 2016-06-06 14:06:06]

yfiedman@, tdresser@: friendly ping!

[tdresser, 2016-06-06 14:46:20]

content/common/input LGTM

[majidvp, 2016-06-06 14:50:30]

majidvp@chromium.org changed reviewers:
+ skyostil@chromium.org
- yfriedman@chromium.org

[majidvp, 2016-06-06 14:50:32]

-yfriedman@: OOO

+skyostil@: Please review content/browser/android/content_view_core_impl.cc

[majidvp, 2016-06-06 14:51:05]

The CQ bit was checked by majidvp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-06 14:51:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1975533002/420001

[commit-bot: I haz the power, 2016-06-06 14:56:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-06 14:56:06]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Sami, 2016-06-08 15:28:43]

Sorry for the delay.

content/browser/android/content_view_core_impl.cc lgtm -- thanks for the
cleanup!

[majidvp, 2016-06-08 16:26:29]

On 2016/06/08 15:28:43, Sami wrote:
> Sorry for the delay.
> 
> content/browser/android/content_view_core_impl.cc lgtm -- thanks for the
> cleanup!

Thanks all reviewer for your 🕑. 🎉

[majidvp, 2016-06-08 16:26:53]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2016-06-08 16:26:55]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
miu@chromium.org, sky@chromium.org, danakj@chromium.org, reveman@chromium.org,
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1975533002/#ps440001
(title: "merge master")

[commit-bot: I haz the power, 2016-06-08 16:27:23]

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

[commit-bot: I haz the power, 2016-06-08 16:41:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-08 16:41:49]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[majidvp, 2016-06-09 13:11:56]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2016-06-09 13:11:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sky@chromium.org, danakj@chromium.org, miu@chromium.org,
sadrul@chromium.org, tdresser@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1975533002/#ps460001
(title: "rebase and minor fix")

[commit-bot: I haz the power, 2016-06-09 13:12:11]

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

[commit-bot: I haz the power, 2016-06-09 13:19:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 13:19:35]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[majidvp, 2016-06-09 18:03:24]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2016-06-09 18:03:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sky@chromium.org, danakj@chromium.org, miu@chromium.org,
sadrul@chromium.org, tdresser@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1975533002/#ps480001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-09 18:03:57]

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

[commit-bot: I haz the power, 2016-06-09 18:12:24]

Description was changed from

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()



BUG=453559
==========

to

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()



BUG=453559
==========

[commit-bot: I haz the power, 2016-06-09 18:12:27]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2016-06-09 18:17:27]

Description was changed from

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()



BUG=453559
==========

to

==========
Change ui::Event::time_stamp from TimeDelta to TimeTicks

Event timestamp values are now consistently coming from the same clock as
base::TimeTicks. Historically this was not the case in particular on Windows
platform which is why we were using  base::TimeDelta instead.

Now that this has been addressed we should use base::TimeTicks to make it clear
that these values are expected to come from the same clock and remove some
crufty code.

The changes in the CL are pretty simple and mechanical:
- Introduce a pair of conversion functions in ui/events/base_event_utils.h to
convert between  WebInputEvent::timeStampSeconds (double) and
ui::Event::timeStamp (TimeTicks)
- TimeDelta() =>TimeTicks()
- TimeDelta::FromMilliseconds(0) => base::TimeTicks()
- TimeDelta::FromInternalValue(TimeTicks::Now().ToInternalValue())  =>
TimeTicks::Now()
- TimeDelta::FromMilliseconds(Time::Now().ToDoubleT() * 1000) =>
TimeTicks::Now()
- Time::NowFromSystemTime() - base::Time() => TimeTicks::Now()

BUG=453559

Committed: https://crrev.com/9b3bda8e667c7e0767476cef4728ffa93f3efdc2
Cr-Commit-Position: refs/heads/master@{#398947}
==========

[commit-bot: I haz the power, 2016-06-09 18:17:29]

Patchset 25 (id:??) landed as
https://crrev.com/9b3bda8e667c7e0767476cef4728ffa93f3efdc2
Cr-Commit-Position: refs/heads/master@{#398947}