Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
823143004:
Add TestMockTimeTaskRunner in base/test. (Closed)
|
Created:
5 years, 11 months ago by engedy Modified:
5 years, 11 months ago CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, oshima+watch_chromium.org, kalyank, erikwright+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd TestMockTimeTaskRunner in base/test.
Multiple implementations of this functionality exist with slight modifications. This CL consolidates them.
BUG=329911
TBR=thestig@chromium.org # For base/base.gyp changes.
Committed: https://crrev.com/8d897fb17470c526155b2a9e86752be9a7f33c03
Cr-Commit-Position: refs/heads/master@{#311091}
Patch Set 1 #Patch Set 2 : Add synchronization for PostTask(). #
Total comments: 76
Patch Set 3 : Addressed comments from bartfab@. #
Total comments: 14
Patch Set 4 : Addressed comments from stevenjb@. #
Total comments: 4
Patch Set 5 : Update year in copyright notice. Rebase. #
Messages
Total messages: 27 (7 generated)
engedy@chromium.org changed reviewers: + bartfab@chromium.org
@Bartosz, I am about to use this in a 4th location, so this seems to be as good a time as any to consolidate the implementations. Being the original author, could you please do an initial pass? There are still 2 major issues with the implementation: 1.) The class name "TestMockTimeTaskRunner" is terrible (and other classes' names could use some more work as well). I am open to suggestions. 2.) The AutomaticRebootManager tests seem to post to the task runner from a worker thread pool. How difficult do you think it would be to avoid this? Or should we make the implementation somewhat thread-safe?
My take on the two open questions: 1) I think the name is not that bad. After all, I came up with |MockTimeSingleThreadTaskRunner| and I did not think it was horrible :). 2) My preference would be to add locking in the methods that access the task queue. I could rework the AutomaticRebootManager tests but making the utility class thread safe will probably open up more use cases in the future.
On 2015/01/07 17:55:42, bartfab wrote: > My take on the two open questions: > > 1) I think the name is not that bad. After all, I came up with > |MockTimeSingleThreadTaskRunner| and I did not think it was horrible :). Did not mean to criticize that :-). I just wanted to make it more consistent with TestSimpleTaskRunner, and in the end it just did not feel right. But happy to hear that it might not be so terrible after all. > 2) My preference would be to add locking in the methods that access the task > queue. I could rework the AutomaticRebootManager tests but making the utility > class thread safe will probably open up more use cases in the future. Sounds good, I will implement this tomorrow.
On 2015/01/07 18:02:15, engedy wrote: > On 2015/01/07 17:55:42, bartfab wrote: > > My take on the two open questions: > > > > 1) I think the name is not that bad. After all, I came up with > > |MockTimeSingleThreadTaskRunner| and I did not think it was horrible :). > > Did not mean to criticize that :-). I just wanted to make it more consistent > with TestSimpleTaskRunner, and in the end it just did not feel right. But happy > to hear that it might not be so terrible after all. > > > 2) My preference would be to add locking in the methods that access the task > > queue. I could rework the AutomaticRebootManager tests but making the utility > > class thread safe will probably open up more use cases in the future. > > Sounds good, I will implement this tomorrow. Done, please take a look.
LGTM. Thanks for taking care of this long-standing clean-up. I will reassign the bug to you so that you can take all the fame for fixing it :). https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... File ash/system/chromeos/session/logout_confirmation_controller_unittest.cc (right): https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:7: #include <queue> Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:8: #include <utility> Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:9: #include <vector> Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:13: #include "base/compiler_specific.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:14: #include "base/location.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:16: #include "base/single_thread_task_runner.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:19: #include "base/time/tick_clock.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:7: #include "base/callback.h" Nit: Do you actually need this? You do not seem to dereference any callbacks or closures. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:15: class CorrespondingMockTickClock : public TickClock { Nit: I am not sure "Corresponding" is a useful part of the class name here. Corresponding to what? https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:18: scoped_refptr<const TestMockTimeTaskRunner> task_runner); Nit: #include "base/memory/ref_counted.h" https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:57: DCHECK(thread_checker_.CalledOnValidThread()); Nit: #include "base/logging.h" https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:61: base::TimeTicks original_now = now_; Nit: const. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:110: DCHECK(thread_checker_.CalledOnValidThread()); Nit: You could actually trivially make this method work correctly on all threads: return thread_checker_.CalledOnValidThread(); https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:11: #include "base/memory/ref_counted.h" Nit: Not used. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:41: // Fast forwards virtual time by |delta|, causing all tasks with a remaining Nit: s/Fast forwards/Fast-forwards/ https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:42: // delay of less than or equal to |delta| to be executed. Nit: s/of // https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:45: // Fast forwards virtual time just until all tasks are executed. Nit: s/Fast forwards/Fast-forwards/ https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:48: // Executes all tasks having no remaining delay. No virtual time will elapse. Nit: Could you add one more sentence which explicitly states that delayed tasks will remain enqueued? https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:77: // Called after the current mock time had been incremented so that subclasses Nit: s/had/has/ https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:79: virtual void OnAfterTimePasses(); Nit: s/Passes/Passed/ https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:82: // activities, e.g., pumping out tasks from additional task runners. Nit: s/pumping out tasks from/pump/ https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:83: virtual void OnAfterRunningTask(); Nit: s/RunningTask/TaskRun/ https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:94: std::vector<TestPendingTask>, Nit: #include <vector> https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:104: base::ThreadChecker thread_checker_; Nit: #include "base/threading/thread_checker.h" https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:108: // |tasks_lock_| is acquired. Nit: s/acquired/held/ https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/session_length_limiter_unittest.cc (right): https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:7: #include <queue> Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:8: #include <utility> Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:9: #include <vector> Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:11: #include "base/callback.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:13: #include "base/location.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:14: #include "base/logging.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:18: #include "base/single_thread_task_runner.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc (left): https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:60: // corresponding device uptime is written to |uptime_file_|, providing a mock Nit: Use the second part of the class comment to document |MockUptimeProvider|. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc (right): https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:8: #include <utility> Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:22: #include "base/single_thread_task_runner.h" Nit: No longer used. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:69: void set_uptime(const base::TimeDelta& uptime) { Nit: This should not be a hacker_style() inline method as it is runtime is anything but trivial. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:132: : public testing::Test { Nit: Why does this suddenly no longer fit on one line? https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:181: MockUptimeProvider* uptime_provider_; // Weak, owner by |task_runner_|. Nit 1: s/owner/owned/ Nit 2: The comment is not really necessary. A scoped_ptr implies ownership. A raw pointer implies no ownership.
Patchset #3 (id:40001) has been deleted
Apologies for forgetting cleaning up the includes. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... File ash/system/chromeos/session/logout_confirmation_controller_unittest.cc (right): https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:7: #include <queue> On 2015/01/08 14:14:34, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:8: #include <utility> On 2015/01/08 14:14:34, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:9: #include <vector> On 2015/01/08 14:14:34, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:13: #include "base/compiler_specific.h" On 2015/01/08 14:14:34, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:14: #include "base/location.h" On 2015/01/08 14:14:34, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:16: #include "base/single_thread_task_runner.h" On 2015/01/08 14:14:34, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/ash/system/chromeos/sess... ash/system/chromeos/session/logout_confirmation_controller_unittest.cc:19: #include "base/time/tick_clock.h" On 2015/01/08 14:14:34, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:7: #include "base/callback.h" On 2015/01/08 14:14:34, bartfab wrote: > Nit: Do you actually need this? You do not seem to dereference any callbacks or > closures. Apparently not. Removed. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:15: class CorrespondingMockTickClock : public TickClock { On 2015/01/08 14:14:34, bartfab wrote: > Nit: I am not sure "Corresponding" is a useful part of the class name here. > Corresponding to what? Removed that part. Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:18: scoped_refptr<const TestMockTimeTaskRunner> task_runner); On 2015/01/08 14:14:34, bartfab wrote: > Nit: #include "base/memory/ref_counted.h" Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:57: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/01/08 14:14:34, bartfab wrote: > Nit: #include "base/logging.h" Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:61: base::TimeTicks original_now = now_; On 2015/01/08 14:14:34, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:110: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/01/08 14:14:34, bartfab wrote: > Nit: You could actually trivially make this method work correctly on all > threads: > > return thread_checker_.CalledOnValidThread(); Nice. Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:11: #include "base/memory/ref_counted.h" On 2015/01/08 14:14:35, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:41: // Fast forwards virtual time by |delta|, causing all tasks with a remaining On 2015/01/08 14:14:35, bartfab wrote: > Nit: s/Fast forwards/Fast-forwards/ Done. Also one more place in the class comment. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:42: // delay of less than or equal to |delta| to be executed. On 2015/01/08 14:14:35, bartfab wrote: > Nit: s/of // Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:45: // Fast forwards virtual time just until all tasks are executed. On 2015/01/08 14:14:35, bartfab wrote: > Nit: s/Fast forwards/Fast-forwards/ Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:48: // Executes all tasks having no remaining delay. No virtual time will elapse. On 2015/01/08 14:14:34, bartfab wrote: > Nit: Could you add one more sentence which explicitly states that delayed tasks > will remain enqueued? Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:77: // Called after the current mock time had been incremented so that subclasses On 2015/01/08 14:14:34, bartfab wrote: > Nit: s/had/has/ Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:79: virtual void OnAfterTimePasses(); On 2015/01/08 14:14:34, bartfab wrote: > Nit: s/Passes/Passed/ Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:82: // activities, e.g., pumping out tasks from additional task runners. On 2015/01/08 14:14:35, bartfab wrote: > Nit: s/pumping out tasks from/pump/ Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:83: virtual void OnAfterRunningTask(); On 2015/01/08 14:14:35, bartfab wrote: > Nit: s/RunningTask/TaskRun/ Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:94: std::vector<TestPendingTask>, On 2015/01/08 14:14:35, bartfab wrote: > Nit: #include <vector> Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:104: base::ThreadChecker thread_checker_; On 2015/01/08 14:14:35, bartfab wrote: > Nit: #include "base/threading/thread_checker.h" Done. https://codereview.chromium.org/823143004/diff/20001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:108: // |tasks_lock_| is acquired. On 2015/01/08 14:14:34, bartfab wrote: > Nit: s/acquired/held/ Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/session_length_limiter_unittest.cc (right): https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:7: #include <queue> On 2015/01/08 14:14:35, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:8: #include <utility> On 2015/01/08 14:14:35, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:9: #include <vector> On 2015/01/08 14:14:35, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:11: #include "base/callback.h" On 2015/01/08 14:14:35, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:13: #include "base/location.h" On 2015/01/08 14:14:35, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:14: #include "base/logging.h" On 2015/01/08 14:14:35, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/session_length_limiter_unittest.cc:18: #include "base/single_thread_task_runner.h" On 2015/01/08 14:14:35, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc (left): https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:60: // corresponding device uptime is written to |uptime_file_|, providing a mock On 2015/01/08 14:14:36, bartfab wrote: > Nit: Use the second part of the class comment to document |MockUptimeProvider|. Done. I have formulated something along that line. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc (right): https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:8: #include <utility> On 2015/01/08 14:14:35, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:22: #include "base/single_thread_task_runner.h" On 2015/01/08 14:14:36, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:69: void set_uptime(const base::TimeDelta& uptime) { On 2015/01/08 14:14:36, bartfab wrote: > Nit: This should not be a hacker_style() inline method as it is runtime is > anything but trivial. Done. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:132: : public testing::Test { On 2015/01/08 14:14:36, bartfab wrote: > Nit: Why does this suddenly no longer fit on one line? I guess I might have added another base class for a short while. Fixed. https://codereview.chromium.org/823143004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:181: MockUptimeProvider* uptime_provider_; // Weak, owner by |task_runner_|. On 2015/01/08 14:14:36, bartfab wrote: > Nit 1: s/owner/owned/ > Nit 2: The comment is not really necessary. A scoped_ptr implies ownership. A > raw pointer implies no ownership. I removed weak, but kept the rest so it is clearer who owns that.
engedy@chromium.org changed reviewers: + phajdan.jr@chromium.org, stevenjb@chromium.org
@Pawel, could you please review base/test/? @Steven, could you please review the 3 unittest changes for Owner's approval?
https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:20: virtual ~MockTickClock(); override https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:23: virtual TimeTicks NowTicks() override; no 'virtual' https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:5: #ifndef BASE_TEST_MOCK_TIME_TASK_RUNNER_H_ BASE_TEST_TEST_MOCK_TIME_TASK_RUNNER_H_ https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:72: TimeDelta delay) override; No 'virtual' https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:75: virtual ~TestMockTimeTaskRunner(); Use 'override' https://codereview.chromium.org/823143004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc (right): https://codereview.chromium.org/823143004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:96: ~TestAutomaticRebootManagerTaskRunner(); override https://codereview.chromium.org/823143004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:179: MockUptimeProvider* uptime_provider_; // Owned by |task_runner_|. Rather than storing this as a member, use a helper method: updtime_provider() { return task_runner_->uptime_provider(); } This is less dangerous and more clear, imho.
https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:20: virtual ~MockTickClock(); On 2015/01/08 17:39:19, stevenjb wrote: > override Done. https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:23: virtual TimeTicks NowTicks() override; On 2015/01/08 17:39:19, stevenjb wrote: > no 'virtual' Done. https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:5: #ifndef BASE_TEST_MOCK_TIME_TASK_RUNNER_H_ On 2015/01/08 17:39:19, stevenjb wrote: > BASE_TEST_TEST_MOCK_TIME_TASK_RUNNER_H_ Ahh, nice catch! Done. https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:72: TimeDelta delay) override; On 2015/01/08 17:39:19, stevenjb wrote: > No 'virtual' Done. https://codereview.chromium.org/823143004/diff/60001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:75: virtual ~TestMockTimeTaskRunner(); On 2015/01/08 17:39:19, stevenjb wrote: > Use 'override' Done. https://codereview.chromium.org/823143004/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc (right): https://codereview.chromium.org/823143004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:96: ~TestAutomaticRebootManagerTaskRunner(); On 2015/01/08 17:39:19, stevenjb wrote: > override Done. https://codereview.chromium.org/823143004/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:179: MockUptimeProvider* uptime_provider_; // Owned by |task_runner_|. On 2015/01/08 17:39:19, stevenjb wrote: > Rather than storing this as a member, use a helper method: > > updtime_provider() { return task_runner_->uptime_provider(); } > > This is less dangerous and more clear, imho. Done.
lgtm
LGTM w/nits https://codereview.chromium.org/823143004/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/823143004/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/823143004/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/823143004/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015
https://codereview.chromium.org/823143004/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/823143004/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/12 15:42:09, Paweł Hajdan Jr. wrote: > nit: 2015 Done. https://codereview.chromium.org/823143004/diff/80001/base/test/test_mock_time... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/823143004/diff/80001/base/test/test_mock_time... base/test/test_mock_time_task_runner.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/12 15:42:09, Paweł Hajdan Jr. wrote: > nit: 2015 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/823143004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
engedy@chromium.org changed reviewers: + thestig@chromium.org
@Lei, could you please sign off on the changes to base.gyp?
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/823143004/100001
lgtm
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8d897fb17470c526155b2a9e86752be9a7f33c03 Cr-Commit-Position: refs/heads/master@{#311091}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/844353002/ by erg@chromium.org. The reason for reverting is: Suspect that this patch broke the chromeos asan bots. It looks like all the AutomaticRebootManagerTestInstance/AutomaticRebootManagerTest tests now leak memory: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
