Use WTF::TimeTicks to represent timestamp in Platform/Core event types

Use WTF::TimeTicks to represent timestamp in Platform/Core event types

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()
 - create appropriate seconds <-> TimeTicks conversions where event
   timestamp needs to be used elsewhere in Blink

BUG=625680
Committed: https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95
Cr-Commit-Position: refs/heads/master@{#438211}

[majidvp, 2016-11-30 22:13:01]

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

[commit-bot: I haz the power, 2016-11-30 22:13:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[majidvp, 2016-11-30 22:17:02]

Description was changed from

==========
[exp] example of time type change in Histogram

BUG=
==========

to

==========
[wip] Update PlatformEvents to use TimeTicks

BUG=625680
==========

[commit-bot: I haz the power, 2016-11-30 22:17:13]

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

[commit-bot: I haz the power, 2016-11-30 22:17:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[majidvp, 2016-12-02 20:56:46]

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

[commit-bot: I haz the power, 2016-12-02 20:57:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 21:37:08]

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

[commit-bot: I haz the power, 2016-12-02 21:37:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[majidvp, 2016-12-05 14:58:48]

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

[commit-bot: I haz the power, 2016-12-05 14:58:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[majidvp, 2016-12-05 16:25:40]

Description was changed from

==========
[wip] Update PlatformEvents to use TimeTicks

BUG=625680
==========

to

==========
Represent PlatformEvent timestamps as WTF::TimeTicks instead of double

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()
 - create appropriate seconds <-> TimeTicks conversions where event 
timestamp needs to be used elsewhere in Blink


BUG=625680
==========

[commit-bot: I haz the power, 2016-12-05 16:27:50]

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

[commit-bot: I haz the power, 2016-12-05 16:27:51]

Dry run: This issue passed the CQ dry run.

[majidvp, 2016-12-05 16:34:51]

Description was changed from

==========
Represent PlatformEvent timestamps as WTF::TimeTicks instead of double

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()
 - create appropriate seconds <-> TimeTicks conversions where event 
timestamp needs to be used elsewhere in Blink


BUG=625680
==========

to

==========
Represent PlatformEvent::timestamp as WTF::TimeTicks instead of double

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()**
 - create appropriate seconds <-> TimeTicks conversions where event 
timestamp needs to be used elsewhere in Blink


** In one case we do 0 -> Now() to fix a pre-existing bug.

BUG=625680
==========

[majidvp, 2016-12-05 16:36:52]

Description was changed from

==========
Represent PlatformEvent::timestamp as WTF::TimeTicks instead of double

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()**
 - create appropriate seconds <-> TimeTicks conversions where event 
timestamp needs to be used elsewhere in Blink


** In one case we do 0 -> Now() to fix a pre-existing bug.

BUG=625680
==========

to

==========
Use WTF::TimeTicks to represent timestamp in Platform/Core event types

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()**
 - create appropriate seconds <-> TimeTicks conversions where event
   timestamp needs to be used elsewhere in Blink


** In one case we do 0 -> Now() to fix a pre-existing bug.

BUG=625680
==========

[majidvp, 2016-12-05 16:37:34]

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

[majidvp, 2016-12-05 16:37:35]

https://codereview.chromium.org/2542693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2542693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:75: 
I like to get rid of these once we switch WebInputEvent to
also hold a TimeTick type instead of a plain double.

https://codereview.chromium.org/2542693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2542693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:1952:
m_globalPositionOnFlingStart, IntSize(), TimeTicks::Now(),

Note: here we go from 0 -> TimeTicks::Now(). But the original 0 timestamp was
incorrect. So this is fixing a bug.

[majidvp, 2016-12-05 16:37:55]

esprehn@ PTAL

[esprehn, 2016-12-05 17:08:11]

Can you fix the bug in a separate patch with a test? Or at least add a test
here.

[majidvp, 2016-12-05 20:55:38]

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

[commit-bot: I haz the power, 2016-12-05 20:56:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 22:42:29]

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

[commit-bot: I haz the power, 2016-12-05 22:42:30]

Dry run: This issue passed the CQ dry run.

[majidvp, 2016-12-06 15:05:02]

Description was changed from

==========
Use WTF::TimeTicks to represent timestamp in Platform/Core event types

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()**
 - create appropriate seconds <-> TimeTicks conversions where event
   timestamp needs to be used elsewhere in Blink


** In one case we do 0 -> Now() to fix a pre-existing bug.

BUG=625680
==========

to

==========
Use WTF::TimeTicks to represent timestamp in Platform/Core event types

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()
 - create appropriate seconds <-> TimeTicks conversions where event
   timestamp needs to be used elsewhere in Blink


BUG=625680
==========

[majidvp, 2016-12-06 15:08:27]

On 2016/12/05 17:08:11, esprehn wrote:
> Can you fix the bug in a separate patch with a test? Or at least add a test
> here.

Moved the bugfix to a separate CL: https://codereview.chromium.org/2556793002
(figuring out what it the right test to add)

[esprehn, 2016-12-06 22:14:19]

https://codereview.chromium.org/2542693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp (right):

https://codereview.chromium.org/2542693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp:82: return
TimeTicks() + TimeDelta::FromSecondsD(ticksInSeconds);
I'm not a fan of the API requiring all this math all over :/ Why is there no
TimeTicks::FromSeconds() ?

https://codereview.chromium.org/2542693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2542693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:69: return TimeTicks()
+ TimeDelta::FromSecondsD(eventTimeStampSeconds);
Can you add a method to TimeTicks for FromSeconds() ?

https://codereview.chromium.org/2542693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:73: return (ticks -
TimeTicks()).InSecondsF();
Why does TimeTicks not have a ToSecondsF() method that subtracts the "zero" from
it?

I'm concerned the base API seems to require a bunch of boilerplate all over.

[majidvp, 2016-12-07 15:03:52]

> third_party/WebKit/Source/web/WebInputEventConversion.cpp:73: return (ticks -
TimeTicks()).InSecondsF();
> Why does TimeTicks not have a ToSecondsF() method that subtracts the "zero"
from
it?

> I'm concerned the base API seems to require a bunch of boilerplate all over.

+miu@ who is the base/time owner who can answer this.

My understanding is that in Chromium they rarely had to convert TimeTicks to
double (only when interfacing with platform code) so the preference was to not
add such a method to discourage its usage as such translations will lose the
clock info that they type carries.

In blink until we completely transition to using time types we need to make
conversion more often so I agree that perhaps we should have this conversion
supported here.

Option 1 - Add TimeTicks::ToSecondsF and TimeTicks::FromSeconds 
Option 2 - Add utility conversion functions WTF::TimeTicksToSeconds and 
           WTF::SecondsToTimeTicks

#1 is more ergonomics while #2 keeps the interface closer to the base.

[miu, 2016-12-07 20:38:07]

On 2016/12/07 15:03:52, majidvp wrote:
> > third_party/WebKit/Source/web/WebInputEventConversion.cpp:73: return (ticks
-
> TimeTicks()).InSecondsF();
> > Why does TimeTicks not have a ToSecondsF() method that subtracts the "zero"
> from
> it?
> 
> > I'm concerned the base API seems to require a bunch of boilerplate all over.
> 
> My understanding is that in Chromium they rarely had to convert TimeTicks to
> double (only when interfacing with platform code) so the preference was to not
> add such a method to discourage its usage as such translations will lose the
> clock info that they type carries.

Yes, the time types allows us to relate time values w.r.t. to the same clock
behavior and timeline origin. That, and the time types:

1. Have a restricted set of operations so that semantically-correct time math is
enforced at compile time. Converting them to a POD type (such as a double),
other than to pass time values to third-party libraries, is bad practice in
Chromium code.

2. Are unit-less (they just happen to have a timebase of microseconds,
internally), and provide InSeconds(), InMilliseconds(), etc.; to extract a value
in a known timebase.

3. Are under/overflow checked at run time.

> In blink until we completely transition to using time types we need to make
> conversion more often so I agree that perhaps we should have this conversion
> supported here.
> 
> Option 1 - Add TimeTicks::ToSecondsF and TimeTicks::FromSeconds 

I don't know how WTF::TimeTicks works, but base::TimeTicks already has an
InSecondsF() method to return a floating-point time value in a one-second
timebase.

[majidvp, 2016-12-07 20:51:22]

Thanks for the explanation.

> > Option 1 - Add TimeTicks::ToSecondsF and TimeTicks::FromSeconds 
> 
> I don't know how WTF::TimeTicks works, but base::TimeTicks already has an
> InSecondsF() method to return a floating-point time value in a one-second
> timebase.

I think that is incorrect. TimeDelta has a InSecondsF() but TimeTicks
does *NOT*.[1] I am using the following pattern to make the translation
to seconds: |(ticks - TimeTicks()).InSecondsF()| 

FYI, WTF::TimeTicks is just a simple wrapper around base::TimeTicks

The suggestion was to capture that pattern in WTF::TimeTicks itself
but if you are fine with that I can add it to base::TimeTicks instead.

[1]
https://codesearch.chromium.org/search/?q=InSecondsF+file:base/time&sq=packag...

[esprehn, 2016-12-07 20:57:29]

We don't need to follow the base api exactly, the purpose of the wrappers is to
make something ergonomic. Let's add the methods we need. We can also discuss
having them added to base afterwards but we don't need to block on it.

[majidvp, 2016-12-12 21:27:55]

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

[commit-bot: I haz the power, 2016-12-12 21:29:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2016-12-12 22:12:21]

lgtm

https://codereview.chromium.org/2542693002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/Time.h (right):

https://codereview.chromium.org/2542693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/Time.h:49: double InSeconds() { return (m_value -
WrappedTimeType()).InSecondsF(); }
const

[commit-bot: I haz the power, 2016-12-13 00:37:08]

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

[commit-bot: I haz the power, 2016-12-13 00:37:09]

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

[majidvp, 2016-12-13 05:28:39]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2016-12-13 05:28:41]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2542693002/#ps120001
(title: "add const")

[commit-bot: I haz the power, 2016-12-13 05:29:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 08:07:38]

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

[commit-bot: I haz the power, 2016-12-13 08:07:40]

Failed to apply patch for
third_party/WebKit/Source/core/events/KeyboardEvent.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/events/KeyboardEvent.cpp:95
  error: third_party/WebKit/Source/core/events/KeyboardEvent.cpp: patch does not
apply

Patch:       third_party/WebKit/Source/core/events/KeyboardEvent.cpp
Index: third_party/WebKit/Source/core/events/KeyboardEvent.cpp
diff --git a/third_party/WebKit/Source/core/events/KeyboardEvent.cpp
b/third_party/WebKit/Source/core/events/KeyboardEvent.cpp
index
ed4874279a25fee3e14338e647b6fb64e540d61b..cc4e4c4ca4a572aed9045a74811af5c653ed76ff
100644
--- a/third_party/WebKit/Source/core/events/KeyboardEvent.cpp
+++ b/third_party/WebKit/Source/core/events/KeyboardEvent.cpp
@@ -95,7 +95,7 @@ KeyboardEvent::KeyboardEvent(const WebKeyboardEvent& key,
           domWindow,
           0,
           static_cast<PlatformEvent::Modifiers>(key.modifiers),
-          key.timeStampSeconds,
+          TimeTicks::FromSeconds(key.timeStampSeconds),
           InputDeviceCapabilities::doesntFireTouchEventsSourceCapabilities()),
       m_keyEvent(makeUnique<WebKeyboardEvent>(key)),
       // TODO(crbug.com/482880): Fix this initialization to lazy
initialization.

[majidvp, 2016-12-13 15:39:16]

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

[commit-bot: I haz the power, 2016-12-13 15:39:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 17:18:14]

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

[commit-bot: I haz the power, 2016-12-13 17:18:15]

Dry run: This issue passed the CQ dry run.

[majidvp, 2016-12-13 17:33:58]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2016-12-13 17:33:59]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2542693002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-13 17:34:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 17:41:37]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481650438672920,
"parent_rev": "76cc141e399e91a46bff854889369cf00075b80e", "commit_rev":
"9b24c087401890378f84ce804d46703757e5e3b2"}

