|
|
Description[Extensions][TaskScheduler] Update ActivityLog for scheduling migration
Migrate the ActivityLog and related classes away from posting tasks to
and checking that they are currently on the content::BrowserThread::DB
thread, and replace this with a singleton SequencedTaskRunner to be used
in the activity log.
BUG=689520
Review-Url: https://codereview.chromium.org/2980503002
Cr-Commit-Position: refs/heads/master@{#491137}
Committed: https://chromium.googlesource.com/chromium/src/+/6fa6b15543d5e4ce35ac67c8a8fd1dee80d2ceea
Patch Set 1 #Patch Set 2 : . #
Total comments: 2
Patch Set 3 : Experiments #
Total comments: 3
Patch Set 4 : Revert to set for testing #Patch Set 5 : Revert to set for testing #
Total comments: 4
Patch Set 6 : Use SingleThreadTaskRunner to be safe #
Total comments: 6
Patch Set 7 : gab's #Patch Set 8 : . #Messages
Total messages: 55 (35 generated)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== al tasks BUG= ========== to ========== [Extensions][TaskScheduler] Update ActivityLog for scheduling migration Migrate the ActivityLog and related classes away from posting tasks to and checking that they are currently on the content::BrowserThread::DB thread, and replace this with a singleton SequencedTaskRunner to be used in the activity log. BUG=689520 ==========
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rdevlin.cronin@chromium.org changed reviewers: + gab@chromium.org, lazyboy@chromium.org
lazyboy@, please take a look. gab@, please see the question in activity_log_task_runner.h. Cheers! https://codereview.chromium.org/2980503002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_log_task_runner.h (right): https://codereview.chromium.org/2980503002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_log_task_runner.h:18: void SetActivityLogTaskRunnerForTesting(base::TaskRunner* task_runner); This appears to be needed for activity log unittests. Without setting the runner here, the sql database always fails to open with a disk I/O error; other similar areas of the code using sql databases seem to do the same thing in unit testing. I've tried adding a ScopedTaskEnvironment, but no luck. gab@, is this expected/correct? Or should there be a way of running a sql database in a unittest with a constructed task runner?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2980503002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_log_task_runner.h (right): https://codereview.chromium.org/2980503002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_log_task_runner.h:18: void SetActivityLogTaskRunnerForTesting(base::TaskRunner* task_runner); On 2017/07/10 18:48:39, Devlin wrote: > This appears to be needed for activity log unittests. Without setting the > runner here, the sql database always fails to open with a disk I/O error; other > similar areas of the code using sql databases seem to do the same thing in unit > testing. I've tried adding a ScopedTaskEnvironment, but no luck. This is a ChromeRenderViewHostTestHarness which has a TestBrowserThreadBundle (which provides a ScopedTaskEnvironment equivalent so you already have a TaskScheduler) > > gab@, is this expected/correct? Or should there be a way of running a sql > database in a unittest with a constructed task runner? This is because the unit tests previously depended on TestBrowserThreadBundle faking the DB thread on the main thread (such that "async" work would execute as part of RunLoop::Run()), now you need to content::RunAllBlockingPoolTasksUntilIdle() instead to flush main thread *and* async work. Or doing what you did ForTesting is also fine (though IMO not as clean) if you prefer to keep everything on the main thread for the unit test.
Hey gab@, mind taking a look at some more questions? Sorry for turning this into a bit of a bikeshed - I wouldn't be entirely opposed to just landing this with the old implementation, but I figure it's worth asking these questions now for when they come up later. https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_database_unittest.cc (left): https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_database_unittest.cc:226: base::RunLoop().RunUntilIdle(); We can remove this here because SetTimerForTesting() just kicked off a timer to execute RecordBatchedActionsWhileTesting(), so doing that immediately followed by a RunLoop().RunUntilIdle() was equivalent to just calling the record method directly. But, it raised a question. The reason I changed this was because RunLoops don't seem to work on other threads (or at least, not whichever thread the task scheduler put us on). The migration doc says "If test code was previously using RunLoop to execute things off the main thread (as TestBrowserThreadBundle grouped everything under a single MessageLoop), flushing tasks will now require asking for that explicitly." Silly question - how do we "ask for that explicitly"? The usual suspects of content::RunAllBlockingPoolTasksUntilIdle() and TaskScheduler::FlushForTesting() don't work. Even though we don't strictly need it here, it seems like a useful thing to be able to do; is there a canonical blocking pool run loop equivalent? https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_database_unittest.cc (right): https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_database_unittest.cc:36: #define TEST_ON_ACTIVITY_LOG_RUNNER(test_class, test_name) \ gab@, I'm curious what you think of a pattern like this. It allows us to still write tests like TEST_MACRO() { <test stuff> } But I'm not sure if it's either too magical, or risks some bad interaction with task scheduling. https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_log_unittest.cc (right): https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_log_unittest.cc:58: class ActivityLogTest : public ChromeRenderViewHostTestHarness { Interestingly, these tests are still failing with a SQL I/O error, even though we have a test thread bundle and are using the normal code path for calling methods (i.e., we just call into the UI activity log methods and that posts tasks to the task runner for the activity database). No sequence checker assertions are raised; we just get a disk I/O error. Any idea why we'd still be seeing an error here, and how we might fix it?
Le mer. 12 juil. 2017 12:30, <rdevlin.cronin@chromium.org> a écrit : > Hey gab@, mind taking a look at some more questions? > > Sorry for turning this into a bit of a bikeshed - I wouldn't be entirely > opposed > to just landing this with the old implementation, but I figure it's worth > asking > these questions now for when they come up later. > > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > File > chrome/browser/extensions/activity_log/activity_database_unittest.cc > (left): > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/activity_log/activity_database_unittest.cc:226: > base::RunLoop().RunUntilIdle(); > We can remove this here because SetTimerForTesting() just kicked off a > timer to execute RecordBatchedActionsWhileTesting(), so doing that > immediately followed by a RunLoop().RunUntilIdle() was equivalent to > just calling the record method directly. > > But, it raised a question. The reason I changed this was because > RunLoops don't seem to work on other threads (or at least, not whichever > thread the task scheduler put us on). The migration doc says > > "If test code was previously using RunLoop to execute things off the > main thread (as TestBrowserThreadBundle grouped everything under a > single MessageLoop), flushing tasks will now require asking for that > explicitly." Silly question - how do we "ask for that explicitly"? The > usual suspects of content::RunAllBlockingPoolTasksUntilIdle() and > TaskScheduler::FlushForTesting() don't work. Even though we don't > strictly need it here, it seems like a useful thing to be able to do; is > there a canonical blocking pool run loop equivalent? > These should work (or ScopedTaskEnvironment::RunUntilIdle()). The problem in your case, since you mentioned timer, might be a delayed task that doesn't expire? > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > File > chrome/browser/extensions/activity_log/activity_database_unittest.cc > (right): > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/activity_log/activity_database_unittest.cc:36: > #define TEST_ON_ACTIVITY_LOG_RUNNER(test_class, test_name) > \ > gab@, I'm curious what you think of a pattern like this. It allows us > to still write tests like > TEST_MACRO() { > <test stuff> > } > > But I'm not sure if it's either too magical, or risks some bad > interaction with task scheduling. > Responding from offline and don't see full definition of macro but I can imagine what it does (post a lambda and run until idle?). Is it overly verbose to post a lambda inline and run until idle? > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > File chrome/browser/extensions/activity_log/activity_log_unittest.cc > (right): > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/activity_log/activity_log_unittest.cc:58: > class ActivityLogTest : public ChromeRenderViewHostTestHarness { > Interestingly, these tests are still failing with a SQL I/O error, even > though we have a test thread bundle and are using the normal code path > for calling methods (i.e., we just call into the UI activity log methods > and that posts tasks to the task runner for the activity database). No > sequence checker assertions are raised; we just get a disk I/O error. > > Any idea why we'd still be seeing an error here, and how we might fix > it? > Sounds like https://bugs.chromium.org/p/chromium/issues/detail?id=739945#c1, maybe SQL uses TLS or something and needs a SingleThreadTaskRunner (instead of SequencedTaskRunner). > https://codereview.chromium.org/2980503002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/07/14 23:09:24, chromium-reviews wrote: > Le mer. 12 juil. 2017 12:30, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=rdevlin.cronin@chromium.org> a écrit : > > > Hey gab@, mind taking a look at some more questions? > > > > Sorry for turning this into a bit of a bikeshed - I wouldn't be entirely > > opposed > > to just landing this with the old implementation, but I figure it's worth > > asking > > these questions now for when they come up later. > > > > > > > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > > File > > chrome/browser/extensions/activity_log/activity_database_unittest.cc > > (left): > > > > > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > > chrome/browser/extensions/activity_log/activity_database_unittest.cc:226: > > base::RunLoop().RunUntilIdle(); > > We can remove this here because SetTimerForTesting() just kicked off a > > timer to execute RecordBatchedActionsWhileTesting(), so doing that > > immediately followed by a RunLoop().RunUntilIdle() was equivalent to > > just calling the record method directly. > > > > But, it raised a question. The reason I changed this was because > > RunLoops don't seem to work on other threads (or at least, not whichever > > thread the task scheduler put us on). The migration doc says > > > > "If test code was previously using RunLoop to execute things off the > > main thread (as TestBrowserThreadBundle grouped everything under a > > single MessageLoop), flushing tasks will now require asking for that > > explicitly." Silly question - how do we "ask for that explicitly"? The > > usual suspects of content::RunAllBlockingPoolTasksUntilIdle() and > > TaskScheduler::FlushForTesting() don't work. Even though we don't > > strictly need it here, it seems like a useful thing to be able to do; is > > there a canonical blocking pool run loop equivalent? > > > > These should work (or ScopedTaskEnvironment::RunUntilIdle()). The problem > in your case, since you mentioned timer, might be a delayed task that > doesn't expire? > > > > > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > > File > > chrome/browser/extensions/activity_log/activity_database_unittest.cc > > (right): > > > > > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > > chrome/browser/extensions/activity_log/activity_database_unittest.cc:36: > > #define TEST_ON_ACTIVITY_LOG_RUNNER(test_class, test_name) > > \ > > gab@, I'm curious what you think of a pattern like this. It allows us > > to still write tests like > > TEST_MACRO() { > > <test stuff> > > } > > > > But I'm not sure if it's either too magical, or risks some bad > > interaction with task scheduling. > > > > Responding from offline and don't see full definition of macro but I can > imagine what it does (post a lambda and run until idle?). Is it overly > verbose to post a lambda inline and run until idle? Back online, here's a simpler way to do the same thing: GetActivityLogTaskRunner()->PostTask(FROM_HERE, []() { (test body goes here...) }); content::RunAllBlockingPoolTasksUntilIdle(); I think that's simple enough to be inline instead of macro? > > > > > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > > File chrome/browser/extensions/activity_log/activity_log_unittest.cc > > (right): > > > > > > > https://codereview.chromium.org/2980503002/diff/40001/chrome/browser/extensio... > > chrome/browser/extensions/activity_log/activity_log_unittest.cc:58: > > class ActivityLogTest : public ChromeRenderViewHostTestHarness { > > Interestingly, these tests are still failing with a SQL I/O error, even > > though we have a test thread bundle and are using the normal code path > > for calling methods (i.e., we just call into the UI activity log methods > > and that posts tasks to the task runner for the activity database). No > > sequence checker assertions are raised; we just get a disk I/O error. > > > > Any idea why we'd still be seeing an error here, and how we might fix > > it? > > > > Sounds like https://bugs.chromium.org/p/chromium/issues/detail?id=739945#c1, > maybe SQL uses TLS or something and needs a SingleThreadTaskRunner (instead > of SequencedTaskRunner). > > > > https://codereview.chromium.org/2980503002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Sounds like https://bugs.chromium.org/p/chromium/issues/detail?id=739945#c1, > maybe SQL uses TLS or something and needs a SingleThreadTaskRunner (instead > of SequencedTaskRunner). Bummer. In that case, we need the "SetTaskRunnerForTesting()" method anyway, so the rest is kind of moot. > Back online, here's a simpler way to do the same thing: > > GetActivityLogTaskRunner()->PostTask(FROM_HERE, []() { > (test body goes here...) > }); > content::RunAllBlockingPoolTasksUntilIdle(); > > I think that's simple enough to be inline instead of macro? (just for conversation, and possibly future CLs) That kind of works, but it's a bit more complicated because the test body includes references to |this|. If we had the ability to base::Bind() capturing lambdas (I think there's a bug to allow that for testing only?), this would be pretty simple, but since right now that's a compiler error, we actually end up with something like: template<typename Function> RunFunctionHelper(Function function) { function(); } template <typename Function> void RunTestOnActivityLogTaskRunner( const tracked_objects::Location& from_here, Function function) { GetActivityLogTaskRunner()->PostTask( from_here, base::Bind(&RunTestHelper<Function>, function)); content::RunAllBlockingPoolTasksUntilIdle(); } TEST_F(...) { RunTestOnActivityLogTaskRunner(FROM_HERE, [this]() { }); } Not drastically more complicated, but perhaps messy enough that it's a push. But, again, kind of moot now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/19 20:54:40, Devlin (OOO through July 24) wrote: > > Sounds like https://bugs.chromium.org/p/chromium/issues/detail?id=739945#c1, > > maybe SQL uses TLS or something and needs a SingleThreadTaskRunner (instead > > of SequencedTaskRunner). > > Bummer. In that case, we need the "SetTaskRunnerForTesting()" method anyway, so > the rest is kind of moot. > > > Back online, here's a simpler way to do the same thing: > > > > GetActivityLogTaskRunner()->PostTask(FROM_HERE, []() { > > (test body goes here...) > > }); > > content::RunAllBlockingPoolTasksUntilIdle(); > > > > I think that's simple enough to be inline instead of macro? > > (just for conversation, and possibly future CLs) > That kind of works, but it's a bit more complicated because the test body > includes references to |this|. If we had the ability to base::Bind() capturing > lambdas (I think there's a bug to allow that for testing only?), this would be > pretty simple, but since right now that's a compiler error, we actually end up > with something like: > > template<typename Function> > RunFunctionHelper(Function function) { > function(); > } > > template <typename Function> > void RunTestOnActivityLogTaskRunner( > const tracked_objects::Location& from_here, > Function function) { > GetActivityLogTaskRunner()->PostTask( > from_here, base::Bind(&RunTestHelper<Function>, function)); > content::RunAllBlockingPoolTasksUntilIdle(); > } > > TEST_F(...) { > RunTestOnActivityLogTaskRunner(FROM_HERE, [this]() { > }); > } > > Not drastically more complicated, but perhaps messy enough that it's a push. > > But, again, kind of moot now. Right inability to capture |this| is annoying. If you have things that are entirely meant to happen on the background thread one solution is to just pull that to a separate class which merely uses a base::SequenceChecker without forcing a specific sequence. That can then be unit tested inline on the main thread whereas in production it's used from a TaskScheduler task runner, in both cases satisfying requirement to be sequence-affine without forcing the test body to bounce to a specific sequence. In general the background runner should be an implementation detail so having to manually post to it in tests perhaps indicates coupling that's tighter than necessary... but lazy task runners can sometimes spread enough to make that necessary.. in which case SetTaskRunnnerForTesting() helps... Hopefully that makes sense and you found something that works for you (will review for real now).
> > Sounds like https://bugs.chromium.org/p/chromium/issues/detail?id=739945#c1, > > maybe SQL uses TLS or something and needs a SingleThreadTaskRunner (instead > > of SequencedTaskRunner). > > Bummer. In that case, we need the "SetTaskRunnerForTesting()" method anyway, so > the rest is kind of moot. Hmmm, don't you need the whole GetActivityLogTaskRunner to be SingleThreadTaskRunner? Making it so in tests but not in prod is merely hiding the issue from tests no? https://codereview.chromium.org/2980503002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_log_task_runner.cc (right): https://codereview.chromium.org/2980503002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_log_task_runner.cc:20: static base::LazySequencedTaskRunner task_runner = The documentation states to put this in the anonymous namespace, I think it's probably equivalent as a static in local scope but I don't see a reason to diverge either and would prefer to have it in anonymous namespace https://codereview.chromium.org/2980503002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_log_task_runner.h (right): https://codereview.chromium.org/2980503002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_log_task_runner.h:16: const scoped_refptr<base::TaskRunner> GetActivityLogTaskRunner(); Use base::SequencedTaskRunner in interface and impl since that's what it is under the hood (or even SingleThreadTaskRunner if that's required, re. SQL disk I/O error discussion)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/07/20 16:26:43, gab wrote: > > > Sounds like https://bugs.chromium.org/p/chromium/issues/detail?id=739945#c1, > > > maybe SQL uses TLS or something and needs a SingleThreadTaskRunner (instead > > > of SequencedTaskRunner). > > > > Bummer. In that case, we need the "SetTaskRunnerForTesting()" method anyway, > so > > the rest is kind of moot. > > Hmmm, don't you need the whole GetActivityLogTaskRunner to be > SingleThreadTaskRunner? Making it so in tests but not in prod is merely hiding > the issue from tests no? I'm not entirely sure. The interesting part is that we have *some* browser test coverage of the database as well, and these tests don't use the SetTaskRunnerForTesting() method, but still pass just fine. However, I don't know if that's a guarantee or not - it could be that we just luck out in the testing environment. Since you brought it up as a concern and I'm not 100% confident in the test coverage, I've made it a SingleThreadTaskRunner to be safe and added a TODO. Seems like we might be able to get away with a sequenced runner, but it might be worth just waiting 'til the SQL issue is resolved and we don't need separate behavior in unittests. https://codereview.chromium.org/2980503002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_log_task_runner.cc (right): https://codereview.chromium.org/2980503002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_log_task_runner.cc:20: static base::LazySequencedTaskRunner task_runner = On 2017/07/20 16:26:43, gab wrote: > The documentation states to put this in the anonymous namespace, I think it's > probably equivalent as a static in local scope but I don't see a reason to > diverge either and would prefer to have it in anonymous namespace Sure thing; done. https://codereview.chromium.org/2980503002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/activity_log_task_runner.h (right): https://codereview.chromium.org/2980503002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/activity_log/activity_log_task_runner.h:16: const scoped_refptr<base::TaskRunner> GetActivityLogTaskRunner(); On 2017/07/20 16:26:43, gab wrote: > Use base::SequencedTaskRunner in interface and impl since that's what it is > under the hood (or even SingleThreadTaskRunner if that's required, re. SQL disk > I/O error discussion) Done.
lgtm w/ nits https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/activity_log/activity_log_task_runner.cc (right): https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/activity_log/activity_log_task_runner.cc:7: #include "base/sequenced_task_runner.h" single_thread_task_runner.h https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/activity_log/activity_log_task_runner.cc:9: #include "base/task_scheduler/single_thread_task_runner_thread_mode.h" You can keep this if you wish but it's really implicit that it comes for free with lazy_task_runner.h IMO https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right): https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:78: base::MessageLoop::current()->QuitWhenIdleClosure()); While you're here, this is a deprecated paradigm, should instantiate the RunLoop above this and pass run_loop.QuitWhenIdleClosure() in here instead.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for your help, gab@! lazyboy@, mind taking a look? https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/activity_log/activity_log_task_runner.cc (right): https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/activity_log/activity_log_task_runner.cc:7: #include "base/sequenced_task_runner.h" On 2017/07/27 18:34:53, gab wrote: > single_thread_task_runner.h Done. https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/activity_log/activity_log_task_runner.cc:9: #include "base/task_scheduler/single_thread_task_runner_thread_mode.h" On 2017/07/27 18:34:53, gab wrote: > You can keep this if you wish but it's really implicit that it comes for free > with lazy_task_runner.h IMO I think for the spirit of IWYU, we should keep it. It'd be possible that lazy_task_runner one day uses a forward declaration (yay, enum classes!), rather than a direct include. https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right): https://codereview.chromium.org/2980503002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:78: base::MessageLoop::current()->QuitWhenIdleClosure()); On 2017/07/27 18:34:53, gab wrote: > While you're here, this is a deprecated paradigm, should instantiate the RunLoop > above this and pass run_loop.QuitWhenIdleClosure() in here instead. Sure, done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
lgtm
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2980503002/#ps120001 (title: "gab's")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2980503002/#ps140001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1501620968203230, "parent_rev": "af1f4dc46488bf90c1cc4607cc42e52024e70e8c", "commit_rev": "6fa6b15543d5e4ce35ac67c8a8fd1dee80d2ceea"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions][TaskScheduler] Update ActivityLog for scheduling migration Migrate the ActivityLog and related classes away from posting tasks to and checking that they are currently on the content::BrowserThread::DB thread, and replace this with a singleton SequencedTaskRunner to be used in the activity log. BUG=689520 ========== to ========== [Extensions][TaskScheduler] Update ActivityLog for scheduling migration Migrate the ActivityLog and related classes away from posting tasks to and checking that they are currently on the content::BrowserThread::DB thread, and replace this with a singleton SequencedTaskRunner to be used in the activity log. BUG=689520 Review-Url: https://codereview.chromium.org/2980503002 Cr-Commit-Position: refs/heads/master@{#491137} Committed: https://chromium.googlesource.com/chromium/src/+/6fa6b15543d5e4ce35ac67c8a8fd... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/6fa6b15543d5e4ce35ac67c8a8fd... |