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..363335219edb4273d2457a6b594fd9bb909b8e04 100644
--- a/extensions/browser/api/execute_code_function.cc
+++ b/extensions/browser/api/execute_code_function.cc
@@ -30,6 +30,10 @@ const char kBadFileEncodingError[] =
     "Could not load file '*' for content script. It isn't UTF-8 encoded.";
 const char kLoadFileError[] = "Failed to load file: \"*\". ";
 
+bool HasLocalizationData(const std::string* data) {

[Devlin, 2016/09/01 04:21:25]

nit: const& instead of const* if we keep this.  That said, it's only called
once, so I think it'd be just as readable to say
bool has_localization_data =
    data->find(...) != std::string::npos;

[lazyboy, 2016/09/01 05:52:58]

Removed, originally I had multiple places that needed this.

+  return data->find(extensions::MessageBundle::kMessageBegin) !=
+         std::string::npos;
+}
 }
 
 namespace extensions {
@@ -42,58 +46,43 @@ 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));
-    return;
-  }
+void ExecuteCodeFunction::GetFileURLAndMaybeLocalize(
+    const std::string& extension_id,
+    const base::FilePath& extension_path,
+    const std::string& extension_default_locale,
+    bool requires_localization,
+    bool success,

[Devlin, 2016/09/01 04:21:25]

If we make the change to not call this when reading the file doesn't succeed, we
don't need this parameter, right?

[lazyboy, 2016/09/01 05:52:58]

Yes,
I originally intended to do that (hence the stray comment in file_reader.h),
however, I thought calling this always makes things deterministic, e.g. we can
always set file_url_ below.
However, what's the point of setting file_url_ if file load is going to fail...
Fixed.

+    std::string* data) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
 
-  ScriptExecutor::ScriptType script_type =
-      ShouldInsertCSS() ? ScriptExecutor::CSS : ScriptExecutor::JAVASCRIPT;
+  file_url_ = net::FilePathToFileURL(resource_.GetFilePath());
 
-  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);
-  }
+  if (!success || !requires_localization || HasLocalizationData(data))

[Devlin, 2016/09/01 04:21:25]

is this supposed to be "!HasLocalizationData"?

[lazyboy, 2016/09/01 05:52:58]

Yes, didn't check other than compiling on last patch set :(
Done. 

+    return;
 
-  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::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.

[Devlin, 2016/09/01 04:21:25]

nit: I don't this comment adds anything anymore.

[lazyboy, 2016/09/01 05:52:58]

Done.

+  std::string error;
+  MessageBundle::ReplaceMessagesWithExternalDictionary(*localization_messages,
+                                                       data, &error);
 }
 
-void ExecuteCodeFunction::GetFileURLAndLocalizeCSS(
-    ScriptExecutor::ScriptType script_type,
+void ExecuteCodeFunction::GetFileURLAndLocalizeComponentResource(
     std::unique_ptr<std::string> data,
+    bool needs_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));
-
-    // 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());
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+  GetFileURLAndMaybeLocalize(extension_id, extension_path,
+                             extension_default_locale, needs_localization, true,
+                             data.get());
 
   // Call back DidLoadAndLocalizeFile on the UI thread. The success parameter

[Devlin, 2016/09/01 04:21:25]

nit: The "Call back DidLoadAndLocalizeFile on the UI thread." doesn't tell us
anything that we didn't know from looking at the code - I'd remove it.

[lazyboy, 2016/09/01 05:52:58]

Done.

-  // is always true, because if loading had failed, we wouldn't have had
-  // anything to localize.
+  // is always true, because we've loaded the content from ResourceBundle.

[Devlin, 2016/09/01 04:21:25]

nitty nit: this could be self-documenting in the call to
GetFileUrlAndMaybeLocalize if we did something like
// |success| is always true because we've loaded the content from
ResourceBundle.
const bool success = true;
GetFileUrlAndMaybeLocalize(id, path, locale, needs, success, data);
PostTask(...this, success, ...);

[lazyboy, 2016/09/01 05:52:58]

Done.

   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
@@ -207,6 +196,13 @@ bool ExecuteCodeFunction::LoadFile(const std::string& file) {
     return false;
   }
 
+  std::string extension_id = extension()->id();
+  base::FilePath extension_path = extension()->path();
+  std::string extension_default_locale;
+  extension()->manifest()->GetString(manifest_keys::kDefaultLocale,
+                                     &extension_default_locale);
+  bool requires_localization = ShouldInsertCSS() && !extension_id.empty();

[Devlin, 2016/09/01 04:21:25]

s/requires_localization/might_require_localization (since we don't know for sure
yet).  I'd recommend this for the parameter name in the function, too.

[lazyboy, 2016/09/01 05:52:58]

Done.

+
   int resource_id;
   const ComponentExtensionResourceManager*
       component_extension_resource_manager =
@@ -219,11 +215,23 @@ 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())));
+    std::unique_ptr<std::string> data(
+        new std::string(resource.data(), resource.size()));
+    content::BrowserThread::PostTask(
+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&ExecuteCodeFunction::GetFileURLAndLocalizeComponentResource,
+                   this, base::Passed(std::move(data)), requires_localization,
+                   extension_id, extension_path, extension_default_locale));
   } else {
+    FileReader::OptionalFileThreadTaskCallback get_file_and_l10n_callback =
+        base::Bind(&ExecuteCodeFunction::GetFileURLAndMaybeLocalize, this,
+                   extension_id, extension_path, extension_default_locale,
+                   requires_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();
   }