[commit-bot: I haz the power, 2016-12-13 17:42:03]

Description was changed from

==========
Use WTF::TimeTicks to represent timestamp in Platform/Core event types

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()
 - create appropriate seconds <-> TimeTicks conversions where event
   timestamp needs to be used elsewhere in Blink


BUG=625680
==========

to

==========
Use WTF::TimeTicks to represent timestamp in Platform/Core event types

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()
 - create appropriate seconds <-> TimeTicks conversions where event
   timestamp needs to be used elsewhere in Blink


BUG=625680

Review-Url: https://codereview.chromium.org/2542693002
==========

[commit-bot: I haz the power, 2016-12-13 17:42:05]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-13 17:45:52]

Description was changed from

==========
Use WTF::TimeTicks to represent timestamp in Platform/Core event types

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()
 - create appropriate seconds <-> TimeTicks conversions where event
   timestamp needs to be used elsewhere in Blink


BUG=625680

Review-Url: https://codereview.chromium.org/2542693002
==========

to

==========
Use WTF::TimeTicks to represent timestamp in Platform/Core event types

This is a fairly mechanical change:
 - Replace WTF::monotonicallyIncreastingTime -> WTF::TimeTicks::Now()
 - Replace 0 -> WTF::TimeTicks()
 - create appropriate seconds <-> TimeTicks conversions where event
   timestamp needs to be used elsewhere in Blink

