[Extensions][Task Scheduler] Migrate ExecuteCodeFunction

extensions/browser/api/execute_code_function.cc

Index: extensions/browser/api/execute_code_function.cc
diff --git a/extensions/browser/api/execute_code_function.cc b/extensions/browser/api/execute_code_function.cc
index 0f2dd5f09f3bb39c867d8b3f15b1afa1cf94670d..e82561aba9846dc12c5bedba59902a00b57126ae 100644
--- a/extensions/browser/api/execute_code_function.cc
+++ b/extensions/browser/api/execute_code_function.cc
@@ -7,6 +7,8 @@
 
 #include "extensions/browser/api/execute_code_function.h"
 
+#include "base/task_scheduler/post_task.h"
+#include "base/threading/thread_restrictions.h"
 #include "extensions/browser/component_extension_resource_manager.h"
 #include "extensions/browser/extension_api_frame_id_map.h"
 #include "extensions/browser/extensions_browser_client.h"
@@ -42,13 +44,13 @@ ExecuteCodeFunction::ExecuteCodeFunction() {
 ExecuteCodeFunction::~ExecuteCodeFunction() {
 }
 
-void ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnFileThread(
+void ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnBackgroundThread(
     const std::string& extension_id,
     const base::FilePath& extension_path,
     const std::string& extension_default_locale,
     bool might_require_localization,
     std::string* data) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+  base::ThreadRestrictions::AssertIOAllowed();
 
   file_url_ = net::FilePathToFileURL(resource_.GetFilePath());

[karandeepb, 2017/07/13 01:47:48]

It seems to me that net::FilePathToFileURL does not need to be executed on a
sequence with FILE IO. Also, it seems that currently file_url_ is modified on
file thread and used on UI thread (Not sure if other constraints make this
thread safe). 

Maybe it would be a good idea to initialize file_url_ on UI thread and skip the
hop to file thread if might_require_localization is false. Though this should
probably be done separately. (Also, didn't really dive into the code so may be
wrong)

[Devlin, 2017/07/13 18:35:09]

Good find!  I agree that this would be better done separately (to keep this
patch targeted and allow reverting of only one if something goes wrong), but
I've added a TODO.

 
@@ -69,23 +71,19 @@ void ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnFileThread(
                                                        data, &error);
 }
 
-void ExecuteCodeFunction::GetFileURLAndLocalizeComponentResourceOnFileThread(
+std::unique_ptr<std::string>
+ExecuteCodeFunction::GetFileURLAndLocalizeComponentResourceOnBackgroundThread(
     std::unique_ptr<std::string> data,
     const std::string& extension_id,
     const base::FilePath& extension_path,
     const std::string& extension_default_locale,
     bool might_require_localization) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
-  GetFileURLAndMaybeLocalizeOnFileThread(
+  base::ThreadRestrictions::AssertIOAllowed();
+  GetFileURLAndMaybeLocalizeOnBackgroundThread(
       extension_id, extension_path, extension_default_locale,
       might_require_localization, data.get());
 
-  bool success = true;
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
-                 resource_.relative_path().AsUTF8Unsafe(), success,
-                 base::Passed(std::move(data))));
+  return data;
 }
 
 void ExecuteCodeFunction::DidLoadAndLocalizeFile(
@@ -223,18 +221,23 @@ bool ExecuteCodeFunction::LoadFile(const std::string& file) {
         ResourceBundle::GetSharedInstance().GetRawDataResource(resource_id);
     std::unique_ptr<std::string> data(
         new std::string(resource.data(), resource.size()));
-    content::BrowserThread::PostTask(
-        content::BrowserThread::FILE, FROM_HERE,
-        base::Bind(&ExecuteCodeFunction::
-                       GetFileURLAndLocalizeComponentResourceOnFileThread,
-                   this, base::Passed(std::move(data)), extension_id,
-                   extension_path, extension_default_locale,
-                   might_require_localization));
+
+    base::PostTaskWithTraitsAndReplyWithResult(

[karandeepb, 2017/07/13 01:47:48]

Changing to a base::PostTask instead of a sequenced task runner, would cause us
to lost the ordering between the different posted tasks. (which we have
currently using the file thread). Is this still correct?

[Devlin, 2017/07/13 18:35:09]

Yep, this shouldn't be a problem.  This function only does one thing at a time,
so even though there are multiple threads involved, it should only be active on
a single one, which means we don't care about the sequence between posted tasks.

+        FROM_HERE, {base::MayBlock()},

[karandeepb, 2017/07/13 01:47:48]

It would be nice to be explicit about the shutdown behavior and task priority.

[Devlin, 2017/07/13 18:35:09]

Done for shutdown behavior.  For priority, it's difficult to say since we can't
really judge the importance of this task.  I'd rather leave it up to the task
scheduler to find the inherited priority than set an explicit one here.

+        base::BindOnce(
+            &ExecuteCodeFunction::
+                GetFileURLAndLocalizeComponentResourceOnBackgroundThread,
+            this, base::Passed(std::move(data)), extension_id, extension_path,
+            extension_default_locale, might_require_localization),
+        base::BindOnce(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
+                       resource_.relative_path().AsUTF8Unsafe(),
+                       true /* We assume this call always succeeds */));
   } else {
     FileReader::OptionalFileThreadTaskCallback get_file_and_l10n_callback =
-        base::Bind(&ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnFileThread,
-                   this, extension_id, extension_path, extension_default_locale,
-                   might_require_localization);
+        base::Bind(
+            &ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnBackgroundThread,
+            this, extension_id, extension_path, extension_default_locale,
+            might_require_localization);
 
     scoped_refptr<FileReader> file_reader(new FileReader(
         resource_, get_file_and_l10n_callback,