Remove some UI->FILE->UI thread hops in ExecuteCodeFunction

extensions/browser/api/execute_code_function.cc

 // Copyright 2014 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.
 
 #ifndef EXTENSIONS_BROWSER_API_EXECUTE_CODE_FUNCTION_IMPL_H_
 #define EXTENSIONS_BROWSER_API_EXECUTE_CODE_FUNCTION_IMPL_H_
 
 #include "extensions/browser/api/execute_code_function.h"
 
 #include "extensions/browser/component_extension_resource_manager.h"
@@ skipping 25 matching lines @@
 
 using api::extension_types::InjectDetails;
 
 ExecuteCodeFunction::ExecuteCodeFunction() {
 }
 
 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);
   }
 
   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));
+  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);
-  }
-
-  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
   // anything to localize.
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
                  resource_.relative_path().AsUTF8Unsafe(), true,
                  base::Passed(std::move(data))));
 }
@@ skipping 89 matching lines @@
 
   if (details_->code.get())
     return Execute(*details_->code);
 
   if (!details_->file.get())
     return false;
 
   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);
 
   if (resource_.extension_root().empty() || resource_.relative_path().empty()) {
     error_ = kNoCodeOrFileToExecuteError;
     return false;
   }
 
   int resource_id;
   const ComponentExtensionResourceManager*
       component_extension_resource_manager =
           ExtensionsBrowserClient::Get()
               ->GetComponentExtensionResourceManager();
   if (component_extension_resource_manager &&
       component_extension_resource_manager->IsComponentExtensionResource(
           resource_.extension_root(),
           resource_.relative_path(),
           &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)));
     file_reader->Start();
   }
 
   return true;
 }
 
 void ExecuteCodeFunction::OnExecuteCodeFinished(const std::string& error,
                                                 const GURL& on_url,
                                                 const base::ListValue& result) {
   if (!error.empty())
     SetError(error);
 
   SendResponse(error.empty());
 }
 
 }  // namespace extensions
 
 #endif  // EXTENSIONS_BROWSER_API_EXECUTE_CODE_FUNCTION_IMPL_H_