Move the IconLoader to use the task scheduler.

chrome/browser/icon_loader.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/icon_loader.h"
 
+#include <utility>
+
 #include "base/bind.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/task_scheduler/task_traits.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
 // static
 IconLoader* IconLoader::Create(const base::FilePath& file_path,
                                IconSize size,
                                IconLoadedCallback callback) {
   return new IconLoader(file_path, size, callback);
 }
 
 void IconLoader::Start() {
   target_task_runner_ = base::ThreadTaskRunnerHandle::Get();
 
-  BrowserThread::PostTaskAndReply(
-      BrowserThread::FILE, FROM_HERE,
+  base::PostTaskWithTraitsAndReply(

[gab, 2017/06/22 19:49:41]

This is fine so long as IconLoader::Start() is only invoked once (should we
DCHECK that?), otherwise this results in a task posted in parallel and therefore
could result in many &IconLoader::ReadGroup invoked on the same |this| in
parallel.

(update: I guess that's already enforced by |delete this| in ReadIcon())

[Avi (use Gerrit), 2017/06/22 20:31:45]

Acknowledged.

+      FROM_HERE, traits(),
       base::BindOnce(&IconLoader::ReadGroup, base::Unretained(this)),
       base::BindOnce(&IconLoader::OnReadGroup, base::Unretained(this)));
 }
 
 IconLoader::IconLoader(const base::FilePath& file_path,
                        IconSize size,
                        IconLoadedCallback callback)
     : file_path_(file_path), icon_size_(size), callback_(callback) {}
 
 IconLoader::~IconLoader() {}
 
 void IconLoader::ReadGroup() {
   group_ = GroupForFilepath(file_path_);
 }
 
 void IconLoader::OnReadGroup() {
-  BrowserThread::PostTask(
-      ReadIconThreadID(), FROM_HERE,
-      base::BindOnce(&IconLoader::ReadIcon, base::Unretained(this)));
+  ReadIconTaskRunner()->PostTask(

[gab, 2017/06/22 19:49:41]

This is also fine because the execution model seems to be that ReadIcon()
invokes IconLoadedCallback and deletes |this|.

However, I'm noting that OnReadGroup() is merely a reply on the original
ThreadTaskRunnerHandle (per PostTaskAndReply) which then reposts ReadIcon()
which itself posts back to ThreadTaskRunnerHandle before deleting |this|, that
middle OnReadGroup() hop seems unnecessary.

Overall all of this is implicitly sequenced by IconLoader itself but you could
make this explicit by using a SequencedTaskRunner and posting ReadGroup() and
ReadIcon() sequentially -- and at that point I'm not sure why ReadGroup() isn't
just part of ReadIcon() (at least it can go in the same task?)

[Avi (use Gerrit), 2017/06/22 20:31:45]

On the Linux platforms, the group determination needs to be done off the UI
thread yet the icon reading needs to be done on the UI thread. If you can
suggest a better way to bounce between those threads I'd be happy to switch to
it.

+      FROM_HERE, base::BindOnce(&IconLoader::ReadIcon, base::Unretained(this)));
 }