Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887953002:
Remove usage of SequencedWorkerPool::GetNamedSequenceToken from net/extras/sqlite/. (Closed)
|
Created:
3 years, 7 months ago by fdoray Modified:
3 years, 7 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove usage of SequencedWorkerPool::GetNamedSequenceToken from net/extras/sqlite/.
SequencedWorkerPool is being deprecated in favor of TaskScheduler.
BUG=667892
Review-Url: https://codereview.chromium.org/2887953002
Cr-Commit-Position: refs/heads/master@{#473275}
Committed: https://chromium.googlesource.com/chromium/src/+/ecba95904f5568af622a7baea4a4dab9b92a9e75
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : self-review #Patch Set 4 : self-review #Patch Set 5 : self-review #
Total comments: 2
Patch Set 6 : CR-remove-comment #
Messages
Total messages: 32 (26 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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 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 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: + rdsmith@chromium.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
/net LGTM, but I'm not familiar enough with use of ScopedTaskEnvironment to review that usage. https://codereview.chromium.org/2887953002/diff/80001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/2887953002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:410: // Wait until the OnLoaded callback has run. This seems inaccurate since the code waits for the task queue to clear, then expects that the OnLoaded callback has run.
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 ScopedTaskEnvironment experts are base/task_scheduler/OWNERS (includes me). What is unclear to you? https://codereview.chromium.org/2887953002/diff/80001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/2887953002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:410: // Wait until the OnLoaded callback has run. On 2017/05/18 20:25:10, Randy Smith (Not in Mondays) wrote: > This seems inaccurate since the code waits for the task queue to clear, then > expects that the OnLoaded callback has run. Done. Removed the comment.
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2887953002/#ps100001 (title: "CR-remove-comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/19 18:29:59, fdoray wrote: > The ScopedTaskEnvironment experts are base/task_scheduler/OWNERS (includes me). Yep. That was meant as a "you aren't getting a second pair of eyes on this--if you want one, find someone else". > What is unclear to you? I think what that does is change the binding behavior of task related declarations that occur in its scope, but that wasn't completely clear from the documentation in scoped_task_environment.h. If you want specific feedback, I was confused about the reference of the token "(Thread|Sequenced)TaskRunnerHandle" in that header and how it related to the later calls in the test file to CreateSequencedTaskRunnerWithTraits(). > https://codereview.chromium.org/2887953002/diff/80001/net/extras/sqlite/sqlit... > File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right): > > https://codereview.chromium.org/2887953002/diff/80001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:410: // Wait until > the OnLoaded callback has run. > On 2017/05/18 20:25:10, Randy Smith (Not in Mondays) wrote: > > This seems inaccurate since the code waits for the task queue to clear, then > > expects that the OnLoaded callback has run. > > Done. Removed the comment.
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495218605713500,
"parent_rev": "2a454ebdc53801c9241e109db01c329c7b514770", "commit_rev":
"ecba95904f5568af622a7baea4a4dab9b92a9e75"}
Message was sent while issue was closed.
Description was changed from ========== Remove usage of SequencedWorkerPool::GetNamedSequenceToken from net/extras/sqlite/. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 ========== to ========== Remove usage of SequencedWorkerPool::GetNamedSequenceToken from net/extras/sqlite/. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 Review-Url: https://codereview.chromium.org/2887953002 Cr-Commit-Position: refs/heads/master@{#473275} Committed: https://chromium.googlesource.com/chromium/src/+/ecba95904f5568af622a7baea4a4... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ecba95904f5568af622a7baea4a4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
