[Extensions] Isolate ExtensionFunction results_ and error_

extensions/browser/extension_function.cc

Index: extensions/browser/extension_function.cc
diff --git a/extensions/browser/extension_function.cc b/extensions/browser/extension_function.cc
index 7216a9ef0c79b38a2212baca7809bc95ba653c1e..d47493e814ff5dbe8000bea2ff868bc5a2ddc1eb 100644
--- a/extensions/browser/extension_function.cc
+++ b/extensions/browser/extension_function.cc
@@ -79,21 +79,9 @@ void LogUma(bool success,
 class ArgumentListResponseValue
     : public ExtensionFunction::ResponseValueObject {
  public:
-  ArgumentListResponseValue(const std::string& function_name,
-                            const char* title,
-                            ExtensionFunction* function,
-                            std::unique_ptr<base::ListValue> result)
-      : function_name_(function_name), title_(title) {
-    if (function->GetResultList()) {
-      DCHECK_EQ(function->GetResultList(), result.get())
-          << "The result set on this function (" << function_name_ << ") "
-          << "either by calling SetResult() or directly modifying |result_| is "
-          << "different to the one passed to " << title_ << "(). "
-          << "The best way to fix this problem is to exclusively use " << title_
-          << "(). SetResult() and |result_| are deprecated.";
-    } else {
-      function->SetResultList(std::move(result));
-    }
+  ArgumentListResponseValue(ExtensionFunction* function,
+                            std::unique_ptr<base::ListValue> result) {
+    SetFunctionResults(function, std::move(result));
     // It would be nice to DCHECK(error.empty()) but some legacy extension
     // function implementations... I'm looking at chrome.input.ime... do this
     // for some reason.
@@ -102,24 +90,15 @@ class ArgumentListResponseValue
   ~ArgumentListResponseValue() override {}
 
   bool Apply() override { return true; }
-
- private:
-  std::string function_name_;
-  const char* title_;
 };
 
 class ErrorWithArgumentsResponseValue : public ArgumentListResponseValue {
  public:
-  ErrorWithArgumentsResponseValue(const std::string& function_name,
-                                  const char* title,
-                                  ExtensionFunction* function,
+  ErrorWithArgumentsResponseValue(ExtensionFunction* function,
                                   std::unique_ptr<base::ListValue> result,
                                   const std::string& error)
-      : ArgumentListResponseValue(function_name,
-                                  title,
-                                  function,
-                                  std::move(result)) {
-    function->SetError(error);
+      : ArgumentListResponseValue(function, std::move(result)) {
+    SetFunctionError(function, error);
   }
 
   ~ErrorWithArgumentsResponseValue() override {}
@@ -132,7 +111,7 @@ class ErrorResponseValue : public ExtensionFunction::ResponseValueObject {
   ErrorResponseValue(ExtensionFunction* function, const std::string& error) {
     // It would be nice to DCHECK(!error.empty()) but too many legacy extension
     // function implementations don't set error but signal failure.
-    function->SetError(error);
+    SetFunctionError(function, error);
   }
 
   ~ErrorResponseValue() override {}
@@ -221,6 +200,22 @@ void UserGestureForTests::DecrementCount() {
 
 }  // namespace
 
+void ExtensionFunction::ResponseValueObject::SetFunctionResults(
+    ExtensionFunction* function,
+    std::unique_ptr<base::ListValue> results) {
+  DCHECK(!function->results_) << "Function " << function->name_
+                              << "already has results set.";
+  function->results_ = std::move(results);
+}
+
+void ExtensionFunction::ResponseValueObject::SetFunctionError(
+    ExtensionFunction* function,
+    const std::string& error) {
+  DCHECK(function->error_.empty()) << "Function " << function->name_
+                                   << "already has an error.";
+  function->error_ = error;
+}
+
 // static
 bool ExtensionFunction::ignore_all_did_respond_for_testing_do_not_use = false;
 
@@ -309,21 +304,11 @@ void ExtensionFunction::SetArgs(const base::ListValue* args) {
   args_ = args->CreateDeepCopy();
 }
 
-void ExtensionFunction::SetResult(std::unique_ptr<base::Value> result) {
-  results_.reset(new base::ListValue());
-  results_->Append(std::move(result));
-}
-
-void ExtensionFunction::SetResultList(
-    std::unique_ptr<base::ListValue> results) {
-  results_ = std::move(results);
-}
-
 const base::ListValue* ExtensionFunction::GetResultList() const {
   return results_.get();
 }
 
-std::string ExtensionFunction::GetError() const {
+const std::string& ExtensionFunction::GetError() const {
   return error_;
 }
 
@@ -336,16 +321,15 @@ bool ExtensionFunction::user_gesture() const {
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::NoArguments() {
-  return ResponseValue(new ArgumentListResponseValue(
-      name(), "NoArguments", this, base::MakeUnique<base::ListValue>()));
+  return ResponseValue(
+      new ArgumentListResponseValue(this, base::MakeUnique<base::ListValue>()));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::OneArgument(
     std::unique_ptr<base::Value> arg) {
   std::unique_ptr<base::ListValue> args(new base::ListValue());
   args->Append(std::move(arg));
-  return ResponseValue(new ArgumentListResponseValue(name(), "OneArgument",
-                                                     this, std::move(args)));
+  return ResponseValue(new ArgumentListResponseValue(this, std::move(args)));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::TwoArguments(
@@ -354,14 +338,12 @@ ExtensionFunction::ResponseValue ExtensionFunction::TwoArguments(
   std::unique_ptr<base::ListValue> args(new base::ListValue());
   args->Append(std::move(arg1));
   args->Append(std::move(arg2));
-  return ResponseValue(new ArgumentListResponseValue(name(), "TwoArguments",
-                                                     this, std::move(args)));
+  return ResponseValue(new ArgumentListResponseValue(this, std::move(args)));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::ArgumentList(
     std::unique_ptr<base::ListValue> args) {
-  return ResponseValue(new ArgumentListResponseValue(name(), "ArgumentList",
-                                                     this, std::move(args)));
+  return ResponseValue(new ArgumentListResponseValue(this, std::move(args)));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::Error(
@@ -396,8 +378,8 @@ ExtensionFunction::ResponseValue ExtensionFunction::Error(
 ExtensionFunction::ResponseValue ExtensionFunction::ErrorWithArguments(
     std::unique_ptr<base::ListValue> args,
     const std::string& error) {
-  return ResponseValue(new ErrorWithArgumentsResponseValue(
-      name(), "ErrorWithArguments", this, std::move(args), error));
+  return ResponseValue(
+      new ErrorWithArgumentsResponseValue(this, std::move(args), error));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::BadMessage() {
@@ -484,9 +466,9 @@ UIThreadExtensionFunction::~UIThreadExtensionFunction() {
   // tricky because checking IsShuttingDown has to be called from the UI thread.
   extensions::ExtensionsBrowserClient* browser_client =
       extensions::ExtensionsBrowserClient::Get();
-  DCHECK(!browser_client || browser_client->IsShuttingDown() || did_respond_ ||
+  DCHECK(!browser_client || browser_client->IsShuttingDown() || did_respond() ||
          ignore_all_did_respond_for_testing_do_not_use)
-      << name_;
+      << name();
 }
 
 UIThreadExtensionFunction*
@@ -590,9 +572,27 @@ void IOThreadExtensionFunction::Destruct() const {
 AsyncExtensionFunction::AsyncExtensionFunction() {
 }
 
+void AsyncExtensionFunction::SetError(const std::string& error) {
+  error_ = error;
+}
+
+const std::string& AsyncExtensionFunction::GetError() const {
+  return error_.empty() ? UIThreadExtensionFunction::GetError() : error_;
+}
+
 AsyncExtensionFunction::~AsyncExtensionFunction() {
 }
 
+void AsyncExtensionFunction::SetResult(std::unique_ptr<base::Value> result) {
+  results_.reset(new base::ListValue());
+  results_->Append(std::move(result));
+}
+
+void AsyncExtensionFunction::SetResultList(
+    std::unique_ptr<base::ListValue> results) {
+  results_ = std::move(results);
+}
+
 ExtensionFunction::ScopedUserGestureForTests::ScopedUserGestureForTests() {
   UserGestureForTests::GetInstance()->IncrementCount();
 }
@@ -602,7 +602,10 @@ ExtensionFunction::ScopedUserGestureForTests::~ScopedUserGestureForTests() {
 }
 
 ExtensionFunction::ResponseAction AsyncExtensionFunction::Run() {
-  return RunAsync() ? RespondLater() : RespondNow(Error(error_));
+  if (RunAsync())
+    return RespondLater();
+  DCHECK(!results_);
+  return RespondNow(Error(error_));
 }
 
 // static
@@ -612,5 +615,12 @@ bool AsyncExtensionFunction::ValidationFailure(
 }
 
 void AsyncExtensionFunction::SendResponse(bool success) {
-  Respond(success ? ArgumentList(std::move(results_)) : Error(error_));
+  ResponseValue response;
+  if (success) {
+    response = ArgumentList(std::move(results_));
+  } else {
+    response = results_ ? ErrorWithArguments(std::move(results_), error_)
+                        : Error(error_);
+  }
+  Respond(std::move(response));
 }