Use TaskScheduler instead of SequencedWorkerPool in portable_device_watcher_win.cc

components/storage_monitor/portable_device_watcher_win.cc

Index: components/storage_monitor/portable_device_watcher_win.cc
diff --git a/components/storage_monitor/portable_device_watcher_win.cc b/components/storage_monitor/portable_device_watcher_win.cc
index 62a08a0c39afbbe1c72921859149ba45472e8257..eafbe3802d6576c2551a0db70bd3188cf8c7d965 100644
--- a/components/storage_monitor/portable_device_watcher_win.cc
+++ b/components/storage_monitor/portable_device_watcher_win.cc
@@ -17,7 +17,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/threading/sequenced_worker_pool.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/win/scoped_co_mem.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/scoped_propvariant.h"
@@ -317,7 +317,6 @@ bool IsMassStoragePortableDevice(const base::string16& pnp_device_id,
 base::string16 GetDeviceNameOnBlockingThread(
     IPortableDeviceManager* portable_device_manager,
     const base::string16& pnp_device_id) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

[Lei Zhang, 2017/06/19 18:24:46]

Can we retain some kind of DCHECK to help make sure these functions are running
on the right sequence?

[fdoray, 2017/06/19 18:35:02]

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...

[Yeol Park, 2017/06/20 09:04:42]

Done.

[Yeol Park, 2017/06/20 09:04:42]

Done.

   DCHECK(portable_device_manager);
   base::string16 name;
   GetFriendlyName(pnp_device_id, portable_device_manager, &name) ||
@@ -331,7 +330,6 @@ base::string16 GetDeviceNameOnBlockingThread(
 bool GetDeviceStorageObjectsOnBlockingThread(
     const base::string16& pnp_device_id,
     PortableDeviceWatcherWin::StorageObjects* storage_objects) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(storage_objects);
   base::win::ScopedComPtr<IPortableDevice> device;
   if (!SetUp(pnp_device_id, &device))
@@ -369,7 +367,6 @@ bool GetDeviceInfoOnBlockingThread(
     IPortableDeviceManager* portable_device_manager,
     const base::string16& pnp_device_id,
     PortableDeviceWatcherWin::DeviceDetails* device_details) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(portable_device_manager);
   DCHECK(device_details);
   DCHECK(!pnp_device_id.empty());
@@ -388,7 +385,6 @@ bool GetDeviceInfoOnBlockingThread(
 // returns true and fills in |portable_device_mgr|. On failure, returns false.
 bool GetPortableDeviceManager(
   base::win::ScopedComPtr<IPortableDeviceManager>* portable_device_mgr) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   HRESULT hr = ::CoCreateInstance(
       __uuidof(PortableDeviceManager), NULL, CLSCTX_INPROC_SERVER,
       IID_PPV_ARGS(portable_device_mgr->GetAddressOf()));
@@ -406,7 +402,6 @@ bool GetPortableDeviceManager(
 // false.
 bool EnumerateAttachedDevicesOnBlockingThread(
     PortableDeviceWatcherWin::Devices* devices) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(devices);
   base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr;
   if (!GetPortableDeviceManager(&portable_device_mgr))
@@ -441,7 +436,6 @@ bool EnumerateAttachedDevicesOnBlockingThread(
 bool HandleDeviceAttachedEventOnBlockingThread(
     const base::string16& pnp_device_id,
     PortableDeviceWatcherWin::DeviceDetails* device_details) {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(device_details);
   base::win::ScopedComPtr<IPortableDeviceManager> portable_device_mgr;
   if (!GetPortableDeviceManager(&portable_device_mgr))
@@ -503,10 +497,10 @@ PortableDeviceWatcherWin::~PortableDeviceWatcherWin() {
 void PortableDeviceWatcherWin::Init(HWND hwnd) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   notifications_ = RegisterPortableDeviceNotification(hwnd);
-  base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
-  media_task_runner_ = pool->GetSequencedTaskRunnerWithShutdownBehavior(
-      pool->GetNamedSequenceToken(kMediaTaskRunnerName),
-      base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+  media_task_runner_ = base::CreateSequencedTaskRunnerWithTraits(

[fdoray, 2017/06/19 18:35:02]

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...

[Yeol Park, 2017/06/20 09:04:42]

I used LazySequencedTaskRunner instead of SequencedTaskRunner.
If the direction is wrong, let me know.

[fdoray, 2017/06/20 12:15:02]

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?

[Yeol Park, 2017/06/21 02:34:32]

Done.

[fdoray, 2017/06/21 12:26:59]

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...
?

+                           {base::MayBlock(), base::TaskPriority::BACKGROUND,
+                            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})
+                           .get();

[fdoray, 2017/06/19 18:35:02]

no .get()

[Yeol Park, 2017/06/20 09:04:42]

Done.

   EnumerateAttachedDevices();
 }