New issue
Advanced search Search tips

Issue 625680 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Use better time types for events in Blink

Project Member Reported by majidvp@chromium.org, Jul 4 2016

Issue description

We use double to represent different time units in Blink. 
Import and use base::TimeTicks and base::Time in blink starting with events.


See more details here: https://groups.google.com/a/chromium.org/forum/#!searchin/platform-architecture-dev/event$20time/platform-architecture-dev/7kg1FAsXkng/l19cEeBPBAAJ
 
Summary: Use better time types events in Blink (was: Use better tume types events in Blink)
Labels: Hotlist-CodeHealth
Components: Blink>Input
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/57f212f9329a3ceec41e466a31b4b83508041e2b

commit 57f212f9329a3ceec41e466a31b4b83508041e2b
Author: majidvp <majidvp@chromium.org>
Date: Fri Dec 02 23:53:10 2016

Provide simple wrappers for base/time types in WTF

- WTF::TimeDelta aliased base::TimeDelta
- WTF::{Time,TimeTicks} are a thin wrapper around base::{Time,TimeTicks} this is
  mainly because we want to control the API surface we expose in particular we
  want to avoid exposing string parsing functions at this time.

Once we have these types we can start replacing usage of double and int to
represent time in blink with more accurate and type safe representations.

Relevant discussion: https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/7kg1FAsXkng/l19cEeBPBAAJ

BUG= 625680 ,  402027 
TEST=Critical functionality is already covered in base/time/time_unittest.cc but
added a smoke test in TimeTest.cpp

