Remove some UI->FILE->UI thread hops in 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 f1a80cfd873fe4f5d3c6f8b14a69e58dfa69da6c..6451b24b1734cb095096a327a73b5c94af4bf65f 100644
--- a/extensions/browser/api/execute_code_function.cc
+++ b/extensions/browser/api/execute_code_function.cc
@@ -43,21 +43,33 @@ ExecuteCodeFunction::~ExecuteCodeFunction() {
 }
 
 void ExecuteCodeFunction::DidLoadFile(bool success,
+                                      const GURL& file_url,
                                       std::unique_ptr<std::string> data) {
   if (!success || !details_->file) {
     DidLoadAndLocalizeFile(resource_.relative_path().AsUTF8Unsafe(), success,
                            std::move(data));
     return;
   }
-
-  ScriptExecutor::ScriptType script_type =
-      ShouldInsertCSS() ? ScriptExecutor::CSS : ScriptExecutor::JAVASCRIPT;
+  file_url_ = file_url;
 
   std::string extension_id;
+  if (extension())
+    extension_id = extension()->id();
+
+  bool needs_css_localization =
+      ShouldInsertCSS() && !extension_id.empty() &&
+      data->find(MessageBundle::kMessageBegin) != std::string::npos;
+
+  if (!needs_css_localization) {
+    DidLoadAndLocalizeFile(resource_.relative_path().AsUTF8Unsafe(), true,
+                           std::move(data));
+    return;
+  }
+
+  // The file is CSS and needs localization.
   base::FilePath extension_path;
   std::string extension_default_locale;
   if (extension()) {
-    extension_id = extension()->id();
     extension_path = extension()->path();
     extension()->manifest()->GetString(manifest_keys::kDefaultLocale,
                                        &extension_default_locale);
@@ -65,31 +77,24 @@ void ExecuteCodeFunction::DidLoadFile(bool success,
 
   content::BrowserThread::PostTask(
       content::BrowserThread::FILE, FROM_HERE,
-      base::Bind(&ExecuteCodeFunction::GetFileURLAndLocalizeCSS, this,
-                 script_type, base::Passed(std::move(data)), extension_id,
-                 extension_path, extension_default_locale));
+      base::Bind(&ExecuteCodeFunction::LocalizeCSS, this,
+                 base::Passed(std::move(data)), extension_id, extension_path,
+                 extension_default_locale));
 }
 
-void ExecuteCodeFunction::GetFileURLAndLocalizeCSS(
-    ScriptExecutor::ScriptType script_type,
+void ExecuteCodeFunction::LocalizeCSS(
     std::unique_ptr<std::string> data,
     const std::string& extension_id,
     const base::FilePath& extension_path,
     const std::string& extension_default_locale) {
-  // Check if the file is CSS and needs localization.
-  if ((script_type == ScriptExecutor::CSS) && !extension_id.empty() &&
-      (data->find(MessageBundle::kMessageBegin) != std::string::npos)) {
-    std::unique_ptr<SubstitutionMap> localization_messages(
-        file_util::LoadMessageBundleSubstitutionMap(
-            extension_path, extension_id, extension_default_locale));
-
-    // We need to do message replacement on the data, so it has to be mutable.
-    std::string error;
-    MessageBundle::ReplaceMessagesWithExternalDictionary(*localization_messages,
-                                                         data.get(), &error);
-  }
+  std::unique_ptr<SubstitutionMap> localization_messages(
+      file_util::LoadMessageBundleSubstitutionMap(extension_path, extension_id,
+                                                  extension_default_locale));
 
-  file_url_ = net::FilePathToFileURL(resource_.GetFilePath());
+  // We need to do message replacement on the data, so it has to be mutable.
+  std::string error;
+  MessageBundle::ReplaceMessagesWithExternalDictionary(*localization_messages,
+                                                       data.get(), &error);
 
   // Call back DidLoadAndLocalizeFile on the UI thread. The success parameter
   // is always true, because if loading had failed, we wouldn't have had
@@ -199,6 +204,14 @@ bool ExecuteCodeFunction::RunAsync() {
   return LoadFile(*details_->file);
 }
 
+void ExecuteCodeFunction::GetFileURL(std::unique_ptr<std::string> data) {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&ExecuteCodeFunction::DidLoadFile, this, true,
+                 net::FilePathToFileURL(resource_.GetFilePath()),
+                 base::Passed(std::move(data))));
+}
+
 bool ExecuteCodeFunction::LoadFile(const std::string& file) {
   resource_ = extension()->GetResource(file);
 
@@ -219,8 +232,11 @@ bool ExecuteCodeFunction::LoadFile(const std::string& file) {
           &resource_id)) {
     base::StringPiece resource =
         ResourceBundle::GetSharedInstance().GetRawDataResource(resource_id);
-    DidLoadFile(true, base::WrapUnique(
-                          new std::string(resource.data(), resource.size())));
+    content::BrowserThread::PostTask(
+        content::BrowserThread::FILE, FROM_HERE,

[Devlin, 2016/08/31 23:20:37]

So, previously it looks like for component extensions, we went
UI -> FILE (localize + file path) -> UI
and for "normal" extensions, we went
UI -> FILE (read content) -> UI -> FILE (localize + path) -> UI

Now, for component, we do
UI -> FILE (file path) -> UI -(maybe)> FILE (l10n) -> UI
and for normal
UI -> FILE (content + path) -> UI -(maybe)> FILE (l10n) -> UI

So we've made the normal case better, and the component case potentially worse
(though only if inserting css, etc).  I wholeheartedly agree that this is
probably better overall.

But I wonder if we can do better-er. :)  We need to localize if:
a. this function can insert css
b. there's an extension
c. the file content includes the messaging prefix

a and b can be determined before hopping to the file thread, and c can be
determined on the file thread.  As such, I wonder if we could make all of these
cases just
UI -> FILE (read content, get file path, optionally localize) -> UI

This would be a bit tricky because we then need to do work on the file thread
before hopping back, but it looks like FileReader is used relatively few places.

What would you think of changing file reader to be something like:

FileReader(Resource resource,
           Callback optional_file_thread_callback,
           Callback origin_thread_callback);

FileReader::Start() {
  JumpToFileThread();
}

FileReader::RunOnFile() {
  data = ReadData();
  if (!optional_file_thread_callback.IsNull())
    optional_file_thread_callback.Run(data);
  JumpToUIThread(data);
}

?

[lazyboy, 2016/09/01 03:52:47]

Done. PTAL, thanks!

+        base::Bind(&ExecuteCodeFunction::GetFileURL, this,
+                   base::Passed(base::WrapUnique(
+                       new std::string(resource.data(), resource.size())))));
   } else {
     scoped_refptr<FileReader> file_reader(new FileReader(
         resource_, base::Bind(&ExecuteCodeFunction::DidLoadFile, this)));