BUG=625680

Committed: https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95
Cr-Commit-Position: refs/heads/master@{#438211}
==========

[commit-bot: I haz the power, 2016-12-13 17:45:53]

Patchset 8 (id:??) landed as
https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95
Cr-Commit-Position: refs/heads/master@{#438211}

[brettw, 2016-12-14 03:31:14]

I'm not objecting to this and I support work in this direction. Thanks!

But for my own information, what is the point of this wrapper? WTF/time.h
includes base/time/time.h. Blink code still sees the base header, the type
base::Time, and everything transitively included from there which includes all
of base/logging.h, safe math primitives, <iosfwd>, <limits>, <utility>, various
streams, <string>, etc.

I don't actually see any problem with these dependencies, but then what's the
wrapper buying?

[esprehn, 2016-12-14 03:51:29]

On 2016/12/14 at 03:31:14, brettw wrote:
> I'm not objecting to this and I support work in this direction. Thanks!
> 
> But for my own information, what is the point of this wrapper? WTF/time.h
includes base/time/time.h. Blink code still sees the base header, the type
base::Time, and everything transitively included from there which includes all
of base/logging.h, safe math primitives, <iosfwd>, <limits>, <utility>, various
streams, <string>, etc.
> 
> I don't actually see any problem with these dependencies, but then what's the
wrapper buying?

You can't declare a variable or call anything in base:: anything in core,
enforced by the presubmit. The wrapper prevents usage of apis we don't want ex.
The parsing code that takes strings.

[brettw, 2016-12-15 18:32:19]

That makes sense, I was misunderstanding the point of the TimeWrapper class.