Make FileReader return ownership of the string content

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, const std::string& data) {
+void ExecuteCodeFunction::DidLoadFile(bool success,
+                                      std::unique_ptr<std::string> data) {
   if (!success || !details_->file) {
-    DidLoadAndLocalizeFile(
-        resource_.relative_path().AsUTF8Unsafe(), success, data);
+    DidLoadAndLocalizeFile(resource_.relative_path().AsUTF8Unsafe(), success,
+                           std::move(data));
     return;
   }
 
   ScriptExecutor::ScriptType script_type =
       ShouldInsertCSS() ? ScriptExecutor::CSS : ScriptExecutor::JAVASCRIPT;
 
   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,
-                 data,
-                 extension_id,
-                 extension_path,
-                 extension_default_locale));
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&ExecuteCodeFunction::GetFileURLAndLocalizeCSS, this,
+                 script_type, base::Passed(std::move(data)), extension_id,
+                 extension_path, extension_default_locale));
 }
 
 void ExecuteCodeFunction::GetFileURLAndLocalizeCSS(
     ScriptExecutor::ScriptType script_type,
-    const std::string& data,
+    std::unique_ptr<std::string> data,
     const std::string& extension_id,
     const base::FilePath& extension_path,
     const std::string& extension_default_locale) {
-  std::string localized_data = data;
   // Check if the file is CSS and needs localization.
   if ((script_type == ScriptExecutor::CSS) && !extension_id.empty() &&
-      (data.find(MessageBundle::kMessageBegin) != std::string::npos)) {
+      (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, &localized_data, &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.
   content::BrowserThread::PostTask(
-      content::BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile,
-                 this,
-                 resource_.relative_path().AsUTF8Unsafe(),
-                 true,
-                 localized_data));
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
+                 resource_.relative_path().AsUTF8Unsafe(), true,
+                 base::Passed(std::move(data))));
 }
 
-void ExecuteCodeFunction::DidLoadAndLocalizeFile(const std::string& file,
-                                                 bool success,
-                                                 const std::string& data) {
+void ExecuteCodeFunction::DidLoadAndLocalizeFile(
+    const std::string& file,
+    bool success,
+    std::unique_ptr<std::string> data) {
   if (success) {
-    if (!base::IsStringUTF8(data)) {
+    if (!base::IsStringUTF8(*data)) {
       error_ = ErrorUtils::FormatErrorMessage(kBadFileEncodingError, file);
       SendResponse(false);
-    } else if (!Execute(data))
+    } else if (!Execute(*data))
       SendResponse(false);
   } else {
     // TODO(viettrungluu): bug: there's no particular reason the path should be
     // UTF-8, in which case this may fail.
     error_ = ErrorUtils::FormatErrorMessage(kLoadFileError, file);
     SendResponse(false);
   }
 }
 
 bool ExecuteCodeFunction::Execute(const std::string& code_string) {
@@ skipping 88 matching lines @@
   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)) {
     const ResourceBundle& rb = ResourceBundle::GetSharedInstance();
-    DidLoadFile(true, rb.GetRawDataResource(resource_id).as_string());
+    DidLoadFile(true, base::WrapUnique(new std::string(
+                          rb.GetRawDataResource(resource_id).as_string())));

[Devlin, 2016/08/12 00:35:11]

Is the compiler smart enough (through rvalue refs) to do only a single copy
here, even though as_string() creates a copy?  Or should we do something like
new std::string(string_piece.data(), string_piece.size())?

[lazyboy, 2016/08/12 01:31:22]

I would *expect*, but not sure, I'm going to explicitly construct it with
data/size from StringPiece as you suggest.
There's no downside to that except someone thinking that it hurts readability.
Or should we do something like

   } 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_