Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709163006:
Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. (Closed)
DescriptionUpdate TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place.
In particular:
- Move mention of TaskScheduler::SetInstance() into a lower prerequisite section
(too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...)
- Add DCHECKs that reference the Prerequisite section.
- Add mentions of base::test::ScopedTaskScheduler in key places
(to increase discovery of it for people new to TaskScheduler)
- Make it clear users shouldn't generally use GetInstance() to know if they need
to SetInstance() before PostTask() -- give people a generic bool, they'll use
it (e.g. TaskRunner::PostTask(), sigh..!) ;)
BUG=553459
Review-Url: https://codereview.chromium.org/2709163006
Cr-Commit-Position: refs/heads/master@{#452872}
Committed: https://chromium.googlesource.com/chromium/src/+/4fe88c1ecfef251dd03a5965e35df69f80a6bf84
Patch Set 1 #
Total comments: 6
Patch Set 2 : tweaks #
Total comments: 4
Patch Set 3 : review:fdoray #
Messages
Total messages: 24 (15 generated)
Description was changed from ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) BUG=553459 ========== to ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) BUG=553459 NOTRY=true ==========
gab@chromium.org changed reviewers: + fdoray@chromium.org, robliao@chromium.org
PTaL, we discussed this a few times offline and I just saw this again (someone not using ScopedTaskScheduler where they should).
NOTRY=true intentional? There are code changes. https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/post_ta... File base/task_scheduler/post_task.cc (right): https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/post_ta... base/task_scheduler/post_task.cc:63: << "Ref. Prerequisite section of post_task.h"; Ref. -> Reference https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/post_ta... File base/task_scheduler/post_task.h (right): https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/post_ta... base/task_scheduler/post_task.h:65: // valid. Note: this is typically done in the main of each process and you most Omit "Note:" Maybe This is typically done during the initialization phase in each process. If your code is not running in this phase, you most likely don't have to worry about this. You will encounter DCHECKs or nullptr dereferences if this is violated. For tests... https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:132: static TaskScheduler* GetInstance(); Might as well hammer home not to call this just to check to see if it was set.
Description was changed from ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) BUG=553459 NOTRY=true ========== to ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) BUG=553459 ==========
On 2017/02/23 23:00:33, robliao wrote: > NOTRY=true intentional? There are code changes. It was but I'd forgotten about DCHECKs, removed. https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/post_ta... File base/task_scheduler/post_task.cc (right): https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/post_ta... base/task_scheduler/post_task.cc:63: << "Ref. Prerequisite section of post_task.h"; On 2017/02/23 23:00:33, robliao wrote: > Ref. -> Reference I use this elision often in comments. I find here it's better because it allows "Prerequisite" to be capitalized without looking awkward. https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/post_ta... File base/task_scheduler/post_task.h (right): https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/post_ta... base/task_scheduler/post_task.h:65: // valid. Note: this is typically done in the main of each process and you most On 2017/02/23 23:00:33, robliao wrote: > Omit "Note:" > > Maybe > This is typically done during the initialization phase in each process. If your > code is not running in this phase, you most likely don't have to worry about > this. You will encounter DCHECKs or nullptr dereferences if this is violated. > For tests... Done and also documented GetInstance() further to make it clear that SetInstance() should be done in strictly one place. https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2709163006/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:132: static TaskScheduler* GetInstance(); On 2017/02/23 23:00:33, robliao wrote: > Might as well hammer home not to call this just to check to see if it was set. Hehe, I just did that as part of resolving your previous comment =P
The CQ bit was checked by gab@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 ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) BUG=553459 ========== to ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask(). BUG=553459 ==========
Description was changed from ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask(). BUG=553459 ========== to ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask() -- give people a bool, they'll use it (e.g. TaskRunner::PostTask(), sigh..!) ;) BUG=553459 ==========
Description was changed from ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask() -- give people a bool, they'll use it (e.g. TaskRunner::PostTask(), sigh..!) ;) BUG=553459 ========== to ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask() -- give people a generic bool, they'll use it (e.g. TaskRunner::PostTask(), sigh..!) ;) BUG=553459 ==========
Description was changed from ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask() -- give people a generic bool, they'll use it (e.g. TaskRunner::PostTask(), sigh..!) ;) BUG=553459 ========== to ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask() -- give people a generic bool, they'll use it (e.g. TaskRunner::PostTask(), sigh..!) ;) BUG=553459 ==========
lgtm! Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:118: // of the worker pool in which a task with given traits should run. To be consistent, add: For tests, prefer ... https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:129: // Retrieve the TaskScheduler set via CreateAndSetDefaultTaskScheduler() or CreateAndSet(Simple|Default)TaskScheduler()
lgtm https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:118: // of the worker pool in which a task with given traits should run. To be consistent, add: For tests, prefer ... https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:129: // Retrieve the TaskScheduler set via CreateAndSetDefaultTaskScheduler() or CreateAndSet(Simple|Default)TaskScheduler()
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2709163006/#ps40001 (title: "review:fdoray")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, done. https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:118: // of the worker pool in which a task with given traits should run. On 2017/02/24 16:53:37, fdoray wrote: > To be consistent, add: For tests, prefer ... Done. https://codereview.chromium.org/2709163006/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:129: // Retrieve the TaskScheduler set via CreateAndSetDefaultTaskScheduler() or On 2017/02/24 16:53:37, fdoray wrote: > CreateAndSet(Simple|Default)TaskScheduler() Done.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487955732609660,
"parent_rev": "02612b1e0615618c3350f29f7f54fc836bd25e64", "commit_rev":
"4fe88c1ecfef251dd03a5965e35df69f80a6bf84"}
Message was sent while issue was closed.
Description was changed from ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask() -- give people a generic bool, they'll use it (e.g. TaskRunner::PostTask(), sigh..!) ;) BUG=553459 ========== to ========== Update TaskScheduler docs to make it more obvious how a TaskScheduler should be put in place. In particular: - Move mention of TaskScheduler::SetInstance() into a lower prerequisite section (too reduce instances of people going crazy with if (!GetInstance()) SetInstance()...) - Add DCHECKs that reference the Prerequisite section. - Add mentions of base::test::ScopedTaskScheduler in key places (to increase discovery of it for people new to TaskScheduler) - Make it clear users shouldn't generally use GetInstance() to know if they need to SetInstance() before PostTask() -- give people a generic bool, they'll use it (e.g. TaskRunner::PostTask(), sigh..!) ;) BUG=553459 Review-Url: https://codereview.chromium.org/2709163006 Cr-Commit-Position: refs/heads/master@{#452872} Committed: https://chromium.googlesource.com/chromium/src/+/4fe88c1ecfef251dd03a5965e35d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4fe88c1ecfef251dd03a5965e35d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
