Move the IconLoader to use the task scheduler.

chrome/browser/icon_loader.cc

Index: chrome/browser/icon_loader.cc
diff --git a/chrome/browser/icon_loader.cc b/chrome/browser/icon_loader.cc
index d679bd3598331ad1bda2c5228218a4bef291b110..aecc25d7ed64ffbd46f03cd8f935a1df3e1dc098 100644
--- a/chrome/browser/icon_loader.cc
+++ b/chrome/browser/icon_loader.cc
@@ -4,7 +4,11 @@
 
 #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"
 
@@ -20,8 +24,8 @@ IconLoader* IconLoader::Create(const base::FilePath& file_path,
 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)));
 }
@@ -38,7 +42,6 @@ void IconLoader::ReadGroup() {
 }
 
 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)));
 }