Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
899863002:
Add support in TestMockTimeTaskRunner for vending out mock Time and mock Clocks. (Closed)
|
Created:
5 years, 10 months ago by engedy Modified:
5 years, 10 months ago CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, oshima+watch_chromium.org, kalyank, gcasto+watchlist_chromium.org, erikwright+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support in TestMockTimeTaskRunner for vending mock Time values and mock Clocks.
Previously, TestMockTimeTaskRunner had only supported vending out TimeTicks and TickClocks. Now it supports mock Time and mock Clocks too (they start at the Unix epoch).
This CL also shortens the names of the methods that vend the current time and time ticks to Now() and NowTicks(), respectively; removes using the "base" namespace prefix inside the "base" namespace; and provides a clean implementation for FastForwardUntilNoTasksRemain() instead of a hacky solution.
BUG=329911
TBR=phajdan.jr@chromium.org # OOO.
Committed: https://crrev.com/5cae03fa3f7f836be37d6dfb37cc8b3d7dcb3c3c
Cr-Commit-Position: refs/heads/master@{#316219}
Patch Set 1 : #Patch Set 2 : Nits, remove base:: mentions. #Patch Set 3 : Fix ash_unittests missing header. #
Total comments: 10
Patch Set 4 : Fixed hackery, and nits. #
Total comments: 4
Patch Set 5 : Addressed comments from thestig@. #Patch Set 6 : Phrasing nit. #
Total comments: 4
Patch Set 7 : Fix bug and phrasing. #Patch Set 8 : Fix DCHECK_GT --> DCHECK_GE. #
Total comments: 42
Patch Set 9 : Addressed comments from bartfab@. #Patch Set 10 : Start ticks from 1 us. #
Total comments: 2
Patch Set 11 : Fix typo and TimeTicks initialization. #Patch Set 12 : Start ticks from 0, some tests depend on this. #
Total comments: 4
Messages
Total messages: 36 (11 generated)
engedy@chromium.org changed reviewers: + phajdan.jr@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
engedy@chromium.org changed reviewers: + bartfab@chromium.org, stevenjb@chromium.org, vabr@chromium.org
engedy@chromium.org changed reviewers: + thestig@chromium.org
@Bartosz, could you please review the whole thing, and also components/policy for OWNER's approval? Everyone else, could you please review for OWNER's approval: @Lei: base/test/ (in Pawel's absence) @Vaclav: components/password_manager @Steven: ash/system/chromeos & chrome/browser/chromeos
@Bartosz: This CL results in a change of |test_start_time_| in remote_commands_queue_unittest.cc. It used to be UnixEpoch() - some, and now it will be exactly UnixEpoch(). Seems to work, and I did not see a reason why it might not, but wanted to point out this neverthesless, being the sole functional change to existing code introduced by this CL.
components/password_manager LGTM
https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (left): https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:77: now_ = original_now + delta; Just trying to understand the original code here... Isn't it possible for |now_| to overflow here, causing the time to go backwards? Is the delta.is_max() check suppose to detect overflow? https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:89: TestMockTimeTaskRunner::TestMockTimeTaskRunner(Time intial_time) typo https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:193: now_ += later_ticks - now_ticks_; Can this make time move backwards? In the original code, unless there's an overflow, I think |now_| can only increase. https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:49: TestMockTimeTaskRunner(Time initial_time); Nobody uses this ctor. If you do keep it, add the explicit keyword.
New patchsets have been uploaded after l-g-t-m from vabr@chromium.org
https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (left): https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:77: now_ = original_now + delta; On 2015/02/04 20:27:37, Lei Zhang wrote: > Just trying to understand the original code here... Isn't it possible for |now_| > to overflow here, causing the time to go backwards? Is the delta.is_max() check > suppose to detect overflow? You are correct: it has been, and it is still possible for the time to overflow. I can be convinced otherwise, but I am leaning towards thinking that it should be up to the consumer to make sure this does not happen -- I have added the sufficient condition for achieving this to the class comment. On the other hand, delta.is_max() was a just a big hack so this method can be called with TimeDelta::Max() to get the implementation of FastForwardUntilNoTasksRemain() for free. I have provided a separate implementation without the hack. https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:89: TestMockTimeTaskRunner::TestMockTimeTaskRunner(Time intial_time) On 2015/02/04 20:27:37, Lei Zhang wrote: > typo Removed ctor, thanks for spotting the typo, though! https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:193: now_ += later_ticks - now_ticks_; On 2015/02/04 20:27:37, Lei Zhang wrote: > Can this make time move backwards? In the original code, unless there's an > overflow, I think |now_| can only increase. Hmm, yes, if the consumer of this class posts tasks with negative delays, then time can move backwards -- this could not happen before. I have added an explicit check here to prevent this, and pointed it out in the comment for this method. https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:49: TestMockTimeTaskRunner(Time initial_time); On 2015/02/04 20:27:37, Lei Zhang wrote: > Nobody uses this ctor. If you do keep it, add the explicit keyword. Removed.
The desired behavior of TestMockTimeTaskRunner is easy to understand, but the implementation is a little bit tricky. Several tests depend on TestMockTimeTaskRunner to be correct, so it might be helpful to have some testing to make sure TestMockTimeTaskRunner is doing the right thing. . o O (Who tests the test utilties?) https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (left): https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:77: now_ = original_now + delta; On 2015/02/05 10:22:57, engedy wrote: > On 2015/02/04 20:27:37, Lei Zhang wrote: > > Just trying to understand the original code here... Isn't it possible for > |now_| > > to overflow here, causing the time to go backwards? Is the delta.is_max() > check > > suppose to detect overflow? > > You are correct: it has been, and it is still possible for the time to overflow. > I can be convinced otherwise, but I am leaning towards thinking that it should > be up to the consumer to make sure this does not happen -- I have added the > sufficient condition for achieving this to the class comment. > > On the other hand, delta.is_max() was a just a big hack so this method can be > called with TimeDelta::Max() to get the implementation of > FastForwardUntilNoTasksRemain() for free. I have provided a separate > implementation without the hack. Is is possible to have both FastForwardBy() and FastForwardUntilNoTasksRemain() call some internal function, so you don't need 2 separate, but nearly identical copies, and you don't need the is_max() hack? https://codereview.chromium.org/899863002/diff/100001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/100001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:94: DCHECK(delta >= TimeDelta()); DCHECK_GE() https://codereview.chromium.org/899863002/diff/100001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:203: if (now_ticks_ < later_ticks) { This is always be true now.
@Lei, PTAL. Writing unit tests for this sounds good, but let me do that in a follow up CL to move it off the critical path. https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (left): https://codereview.chromium.org/899863002/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:77: now_ = original_now + delta; On 2015/02/05 10:52:59, Lei Zhang wrote: > On 2015/02/05 10:22:57, engedy wrote: > > On 2015/02/04 20:27:37, Lei Zhang wrote: > > > Just trying to understand the original code here... Isn't it possible for > > |now_| > > > to overflow here, causing the time to go backwards? Is the delta.is_max() > > check > > > suppose to detect overflow? > > > > You are correct: it has been, and it is still possible for the time to > overflow. > > I can be convinced otherwise, but I am leaning towards thinking that it > should > > be up to the consumer to make sure this does not happen -- I have added the > > sufficient condition for achieving this to the class comment. > > > > On the other hand, delta.is_max() was a just a big hack so this method can be > > called with TimeDelta::Max() to get the implementation of > > FastForwardUntilNoTasksRemain() for free. I have provided a separate > > implementation without the hack. > > Is is possible to have both FastForwardBy() and FastForwardUntilNoTasksRemain() > call some internal function, so you don't need 2 separate, but nearly identical > copies, and you don't need the is_max() hack? I went with a slightly less evil version of the old solution. PTAL. Alternatively, we could create an internal implementation that either: * takes a callback to evaluate the exit condition (this could be exposed to consumers), * is a template that takes a functor (this could be exposed to consumers). https://codereview.chromium.org/899863002/diff/100001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/100001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:94: DCHECK(delta >= TimeDelta()); On 2015/02/05 10:52:59, Lei Zhang wrote: > DCHECK_GE() Done. https://codereview.chromium.org/899863002/diff/100001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:203: if (now_ticks_ < later_ticks) { On 2015/02/05 10:52:59, Lei Zhang wrote: > This is always be true now. Ah, sorry, done.
https://codereview.chromium.org/899863002/diff/140001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/140001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:96: TimeTicks original_now_ticks; Do you to initialize this to |now_ticks_| ? https://codereview.chromium.org/899863002/diff/140001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/140001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:119: // all tasks with a remaining delay less than or equal to |max_delta| >= 0, nit: phrasing is still a bit weird. How about: Given a non-negative |max_delta|, runs ...
https://codereview.chromium.org/899863002/diff/140001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/140001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:96: TimeTicks original_now_ticks; On 2015/02/05 18:25:26, Lei Zhang wrote: > Do you to initialize this to |now_ticks_| ? Yes, this was a bug. Actually failed some try bots, too. Fixed. But +1 for having unittests for this. https://codereview.chromium.org/899863002/diff/140001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/140001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:119: // all tasks with a remaining delay less than or equal to |max_delta| >= 0, On 2015/02/05 18:25:26, Lei Zhang wrote: > nit: phrasing is still a bit weird. How about: > > Given a non-negative |max_delta|, runs ... Done. Also one place above.
Thank, lgtm.
On 2015/02/05 18:34:13, Lei Zhang wrote: > Thank, lgtm. Thanks, even!
New patchsets have been uploaded after l-g-t-m from thestig@chromium.org
@Bartosz, friendly ping. In case are busy, feel free to just look at components/policy.
Oops, I forgot to hit send... several days ago :( https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... File ash/system/chromeos/session/logout_confirmation_controller_unittest.cc (right): https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:12: #include "base/time/tick_clock.h" Nit: No need for this include. The type is never dereferenced. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:54: ~MockClock() override; Nit: No need to override the destructor if your new destructor is empty (the same applies to MockTickClock). https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:94: DCHECK_GE(delta, TimeDelta()); Nit 1: Expected value first, actual value second. Nit 2: This will be DCHECK()ed by ProcessAllTasksNoLaterThan() for you. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:96: TimeTicks original_now_ticks = now_ticks_; Nit: const. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:102: ProcessAllTasksNoLaterThan(TimeDelta()); Nit: Add DCHECK(thread_checker_.CalledOnValidThread()). https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:182: DCHECK_GE(max_delta, TimeDelta()); Expected value first, actual value second. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:184: for (;;) { Nit: while (true) is more common. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:187: if (!DequeueNextTask(original_now_ticks, max_delta, &task_info)) Why do you not use this as the loop condition? It used to be: while (X) { ... } Now we have: for (;;) { if (!X) break; ... } They are equivalent but the former uses the language's primitives correctly. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:191: // moving the clock backwards in this case. Why not just weed out negative delays in PostDelayedTask()? https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:36: // - It does not check for overflow when doing time arithmetics. A sufficient As a side note: I do not believe any part of our code cares about time overflows. The Time* classes are meant for dealing with real time on a human lifetime scale. Values in the range of centuries and beyond will not overflow Time*. No test should ever need to advance time by 10^20 years or some such. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:49: // whose time ticks will still start at zero. Starting at zero is problematic. Zero is a special magic value that is often used to mean "not initialized." There is even an TimeTicks::is_null() to test for it. This design of Time and TimeTicks is bad of course - arithmetic with real times can always produce a zero, inadvertently making is_null() return true from now on. Changing the fundamental design of Time and TimeTicks would be a large endeavor. The least we can do to work around it is to start with a non-zero TimeTicks. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:122: // processed task. Pass in TimeDelta::Max() as |max_delta| to run all tasks. It looks like my use of TimeDelta::Max() to indicate "until no more tasks remain" was called an evil hack, removed and re-added in the end? :) https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:126: // the same amount. Calls OnAfterTimePassed() if |now_ticks_| < |later_ticks|. Nit: Could you reverse this to say "|later_ticks| > |now_ticks_|". It is easier to parse and matches the order of variables that the next sentence uses.
https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... File ash/system/chromeos/session/logout_confirmation_controller_unittest.cc (right): https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:12: #include "base/time/tick_clock.h" On 2015/02/12 09:50:55, bartfab wrote: > Nit: No need for this include. The type is never dereferenced. It is needed for passing the scoped_ptr returned by the task runner into SetClockForTesting(). https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:54: ~MockClock() override; On 2015/02/12 09:50:55, bartfab wrote: > Nit: No need to override the destructor if your new destructor is empty (the > same applies to MockTickClock). Done. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:94: DCHECK_GE(delta, TimeDelta()); On 2015/02/12 09:50:55, bartfab wrote: > Nit 1: Expected value first, actual value second. I believe the current order is better. DCHECK makes no distinction between the first and second argument. On failure, it will print: Check failed: delta >= TimeDelta() Which seems to be more in line with prevalent Chromium style that the constant is on the right hand side in comparisons/equivalence tests. > Nit 2: This will be DCHECK()ed by ProcessAllTasksNoLaterThan() for you. I would prefer to also have a DCHECK here. I like it because it formalizes the precondition mentioned in the declaration-side comment that the delta must be non-negative. It also makes life easier for those who misuse this class: as opposed to seeing a DCHECK fail in an internal implementation detail, they see a DCHECK fail in a public method, so it is even more obvious they are using the API incorrectly. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:96: TimeTicks original_now_ticks = now_ticks_; On 2015/02/12 09:50:55, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:102: ProcessAllTasksNoLaterThan(TimeDelta()); On 2015/02/12 09:50:55, bartfab wrote: > Nit: Add DCHECK(thread_checker_.CalledOnValidThread()). Done. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:182: DCHECK_GE(max_delta, TimeDelta()); On 2015/02/12 09:50:55, bartfab wrote: > Expected value first, actual value second. Please see above. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:184: for (;;) { On 2015/02/12 09:50:55, bartfab wrote: > Nit: while (true) is more common. Done. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:187: if (!DequeueNextTask(original_now_ticks, max_delta, &task_info)) On 2015/02/12 09:50:55, bartfab wrote: > Why do you not use this as the loop condition? > > It used to be: > > while (X) { > ... > } > > Now we have: > > for (;;) { > if (!X) > break; > ... > } > > They are equivalent but the former uses the language's primitives correctly. I have restructured so that OnBeforeSelectingTask() need not be repeated. Happy to revert if you feel strongly about it. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:191: // moving the clock backwards in this case. On 2015/02/12 09:50:55, bartfab wrote: > Why not just weed out negative delays in PostDelayedTask()? The contract of TaskRunner does not forbid negative delays, so I would feel bad about suddenly making such requirements that here. (I admit, however, that recently TaskRunners seem to have stopped guaranteeing that delays will be respected.) https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:36: // - It does not check for overflow when doing time arithmetics. A sufficient On 2015/02/12 09:50:55, bartfab wrote: > As a side note: I do not believe any part of our code cares about time > overflows. The Time* classes are meant for dealing with real time on a human > lifetime scale. Values in the range of centuries and beyond will not overflow > Time*. No test should ever need to advance time by 10^20 years or some such. I agree, but Lei was concerned about the topic. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:49: // whose time ticks will still start at zero. On 2015/02/12 09:50:55, bartfab wrote: > Starting at zero is problematic. Zero is a special magic value that is often > used to mean "not initialized." There is even an TimeTicks::is_null() to test > for it. > > This design of Time and TimeTicks is bad of course - arithmetic with real times > can always produce a zero, inadvertently making is_null() return true from now > on. Changing the fundamental design of Time and TimeTicks would be a large > endeavor. The least we can do to work around it is to start with a non-zero > TimeTicks. Done. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:122: // processed task. Pass in TimeDelta::Max() as |max_delta| to run all tasks. On 2015/02/12 09:50:55, bartfab wrote: > It looks like my use of TimeDelta::Max() to indicate "until no more tasks > remain" was called an evil hack, removed and re-added in the end? :) Well, it has been made a wee bit less evil: the contract of FastForwardBy() is now cleaner, as it no longer has this undocumented surprise built in for the handling of TimeDelta::Max(). As a matter of fact, though, I believe your original code did not use this hack, I am afraid I am responsible for that. :) https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:126: // the same amount. Calls OnAfterTimePassed() if |now_ticks_| < |later_ticks|. On 2015/02/12 09:50:55, bartfab wrote: > Nit: Could you reverse this to say "|later_ticks| > |now_ticks_|". It is easier > to parse and matches the order of variables that the next sentence uses. Done.
LGTM with a couple more nits. https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... File ash/system/chromeos/session/logout_confirmation_controller_unittest.cc (right): https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:12: #include "base/time/tick_clock.h" On 2015/02/12 11:27:03, engedy wrote: > On 2015/02/12 09:50:55, bartfab wrote: > > Nit: No need for this include. The type is never dereferenced. > > It is needed for passing the scoped_ptr returned by the task runner into > SetClockForTesting(). You do not need to define the types you pass around via a pointer. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:94: DCHECK_GE(delta, TimeDelta()); On 2015/02/12 11:27:03, engedy wrote: > On 2015/02/12 09:50:55, bartfab wrote: > > Nit 1: Expected value first, actual value second. > > I believe the current order is better. DCHECK makes no distinction between the > first and second argument. On failure, it will print: > > Check failed: > delta >= TimeDelta() > > Which seems to be more in line with prevalent Chromium style that the constant > is on the right hand side in comparisons/equivalence tests. > > > Nit 2: This will be DCHECK()ed by ProcessAllTasksNoLaterThan() for you. > > I would prefer to also have a DCHECK here. I like it because it formalizes the > precondition mentioned in the declaration-side comment that the delta must be > non-negative. It also makes life easier for those who misuse this class: as > opposed to seeing a DCHECK fail in an internal implementation detail, they see a > DCHECK fail in a public method, so it is even more obvious they are using the > API incorrectly. You are right that DCHECK_xx() does not eforce an argument order. But for consistency with the semantically strongly related EXPECT_xx() and ASSERT_xx(), which are sensitive to argument order in the messages they print, it is customary to put the expected value first. Ack on having a DCHECK() here, in the public method. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:182: DCHECK_GE(max_delta, TimeDelta()); On 2015/02/12 11:27:03, engedy wrote: > On 2015/02/12 09:50:55, bartfab wrote: > > Expected value first, actual value second. > > Please see above. As above :). https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:187: if (!DequeueNextTask(original_now_ticks, max_delta, &task_info)) On 2015/02/12 11:27:03, engedy wrote: > On 2015/02/12 09:50:55, bartfab wrote: > > Why do you not use this as the loop condition? > > > > It used to be: > > > > while (X) { > > ... > > } > > > > Now we have: > > > > for (;;) { > > if (!X) > > break; > > ... > > } > > > > They are equivalent but the former uses the language's primitives correctly. > > I have restructured so that OnBeforeSelectingTask() need not be repeated. Happy > to revert if you feel strongly about it. Did you mean that the refactor avoids a spurious call to OnBeforeSelectingTask()? If so, no, it does not. The number and order of calls to this method is unchanged. The only difference is that all calls come from one line instead of two different lines in the code now. I see no harm in the way it was done before but I am fine with the new version as well. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:191: // moving the clock backwards in this case. On 2015/02/12 11:27:03, engedy wrote: > On 2015/02/12 09:50:55, bartfab wrote: > > Why not just weed out negative delays in PostDelayedTask()? > > The contract of TaskRunner does not forbid negative delays, so I would feel bad > about suddenly making such requirements that here. > > (I admit, however, that recently TaskRunners seem to have stopped guaranteeing > that delays will be respected.) AFAICT, they never guaranteed to respect delays.
owner lgtm https://codereview.chromium.org/899863002/diff/220001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/220001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:36: // - It does not check for overflow when doing time arithmetics. A sufficient nit: s/arithmetics/arithmetic/
New patchsets have been uploaded after l-g-t-m from bartfab@chromium.org,stevenjb@chromium.org
https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... File ash/system/chromeos/session/logout_confirmation_controller_unittest.cc (right): https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:12: #include "base/time/tick_clock.h" On 2015/02/12 12:36:07, bartfab wrote: > On 2015/02/12 11:27:03, engedy wrote: > > On 2015/02/12 09:50:55, bartfab wrote: > > > Nit: No need for this include. The type is never dereferenced. > > > > It is needed for passing the scoped_ptr returned by the task runner into > > SetClockForTesting(). > > You do not need to define the types you pass around via a pointer. Note that this is not a raw pointer, but a scoped_ptr. I have not looked into the implementation of scoped_ptr<T> too deeply, but I believe the definition of T is needed so that the code for ~scoped_ptr<T>(), necessitated by the destruction of the temporary on line 41, and containing an invocation to ~T(), can be generated. (I tried, just to be sure, and it won't compile without the include.) https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:94: DCHECK_GE(delta, TimeDelta()); On 2015/02/12 12:36:07, bartfab wrote: > You are right that DCHECK_xx() does not eforce an argument order. But for > consistency with the semantically strongly related EXPECT_xx() and ASSERT_xx(), > which are sensitive to argument order in the messages they print, it is > customary to put the expected value first. Yes, but there is also EXPECT_THAT, which in turn requires the actual value first and the expectation second. So, instead of going into lengthy arguments about which semantical relationships are stronger, let us be consistent with other usages of DCHECK_GE itself in src/base: https://code.google.com/p/chromium/codesearch#search/&q=DCHECK_GE%20package:%... This shows about 40-50 usages; and actually, there is not a single usage with the expectation first. Therefore, I would propose keeping the current order. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:182: DCHECK_GE(max_delta, TimeDelta()); On 2015/02/12 12:36:08, bartfab wrote: > On 2015/02/12 11:27:03, engedy wrote: > > On 2015/02/12 09:50:55, bartfab wrote: > > > Expected value first, actual value second. > > > > Please see above. > > As above :). Please see above, once again. :) https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:187: if (!DequeueNextTask(original_now_ticks, max_delta, &task_info)) On 2015/02/12 12:36:08, bartfab wrote: > On 2015/02/12 11:27:03, engedy wrote: > > On 2015/02/12 09:50:55, bartfab wrote: > > > Why do you not use this as the loop condition? > > > > > > It used to be: > > > > > > while (X) { > > > ... > > > } > > > > > > Now we have: > > > > > > for (;;) { > > > if (!X) > > > break; > > > ... > > > } > > > > > > They are equivalent but the former uses the language's primitives correctly. > > > > I have restructured so that OnBeforeSelectingTask() need not be repeated. > Happy > > to revert if you feel strongly about it. > > Did you mean that the refactor avoids a spurious call to > OnBeforeSelectingTask()? If so, no, it does not. The number and order of calls > to this method is unchanged. The only difference is that all calls come from one > line instead of two different lines in the code now. I see no harm in the way it > was done before but I am fine with the new version as well. Sorry, I was ambiguous. Yes, I wanted to reduce the call sites from 2 to 1, without changing the behavior. Ack. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:191: // moving the clock backwards in this case. On 2015/02/12 12:36:07, bartfab wrote: > On 2015/02/12 11:27:03, engedy wrote: > > On 2015/02/12 09:50:55, bartfab wrote: > > > Why not just weed out negative delays in PostDelayedTask()? > > > > The contract of TaskRunner does not forbid negative delays, so I would feel > bad > > about suddenly making such requirements that here. > > > > (I admit, however, that recently TaskRunners seem to have stopped guaranteeing > > that delays will be respected.) > > AFAICT, they never guaranteed to respect delays. Hmm, you are right. I must have mixed it up with something else. https://codereview.chromium.org/899863002/diff/220001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/220001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:36: // - It does not check for overflow when doing time arithmetics. A sufficient On 2015/02/12 18:26:26, stevenjb wrote: > nit: s/arithmetics/arithmetic/ Done.
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/899863002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/899863002/260001
https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:49: // whose time ticks will still start at zero. On 2015/02/12 11:27:03, engedy wrote: > On 2015/02/12 09:50:55, bartfab wrote: > > Starting at zero is problematic. Zero is a special magic value that is often > > used to mean "not initialized." There is even an TimeTicks::is_null() to test > > for it. > > > > This design of Time and TimeTicks is bad of course - arithmetic with real > times > > can always produce a zero, inadvertently making is_null() return true from now > > on. Changing the fundamental design of Time and TimeTicks would be a large > > endeavor. The least we can do to work around it is to start with a non-zero > > TimeTicks. > > Done. I needed to revert this change, some tests depend on that Ticks will start from 0. Happy to change this in a follow-up CL if you are strongly against this. (Note, though, that this was not introduced in the current CL.)
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/5cae03fa3f7f836be37d6dfb37cc8b3d7dcb3c3c Cr-Commit-Position: refs/heads/master@{#316219}
Message was sent while issue was closed.
Still LGTM. https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... File ash/system/chromeos/session/logout_confirmation_controller_unittest.cc (right): https://codereview.chromium.org/899863002/diff/180001/ash/system/chromeos/ses... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:12: #include "base/time/tick_clock.h" On 2015/02/13 11:08:04, engedy wrote: > On 2015/02/12 12:36:07, bartfab wrote: > > On 2015/02/12 11:27:03, engedy wrote: > > > On 2015/02/12 09:50:55, bartfab wrote: > > > > Nit: No need for this include. The type is never dereferenced. > > > > > > It is needed for passing the scoped_ptr returned by the task runner into > > > SetClockForTesting(). > > > > You do not need to define the types you pass around via a pointer. > > Note that this is not a raw pointer, but a scoped_ptr. > > I have not looked into the implementation of scoped_ptr<T> too deeply, but I > believe the definition of T is needed so that the code for ~scoped_ptr<T>(), > necessitated by the destruction of the temporary on line 41, and containing an > invocation to ~T(), can be generated. > > (I tried, just to be sure, and it won't compile without the include.) Aye, your analysis is correct. I had missed the fact that this is a scoped_ptr. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:94: DCHECK_GE(delta, TimeDelta()); On 2015/02/13 11:08:05, engedy wrote: > On 2015/02/12 12:36:07, bartfab wrote: > > You are right that DCHECK_xx() does not eforce an argument order. But for > > consistency with the semantically strongly related EXPECT_xx() and > ASSERT_xx(), > > which are sensitive to argument order in the messages they print, it is > > customary to put the expected value first. > > Yes, but there is also EXPECT_THAT, which in turn requires the actual value > first and the expectation second. So, instead of going into lengthy arguments > about which semantical relationships are stronger, let us be consistent with > other usages of DCHECK_GE itself in src/base: > > https://code.google.com/p/chromium/codesearch#search/&q=DCHECK_GE%20package:%... > > This shows about 40-50 usages; and actually, there is not a single usage with > the expectation first. Therefore, I would propose keeping the current order. Acknowledged. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:182: DCHECK_GE(max_delta, TimeDelta()); On 2015/02/13 11:08:05, engedy wrote: > On 2015/02/12 12:36:08, bartfab wrote: > > On 2015/02/12 11:27:03, engedy wrote: > > > On 2015/02/12 09:50:55, bartfab wrote: > > > > Expected value first, actual value second. > > > > > > Please see above. > > > > As above :). > > Please see above, once again. :) Acknowledged. https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:49: // whose time ticks will still start at zero. On 2015/02/13 13:07:49, engedy wrote: > On 2015/02/12 11:27:03, engedy wrote: > > On 2015/02/12 09:50:55, bartfab wrote: > > > Starting at zero is problematic. Zero is a special magic value that is often > > > used to mean "not initialized." There is even an TimeTicks::is_null() to > test > > > for it. > > > > > > This design of Time and TimeTicks is bad of course - arithmetic with real > > times > > > can always produce a zero, inadvertently making is_null() return true from > now > > > on. Changing the fundamental design of Time and TimeTicks would be a large > > > endeavor. The least we can do to work around it is to start with a non-zero > > > TimeTicks. > > > > Done. > > I needed to revert this change, some tests depend on that Ticks will start from > 0. Happy to change this in a follow-up CL if you are strongly against this. > (Note, though, that this was not introduced in the current CL.) SGTM. The correct solution is to get rid of is_null() and the magical meaning assigned to the zero value. This is actually being discussed right now, so I have hope that it will be fixed eventually. https://codereview.chromium.org/899863002/diff/260001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/260001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:79: : now_(Time::UnixEpoch()) { Nit: Why break this line? It fits on one line. https://codereview.chromium.org/899863002/diff/260001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/260001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:36: // - It does not check for overflow when doing time arithmetic``. A sufficient What are the two grave accents for?
Message was sent while issue was closed.
https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/180001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:49: // whose time ticks will still start at zero. On 2015/02/16 13:40:34, bartfab wrote: > On 2015/02/13 13:07:49, engedy wrote: > > On 2015/02/12 11:27:03, engedy wrote: > > > On 2015/02/12 09:50:55, bartfab wrote: > > > > Starting at zero is problematic. Zero is a special magic value that is > often > > > > used to mean "not initialized." There is even an TimeTicks::is_null() to > > test > > > > for it. > > > > > > > > This design of Time and TimeTicks is bad of course - arithmetic with real > > > times > > > > can always produce a zero, inadvertently making is_null() return true from > > now > > > > on. Changing the fundamental design of Time and TimeTicks would be a large > > > > endeavor. The least we can do to work around it is to start with a > non-zero > > > > TimeTicks. > > > > > > Done. > > > > I needed to revert this change, some tests depend on that Ticks will start > from > > 0. Happy to change this in a follow-up CL if you are strongly against this. > > (Note, though, that this was not introduced in the current CL.) > > SGTM. The correct solution is to get rid of is_null() and the magical meaning > assigned to the zero value. This is actually being discussed right now, so I > have hope that it will be fixed eventually. Awesome, glad to hear that! https://codereview.chromium.org/899863002/diff/260001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/899863002/diff/260001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:79: : now_(Time::UnixEpoch()) { On 2015/02/16 13:40:34, bartfab wrote: > Nit: Why break this line? It fits on one line. Done in https://codereview.chromium.org/929923003. https://codereview.chromium.org/899863002/diff/260001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/899863002/diff/260001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:36: // - It does not check for overflow when doing time arithmetic``. A sufficient On 2015/02/16 13:40:34, bartfab wrote: > What are the two grave accents for? Uhm. Typo. Fixed in https://codereview.chromium.org/929923003. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
