Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943923002:
Use TaskScheduler instead of SequencedWorkerPool in portable_device_watcher_win.cc (Closed)
DescriptionUse TaskScheduler instead of SequencedWorkerPool in portable_device_watcher_win.cc
SequencedWorkerPool is being deprecated in favor of TaskScheduler.
BUG=667892
Review-Url: https://codereview.chromium.org/2943923002
Cr-Commit-Position: refs/heads/master@{#481530}
Committed: https://chromium.googlesource.com/chromium/src/+/db29a7876e330ce76c69967c3dc84f905b72ae45
Patch Set 1 #
Total comments: 13
Patch Set 2 : Use LazySequencedTaskRunner instead of SequencedTaskRunner. #Patch Set 3 : Use SequencedTaskRunners #Messages
Total messages: 26 (13 generated)
Description was changed from ========== Use TaskScheduler instead of SequencedWorkerPool in portable_device_watcher_win.cc SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 ========== to ========== Use TaskScheduler instead of SequencedWorkerPool in portable_device_watcher_win.cc SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 ==========
peary2@gmail.com changed reviewers: + fdoray@chromium.org, thestig@chromium.org
I met this issue while solving problem codereview.chromium.org/2889683003. It seems that I have to solve this problem before I can proceed with review 2889683003. PTAL :)
https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (left): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:320: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Can we retain some kind of DCHECK to help make sure these functions are running on the right sequence? https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (right): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:36: const char kMediaTaskRunnerName[] = "media-task-runner"; I think this needs to be removed too.
https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (left): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:320: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2017/06/19 18:24:46, Lei Zhang wrote: > Can we retain some kind of DCHECK to help make sure these functions are running > on the right sequence? DCHECK(media_task_runner_->RunsTasksInCurrentSequence()) would be the correct way of verifying that tasks run on the right sequence. If you don't have access to the media_task_runner_, you can still check that the task was posted with the MayBlock() trait: base::ThreadRestrictions::AssertIOAllowed(); More details at https://chromium.googlesource.com/chromium/src/+/master/docs/task_scheduler_m... https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (right): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:500: media_task_runner_ = base::CreateSequencedTaskRunnerWithTraits( Do tasks posted to this TaskRunner need to run in sequence with other tasks with the "media-task-runner" sequence token? https://cs.chromium.org/search/?q=media-task-runner They did before this change. The recommended way of posting tasks to the same sequence when there isn't a clear place to own/access the common SequencedTaskRunner is to use lazy_task_runner.h https://chromium.googlesource.com/chromium/src/+/0de94189083a49deb78305c2793a... https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:503: .get(); no .get()
Thank you for Information. I used LazySequencedTaskRunner instead of SequencedTaskRunner. If the direction is wrong, let me know :). https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (left): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:320: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2017/06/19 18:35:02, fdoray wrote: > On 2017/06/19 18:24:46, Lei Zhang wrote: > > Can we retain some kind of DCHECK to help make sure these functions are > running > > on the right sequence? > > DCHECK(media_task_runner_->RunsTasksInCurrentSequence()) would be the correct > way of verifying that tasks run on the right sequence. If you don't have access > to the media_task_runner_, you can still check that the task was posted with the > MayBlock() trait: > > base::ThreadRestrictions::AssertIOAllowed(); > > More details at > https://chromium.googlesource.com/chromium/src/+/master/docs/task_scheduler_m... Done. https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:320: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2017/06/19 18:24:46, Lei Zhang wrote: > Can we retain some kind of DCHECK to help make sure these functions are running > on the right sequence? Done. https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (right): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:36: const char kMediaTaskRunnerName[] = "media-task-runner"; On 2017/06/19 18:24:46, Lei Zhang wrote: > I think this needs to be removed too. Done. https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:500: media_task_runner_ = base::CreateSequencedTaskRunnerWithTraits( I used LazySequencedTaskRunner instead of SequencedTaskRunner. If the direction is wrong, let me know. https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:503: .get(); On 2017/06/19 18:35:02, fdoray wrote: > no .get() Done.
https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (right): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:500: media_task_runner_ = base::CreateSequencedTaskRunnerWithTraits( On 2017/06/20 09:04:42, Yeol Park wrote: > I used LazySequencedTaskRunner instead of SequencedTaskRunner. > If the direction is wrong, let me know. Previously, |media_task_runner_| was created with the same sequence token as https://cs.chromium.org/chromium/src/chrome/browser/media_galleries/fileapi/m... and https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content.... Mutual exclusion was guaranteed between tasks posted to these SequencedTaskRunner. With this CL, you no longer have mutual exclusion with these other SequencedTaskRunners. Is it an issue?
Thank you for your information :) https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (right): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:500: media_task_runner_ = base::CreateSequencedTaskRunnerWithTraits( On 2017/06/20 12:15:02, fdoray wrote: > On 2017/06/20 09:04:42, Yeol Park wrote: > > I used LazySequencedTaskRunner instead of SequencedTaskRunner. > > If the direction is wrong, let me know. > > Previously, |media_task_runner_| was created with the same sequence token as > https://cs.chromium.org/chromium/src/chrome/browser/media_galleries/fileapi/m... > and > https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content.... > Mutual exclusion was guaranteed between tasks posted to these > SequencedTaskRunner. > > With this CL, you no longer have mutual exclusion with these other > SequencedTaskRunners. Is it an issue? Done.
The CQ bit was checked by peary2@gmail.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
thestig@: Can you share your thoughts about using a different sequence in this file? https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... File components/storage_monitor/portable_device_watcher_win.cc (right): https://codereview.chromium.org/2943923002/diff/1/components/storage_monitor/... components/storage_monitor/portable_device_watcher_win.cc:500: media_task_runner_ = base::CreateSequencedTaskRunnerWithTraits( On 2017/06/21 02:34:32, Yeol Park wrote: > On 2017/06/20 12:15:02, fdoray wrote: > > On 2017/06/20 09:04:42, Yeol Park wrote: > > > I used LazySequencedTaskRunner instead of SequencedTaskRunner. > > > If the direction is wrong, let me know. > > > > Previously, |media_task_runner_| was created with the same sequence token as > > > https://cs.chromium.org/chromium/src/chrome/browser/media_galleries/fileapi/m... > > and > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content.... > > Mutual exclusion was guaranteed between tasks posted to these > > SequencedTaskRunner. > > > > With this CL, you no longer have mutual exclusion with these other > > SequencedTaskRunners. Is it an issue? > > Done. Is it an issue that with your CL, tasks posted to |media_task_runner_| will no longer run on the same sequence as tasks posted to https://cs.chromium.org/chromium/src/chrome/browser/media_galleries/fileapi/m... and https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content... ?
On 2017/06/21 12:26:59, fdoray wrote: > thestig@: Can you share your thoughts about using a different sequence in this > file? It's been a while. I will check and see if the token names are the same due to coincidence or not.
On 2017/06/21 22:23:57, Lei Zhang wrote: > On 2017/06/21 12:26:59, fdoray wrote: > > thestig@: Can you share your thoughts about using a different sequence in this > > file? > > It's been a while. I will check and see if the token names are the same due to > coincidence or not. The two are probably named the same by coincidence.
lgtm
The CQ bit was checked by peary2@gmail.com 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: This issue passed the CQ dry run.
The CQ bit was checked by thestig@chromium.org
lgtm
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": 40001, "attempt_start_ts": 1498142845626090,
"parent_rev": "1f83b935b39059fb1f407857b0da6f4614e16e71", "commit_rev":
"db29a7876e330ce76c69967c3dc84f905b72ae45"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of SequencedWorkerPool in portable_device_watcher_win.cc SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 ========== to ========== Use TaskScheduler instead of SequencedWorkerPool in portable_device_watcher_win.cc SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 Review-Url: https://codereview.chromium.org/2943923002 Cr-Commit-Position: refs/heads/master@{#481530} Committed: https://chromium.googlesource.com/chromium/src/+/db29a7876e330ce76c69967c3dc8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/db29a7876e330ce76c69967c3dc8... |
