Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
296043005:
cc: Add UniqueNotifier to cc/base. (Closed)
|
Created:
6 years, 7 months ago by vmpstr Modified:
6 years, 6 months ago Reviewers:
reveman CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Add UniqueNotifier to cc/base.
This patch adds a unique notifier, a class that supports scheduling a
callback. It also supports calling additional schedules which
effectively cancel a previous callback and reschedule a new one.
Both version with delayed tasks and immediate tasks are supported.
R=reveman
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273433
Patch Set 1 #Patch Set 2 : #Patch Set 3 : typo #
Total comments: 6
Patch Set 4 : split delayed #
Total comments: 5
Patch Set 5 : update #
Total comments: 2
Patch Set 6 : update #
Total comments: 1
Patch Set 7 : add cancel #
Total comments: 8
Patch Set 8 : update #
Total comments: 11
Patch Set 9 : #Patch Set 10 : #
Total comments: 7
Patch Set 11 : #Patch Set 12 : update #Patch Set 13 : #
Total comments: 4
Patch Set 14 : update #
Messages
Total messages: 29 (0 generated)
Please take a look when you have time. I was kind of working with the assumption that we can use this instead of PBRWP::ScheduleCheckForCompletedRasterTasks, as well as TileManager::ScheduleCheckReadyToActivate. So, if you see any issue where it would not be usable in those situations, please let me know.
It would be nice if we could also use this in ThreadProxy::RenewTreePriority. https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.cc File cc/base/unique_notifier.cc (right): https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.... cc/base/unique_notifier.cc:26: weak_ptr_factory_.InvalidateWeakPtrs(); No need to do this explicitly. I think it's pretty clear that this will happen implicitly. https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.h File cc/base/unique_notifier.h (right): https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.... cc/base/unique_notifier.h:23: UniqueNotifier(const scoped_refptr<base::SequencedTaskRunner>& task_runner, no need to pass ownership here, base::SequencedTaskRunner* please https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.... cc/base/unique_notifier.h:25: const base::TimeDelta& delay); I would prefer if we had two classes (UniqueNotifier, DelayedUniqueNotifier) instead of special casing delay=0. I think this will both make the usage and the implementation a bit cleaner. Maybe DelayedUniqueNotifier could derive from UniqueNotifier class and Schedule() could be virtual.
PTAL https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.cc File cc/base/unique_notifier.cc (right): https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.... cc/base/unique_notifier.cc:26: weak_ptr_factory_.InvalidateWeakPtrs(); On 2014/05/21 17:03:37, reveman wrote: > No need to do this explicitly. I think it's pretty clear that this will happen > implicitly. Done. https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.h File cc/base/unique_notifier.h (right): https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.... cc/base/unique_notifier.h:23: UniqueNotifier(const scoped_refptr<base::SequencedTaskRunner>& task_runner, On 2014/05/21 17:03:37, reveman wrote: > no need to pass ownership here, base::SequencedTaskRunner* please Done. https://codereview.chromium.org/296043005/diff/40001/cc/base/unique_notifier.... cc/base/unique_notifier.h:25: const base::TimeDelta& delay); On 2014/05/21 17:03:37, reveman wrote: > I would prefer if we had two classes (UniqueNotifier, DelayedUniqueNotifier) > instead of special casing delay=0. I think this will both make the usage and the > implementation a bit cleaner. > > Maybe DelayedUniqueNotifier could derive from UniqueNotifier class and > Schedule() could be virtual. I don't think deriving from each other would make much sense, since the classes wouldn't share much implementation aside from the conceptual thing they do. I'll split this into two different classes for now. If in the future we want to pass a delayed unique notifier pointer as a unique notifier, changing it to a hierarchy should be straightforward.
https://codereview.chromium.org/296043005/diff/50001/cc/base/unique_notifier_... File cc/base/unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/50001/cc/base/unique_notifier_... cc/base/unique_notifier_unittest.cc:39: run_loop_->Run(); I think you can replace all this with: base::RunLoop().RunUntilIdle(); and remove both task_runner_ and run_loop_ from this class. https://codereview.chromium.org/296043005/diff/50001/cc/base/unique_notifier_... cc/base/unique_notifier_unittest.cc:50: notifier_ = make_scoped_ptr(new UniqueNotifier( Can you put this on the stack instead and remove UniqueNotifierTest::notifier_?
PTAL https://codereview.chromium.org/296043005/diff/50001/cc/base/unique_notifier_... File cc/base/unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/50001/cc/base/unique_notifier_... cc/base/unique_notifier_unittest.cc:39: run_loop_->Run(); On 2014/05/21 18:08:31, reveman wrote: > I think you can replace all this with: > base::RunLoop().RunUntilIdle(); > and remove both task_runner_ and run_loop_ from this class. Done. https://codereview.chromium.org/296043005/diff/50001/cc/base/unique_notifier_... cc/base/unique_notifier_unittest.cc:50: notifier_ = make_scoped_ptr(new UniqueNotifier( On 2014/05/21 18:08:31, reveman wrote: > Can you put this on the stack instead and remove UniqueNotifierTest::notifier_? Done.
https://codereview.chromium.org/296043005/diff/50001/cc/base/unique_notifier_... File cc/base/unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/50001/cc/base/unique_notifier_... cc/base/unique_notifier_unittest.cc:70: notifier_->Schedule(); I liked this part of test. Could you keep it in latest patch by having the notifier go out of scope here? https://codereview.chromium.org/296043005/diff/70001/cc/base/delayed_unique_n... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/70001/cc/base/delayed_unique_n... cc/base/delayed_unique_notifier_unittest.cc:16: class DelayedUniqueNotifierTest : public testing::Test { Please make similar adjustments to this class as what you've done to UniqueNotifierTest in latest patch. https://codereview.chromium.org/296043005/diff/70001/cc/base/unique_notifier_... File cc/base/unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/70001/cc/base/unique_notifier_... cc/base/unique_notifier_unittest.cc:35: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; nit: remove this and just use base::MessageLoopProxy::current() directly when creating a notifier.
PTAL.
https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... cc/base/delayed_unique_notifier_unittest.cc:28: void RunForTime(base::TimeDelta delay) { RunLoop::RunUntilIdle() doesn't wait for delayed tasks to run?
On 2014/05/21 20:01:56, reveman wrote: > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > File cc/base/delayed_unique_notifier_unittest.cc (right): > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > cc/base/delayed_unique_notifier_unittest.cc:28: void RunForTime(base::TimeDelta > delay) { > RunLoop::RunUntilIdle() doesn't wait for delayed tasks to run? It does, but in the test I want to make sure that it's not triggered after 0.5 seconds, etc. So I want to be in more control over how long to run for in this case.
On 2014/05/21 20:02:51, vmpstr wrote: > On 2014/05/21 20:01:56, reveman wrote: > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > File cc/base/delayed_unique_notifier_unittest.cc (right): > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > cc/base/delayed_unique_notifier_unittest.cc:28: void > RunForTime(base::TimeDelta > > delay) { > > RunLoop::RunUntilIdle() doesn't wait for delayed tasks to run? > > It does, but in the test I want to make sure that it's not triggered after 0.5 > seconds, etc. So I want to be in more control over how long to run for in this > case. Ah, I see what you're doing now. What if the kernel decides to deschedule you for 2 seconds, will these tests still pass/fail as expected?
On 2014/05/21 20:15:01, reveman wrote: > On 2014/05/21 20:02:51, vmpstr wrote: > > On 2014/05/21 20:01:56, reveman wrote: > > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > > File cc/base/delayed_unique_notifier_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > > cc/base/delayed_unique_notifier_unittest.cc:28: void > > RunForTime(base::TimeDelta > > > delay) { > > > RunLoop::RunUntilIdle() doesn't wait for delayed tasks to run? > > > > It does, but in the test I want to make sure that it's not triggered after 0.5 > > seconds, etc. So I want to be in more control over how long to run for in this > > case. > > Ah, I see what you're doing now. What if the kernel decides to deschedule you > for 2 seconds, will these tests still pass/fail as expected? Heh... I'm descheduled between Schedule and RunForTime for long enough, then it won't pass :\ That's kinda why I picked 1 second and half a second as the delay, trying to eliminate any effect de-schedules have. I'm not sure I know of a better way to test this though. Any suggestions?
On 2014/05/21 20:17:23, vmpstr wrote: > On 2014/05/21 20:15:01, reveman wrote: > > On 2014/05/21 20:02:51, vmpstr wrote: > > > On 2014/05/21 20:01:56, reveman wrote: > > > > > > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > > > File cc/base/delayed_unique_notifier_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > > > cc/base/delayed_unique_notifier_unittest.cc:28: void > > > RunForTime(base::TimeDelta > > > > delay) { > > > > RunLoop::RunUntilIdle() doesn't wait for delayed tasks to run? > > > > > > It does, but in the test I want to make sure that it's not triggered after > 0.5 > > > seconds, etc. So I want to be in more control over how long to run for in > this > > > case. > > > > Ah, I see what you're doing now. What if the kernel decides to deschedule you > > for 2 seconds, will these tests still pass/fail as expected? > > Heh... I'm descheduled between Schedule and RunForTime for long enough, then it > won't pass :\ That's kinda why I picked 1 second and half a second as the delay, > trying to eliminate any effect de-schedules have. I'm not sure I know of a > better way to test this though. Any suggestions? Can you change the tests so they instead check what the current time is when these callbacks fire? Making sure it's not less than the requested delay..
On 2014/05/21 22:18:24, reveman wrote: > On 2014/05/21 20:17:23, vmpstr wrote: > > On 2014/05/21 20:15:01, reveman wrote: > > > On 2014/05/21 20:02:51, vmpstr wrote: > > > > On 2014/05/21 20:01:56, reveman wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > > > > File cc/base/delayed_unique_notifier_unittest.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > > > > cc/base/delayed_unique_notifier_unittest.cc:28: void > > > > RunForTime(base::TimeDelta > > > > > delay) { > > > > > RunLoop::RunUntilIdle() doesn't wait for delayed tasks to run? > > > > > > > > It does, but in the test I want to make sure that it's not triggered after > > 0.5 > > > > seconds, etc. So I want to be in more control over how long to run for in > > this > > > > case. > > > > > > Ah, I see what you're doing now. What if the kernel decides to deschedule > you > > > for 2 seconds, will these tests still pass/fail as expected? > > > > Heh... I'm descheduled between Schedule and RunForTime for long enough, then > it > > won't pass :\ That's kinda why I picked 1 second and half a second as the > delay, > > trying to eliminate any effect de-schedules have. I'm not sure I know of a > > better way to test this though. Any suggestions? > > Can you change the tests so they instead check what the current time is when > these callbacks fire? Making sure it's not less than the requested delay.. That would work for the schedule case, but not for the reschedule case. I would still have to wait somehow and reschedule again. Sleep? then that doesn't give the run loop a chance to run in case it erroneously wants to notify early...
On 2014/05/21 22:21:43, vmpstr wrote: > On 2014/05/21 22:18:24, reveman wrote: > > On 2014/05/21 20:17:23, vmpstr wrote: > > > On 2014/05/21 20:15:01, reveman wrote: > > > > On 2014/05/21 20:02:51, vmpstr wrote: > > > > > On 2014/05/21 20:01:56, reveman wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > > > > > File cc/base/delayed_unique_notifier_unittest.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/296043005/diff/90001/cc/base/delayed_unique_n... > > > > > > cc/base/delayed_unique_notifier_unittest.cc:28: void > > > > > RunForTime(base::TimeDelta > > > > > > delay) { > > > > > > RunLoop::RunUntilIdle() doesn't wait for delayed tasks to run? > > > > > > > > > > It does, but in the test I want to make sure that it's not triggered > after > > > 0.5 > > > > > seconds, etc. So I want to be in more control over how long to run for > in > > > this > > > > > case. > > > > > > > > Ah, I see what you're doing now. What if the kernel decides to deschedule > > you > > > > for 2 seconds, will these tests still pass/fail as expected? > > > > > > Heh... I'm descheduled between Schedule and RunForTime for long enough, then > > it > > > won't pass :\ That's kinda why I picked 1 second and half a second as the > > delay, > > > trying to eliminate any effect de-schedules have. I'm not sure I know of a > > > better way to test this though. Any suggestions? > > > > Can you change the tests so they instead check what the current time is when > > these callbacks fire? Making sure it's not less than the requested delay.. > > That would work for the schedule case, but not for the reschedule case. I would > still have to wait somehow and reschedule again. Sleep? then that doesn't give > the run loop a chance to run in case it erroneously wants to notify early... btw, I added a Cancel to the delayed check, since PBRWP requires that, so PTAL.
I think you need to remove/replace the unit tests that depend on timers. You can instrument the the task runner by creating some client interface if you feel it's necessary to verify the implementation of the class in that way. I'm not sure it's worth much though as the task runner used for non tests is not going to give you any of these guarantees so it's not something you can provide to the user of this class anyhow. https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier.cc (right): https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier.cc:43: weak_ptr_factory_.InvalidateWeakPtrs(); This is going to be a problem as calling Schedule() and Cancel() repeatably will cause the message pump task queue to fill up. Maybe you can set next_notification_time_ to base::TimeTicks() instead. https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier.h (right): https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier.h:31: // pending, then it will happen in given delay from now. That is, if delay is happen in (at least) given delay from now. https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier.h:33: // in 6ms), then calling schedule will ensure that the only notification that s/in 6ms/in no less than 6ms/ https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier.h:34: // arrives will happen in 16ms from now. in (at least) 16ms from now.
PTAL https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier.cc (right): https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier.cc:43: weak_ptr_factory_.InvalidateWeakPtrs(); On 2014/05/22 23:24:28, reveman wrote: > This is going to be a problem as calling Schedule() and Cancel() repeatably will > cause the message pump task queue to fill up. Maybe you can set > next_notification_time_ to base::TimeTicks() instead. Makes sense. Done. Now it should only ever have one task in the task queue. https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier.h (right): https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier.h:31: // pending, then it will happen in given delay from now. That is, if delay is On 2014/05/22 23:24:28, reveman wrote: > happen in (at least) given delay from now. Done. https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier.h:33: // in 6ms), then calling schedule will ensure that the only notification that On 2014/05/22 23:24:28, reveman wrote: > s/in 6ms/in no less than 6ms/ Done. https://codereview.chromium.org/296043005/diff/110001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier.h:34: // arrives will happen in 16ms from now. On 2014/05/22 23:24:28, reveman wrote: > in (at least) 16ms from now. Done. https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:36: void RunForTime(base::TimeDelta delay) { Note that base::RunLoop().RunUntilIdle() doesn't seem to handle delayed tasks, so this is still needed. https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:50: base::TimeDelta run_delay = base::TimeDelta::FromMilliseconds(600); This is a delay just so if we schedule on the same instance as we run for time, the order is determined. I believe this is test is now thread scheduling independent, but please take a close look to see if you can spot some cases where I might be wrong. https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:91: for (int i = 0; i < 100000; ++i) { I could do some sort of a sleep here (as long as I don't let the runloop run). Let me know if you prefer that, but I don't really like having sleep in tests. This might be an exception.
https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:36: void RunForTime(base::TimeDelta delay) { On 2014/05/27 18:14:59, vmpstr wrote: > Note that base::RunLoop().RunUntilIdle() doesn't seem to handle delayed tasks, > so this is still needed. How about changing Notify() to quit the message loop when notification_count_ reached expected_notification_count_? This function becomes "void Run()" and it can post a 5 sec delayed task that should never run under normal circumstances but makes sure the test doesn't lock up completely when things are so broken that notifications never happen. This way we're not wasting time waiting past the last notification. https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:49: base::TimeDelta delay = base::TimeDelta::FromMilliseconds(500); Could this delay be 0 or TimeDelta::FromInternalValue(1)? Half a second is a long time to wait for this test to run. Ideally it wouldn't rely on a real delay at all. https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:50: base::TimeDelta run_delay = base::TimeDelta::FromMilliseconds(600); On 2014/05/27 18:14:59, vmpstr wrote: > This is a delay just so if we schedule on the same instance as we run for time, > the order is determined. > > I believe this is test is now thread scheduling independent, but please take a > close look to see if you can spot some cases where I might be wrong. My suggestion above would allow you to remove the run_delay part. Please consider that. https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:91: for (int i = 0; i < 100000; ++i) { On 2014/05/27 18:14:59, vmpstr wrote: > I could do some sort of a sleep here (as long as I don't let the runloop run). > Let me know if you prefer that, but I don't really like having sleep in tests. > This might be an exception. 100000 seems a bit random. Maybe we can just busy loop until TimeTicks::Now() is some amount greater than the initial value instead?
PTAL https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:36: void RunForTime(base::TimeDelta delay) { On 2014/05/27 19:02:17, reveman wrote: > On 2014/05/27 18:14:59, vmpstr wrote: > > Note that base::RunLoop().RunUntilIdle() doesn't seem to handle delayed tasks, > > so this is still needed. > > How about changing Notify() to quit the message loop when notification_count_ > reached expected_notification_count_? This function becomes "void Run()" and it > can post a 5 sec delayed task that should never run under normal circumstances > but makes sure the test doesn't lock up completely when things are so broken > that notifications never happen. > > This way we're not wasting time waiting past the last notification. Done. https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:49: base::TimeDelta delay = base::TimeDelta::FromMilliseconds(500); On 2014/05/27 19:02:17, reveman wrote: > Could this delay be 0 or TimeDelta::FromInternalValue(1)? Half a second is a > long time to wait for this test to run. Ideally it wouldn't rely on a real delay > at all. Set to to FromInternalValue(1) https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:50: base::TimeDelta run_delay = base::TimeDelta::FromMilliseconds(600); On 2014/05/27 19:02:17, reveman wrote: > On 2014/05/27 18:14:59, vmpstr wrote: > > This is a delay just so if we schedule on the same instance as we run for > time, > > the order is determined. > > > > I believe this is test is now thread scheduling independent, but please take a > > close look to see if you can spot some cases where I might be wrong. > > My suggestion above would allow you to remove the run_delay part. Please > consider that. Done. https://codereview.chromium.org/296043005/diff/130001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:91: for (int i = 0; i < 100000; ++i) { On 2014/05/27 19:02:17, reveman wrote: > On 2014/05/27 18:14:59, vmpstr wrote: > > I could do some sort of a sleep here (as long as I don't let the runloop run). > > Let me know if you prefer that, but I don't really like having sleep in tests. > > This might be an exception. > > 100000 seems a bit random. Maybe we can just busy loop until TimeTicks::Now() is > some amount greater than the initial value instead? Done.
Please add a test with delay=0. I'm still wondering if we shouldn't add a simple client interface that we can mock or some "virtual for testing" functions instead of trying to test this with a real message loop. Wdyt? https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:45: // Pump through the remained or messages. nit: I don't understand this comment https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:46: base::RunLoop().RunUntilIdle(); Why do we need this? I assume run_loop_.reset() doesn't work. https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:56: TEST_F(DelayedUniqueNotifierTest, HalfSecondDelay) { HalfSecondDelay is not accurate anymore https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:144: } nit: blank line here
On 2014/05/27 21:20:24, reveman wrote: > Please add a test with delay=0. Done. > I'm still wondering if we shouldn't add a simple > client interface that we can mock or some "virtual for testing" functions > instead of trying to test this with a real message loop. Wdyt? > Do you mean creating our own run loop type of thing? I honestly don't think it's worth the effort here. I mean we could just add ForTesting functions to the delayed notifier itself and just ensure that notification is pending and that next run time is correct. To be totally honest, it's not that complicated of a class. I just figured testing behavior is better than testing internal state, but we could do the latter instead.
PTAL. https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:45: // Pump through the remained or messages. On 2014/05/27 21:20:24, reveman wrote: > nit: I don't understand this comment Changed it a bit. The problem is that if we erroneously schedule two runs with 0 delay, then run loop will exit after the first run and the second run would still be in queue, thus the test would pass when it shouldn't have. https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:56: TEST_F(DelayedUniqueNotifierTest, HalfSecondDelay) { On 2014/05/27 21:20:24, reveman wrote: > HalfSecondDelay is not accurate anymore Oops. Changed. https://codereview.chromium.org/296043005/diff/170001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:144: } On 2014/05/27 21:20:24, reveman wrote: > nit: blank line here Done.
On 2014/05/27 21:50:31, vmpstr wrote: > On 2014/05/27 21:20:24, reveman wrote: > > Please add a test with delay=0. > > Done. > > > I'm still wondering if we shouldn't add a simple > > client interface that we can mock or some "virtual for testing" functions > > instead of trying to test this with a real message loop. Wdyt? > > > > Do you mean creating our own run loop type of thing? I honestly don't think it's > worth the effort here. I mean we could just add ForTesting functions to the > delayed notifier itself and just ensure that notification is pending and that > next run time is correct. To be totally honest, it's not that complicated of a > class. I just figured testing behavior is better than testing internal state, > but we could do the latter instead. I was thinking you could use base::TestSimpleTaskRunner and add: // Virtual for testing. virtual base::TimeTicks Now() const; to the DelayedUniqueNotifier class. You can then override this in a derived FakeDelayedUniqueNotifier class, which also has a SetNow() function. Similar to what we're doing to test DelayBasedTimeSource class. It feels like something is wrong when our unit tests rely on system time and/or require a minimum amount of time to run.
PTAL. The test now doesn't use either the run loop or real time.
PTAL. The test now doesn't use either the run loop or real time.
thanks! lgtm with one small test request https://codereview.chromium.org/296043005/diff/230001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/230001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:25: virtual base::TimeTicks Now() const OVERRIDE { return now_; } nit: Add // Overridden from DelayedUniqueNotifier: https://codereview.chromium.org/296043005/diff/230001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:194: TEST_F(DelayedUniqueNotifierTest, Cancel) { As part of this test, can you add a simple check to ensure that repeatedly calling Schedule() and Cancel() doesn't cause more pending tasks to be added.
https://codereview.chromium.org/296043005/diff/230001/cc/base/delayed_unique_... File cc/base/delayed_unique_notifier_unittest.cc (right): https://codereview.chromium.org/296043005/diff/230001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:25: virtual base::TimeTicks Now() const OVERRIDE { return now_; } On 2014/05/28 19:50:02, reveman wrote: > nit: Add // Overridden from DelayedUniqueNotifier: Done. https://codereview.chromium.org/296043005/diff/230001/cc/base/delayed_unique_... cc/base/delayed_unique_notifier_unittest.cc:194: TEST_F(DelayedUniqueNotifierTest, Cancel) { On 2014/05/28 19:50:02, reveman wrote: > As part of this test, can you add a simple check to ensure that repeatedly > calling Schedule() and Cancel() doesn't cause more pending tasks to be added. Done.
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/296043005/240001
Message was sent while issue was closed.
Change committed as 273433 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
