New issue
Advanced search Search tips

Issue 866930 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Aug 17
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-08-31
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Migrate Blink off of WTF::ScopedMockClock and CurrentTimeTicks()

Project Member Reported by charliea@chromium.org, Jul 24

Issue description

Now that base::TimeTicks() is directly overrideable via ScopedTimeClockOverrides (https://goo.gl/GQqTYW), use of ScopedMockClock and CurrentTimeTicks() isn't necessary anymore. CurrentTimeTicks() is a wrapper around base::TimeTicks() that must be used when retrieving a monotonic time in order for that time to be correctly mocked through WTF::ScopedMockClock. There are instances, though, where we fail to use that wrapper in Blink (https://goo.gl/B42P1P), which results in random patches of code that fail to respect the mock clock but a majority of Blink that does.

Now that base::TimeTicks() is directly mockable via ScopedTimeClockOverrides, though, we can just use base::TimeTicks() directly, kill CurrentTimeTicks(), and perhaps create some new ScopedMockClock-like interface to ScopedTimeClockOverrides.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 9

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

commit 7835120585e31de20dd4e0649530ec9f3591881a
Author: Charlie Andrews <charliea@chromium.org>
Date: Thu Aug 09 03:33:32 2018

Add ScopedMockClockOverride to simplify mocking Now() in tests

This API is similar to the way that overriding is currently done in
Blink (e.g. https://goo.gl/2Fj8sB) and should help pave the way for a
migration off of Blink's ScopedMockClock.

Blink's ScopedMockClock is especially awkward because it relies on code
throughout Blink to use CurrentTimeTicks() instead of TimeTicks::Now(). If
TimeTicks::Now() is used directly (as it is in numerous places
throughout Blink - see https://goo.gl/B42P1P), then the override is
ignored, causing strange behaviors in test.

ScopedTimeClockOverrides is also awkward for the typical case of setting
a global mock clock and periodically advancing it in tests. This new
ScopedMockClockOverride obviates the need for a helper global variable
or function to override Time or TimeTicks in tests.

This solution was discussed with jbroman@ and others in
https://goo.gl/AT4qs5 and I volunteered to take it on.

Bug:  866930 
Change-Id: I14ba5a7eab8a246851477357d34bae7fb4ca5c50
Reviewed-on: https://chromium-review.googlesource.com/1148581
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Commit-Queue: Charlie Andrews <charliea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581776}
[modify] https://crrev.com/7835120585e31de20dd4e0649530ec9f3591881a/base/BUILD.gn
[modify] https://crrev.com/7835120585e31de20dd4e0649530ec9f3591881a/base/task/sequence_manager/lazily_deallocated_deque_unittest.cc
[modify] https://crrev.com/7835120585e31de20dd4e0649530ec9f3591881a/base/test/BUILD.gn
[add] https://crrev.com/7835120585e31de20dd4e0649530ec9f3591881a/base/test/scoped_mock_clock_override.cc
[add] https://crrev.com/7835120585e31de20dd4e0649530ec9f3591881a/base/test/scoped_mock_clock_override.h
[add] https://crrev.com/7835120585e31de20dd4e0649530ec9f3591881a/base/test/scoped_mock_clock_override_unittest.cc

NextAction: 2018-08-31
I'm going to abandon this bug as WontFix. It turns out that this was harder than I thought. A full explanation is available here: https://drive.google.com/u/0/open?id=1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo

For now, I'm going to leave the code because sahel@ believes the code will still be useful to them. I'll set a NextAction date two weeks from now to circle back around and make sure that it actually gets used.
Status: WontFix (was: Assigned)
The NextAction date has arrived: 2018-08-31
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 8

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

commit 9af49803d98a7af9d37aaae8f3f92340868a599b
Author: Adithya Srinivasan <adithyas@chromium.org>
Date: Thu Nov 08 17:02:36 2018

Add note in base::ScopedMockClockOverride documentation

This just duplicates documentation from
base::subtle::ScopedTimeClockOverrides but I thought it was worth adding
it here, as ScopedMockClockOverride doesn't currently fix any of the
limitations.

Also, because of this limitation we can't use ScopedMockClockOverride as-is
in Blink tests. We create a task scheduler and a thread pool before running
tests on the main thread (and also call TimeTicks::Now from one of the
threads created).

Bug:  866930 
Change-Id: If7b081bd980b8ac8780c892c21b082923edd1a29
Reviewed-on: https://chromium-review.googlesource.com/c/1324103
Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606502}
[modify] https://crrev.com/9af49803d98a7af9d37aaae8f3f92340868a599b/base/test/scoped_mock_clock_override.h

Sign in to add a comment