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

There are 3 types of tasks in ExecuteCodeFunction that require FILE thread:
a) Reading the file content (for non-component extension files)
b) Retrieving the file GURL of the script file from base::FilePath
c) content l10n (if the script is CSS)

This CL does all of these in on UI->FILE hop and doesn't require
additional FILE thread hop.
Previously, the most common case, i.e. chrome.tabs.executeScript()
with {file:...}, would require UI->FILE->UI->FILE->UI. With this CL
this become UI->FILE->UI.

This CL accomplishes this with adding an optional callback to FileReader,
to specify additional tasks to be run on FILE thread.

So the thread hop changes are:
Component extension's executeScript() with file:... url would stay same.
*All* other means of executing script file:... url, e.g.
chrome.tabs.executeScript(,{file:..}) would require two (UI->FILE->UI) less
thread hops, whee!

BUG=622464
Test=None, internal only change.

Review-Url: https://codereview.chromium.org/2301713002
Cr-Commit-Position: refs/heads/master@{#419023}

chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc

@@ -90,9 +90,11 @@ class ContentScriptLoader {
 
     extensions::ExtensionResource resource = resources_.front();
     resources_.pop();
-    scoped_refptr<FileReader> reader(
-        new FileReader(resource, base::Bind(&ContentScriptLoader::OnFileLoaded,
-                                            base::Unretained(this))));
+    scoped_refptr<FileReader> reader(new FileReader(
+        resource,
+        FileReader::OptionalFileThreadTaskCallback(),  // null callback.
+        base::Bind(&ContentScriptLoader::OnFileLoaded,
+                   base::Unretained(this))));
     reader->Start();
   }
 

extensions/browser/api/execute_code_function.cc

@@ -42,62 +42,49 @@ 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;
-  }
 
-  ScriptExecutor::ScriptType script_type =
-      ShouldInsertCSS() ? ScriptExecutor::CSS : ScriptExecutor::JAVASCRIPT;
+  bool needs_message_substituion =
+      data->find(extensions::MessageBundle::kMessageBegin) != std::string::npos;
+  if (!needs_message_substituion)
+    return;
 
-  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);
-  }
+  std::unique_ptr<SubstitutionMap> localization_messages(
+      file_util::LoadMessageBundleSubstitutionMap(extension_path, extension_id,
+                                                  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,
     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());
-
-  // 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.
+    const std::string& extension_default_locale,
+    bool might_require_localization) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+  GetFileURLAndMaybeLocalizeOnFileThread(
+      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(), true,
+                 resource_.relative_path().AsUTF8Unsafe(), success,
                  base::Passed(std::move(data))));
 }
 
@@ -207,6 +194,15 @@ bool ExecuteCodeFunction::LoadFile(const std::string& file) {
     return false;
   }
 
+  const 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);
+  // TODO(lazyboy): |extension_id| should not be empty(), turn this into a
+  // DCHECK.
+  bool might_require_localization = ShouldInsertCSS() && !extension_id.empty();
+
   int resource_id;
   const ComponentExtensionResourceManager*
       component_extension_resource_manager =
@@ -219,11 +215,25 @@ 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::
+                       GetFileURLAndLocalizeComponentResourceOnFileThread,
+                   this, base::Passed(std::move(data)), extension_id,
+                   extension_path, extension_default_locale,
+                   might_require_localization));
   } 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();
   }
 

extensions/browser/api/execute_code_function.h