Review-Url: https://codereview.chromium.org/2543663002
Cr-Commit-Position: refs/heads/master@{#436084}

[modify] https://crrev.com/57f212f9329a3ceec41e466a31b4b83508041e2b/third_party/WebKit/Source/wtf/BUILD.gn
[modify] https://crrev.com/57f212f9329a3ceec41e466a31b4b83508041e2b/third_party/WebKit/Source/wtf/CurrentTime.cpp
[modify] https://crrev.com/57f212f9329a3ceec41e466a31b4b83508041e2b/third_party/WebKit/Source/wtf/CurrentTime.h
[add] https://crrev.com/57f212f9329a3ceec41e466a31b4b83508041e2b/third_party/WebKit/Source/wtf/Time.h
[add] https://crrev.com/57f212f9329a3ceec41e466a31b4b83508041e2b/third_party/WebKit/Source/wtf/TimeTest.cpp

Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dbb3776482cac31616c9309bf727c594a1653e2e

commit dbb3776482cac31616c9309bf727c594a1653e2e
Author: dtapuska <dtapuska@chromium.org>
Date: Fri Dec 09 15:17:19 2016

Ensure Char events sent to pepper indicate the correct timestamp seconds.

Previously the wall clock was being used but really it should be
the realtime clock.

BUG= 625680 

Review-Url: https://codereview.chromium.org/2556353003
Cr-Commit-Position: refs/heads/master@{#437541}

[modify] https://crrev.com/dbb3776482cac31616c9309bf727c594a1653e2e/content/renderer/render_frame_impl.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ceb9f02292f683da9bbb080918423eeda5ce8c95

commit ceb9f02292f683da9bbb080918423eeda5ce8c95
Author: majidvp <majidvp@chromium.org>
Date: Tue Dec 13 17:41:32 2016

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
Cr-Commit-Position: refs/heads/master@{#438211}

[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/CompositionEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/DragEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/DragEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/Event.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/Event.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/EventTarget.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/FocusEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/GestureEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/GestureEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/KeyboardEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/MouseEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/MouseEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/MouseRelatedEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/MouseRelatedEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/TextEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/TouchEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/TouchEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/UIEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/UIEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/UIEventWithKeyState.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/UIEventWithKeyState.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/WheelEvent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/events/WheelEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/html/forms/TypeAhead.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/html/forms/TypeAhead.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/input/EventHandlerTest.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/input/GestureManager.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/input/GestureManager.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/input/MouseEventManager.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/input/MouseEventManager.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/input/TouchEventManager.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/page/AutoscrollController.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/page/AutoscrollController.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/page/ContextMenuControllerTest.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/core/page/DragController.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/modules/webmidi/MIDIMessageEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/platform/PlatformEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/platform/PlatformGestureEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/platform/PlatformMouseEvent.h
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/web/WebFrameWidgetBase.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/web/WebInputEventConversion.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/web/tests/VisualViewportTest.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/web/tests/WebInputEventConversionTest.cpp
[modify] https://crrev.com/ceb9f02292f683da9bbb080918423eeda5ce8c95/third_party/WebKit/Source/wtf/Time.h

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6650abf280f2987af809b0111ad00e5dba5723e4

commit 6650abf280f2987af809b0111ad00e5dba5723e4
Author: majidvp <majidvp@chromium.org>
Date: Thu Dec 22 01:36:33 2016

Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=TimeTest.JsTime

BUG= 625680 

Review-Url: https://codereview.chromium.org/2593073002
Cr-Commit-Position: refs/heads/master@{#440304}

[modify] https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4/base/time/time.cc
[modify] https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4/base/time/time_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ff9ceb9be03477632e2808268882889ff551185

commit 7ff9ceb9be03477632e2808268882889ff551185
Author: majidvp <majidvp@chromium.org>
Date: Thu Jan 26 16:13:42 2017

Revert of base::Time should correctly handle -epoch time values from Javascript (patchset #1 id:1 of https://codereview.chromium.org/2593073002/ )

Reason for revert:
The patched broke EME player which was relying on null time being converted to 0 in FromJsTime.

After discussing this, we agreed to try to change all conversion methods together to ensure consistency. A precursor to doing this will be to get stop treating zero as null.

For more info see:  https://bugs.chromium.org/p/chromium/issues/detail?id=679079#c13

Original issue's description:
> Correctly handle -epoch time values when converting from JS time to base::Time
>
> -11644473600 seconds (which represents windows epoch time of
> |1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
> this value is internally represented by 0 which is mistakenly confused with a
> null time value.
>
> FromJsTime is meant to be used to convert time values coming from Javascript
> for which 0 or -epoch do not represent null values. So the extra check was
> incorrect.
>
> * In fact there is a comment in FromJsTime method making it clear that 0 is a
> valid value but this was missed in ToJsTime method.
>
> TEST=./base_unittests --gtest_filter=TimeTest.JsTime
>
> BUG= 625680 
>
> Committed: https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4
> Cr-Commit-Position: refs/heads/master@{#440304}

TBR=miu@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 625680 

Review-Url: https://codereview.chromium.org/2655233003
Cr-Commit-Position: refs/heads/master@{#446340}

[modify] https://crrev.com/7ff9ceb9be03477632e2808268882889ff551185/base/time/time.cc
[modify] https://crrev.com/7ff9ceb9be03477632e2808268882889ff551185/base/time/time_unittest.cc

Cc: xhw...@chromium.org
Related to this, there's also PP_Time which is also a double, and the conversion from PP_Time to base::Time is also tricky:

https://cs.chromium.org/chromium/src/ppapi/shared_impl/time_conversion.cc?rcl=1485476659&l=11
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 27 2017

Labels: merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ccfd6f67407374d5a23fb98eab6b39d887a0291

commit 2ccfd6f67407374d5a23fb98eab6b39d887a0291
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Fri Jan 27 08:30:15 2017

Revert of base::Time should correctly handle -epoch time values from Javascript (patchset #1 id:1 of https://codereview.chromium.org/2593073002/ )

Reason for revert:
The patched broke EME player which was relying on null time being converted to 0 in FromJsTime.

After discussing this, we agreed to try to change all conversion methods together to ensure consistency. A precursor to doing this will be to get stop treating zero as null.

For more info see:  https://bugs.chromium.org/p/chromium/issues/detail?id=679079#c13

Original issue's description:
> Correctly handle -epoch time values when converting from JS time to base::Time
>
> -11644473600 seconds (which represents windows epoch time of
> |1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
> this value is internally represented by 0 which is mistakenly confused with a
> null time value.
>
> FromJsTime is meant to be used to convert time values coming from Javascript
> for which 0 or -epoch do not represent null values. So the extra check was
> incorrect.
>
> * In fact there is a comment in FromJsTime method making it clear that 0 is a
> valid value but this was missed in ToJsTime method.
>
> TEST=./base_unittests --gtest_filter=TimeTest.JsTime
>
> BUG= 625680 
>
> Committed: https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4
> Cr-Commit-Position: refs/heads/master@{#440304}

TBR=miu@chromium.org,majidvp@chromium.org
BUG= 625680 , 679079 

Review-Url: https://codereview.chromium.org/2655233003
Cr-Commit-Position: refs/heads/master@{#446340}
(cherry picked from commit 7ff9ceb9be03477632e2808268882889ff551185)

Review-Url: https://codereview.chromium.org/2663493002 .
Cr-Commit-Position: refs/branch-heads/2987@{#138}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/2ccfd6f67407374d5a23fb98eab6b39d887a0291/base/time/time.cc
[modify] https://crrev.com/2ccfd6f67407374d5a23fb98eab6b39d887a0291/base/time/time_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0651393fc46e6637ac0a5bc29c816c3f4ee9088

commit a0651393fc46e6637ac0a5bc29c816c3f4ee9088
Author: majidvp <majidvp@chromium.org>
Date: Fri Feb 03 18:06:39 2017

Do not expose is_null API for time values in Blink

base::Time and base::TimeTicks interfaces expose a is_null() method. Unfortunately
in current implementation is_null() == is_zero() which means that it is possible
to write code that produces valid zero time but is confused with null time.
So depending on this API can lead to brittle and buggy code. There is also a desire
to eventually get rid of |is_null()| in base/ interface.

Given above we shouldn't depend on this behavior in Blink. Any code that needs to have
a "null" or "invalid" time value can use WTF::Optional<WTF::Time> instead which
is more readable and explicit.

BUG= 625680 

Review-Url: https://codereview.chromium.org/2656843003
Cr-Commit-Position: refs/heads/master@{#448023}

[modify] https://crrev.com/a0651393fc46e6637ac0a5bc29c816c3f4ee9088/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/a0651393fc46e6637ac0a5bc29c816c3f4ee9088/third_party/WebKit/Source/core/input/GestureManager.cpp
[modify] https://crrev.com/a0651393fc46e6637ac0a5bc29c816c3f4ee9088/third_party/WebKit/Source/core/input/GestureManager.h
[modify] https://crrev.com/a0651393fc46e6637ac0a5bc29c816c3f4ee9088/third_party/WebKit/Source/wtf/Time.h

Cc: bokan@chromium.org
Status: Fixed (was: Started)
Summary: Use better time types for events in Blink (was: Use better time types events in Blink)
I have not done anything further on this issue. But dcheng@ has managed to do the biggest remaining piece which is to have WebInputEvent use proper time types \o/.  See https://chromium-review.googlesource.com/c/chromium/src/+/793050

Marking this as fixed given the above and the fact that the larger effort is
tracked in 763980.


Sign in to add a comment