Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557083002:
Run ScopedTaskScheduler tasks synchronously. (Closed)
|
Created:
4 years ago by fdoray Modified:
4 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, alemate+watch_chromium.org, sadrul, vmpstr+watch_chromium.org, jam, robliao+watch_chromium.org, achuith+watch_chromium.org, fdoray+watch_chromium.org, oshima+watch_chromium.org, darin-cc_chromium.org, gab+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRun ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
BUG=553459
Committed: https://crrev.com/ad58f8382643800d7107f43c91bd2f9776da4900
Cr-Commit-Position: refs/heads/master@{#439505}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : git cl try #
Total comments: 30
Patch Set 4 : CR gab robliao #15-16 #Patch Set 5 : self-review #
Total comments: 4
Patch Set 6 : CR gab #20 #
Total comments: 8
Patch Set 7 : CR robliao #27 #
Total comments: 4
Patch Set 8 : CR gab #37 #
Total comments: 3
Patch Set 9 : add owners #
Total comments: 10
Patch Set 10 : CR dcheng #53 #Patch Set 11 : fix linux build error #Patch Set 12 : rebase #Patch Set 13 : rebase #Messages
Total messages: 93 (48 generated)
The CQ bit was checked by fdoray@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_chromium_chromeos_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 fdoray@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by fdoray@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...
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
This is awesome :) https://codereview.chromium.org/2557083002/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:293: // Create |shutdown_event_| to avoid a DCHECK failure when a task blocking // Create a dummy |shutdown_event_| to satisfy TaskTracker's expectation of its existence during shutdown (e.g. in OnBlockingShutdownTasksComplete()). https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:86: // watch file descriptors from outside of TaskScheduler tasks. But doesn't this now give an API to other threads sharing this MessageLoop which they didn't have previously? https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.h:20: // thread where the ScopedTaskScheduler lives. The destructor of runs remaining rm "of" https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... File base/test/scoped_task_scheduler_unittest.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler_unittest.cc:29: thread_checker_.DetachFromThread(); Why detach? https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler_unittest.cc:33: ThreadCheckerImpl thread_checker_; If all you need is these two members and they're used in 2/3 tests, feels like you can inline them in tests that need them and do without the fixture? https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (left): https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:62: message_loop_.reset(new base::MessageLoopForIO()); I prefered that the loop be owned by TestBrowserThreadBundle. Can ScopedTaskScheduler take an optional externally provided MessageLoop (like base::Thread)? Also avoids adding MessageLoop::current() calls below. https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:65: : base::test::ScopedTaskScheduler::UI_MAIN_LOOP); So if using a REAL_IO_THREAD, it's not possible to have a TaskScheduler than can do async I/O? I think we should have a REAL_TASK_SCHEDULER option that would create a simple task scheduler without a MessageLoop. https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:6: // TestBrowserThreads, a blocking pool and a task scheduler in unit tests. For , and https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:21: // blocking pool tasks and remaining BLOCK_SHUTDOWN task scheduler tasks. , and
https://codereview.chromium.org/2557083002/diff/40001/base/message_loop/messa... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/message_loop/messa... base/message_loop/message_loop.cc:371: if (task_runner_) Should we DCHECK here? Seems unexpected to silently reset and not set a ThreadTaskRunnerHandle, even if there is no valid task_runner_ available. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:57: // Posts |task| to this TaskScheduler with |sequence_token|. Nit: Add what the bool (return value) indicates. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.h:25: // base::test::ScopedTaskScheduler scoped_task_scheduler; Nit: It might be clearer to have a leading section explain what the code does below instead of adding those notes in the comments. e.g. In the following snippet, RunUntilIdle() returns after "A" is run. ... In this snippet, run_loop.Run() returns after running "B" and "RunLoop::Quit". "C" is not run and "D" is run since it's BLOCK_SHUTDOWN. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.h:43: DEFAULT = 0x0, Is this going to be a future set of flags? If not, using decimal should be fine. https://codereview.chromium.org/2557083002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc (left): https://codereview.chromium.org/2557083002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:340: base::test::ScopedTaskScheduler scoped_task_scheduler_; Comment on this change (and the next one) in the CL description. Maybe it would be better put in a separate CL?
Description was changed from
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
BUG=553459
==========
to
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
This CL prevents a TestBrowserThreadBundle or a MessageLoop from being
initialized in the scope of a ScopedTaskScheduler. This CL fixes tests
that previously did this.
BUG=553459
==========
Description was changed from
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
This CL prevents a TestBrowserThreadBundle or a MessageLoop from being
initialized in the scope of a ScopedTaskScheduler. This CL fixes tests
that previously did this.
BUG=553459
==========
to
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
BUG=553459
==========
PTAnL https://codereview.chromium.org/2557083002/diff/40001/base/message_loop/messa... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/message_loop/messa... base/message_loop/message_loop.cc:371: if (task_runner_) On 2016/12/09 01:32:54, robliao wrote: > Should we DCHECK here? Seems unexpected to silently reset and not set a > ThreadTaskRunnerHandle, even if there is no valid task_runner_ available. It is intentional to have no ThreadTaskRunnerHandle on the current thread after calling SetThreadTaskRunnerHandle() with |task_runner_| == nullptr. This is what allows the ScopedTaskScheduler's TaskTracker to set its own Thread/SequencedTaskRunnerHandle. https://codereview.chromium.org/2557083002/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:293: // Create |shutdown_event_| to avoid a DCHECK failure when a task blocking On 2016/12/08 20:15:45, gab wrote: > // Create a dummy |shutdown_event_| to satisfy TaskTracker's expectation of its > existence during shutdown (e.g. in OnBlockingShutdownTasksComplete()). Done. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:57: // Posts |task| to this TaskScheduler with |sequence_token|. On 2016/12/09 01:32:54, robliao wrote: > Nit: Add what the bool (return value) indicates. Done. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:86: // watch file descriptors from outside of TaskScheduler tasks. On 2016/12/08 20:15:45, gab wrote: > But doesn't this now give an API to other threads sharing this MessageLoop which > they didn't have previously? Removed. Offline discussion: We will stop initializing FileDescriptorWatcher in TaskScheduler. It will be initialized for the whole process independently from TaskScheduler. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.h:20: // thread where the ScopedTaskScheduler lives. The destructor of runs remaining On 2016/12/08 20:15:45, gab wrote: > rm "of" Done. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.h:25: // base::test::ScopedTaskScheduler scoped_task_scheduler; On 2016/12/09 01:32:54, robliao wrote: > Nit: It might be clearer to have a leading section explain what the code does > below instead of adding those notes in the comments. > > e.g. > > In the following snippet, RunUntilIdle() returns after "A" is run. > > ... > > In this snippet, run_loop.Run() returns after running "B" and "RunLoop::Quit". > "C" is not run and "D" is run since it's BLOCK_SHUTDOWN. Done. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler.h:43: DEFAULT = 0x0, On 2016/12/09 01:32:54, robliao wrote: > Is this going to be a future set of flags? If not, using decimal should be fine. Removed. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... File base/test/scoped_task_scheduler_unittest.cc (right): https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler_unittest.cc:29: thread_checker_.DetachFromThread(); On 2016/12/08 20:15:45, gab wrote: > Why detach? Added comment. https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... base/test/scoped_task_scheduler_unittest.cc:33: ThreadCheckerImpl thread_checker_; On 2016/12/08 20:15:45, gab wrote: > If all you need is these two members and they're used in 2/3 tests, feels like > you can inline them in tests that need them and do without the fixture? Done. https://codereview.chromium.org/2557083002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc (left): https://codereview.chromium.org/2557083002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:340: base::test::ScopedTaskScheduler scoped_task_scheduler_; On 2016/12/09 01:32:54, robliao wrote: > Comment on this change (and the next one) in the CL description. Maybe it would > be better put in a separate CL? Done. Cannot be in a separate CL: this CL prevents a TestBrowserThreadBundle from being instantiated after a ScopedTaskScheduler. https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (left): https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:62: message_loop_.reset(new base::MessageLoopForIO()); On 2016/12/08 20:15:45, gab wrote: > I prefered that the loop be owned by TestBrowserThreadBundle. Can > ScopedTaskScheduler take an optional externally provided MessageLoop (like > base::Thread)? > > Also avoids adding MessageLoop::current() calls below. Done. https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:65: : base::test::ScopedTaskScheduler::UI_MAIN_LOOP); On 2016/12/08 20:15:45, gab wrote: > So if using a REAL_IO_THREAD, it's not possible to have a TaskScheduler than can > do async I/O? > > I think we should have a REAL_TASK_SCHEDULER option that would create a simple > task scheduler without a MessageLoop. n/a See https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:6: // TestBrowserThreads, a blocking pool and a task scheduler in unit tests. For On 2016/12/08 20:15:45, gab wrote: > , and Done. https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:21: // blocking pool tasks and remaining BLOCK_SHUTDOWN task scheduler tasks. On 2016/12/08 20:15:45, gab wrote: > , and Done.
https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:65: : base::test::ScopedTaskScheduler::UI_MAIN_LOOP); On 2016/12/09 16:26:42, fdoray wrote: > On 2016/12/08 20:15:45, gab wrote: > > So if using a REAL_IO_THREAD, it's not possible to have a TaskScheduler than > can > > do async I/O? > > > > I think we should have a REAL_TASK_SCHEDULER option that would create a simple > > task scheduler without a MessageLoop. > > n/a See > https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... But we might still want a REAL_TASK_SCHEDULER? Would mean we'd keep TaskScheduler::FlushForTesting() too I guess which could be confusing when the MessageLoop based one is around... guess not then... just thinking out loud here :) https://codereview.chromium.org/2557083002/diff/80001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/80001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:69: MessageLoop::current() ? nullptr : MakeUnique<MessageLoop>(); Not a fan of MessageLoop::current() for all the reasons we're trying to minimize its usage (e.g. unclear where the global is coming from). I'd rather have an alternate constructor that takes one explicitly to highlight when reading that it's sharing an existing MessageLoop (just like base::Thread's constructor). I guess that would mean : // |message_loop_owned_| will be non-null if this TestTaskScheduler owns the // MessageLoop (wasn't handed one). |message_loop_| will always be set and // should be used by the class. std::unique_ptr<MessageLoop> message_loop_owned_; MessageLoop* message_loop_; as members though.. but I'm okay paying this cost in the anonymous namespace to have a more readable API, WDYT? https://codereview.chromium.org/2557083002/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2557083002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:69: scoped_task_scheduler_ = base::MakeUnique<base::test::ScopedTaskScheduler>(); I know MakeUnique is prefered today, but the ultimate preference is looking like surrounding code so unless you update all surrounding reset(new ...) calls: you should use that semantic too.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2557083002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:65: : base::test::ScopedTaskScheduler::UI_MAIN_LOOP); On 2016/12/09 20:04:17, gab wrote: > On 2016/12/09 16:26:42, fdoray wrote: > > On 2016/12/08 20:15:45, gab wrote: > > > So if using a REAL_IO_THREAD, it's not possible to have a TaskScheduler than > > can > > > do async I/O? > > > > > > I think we should have a REAL_TASK_SCHEDULER option that would create a > simple > > > task scheduler without a MessageLoop. > > > > n/a See > > > https://codereview.chromium.org/2557083002/diff/40001/base/test/scoped_task_s... > > But we might still want a REAL_TASK_SCHEDULER? Would mean we'd keep > TaskScheduler::FlushForTesting() too I guess which could be confusing when the > MessageLoop based one is around... guess not then... just thinking out loud here > :) I would wait until we have a real use case before adding the REAL_TASK_SCHEDULER option. Unless tasks synchronously wait for other tasks to run (which is bad), this option should not be needed. https://codereview.chromium.org/2557083002/diff/80001/base/test/scoped_task_s... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/80001/base/test/scoped_task_s... base/test/scoped_task_scheduler.cc:69: MessageLoop::current() ? nullptr : MakeUnique<MessageLoop>(); On 2016/12/09 20:04:17, gab wrote: > Not a fan of MessageLoop::current() for all the reasons we're trying to minimize > its usage (e.g. unclear where the global is coming from). > > I'd rather have an alternate constructor that takes one explicitly to highlight > when reading that it's sharing an existing MessageLoop (just like base::Thread's > constructor). > > I guess that would mean : > > // |message_loop_owned_| will be non-null if this TestTaskScheduler owns the > // MessageLoop (wasn't handed one). |message_loop_| will always be set and > // should be used by the class. > std::unique_ptr<MessageLoop> message_loop_owned_; > MessageLoop* message_loop_; > > as members though.. but I'm okay paying this cost in the anonymous namespace to > have a more readable API, WDYT? Done. https://codereview.chromium.org/2557083002/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2557083002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:69: scoped_task_scheduler_ = base::MakeUnique<base::test::ScopedTaskScheduler>(); On 2016/12/09 20:04:17, gab wrote: > I know MakeUnique is prefered today, but the ultimate preference is looking like > surrounding code so unless you update all surrounding reset(new ...) calls: you > should use that semantic too. Done.
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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm + comments https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... base/test/scoped_task_scheduler.cc:70: // MessageLoop (wasn't handed one). |message_loop_| will always be set and is s/handed one/provided one at construction/ https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:43: // When |scoped_task_scheduler| is destroyed, it runs "D" since it's It's not immediately clear that the entire code sample above is to be taken together. To establish flow... s/In this snippet/Continuing here,/ s/When |scoped_task_scheduler| is destroyed/At this point, |scoped_task_scheduler| will be destroyed. After that, / https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... File base/test/scoped_task_scheduler_unittest.cc (right): https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... base/test/scoped_task_scheduler_unittest.cc:236: Bind([]() { Optional: For readability, it might be better to format these three consistently (git cl format does strange things with lambda arguments).
lgtm + comments
The CQ bit was checked by fdoray@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...
gab@: PTAL https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... base/test/scoped_task_scheduler.cc:70: // MessageLoop (wasn't handed one). |message_loop_| will always be set and is On 2016/12/13 03:27:42, robliao wrote: > s/handed one/provided one at construction/ Done. https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:43: // When |scoped_task_scheduler| is destroyed, it runs "D" since it's On 2016/12/13 03:27:42, robliao wrote: > It's not immediately clear that the entire code sample above is to be taken > together. To establish flow... > s/In this snippet/Continuing here,/ > > s/When |scoped_task_scheduler| is destroyed/At this point, > |scoped_task_scheduler| will be destroyed. After that, / Done. https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... File base/test/scoped_task_scheduler_unittest.cc (right): https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... base/test/scoped_task_scheduler_unittest.cc:236: Bind([]() { On 2016/12/13 03:27:42, robliao wrote: > Optional: For readability, it might be better to format these three consistently > (git cl format does strange things with lambda arguments). Doesn't work. There is a PRESUBMIT check that verifies that base/ is git cl formatted.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2557083002/#ps120001 (title: "CR robliao #27")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... File base/test/scoped_task_scheduler_unittest.cc (right): https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... base/test/scoped_task_scheduler_unittest.cc:236: Bind([]() { On 2016/12/13 19:14:14, fdoray wrote: > On 2016/12/13 03:27:42, robliao wrote: > > Optional: For readability, it might be better to format these three > consistently > > (git cl format does strange things with lambda arguments). > > Doesn't work. There is a PRESUBMIT check that verifies that base/ is git cl > formatted. Interesting. Might be good feedback for the folks who maintain base + git cl. Thanks for checking!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm https://codereview.chromium.org/2557083002/diff/120001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/120001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:26: // Continuing here, RunUntilIdle() returns after "A" is run. I don't understand what "Continuing here" means? I prefered previous sentence with "In this snippet"?
https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/100001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:43: // When |scoped_task_scheduler| is destroyed, it runs "D" since it's On 2016/12/13 03:27:42, robliao wrote: > It's not immediately clear that the entire code sample above is to be taken > together. To establish flow... > s/In this snippet/Continuing here,/ > > s/When |scoped_task_scheduler| is destroyed/At this point, > |scoped_task_scheduler| will be destroyed. After that, / The 2 first blocks of code don't have to be taken together. The last paragraph ("At this point, ...") has to be taken together with the last block of code. https://codereview.chromium.org/2557083002/diff/120001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/120001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:26: // Continuing here, RunUntilIdle() returns after "A" is run. On 2016/12/13 19:56:34, gab wrote: > I don't understand what "Continuing here" means? I prefered previous sentence > with "In this snippet"? Done.
fdoray@chromium.org changed reviewers: + danakj@chromium.org, sky@chromium.org
sky@: Please review changes in content/public/test/* danakj@: Please review changes in base/*
https://codereview.chromium.org/2557083002/diff/120001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/120001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:26: // Continuing here, RunUntilIdle() returns after "A" is run. On 2016/12/13 20:38:31, fdoray wrote: > On 2016/12/13 19:56:34, gab wrote: > > I don't understand what "Continuing here" means? I prefered previous sentence > > with "In this snippet"? > > Done. I should have been more prescriptive on this comment. The "continuing here" should have gone to the "in this snippet below" The main concern here is it looks like these snippets occur independently. This is especially confusing towards the bottom when |scoped_task_scheduler| is mentioned again. Having the paragraphs flow enforces that this is meant to be read together as one large snippet.
https://codereview.chromium.org/2557083002/diff/120001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/120001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:26: // Continuing here, RunUntilIdle() returns after "A" is run. On 2016/12/13 21:26:37, robliao wrote: > On 2016/12/13 20:38:31, fdoray wrote: > > On 2016/12/13 19:56:34, gab wrote: > > > I don't understand what "Continuing here" means? I prefered previous > sentence > > > with "In this snippet"? > > > > Done. > > I should have been more prescriptive on this comment. > > The "continuing here" should have gone to the "in this snippet below" > > The main concern here is it looks like these snippets occur independently. This > is especially confusing towards the bottom when |scoped_task_scheduler| is > mentioned again. > > Having the paragraphs flow enforces that this is meant to be read together as > one large snippet. Ah, I indeed read them as independent -- hence why I didn't understand the "Continuing here", makes sense to have it below.
LGTM https://codereview.chromium.org/2557083002/diff/140001/content/public/test/te... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2557083002/diff/140001/content/public/test/te... content/public/test/test_browser_thread_bundle.cc:68: task_scheduler_.reset( Use MakeUnique if possible (see thread on chromium-dev).
https://codereview.chromium.org/2557083002/diff/140001/content/public/test/te... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2557083002/diff/140001/content/public/test/te... content/public/test/test_browser_thread_bundle.cc:68: task_scheduler_.reset( On 2016/12/13 23:24:04, sky wrote: > Use MakeUnique if possible (see thread on chromium-dev). It used to but I pushed back on it earlier because code in rest of this method uses .reset() and I think our style guide prefers to follow nearby code over every other preference?
Description was changed from
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
BUG=553459
==========
to
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
TBR=danakj@chromium.org
BUG=553459
==========
Final thought. https://codereview.chromium.org/2557083002/diff/140001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/140001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Let's add file-specific OWNERS for scoped_task_scheduler* pointing to file://base/task_scheduler ?
I don't think there is a hard and fast rules on changes like this. For example, if changing a parameter in a function that has lots of NULL it's quite common to change at least the new usage to nullptr. Same thing applies here. But I don't feel strongly, which is why I LGTMd the patch. On Wed, Dec 14, 2016 at 6:43 AM, <gab@chromium.org> wrote: > > https://codereview.chromium.org/2557083002/diff/140001/content/public/test/te... > File content/public/test/test_browser_thread_bundle.cc (right): > > https://codereview.chromium.org/2557083002/diff/140001/content/public/test/te... > content/public/test/test_browser_thread_bundle.cc:68: > task_scheduler_.reset( > On 2016/12/13 23:24:04, sky wrote: >> Use MakeUnique if possible (see thread on chromium-dev). > > It used to but I pushed back on it earlier because code in rest of this > method uses .reset() and I think our style guide prefers to follow > nearby code over every other preference? > > https://codereview.chromium.org/2557083002/ -- 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.
Description was changed from
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
TBR=danakj@chromium.org
BUG=553459
==========
to
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
BUG=553459
==========
On 2016/12/14 16:51:26, sky wrote: > I don't think there is a hard and fast rules on changes like this. For > example, if changing a parameter in a function that has lots of NULL > it's quite common to change at least the new usage to nullptr. Same > thing applies here. But I don't feel strongly, which is why I LGTMd > the patch. I'll keep the code as-is.
fdoray@chromium.org changed reviewers: + dcheng@chromium.org - danakj@chromium.org
dcheng@: Please review changes in base/
dcheng@: Please review changes in base/ Note: These changes were already reviewed by base/task_scheduler OWNERS. I think I only need a rubberstamp lgtm.
https://codereview.chromium.org/2557083002/diff/160001/base/message_loop/mess... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/2557083002/diff/160001/base/message_loop/mess... base/message_loop/message_loop.cc:360: DCHECK(!task_runner || task_runner->BelongsToCurrentThread()); I would prefer we limit the API changes here to be available in tests only (though I'm not sure if that's possible). Or are there other situations where this would be useful? https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... base/test/scoped_task_scheduler.cc:156: return std::vector<const HistogramBase*>(); Alternatively, {} suffices. I don't feel strongly though, and I have to admit, I'm a bit curious if this will be optimized to the same thing. https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... base/test/scoped_task_scheduler.cc:175: Passed(std::move(task)), sequence_token), I'd prefer to just write this as Passed(&task) to remain consistent with other code. https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:53: ScopedTaskScheduler(MessageLoop* external_message_loop); Nit: explicit
dcheng@: PTAnL https://codereview.chromium.org/2557083002/diff/160001/base/message_loop/mess... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/2557083002/diff/160001/base/message_loop/mess... base/message_loop/message_loop.cc:360: DCHECK(!task_runner || task_runner->BelongsToCurrentThread()); On 2016/12/15 06:47:36, dcheng wrote: > I would prefer we limit the API changes here to be available in tests only > (though I'm not sure if that's possible). Or are there other situations where > this would be useful? Done. https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... base/test/scoped_task_scheduler.cc:156: return std::vector<const HistogramBase*>(); On 2016/12/15 06:47:36, dcheng wrote: > Alternatively, {} suffices. I don't feel strongly though, and I have to admit, > I'm a bit curious if this will be optimized to the same thing. Done. https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... base/test/scoped_task_scheduler.cc:175: Passed(std::move(task)), sequence_token), On 2016/12/15 06:47:36, dcheng wrote: > I'd prefer to just write this as Passed(&task) to remain consistent with other > code. Done. https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... File base/test/scoped_task_scheduler.h (right): https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... base/test/scoped_task_scheduler.h:53: ScopedTaskScheduler(MessageLoop* external_message_loop); On 2016/12/15 06:47:36, dcheng wrote: > Nit: explicit Done.
https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... base/test/scoped_task_scheduler.cc:175: Passed(std::move(task)), sequence_token), On 2016/12/15 06:47:36, dcheng wrote: > I'd prefer to just write this as Passed(&task) to remain consistent with other > code. I thought https://groups.google.com/a/chromium.org/forum/#!msg/cxx/KXWZtoP5HJ4/5TRPU81Z... had settled on a preference for explicit std::move?
On 2016/12/15 17:56:29, gab wrote: > https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... > File base/test/scoped_task_scheduler.cc (right): > > https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... > base/test/scoped_task_scheduler.cc:175: Passed(std::move(task)), > sequence_token), > On 2016/12/15 06:47:36, dcheng wrote: > > I'd prefer to just write this as Passed(&task) to remain consistent with other > > code. > > I thought > https://groups.google.com/a/chromium.org/forum/#!msg/cxx/KXWZtoP5HJ4/5TRPU81Z... > had settled on a preference for explicit std::move? My final statement was a reference to the eventual state of the world (simply using std::move with callbacks). It's still pretty idiomatic to write Passed(&x) instead of Passed(std::move(x)): there are 893 instances of the former and 155 of the latter.
lgtm
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2557083002/#ps180001 (title: "CR dcheng #53")
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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... File base/test/scoped_task_scheduler.cc (right): https://codereview.chromium.org/2557083002/diff/160001/base/test/scoped_task_... base/test/scoped_task_scheduler.cc:156: return std::vector<const HistogramBase*>(); On 2016/12/15 16:39:32, fdoray wrote: > On 2016/12/15 06:47:36, dcheng wrote: > > Alternatively, {} suffices. I don't feel strongly though, and I have to admit, > > I'm a bit curious if this will be optimized to the same thing. > > Done. Breaks linux_chromium_compile_dbg_ng and linux_chromium_rel_ng .
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2557083002/#ps200001 (title: "fix linux build error")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by fdoray@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by fdoray@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by fdoray@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2557083002/#ps220001 (title: "rebase")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2557083002/#ps240001 (title: "rebase")
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": 240001, "attempt_start_ts": 1482167314237390,
"parent_rev": "45274c4148447ac733fc2108d743e9ae84e2860f", "commit_rev":
"69c6484462e67e779f9ecc1ab32c08ce877bd152"}
Message was sent while issue was closed.
Description was changed from
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
BUG=553459
==========
to
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
BUG=553459
Review-Url: https://codereview.chromium.org/2557083002
==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
BUG=553459
Review-Url: https://codereview.chromium.org/2557083002
==========
to
==========
Run ScopedTaskScheduler tasks synchronously.
With this CL, tasks posted to the base/task_scheduler/post_task.h API
no longer run asynchronously on a dedicated thread. Instead, they are
posted to a MessageLoop on the thread where the ScopedTaskScheduler
lives and run synchronously when RunLoop::Run/RunUntilIdle() is
invoked. This CL also allows usage of the
base/task_scheduler/post_task.h API within the scope of a
TestBrowserThreadBundle.
Benefits of running TaskScheduler tasks synchronously:
- Tests are more deterministic.
- It is easier to wait for a chain of tasks involving TaskScheduler.
E.g.:
base::PostTaskAndReplyWithTraits(
FROM_HERE, TaskTraits(), Bind(&A), Bind(&B));
base::RunLoop().RunUntilIdle();
// Without this CL, this returns immediately because there
// are no pending tasks on the main thread initially. With
// this CL, this returns when A and B have run.
Note that this CL prevents a TestBrowserThreadBundle or a MessageLoop
from being initialized in the scope of a ScopedTaskScheduler. It
fixes tests that previously did this.
BUG=553459
Committed: https://crrev.com/ad58f8382643800d7107f43c91bd2f9776da4900
Cr-Commit-Position: refs/heads/master@{#439505}
==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/ad58f8382643800d7107f43c91bd2f9776da4900 Cr-Commit-Position: refs/heads/master@{#439505} |