@@ -52,17 +52,27 @@ class ExecuteCodeFunction : public AsyncExtensionFunction {
   std::unique_ptr<api::extension_types::InjectDetails> details_;
 
  private:
-  // Called when contents from the file whose path is specified in JSON
-  // arguments has been loaded.
-  void DidLoadFile(bool success, std::unique_ptr<std::string> data);
-
-  // Runs on FILE thread. Loads message bundles for the extension and
-  // localizes the CSS data. Calls back DidLoadAndLocalizeFile on the UI thread.
-  void GetFileURLAndLocalizeCSS(ScriptExecutor::ScriptType script_type,
-                                std::unique_ptr<std::string> data,
-                                const std::string& extension_id,
-                                const base::FilePath& extension_path,
-                                const std::string& extension_default_locale);
+  // Retrieves the file url for the given |extension_path| and optionally
+  // localizes |data|.
+  // Localization depends on whether |might_require_localization| was specified.
+  // Only CSS file content needs to be localized.
+  void GetFileURLAndMaybeLocalizeOnFileThread(
+      const std::string& extension_id,
+      const base::FilePath& extension_path,
+      const std::string& extension_default_locale,
+      bool might_require_localization,
+      std::string* data);
+
+  // Retrieves the file url for the given |extension_path| and optionally
+  // localizes |data|.
+  // Similar to GetFileURLAndMaybeLocalizeOnFileThread, but only applies to
+  // component extension resource.
+  void GetFileURLAndLocalizeComponentResourceOnFileThread(
+      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);
 
   // Run in UI thread.  Code string contains the code to be executed. Returns
   // true on success. If true is returned, this does an AddRef.

extensions/browser/file_reader.cc

@@ -5,16 +5,20 @@
 #include "extensions/browser/file_reader.h"
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/files/file_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
-FileReader::FileReader(const extensions::ExtensionResource& resource,
-                       const Callback& callback)
+FileReader::FileReader(
+    const extensions::ExtensionResource& resource,
+    const OptionalFileThreadTaskCallback& optional_file_thread_task_callback,
+    const DoneCallback& done_callback)
     : resource_(resource),
-      callback_(callback),
+      optional_file_thread_task_callback_(optional_file_thread_task_callback),
+      done_callback_(done_callback),
       origin_task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
 
 void FileReader::Start() {
@@ -28,6 +32,17 @@ FileReader::~FileReader() {}
 void FileReader::ReadFileOnBackgroundThread() {
   std::unique_ptr<std::string> data(new std::string());
   bool success = base::ReadFileToString(resource_.GetFilePath(), data.get());
+
+  if (!optional_file_thread_task_callback_.is_null()) {
+    if (success) {
+      base::ResetAndReturn(&optional_file_thread_task_callback_)
+          .Run(data.get());
+    } else {
+      optional_file_thread_task_callback_.Reset();
+    }
+  }
+
   origin_task_runner_->PostTask(
-      FROM_HERE, base::Bind(callback_, success, base::Passed(std::move(data))));
+      FROM_HERE, base::Bind(base::ResetAndReturn(&done_callback_), success,
+                            base::Passed(std::move(data))));
 }

extensions/browser/file_reader.h

@@ -19,10 +19,15 @@
 class FileReader : public base::RefCountedThreadSafe<FileReader> {
  public:
   // Reports success or failure and the data of the file upon success.
-  typedef base::Callback<void(bool, std::unique_ptr<std::string>)> Callback;
+  using DoneCallback = base::Callback<void(bool, std::unique_ptr<std::string>)>;
+  // Lets the caller accomplish tasks on the file data, after the file content
+  // has been read.
+  // If the file reading doesn't succeed, this will be ignored.
+  using OptionalFileThreadTaskCallback = base::Callback<void(std::string*)>;
 
   FileReader(const extensions::ExtensionResource& resource,
-             const Callback& callback);
+             const OptionalFileThreadTaskCallback& file_thread_task_callback,
+             const DoneCallback& done_callback);
 
   // Called to start reading the file on a background thread.  Upon completion,
   // the callback will be notified of the results.
@@ -36,7 +41,8 @@ class FileReader : public base::RefCountedThreadSafe<FileReader> {
   void ReadFileOnBackgroundThread();
 
   extensions::ExtensionResource resource_;
-  Callback callback_;
+  OptionalFileThreadTaskCallback optional_file_thread_task_callback_;
+  DoneCallback done_callback_;
   const scoped_refptr<base::SingleThreadTaskRunner> origin_task_runner_;
 };
 

extensions/browser/file_reader_unittest.cc

@@ -35,7 +35,7 @@ class Receiver {
   Receiver() : succeeded_(false) {
   }
 
-  FileReader::Callback NewCallback() {
+  FileReader::DoneCallback NewCallback() {
     return base::Bind(&Receiver::DidReadFile, base::Unretained(this));
   }
 
@@ -67,7 +67,8 @@ void RunBasicTest(const char* filename) {
   Receiver receiver;
 
   scoped_refptr<FileReader> file_reader(
-      new FileReader(resource, receiver.NewCallback()));
+      new FileReader(resource, FileReader::OptionalFileThreadTaskCallback(),
+                     receiver.NewCallback()));
   file_reader->Start();
 
   base::RunLoop().Run();
@@ -95,7 +96,8 @@ TEST_F(FileReaderTest, NonExistantFile) {
   Receiver receiver;
 
   scoped_refptr<FileReader> file_reader(
-      new FileReader(resource, receiver.NewCallback()));
+      new FileReader(resource, FileReader::OptionalFileThreadTaskCallback(),
+                     receiver.NewCallback()));
   file_reader->Start();
 
   base::RunLoop().Run();