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 24 matching lines @@
 namespace extensions {
 
 using api::extension_types::InjectDetails;
 
 ExecuteCodeFunction::ExecuteCodeFunction() {
 }
 
 ExecuteCodeFunction::~ExecuteCodeFunction() {
 }
 
-void ExecuteCodeFunction::DidLoadFile(bool success,
-                                      std::unique_ptr<std::string> data) {
-  if (!success || !details_->file) {
-    DidLoadAndLocalizeFile(resource_.relative_path().AsUTF8Unsafe(), success,
-                           std::move(data));
+void ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnFileThread(
+    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);
+
+  file_url_ = net::FilePathToFileURL(resource_.GetFilePath());
+
+  if (!might_require_localization)
+    return;
+
+  if (data->find(extensions::MessageBundle::kMessageBegin) ==

[Devlin, 2016/09/01 06:15:21]

nit: either a comment explaining this or a self-documenting variable (bool
needs_message_substitution) would be in order.

[lazyboy, 2016/09/01 18:10:18]

Done.

+      std::string::npos) {
     return;
   }
 
-  ScriptExecutor::ScriptType script_type =
-      ShouldInsertCSS() ? ScriptExecutor::CSS : ScriptExecutor::JAVASCRIPT;
+  std::unique_ptr<SubstitutionMap> localization_messages(
+      file_util::LoadMessageBundleSubstitutionMap(extension_path, extension_id,
+                                                  extension_default_locale));
 
-  std::string extension_id;
-  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));
+  std::string error;
+  MessageBundle::ReplaceMessagesWithExternalDictionary(*localization_messages,
+                                                       data, &error);
 }
 
-void ExecuteCodeFunction::GetFileURLAndLocalizeCSS(
-    ScriptExecutor::ScriptType script_type,
+void ExecuteCodeFunction::GetFileURLAndLocalizeComponentResourceOnFileThread(
     std::unique_ptr<std::string> data,
+    bool might_require_localization,
     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));
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+  GetFileURLAndMaybeLocalizeOnFileThread(
+      extension_id, extension_path, extension_default_locale,
+      might_require_localization, data.get());
 
-    // 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());
-
-  // 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.
+  bool success = true;
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
-                 resource_.relative_path().AsUTF8Unsafe(), true,
+                 resource_.relative_path().AsUTF8Unsafe(), success,
                  base::Passed(std::move(data))));
 }
 
 void ExecuteCodeFunction::DidLoadAndLocalizeFile(
     const std::string& file,
     bool success,
     std::unique_ptr<std::string> data) {
   if (success) {
     if (!base::IsStringUTF8(*data)) {
       error_ = ErrorUtils::FormatErrorMessage(kBadFileEncodingError, file);
@@ skipping 89 matching lines @@
 }
 
 bool ExecuteCodeFunction::LoadFile(const std::string& file) {
   resource_ = extension()->GetResource(file);
 
   if (resource_.extension_root().empty() || resource_.relative_path().empty()) {
     error_ = kNoCodeOrFileToExecuteError;
     return false;
   }
 
+  std::string extension_id = extension()->id();

[Devlin, 2016/09/01 06:15:21]

const& should be sufficient here.

[lazyboy, 2016/09/01 18:10:18]

Done.

+  base::FilePath extension_path = extension()->path();
+  std::string extension_default_locale;
+  extension()->manifest()->GetString(manifest_keys::kDefaultLocale,
+                                     &extension_default_locale);
+  bool might_require_localization = ShouldInsertCSS() && !extension_id.empty();

[Devlin, 2016/09/01 06:15:21]

Hmm... can extension_id actually be empty?  Before this was a defensive check if
extension() was null, but I don't think that could ever actually happen.

[lazyboy, 2016/09/01 18:10:18]

That's probably because we checked extension() in other thread?

 but I don't think that could ever actually happen.

If extension() is not null, then extension_id is guaranteed to be defined? I've
put a todo for this, to avoid more behavior changes in this CL.

[Devlin, 2016/09/01 18:15:42]

extension_id should definitely always be defined for all extensions.  Otherwise
much badness ensues (e.g., failure to properly place the extension in a set,
look it up in the registry, etc).  It should only ever happen in very small unit
tests, and even then, rarely.  A TODO for now is fine, but I'd like to migrate
it to a DCHECK.

+
   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())));
+    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)),
+                   might_require_localization, extension_id, extension_path,
+                   extension_default_locale));
   } else {
+    FileReader::OptionalFileThreadTaskCallback get_file_and_l10n_callback =
+        base::Bind(&ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnFileThread,
+                   this, extension_id, extension_path, extension_default_locale,
+                   might_require_localization);
+
     scoped_refptr<FileReader> file_reader(new FileReader(
-        resource_, base::Bind(&ExecuteCodeFunction::DidLoadFile, this)));
+        resource_, get_file_and_l10n_callback,
+        base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
+                   resource_.relative_path().AsUTF8Unsafe())));
     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_