Switch BookmarkFaviconFetcher to the TaskScheduler API.

chrome/browser/bookmarks/bookmark_html_writer.cc

Index: chrome/browser/bookmarks/bookmark_html_writer.cc
diff --git a/chrome/browser/bookmarks/bookmark_html_writer.cc b/chrome/browser/bookmarks/bookmark_html_writer.cc
index 5bfecb94303cda68a4b3810baf2c4e3b257a93fe..f9027cd9e271b4a16179576920c595dde3c7407d 100644
--- a/chrome/browser/bookmarks/bookmark_html_writer.cc
+++ b/chrome/browser/bookmarks/bookmark_html_writer.cc
@@ -19,6 +19,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/values.h"
@@ -449,8 +450,12 @@ void BookmarkFaviconFetcher::ExecuteWriter() {
   // for the duration of the write), as such we make a copy of the
   // BookmarkModel using BookmarkCodec then write from that.
   BookmarkCodec codec;
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
+  // TODO(https://crbug.com/676387) make this sequenced when 676387 is fixed.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_ =

[sky, 2017/05/22 16:40:46]

Forgive my ignorance, but what happens if ExecuteWriter is called twice in a
row. Might they both end up on separate background threads?

[Sébastien Marchand, 2017/05/22 17:19:17]

+fdoray@/robliao@ as I'm not entirely sure, one solution would be to add a
"SequencedTaskRunner" member to this class and to use it to run these tasks.
(and otherwise I should remove the trailing underscore if I stick with this
solution)

[robliao, 2017/05/22 17:27:25]

As coded here, if ExecuteRunner is executed twice in a row, then the code will
be executed on separate background threads.

If this is undesirable BookmarkFaviconFetcher will need to hold on to a task
runner for future use. 

If this is okay, instead the code should just post a parallel task since the
task runner is immediately thrown away.

Nit: task_runner_ -> task_runner

[sky, 2017/05/22 17:33:55]

While likely rare, we should make sure there aren't two running at the same
time.

[Sébastien Marchand, 2017/05/23 16:56:48]

One way to do this would be to add a SequencedTaskRunner member to this class
and to use it to run these tasks in sequence, this'll work if we assume that
this class is used as a singleton (otherwise 2 different instances will use a
separate task runner and might run in parallel). This isn't really the case,
this class could be instantiated directly or used via its global instance (via
WriteBookmarks).

There doesn't seem to be any synchronization around |g_fetcher|, WriteBookmarks
create it if needed and this function destroys it, this could be racy if 2
separate threads call WriteBookmarks.

Another option could be to use a single
LazyInstance<scoped_refptr<SequencedTaskRunner>> instance in this file, so all
the write operations will use the same task runner. This is probably the safest
approach for now.

One of the goal of this CL was to dogfood the new task scheduler API, I think
that this class is kind of hostile to task runners as is and this is probably
good feedback to the task scheduler team.

[gab, 2017/05/23 17:17:00]

I agree with sky that although there's only one call in this file, multiple
calls of it must be sequenced with each other.

Looks like sequencing only needs to happen for same |path_| though so having a
SequencedTaskRunner per BookmarkFaviconFetcher instance sounds like it would
work?

+      base::CreateSingleThreadTaskRunnerWithTraits(
+          {base::MayBlock(), base::TaskPriority::BACKGROUND});
+  task_runner_->PostTask(
+      FROM_HERE,
       base::BindOnce(
           &Writer::DoWrite,
           new Writer(codec.Encode(